Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(server): Application sync fails when CreateNamespace and ServerSideApply flags are set in the sync options #563

Closed
wants to merge 4 commits into from

Conversation

anandf
Copy link
Contributor

@anandf anandf commented Jan 14, 2024

Root Cause:

When server side apply is set to true, dry run is run in server mode, if the dry run fails in server mode, then it is re-run in client mode. But the server side apply is still set. This is not a supported configuration in kubectl when --dry-run is set to client, --server-side flag is not supported.

# kubectl apply -k kustomize-guestbook --dry-run=client --server-side -n guestbook -o json
error: --dry-run=client doesn't work with --server-side (did you mean --dry-run=server instead?)

Solution

When kubectl apply --dry-run=server --server-side fails, then re-run the dry run in client mode and with server-side flag removed as below

# kubectl apply -k kustomize-guestbook --dry-run=client -n guestbook -o json

@anandf anandf changed the title Fixed server side flag to false when rerunning in dry run in client mode fix(server): Application sync fails when CreateNamespace and ServerSideApply flags are set in the sync options Jan 14, 2024
Copy link

codecov bot commented Jan 14, 2024

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (aba3819) 54.47% compared to head (4357008) 54.47%.

Files Patch % Lines
pkg/sync/sync_context.go 66.66% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #563      +/-   ##
==========================================
- Coverage   54.47%   54.47%   -0.01%     
==========================================
  Files          41       41              
  Lines        4793     4795       +2     
==========================================
+ Hits         2611     2612       +1     
- Misses       1969     1970       +1     
  Partials      213      213              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@anandf
Copy link
Contributor Author

anandf commented Jan 14, 2024

Steps to reproduce the error

  • Create the kind cluster
kind create cluster --name argocd
  • Install the 2.10.0-rc1 version of argocd in argocd namespace
kubectl create ns argocd && kubectl apply -f https://raw.githubusercontent.com/argoproj/argo-cd/v2.10.0-rc1/manifests/install.yaml -n argocd
  • Update the image containing the fix
kubectl config set-context --current --namespace argocd

argocd app create guestbook --core --repo https://github.com/anandf/argocd-example-apps.git \
--path kustomize-guestbook \
--dest-namespace guestbook \
--dest-server https://kubernetes.default.svc \
--sync-policy automated \
--self-heal \
--sync-option CreateNamespace=true \
--sync-option ServerSideApply=true
  • List the application and notice the status and health of the application.
argocd app list --core

The argocd application guestbook will fail to sync and will have the sync failed status with messagenamespace \'guestbook\' not found

@anandf
Copy link
Contributor Author

anandf commented Jan 14, 2024

Steps to build the fix

  • Clone the argo-cd repo for branch release-2.10
git clone -b release-2.10 [email protected]:argoproj/argo-cd
  • Update the go.mod file to pick the gitops-engine library containing the fix and update the modules
go mod edit -replace github.com/argoproj/gitops-engine=github.com/anandf/gitops-engine@b39d57e63236b3f430a8b229514f76a5a53f5d46
go mod tidy
  • Build the argocd image and push it to quay or docker>
export QUAY_USER=<quay_userid>
IMAGE_PREFIX=quay.io/$QUAY_USER/ make image && docker push quay.io/$QUAY_USER/argocd:latest

Steps to apply the fix and validate

  • Delete and recreate the kind cluster
kind delete cluster --name argocd
kind create cluster --name argocd
  • Install the 2.10.0-rc1 version of ArgoCD in argocd namespace
kubectl create ns argocd && kubectl apply -f https://raw.githubusercontent.com/argoproj/argo-cd/v2.10.0-rc1/manifests/install.yaml -n argocd
  • Update the image containing the fix
kubectl set image -n argocd statefulset argocd-application-controller \*=quay.io/$QUAY_USER/argocd:latest
  • Wait for the rollout to complete and create the application
kubectl rollout status statefulset/argocd-application-controller -n argocd
  • Creat the application with syncOptions CreateNamespace=true and ServerSideApply=true as below
kubectl config set-context --current --namespace argocd

argocd app create guestbook --core --repo https://github.com/anandf/argocd-example-apps.git \
--path kustomize-guestbook \
--dest-namespace guestbook \
--dest-server https://kubernetes.default.svc \
--sync-policy automated \
--self-heal \
--sync-option CreateNamespace=true \
--sync-option ServerSideApply=true
  • List the application and ensure that the application gets synced and reach an healthy state.
argocd app list --core

@pasha-codefresh
Copy link
Member

pasha-codefresh commented Jan 16, 2024

@anandf thank you for your PR. Last time when i have fixed this issue argoproj/argo-cd#16177
in these PRs
#548
#546

In high level
Argocd always used client side dry run for both, client and server apply. I have noticed only issue with hooks that i described originally and for compatibility we decided not to change how it worked before if error occur.
Before my fix it always took server side apply with client side dry run.

Could you please confirm that before my changes you were not able reproduce this issue? Because with your change you also going to change how you applying resources and customer even will not know that they doing it with client side apply.

Also important to say that this issue was opened before my fix
argoproj/argo-cd#13874

So looks like it is just another not covered case with ServerSideApply

Thanks!

Copy link
Contributor

@leoluz leoluz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor comments.
Otherwise LGTM
Thanks for contributing!

pkg/sync/sync_context.go Outdated Show resolved Hide resolved
pkg/sync/sync_context.go Outdated Show resolved Hide resolved
@leoluz
Copy link
Contributor

leoluz commented Jan 16, 2024

Could you please confirm that before my changes you were not able reproduce this issue? Because with your change you also going to change how you applying resources and customer even will not know that they doing it with client side apply.

@pasha-codefresh This change will only affect the sync during dryrun with strategy DryRunServer. It doesn't seem to change how Argo CD actually applies the resource in the cluster. Can you please take another look in his changes and confirm?

@pasha-codefresh
Copy link
Member

Could you please confirm that before my changes you were not able reproduce this issue? Because with your change you also going to change how you applying resources and customer even will not know that they doing it with client side apply.

@pasha-codefresh This change will only affect the sync during dryrun with strategy DryRunServer. It doesn't seem to change how Argo CD actually applies the resource in the cluster. Can you please take another look in his changes and confirm?

Yes, you are right , lgtm

Copy link

Quality Gate Passed Quality Gate passed

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

Copy link
Contributor

@leoluz leoluz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A did more investigation and I don't think that we should be going in this direction.
Explanation here

@anandf
Copy link
Contributor Author

anandf commented Jan 22, 2024

Taking a different approach and created a new PR #564

@leoluz
Copy link
Contributor

leoluz commented Jan 23, 2024

Closing in favour of #564

@leoluz leoluz closed this Jan 23, 2024
@anandf anandf deleted the fix_dryrun_serverside branch February 7, 2024 07:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants