fix: use csapgrade to patch managedFields for client-side apply migration#26289
fix: use csapgrade to patch managedFields for client-side apply migration#26289reggie-k merged 9 commits intoargoproj:masterfrom
Conversation
❌ Preview Environment deleted from BunnyshellAvailable commands (reply to this comment):
|
reggie-k
left a comment
There was a problem hiding this comment.
Thanks for the PR, Peter!
First of all, I apologize for accidentally pushing to your branch the changed manifests I made while testing :(
Next, I am concerned about the text parsing of the annotation size error, and like Alex and Eugene am wondering what would be the downsides of skipping the attempt to perform the CSA migration that uses the annotation in favor of the CSA upgrade?
Also, I have tested the change locally with Argo CD self-managing Kustomize-based bootstrapped Argo CD and it worked - I saw that after the upgrade with SSA, the kubectl-client-side-apply field manager was migrated to argocd-controller, the app was synced and self-upgraded.
The k3s field manager for the AppSet status remained k3s which I think is expected? Unless the status manager should also be argocd-controller.
c258244 to
5e69366
Compare
@reggie-k No worries. After looking more, The original method actually uses the same csaupgrade package so i think its safe to always use this new CSA upgrade. |
b8ba1fa to
1a4b165
Compare
Signed-off-by: Peter Jiang <peterjiang823@gmail.com>
Signed-off-by: Peter Jiang <peterjiang823@gmail.com>
Signed-off-by: Peter Jiang <peterjiang823@gmail.com>
Signed-off-by: Peter Jiang <peterjiang823@gmail.com>
Signed-off-by: Peter Jiang <peterjiang823@gmail.com>
8bb8975 to
cd569c1
Compare
Signed-off-by: Peter Jiang <peterjiang823@gmail.com>
I only meant to request copilot, sorry about that! |
There was a problem hiding this comment.
Pull request overview
Refactors Argo CD’s client-side-apply → server-side-apply migration to avoid last-applied-configuration annotation size limits by patching managedFields via k8s.io/client-go/util/csaupgrade, addressing failures on large resources (e.g., CRDs).
Changes:
- Replace the existing CSA migration step with a
csaupgrade-generated JSON patch that updatesmanagedFields. - Add unit tests covering the new migration helper behavior.
- Update user documentation to reflect the new managedFields-based migration flow.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| gitops-engine/pkg/sync/sync_context.go | Introduces performCSAUpgradeMigration using csaupgrade and wires it into the SSA apply flow. |
| gitops-engine/pkg/sync/sync_context_test.go | Adds tests validating migration behavior with/without a CSA manager present. |
| docs/user-guide/sync-options.md | Updates migration documentation to describe managedFields patching instead of last-applied annotation usage. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Signed-off-by: Peter Jiang <peterjiang823@gmail.com>
| err = sc.performClientSideApplyMigration(t.targetObj, sc.clientSideApplyMigrationManager) | ||
| err = sc.performCSAUpgradeMigration(t.liveObj, sc.clientSideApplyMigrationManager) | ||
| if err != nil { | ||
| return common.ResultCodeSyncFailed, fmt.Sprintf("Failed to perform client-side apply migration: %v", err) |
There was a problem hiding this comment.
i know not your code but should add the resource name here?
fmt.Sprintf("Failed to perform client-side apply migration for %s: %v", kubeutil.GetResourceKey(t.liveObj), err)
|
Codewise, it LGTM. Are there any other validations we need to perform to feel comfortable with the changes? |
alexrecuenco-hf
left a comment
There was a problem hiding this comment.
Sorry for taking so long.
Everything looks good! Except for this race condition.
Does anyone know how we could test this race-condition?
Signed-off-by: Peter Jiang <peterjiang823@gmail.com>
|
Addressed remaining comments.
cc @reggie-k |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #26289 +/- ##
==========================================
+ Coverage 62.64% 62.67% +0.03%
==========================================
Files 412 412
Lines 55467 55585 +118
==========================================
+ Hits 34746 34840 +94
- Misses 17396 17416 +20
- Partials 3325 3329 +4 ☔ View full report in Codecov by Sentry. |
|
@pjiang-dev So 2 (self managing Helm) is tricky: - apiVersion: apiextensions.k8s.io/v1
fieldsType: FieldsV1
fieldsV1:
f:metadata:
f:annotations:
f:helm.sh/resource-policy: {}
f:labels:
f:app.kubernetes.io/name: {}
f:app.kubernetes.io/part-of: {}
f:spec:
f:group: {}
f:names:
f:kind: {}
f:listKind: {}
f:plural: {}
f:shortNames: {}
f:singular: {}
f:scope: {}
f:versions: {}
manager: argocd-controller
operation: Apply
time: "2026-02-19T03:54:36Z"
- apiVersion: apiextensions.k8s.io/v1
fieldsType: FieldsV1
fieldsV1:
f:metadata:
f:annotations:
.: {}
f:helm.sh/resource-policy: {}
f:meta.helm.sh/release-name: {}
f:meta.helm.sh/release-namespace: {}
f:labels:
.: {}
f:app.kubernetes.io/managed-by: {}
f:app.kubernetes.io/name: {}
f:app.kubernetes.io/part-of: {}
f:spec:
f:conversion:
.: {}
f:strategy: {}
f:group: {}
f:names:
f:kind: {}
f:listKind: {}
f:plural: {}
f:shortNames: {}
f:singular: {}
f:scope: {}
manager: helm
operation: Update
time: "2026-02-19T03:47:23Z"Will this result in a conflict in the future? helm update manager and argocd-controller apply manager manage some shared fields. |
|
@reggie-k For kustomize i belive there is no field manager involved so it would behave the same as regular self-managed install. |
|
@pjiang-dev retested with Kustomize - after the migration |
…tion (#26289) Signed-off-by: Peter Jiang <peterjiang823@gmail.com>
|
🍒 Cherry-pick PR created for 3.3: #26516 |
…tion (argoproj#26289) Signed-off-by: Peter Jiang <peterjiang823@gmail.com>
fixes #26279
This PR refactors the existing Client Side Migration feature https://argo-cd.readthedocs.io/en/latest/user-guide/sync-options/#client-side-apply-migration to use the https://pkg.go.dev/k8s.io/client-go/util/csaupgrade to patch managedFields instead of using a client-side-apply as we were hitting issues with CRDs getting
Too long: may not be more than 262144 bytesChecklist: