From e2ded0653dcae9a5ac99cdf24df73e014bda86ab Mon Sep 17 00:00:00 2001 From: Gianluca Mardente Date: Sat, 29 Aug 2026 17:57:33 +0200 Subject: [PATCH] fix: retry ClusterSummary status update in updateStatusForNonReferencedHelmReleases In SyncModeContinuousWithDriftDetection, updateStatusForNonReferencedHelmReleases did a single Get plus a single Status().Update with no retry, unlike its two siblings on the same pass (updateStatusForReferencedHelmReleases, updateValueHashOnHelmChartSummary), which both already wrap their Get+Update in retry.RetryOnConflict. A concurrent status write (e.g. updateValueHashOnHelmChartSummary a few ms earlier) can bump the ClusterSummary's resourceVersion between this function's cached Get and its own Status().Update, so the write loses with: Operation cannot be fulfilled on clustersummaries.config.projectsveltos.io "...": the object has been modified; please apply your changes to the latest version and try again handleCharts returns that error immediately, even though the Helm deploy already succeeded. Consequences while this keeps happening: - the feature hash never advances, so every reconcile re-runs a full no-op deploy pass - the Helm feature reports Failed with consecutiveFailures climbing, while the release itself is healthy and deployed - drift detection registration (postProcessDeployedHelmCharts) is never reached, so ResourceSummary.spec.chartResources stays empty and out-of-band drift goes undetected This mostly hits profiles under frequent reconcile pressure (e.g. an HPA-autoscaled Deployment keeps requesting reconciliation), since each failed pass's own status write (consecutiveFailures) supplies the next conflicting write, making the loop self-sustaining once triggered. Fix: wrap the Get and Status().Update in updateStatusForNonReferencedHelmReleases in retry.RetryOnConflict, matching its two siblings exactly. --- api/v1beta1/zz_generated.deepcopy.go | 5 ++-- controllers/handlers_helm.go | 38 ++++++++++++++------------- controllers/handlers_helm_test.go | 39 ++++++++++++++++++++++++++++ 3 files changed, 61 insertions(+), 21 deletions(-) diff --git a/api/v1beta1/zz_generated.deepcopy.go b/api/v1beta1/zz_generated.deepcopy.go index ae5338bc..c6e84b84 100644 --- a/api/v1beta1/zz_generated.deepcopy.go +++ b/api/v1beta1/zz_generated.deepcopy.go @@ -21,12 +21,11 @@ limitations under the License. package v1beta1 import ( + apiv1beta1 "github.com/projectsveltos/libsveltos/api/v1beta1" corev1 "k8s.io/api/core/v1" - v1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/util/intstr" - - apiv1beta1 "github.com/projectsveltos/libsveltos/api/v1beta1" ) // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. diff --git a/controllers/handlers_helm.go b/controllers/handlers_helm.go index 9949abe1..c61ae5d1 100644 --- a/controllers/handlers_helm.go +++ b/controllers/handlers_helm.go @@ -3356,13 +3356,6 @@ func updateStatusForNonReferencedHelmReleases(ctx context.Context, c client.Clie currentlyReferenced[helmInfo(instantiatedChart.ReleaseNamespace, instantiatedChart.ReleaseName)] = true } - currentClusterSummary := &configv1beta1.ClusterSummary{} - err := c.Get(ctx, - types.NamespacedName{Namespace: dCtx.clusterSummary.Namespace, Name: dCtx.clusterSummary.Name}, currentClusterSummary) - if err != nil { - return dCtx.clusterSummary, err - } - // Index the in-memory FailureMessages written by walkChartsAndDeploy so they are // not lost when we overwrite the status from the freshly fetched currentClusterSummary. inMemoryFailure := make(map[string]*string, len(dCtx.clusterSummary.Status.HelmReleaseSummaries)) @@ -3371,21 +3364,30 @@ func updateStatusForNonReferencedHelmReleases(ctx context.Context, c client.Clie inMemoryFailure[helmInfo(s.ReleaseNamespace, s.ReleaseName)] = s.FailureMessage } - helmReleaseSummaries := make([]configv1beta1.HelmChartSummary, 0, len(currentClusterSummary.Status.HelmReleaseSummaries)) - for i := range currentClusterSummary.Status.HelmReleaseSummaries { - summary := ¤tClusterSummary.Status.HelmReleaseSummaries[i] - if _, ok := currentlyReferenced[helmInfo(summary.ReleaseNamespace, summary.ReleaseName)]; ok { - entry := *summary - if msg, exists := inMemoryFailure[helmInfo(summary.ReleaseNamespace, summary.ReleaseName)]; exists { - entry.FailureMessage = msg + currentClusterSummary := &configv1beta1.ClusterSummary{} + err := retry.RetryOnConflict(retry.DefaultRetry, func() error { + err := c.Get(ctx, + types.NamespacedName{Namespace: dCtx.clusterSummary.Namespace, Name: dCtx.clusterSummary.Name}, currentClusterSummary) + if err != nil { + return err + } + + helmReleaseSummaries := make([]configv1beta1.HelmChartSummary, 0, len(currentClusterSummary.Status.HelmReleaseSummaries)) + for i := range currentClusterSummary.Status.HelmReleaseSummaries { + summary := ¤tClusterSummary.Status.HelmReleaseSummaries[i] + if _, ok := currentlyReferenced[helmInfo(summary.ReleaseNamespace, summary.ReleaseName)]; ok { + entry := *summary + if msg, exists := inMemoryFailure[helmInfo(summary.ReleaseNamespace, summary.ReleaseName)]; exists { + entry.FailureMessage = msg + } + helmReleaseSummaries = append(helmReleaseSummaries, entry) } - helmReleaseSummaries = append(helmReleaseSummaries, entry) } - } - currentClusterSummary.Status.HelmReleaseSummaries = helmReleaseSummaries + currentClusterSummary.Status.HelmReleaseSummaries = helmReleaseSummaries - err = c.Status().Update(ctx, currentClusterSummary) + return c.Status().Update(ctx, currentClusterSummary) + }) if err != nil { return dCtx.clusterSummary, err } diff --git a/controllers/handlers_helm_test.go b/controllers/handlers_helm_test.go index 53ca41b1..3b95aec7 100644 --- a/controllers/handlers_helm_test.go +++ b/controllers/handlers_helm_test.go @@ -35,14 +35,17 @@ import ( "helm.sh/helm/v4/pkg/cli" releasecommon "helm.sh/helm/v4/pkg/release/common" corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/labels" + "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/types" "k8s.io/klog/v2/textlogger" clusterv1 "sigs.k8s.io/cluster-api/api/core/v1beta2" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" + "sigs.k8s.io/controller-runtime/pkg/client/interceptor" configv1beta1 "github.com/projectsveltos/addon-controller/api/v1beta1" "github.com/projectsveltos/addon-controller/controllers" @@ -1054,6 +1057,42 @@ var _ = Describe("HandlersHelm", func() { }, timeout, pollingInterval).Should(BeTrue()) }) + It("UpdateStatusForNonReferencedHelmReleases retries on a status update conflict (regression for #1933)", + func() { + // No helm charts referenced and no existing HelmReleaseSummaries: the only side + // effect left is the unconditional Status().Update() at the end of the function. + initObjects := []client.Object{ + clusterSummary, + } + + // Simulate the cached-client race from #1933: a concurrent write (e.g. + // updateValueHashOnHelmChartSummary) lands between this function's Get and its + // own Status().Update, so the first attempt loses with a stale-resourceVersion + // conflict. Only the first status update conflicts, mirroring a real race rather + // than a permanently broken client. + conflictCount := 0 + c := fake.NewClientBuilder().WithScheme(scheme).WithStatusSubresource(initObjects...).WithObjects(initObjects...). + WithInterceptorFuncs(interceptor.Funcs{ + SubResourceUpdate: func(ctx context.Context, cl client.Client, subResourceName string, + obj client.Object, opts ...client.SubResourceUpdateOption) error { + + if subResourceName == testStatusField && conflictCount == 0 { + conflictCount++ + return apierrors.NewConflict( + schema.GroupResource{Group: configv1beta1.GroupVersion.Group, Resource: "clustersummaries"}, + obj.GetName(), + fmt.Errorf("the object has been modified; please apply your changes to the latest version and try again")) + } + return cl.SubResource(subResourceName).Update(ctx, obj, opts...) + }, + }).Build() + + _, err := controllers.UpdateStatusForNonReferencedHelmReleases(context.TODO(), c, + controllers.NewDeploymentContext(clusterSummary, nil, nil), textlogger.NewLogger(textlogger.NewConfig())) + Expect(err).To(BeNil()) + Expect(conflictCount).To(Equal(1)) + }) + It("updateChartsInClusterConfiguration updates ClusterConfiguration with deployed helm releases", func() { chartDeployed := []configv1beta1.Chart{ {