forwardport - Fix drift correction regression when correctDrift.force is false (#4879)#4939
Conversation
…cher#4879) * Fix drift correction regression when correctDrift.force is false The Helm v4 migration set ServerSideApply to "auto" for non-force rollbacks. This caused Helm to auto-detect and reuse the apply method from the original release, which is SSA. Server-Side Apply tracks field ownership per field manager, so rollback only reverts fields owned by Helm's manager — manual changes owned by a different manager are silently ignored. Setting ServerSideApply to "false" for all rollbacks forces client-side three-way merge, which compares the full resource state and patches all drifted fields regardless of ownership. This restores the Helm v3 behavior. Refers to: rancher#4878 --------- Signed-off-by: Xavi Garcia <xavi.garcia@suse.com>
There was a problem hiding this comment.
Pull request overview
This PR fixes a drift-correction regression introduced during the Helm v4 migration by forcing rollbacks to always use client-side three-way merge (disabling Server-Side Apply), ensuring rollback reverts drifted fields regardless of SSA field ownership.
Changes:
- Always set Helm rollback
ServerSideApplyto"false"so non-force drift correction behaves like Helm v3 (no SSA field-ownership limitation). - Expand/adjust drift-correction integration tests to cover additional drift scenarios (labels, replicas, external deletions, keepFailHistory, multi-resource drift, paused/off-schedule behavior, and a documented Helm limitation).
- Update test assets to explicitly set Service
type: ClusterIPto support immutable-field drift scenarios.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| internal/helmdeployer/rollback.go | Forces client-side apply for all rollbacks to restore drift correction behavior when correctDrift.force is false. |
| integrationtests/agent/bundle_deployment_drift_test.go | Adds/updates integration coverage for multiple drift scenarios and rollback-history behavior. |
| integrationtests/agent/assets/deployment-v1.yaml | Sets Service type explicitly for drift tests involving Service type changes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| err = k8sClient.Get(ctx, nsn, &cm) | ||
| Expect(apierrors.IsNotFound(err)).To(BeTrue()) |
There was a problem hiding this comment.
The test assumes the resource is gone immediately after Delete(), but Kubernetes deletion is asynchronous (and may be delayed by finalizers). This can make the NotFound assertion flaky; consider wrapping the Get/IsNotFound check in Eventually() to wait for actual removal.
| err = k8sClient.Get(ctx, nsn, &cm) | |
| Expect(apierrors.IsNotFound(err)).To(BeTrue()) | |
| Eventually(func(g Gomega) { | |
| err := k8sClient.Get(ctx, nsn, &cm) | |
| g.Expect(apierrors.IsNotFound(err)).To(BeTrue()) | |
| }).Should(Succeed()) |
| err = k8sClient.Get(ctx, nsn, &corev1.Service{}) | ||
| Expect(apierrors.IsNotFound(err)).To(BeTrue()) |
There was a problem hiding this comment.
The test assumes the resource is gone immediately after Delete(), but Kubernetes deletion is asynchronous (and may be delayed by finalizers). This can make the NotFound assertion flaky; consider wrapping the Get/IsNotFound check in Eventually() to wait for actual removal.
| err = k8sClient.Get(ctx, nsn, &corev1.Service{}) | |
| Expect(apierrors.IsNotFound(err)).To(BeTrue()) | |
| Eventually(func() error { | |
| err := k8sClient.Get(ctx, nsn, &corev1.Service{}) | |
| if apierrors.IsNotFound(err) { | |
| return nil | |
| } | |
| if err != nil { | |
| return err | |
| } | |
| return fmt.Errorf("service %s still exists", nsn.String()) | |
| }, time.Minute, time.Second).Should(Succeed()) |
| err = k8sClient.Get(ctx, nsn, &cm) | ||
| Expect(apierrors.IsNotFound(err)).To(BeTrue()) |
There was a problem hiding this comment.
The test assumes the resource is gone immediately after Delete(), but Kubernetes deletion is asynchronous (and may be delayed by finalizers). This can make the NotFound assertion flaky; consider wrapping the Get/IsNotFound check in Eventually() to wait for actual removal.
| err = k8sClient.Get(ctx, nsn, &cm) | |
| Expect(apierrors.IsNotFound(err)).To(BeTrue()) | |
| Eventually(func() bool { | |
| err := k8sClient.Get(ctx, nsn, &cm) | |
| return apierrors.IsNotFound(err) | |
| }).Should(BeTrue()) |
The Helm v4 migration set ServerSideApply to "auto" for non-force rollbacks. This caused Helm to auto-detect and reuse the apply method from the original release, which is SSA. Server-Side Apply tracks field ownership per field manager, so rollback only reverts fields owned by Helm's manager — manual changes owned by a different manager are silently ignored.
Setting ServerSideApply to "false" for all rollbacks forces client-side three-way merge, which compares the full resource state and patches all drifted fields regardless of ownership. This restores the Helm v3 behavior.
Refers to: #4938
Forwardport #4879
Additional Information
Checklist
- [ ] I have updated the documentation via a pull request in the fleet-product-docs repository.