From bff4412fc92aeda49ccba24c4b7409ef9cd6b204 Mon Sep 17 00:00:00 2001 From: Andrew Pryde Date: Wed, 16 Sep 2026 15:23:35 +0100 Subject: [PATCH] fix: clear stale Helm Conflict entry when ownership is won mid-deploy (#1975) buildReferencedHelmReleaseSummaries is the only code that computes the Status and ConflictMessage fields of Status.HelmReleaseSummaries. It runs once per pass, from updateStatusForReferencedHelmReleases at the start of handleCharts. walkChartsAndDeploy then checks ownership again, per chart, in deploySingleChart. The two checks can disagree. If the ClusterSummary that holds a release unregisters between them, this ClusterSummary can win the chart and deploy it, while the entry written moments earlier still says Conflict and names a ClusterSummary that is already gone. Nothing later in the pass corrected that entry. updateValueHashOnHelmChartSummary wrote ValuesHash, PatchesHash and NeedsRedeploy, but not Status. updateStatusForNonReferencedHelmReleases re-reads the ClusterSummary and copies each referenced entry verbatim, overriding only FailureMessage. The pass ends with no error, so the feature goes to Provisioned. The state is then permanent: shouldRedeploy returns false for a deployed feature whose hash has not changed, so proceedDeployingFeature is never reached and the Helm handler never runs again to recompute the entry. The profile reports a Conflict against a ClusterSummary that no longer exists, until an operator clears the feature summary by hand. This is the sibling case of the race fixed in #1741. There the deploy fails with a NonRetriableError and helmConflictResolved recovers it. Here the deploy succeeds, so proceedDeployingFeature returns on the Provisioned branch, before reaching the deployerError != nil block that guards helmConflictResolved. updateValueHashOnHelmChartSummary is the one status write reached per chart after deploySingleChart confirmed ownership, so it now resets the entry to Managing and clears ConflictMessage. A fresh chartManager.CanManageChart check guards the write, so the correction cannot apply to a release this ClusterSummary does not own even if the function gains another caller. DryRun is excluded: it can pass the ownership check without ever registering, and it no-ops the two functions that otherwise maintain these entries. The correction relies on updateStatusForNonReferencedHelmReleases, which runs afterwards and does its own Status().Update, to carry the value forward. It does, because it re-reads from the API server rather than using the in-memory copy. --- controllers/clustersummary_deployer_test.go | 92 ++++++++ controllers/handlers_helm.go | 20 ++ controllers/handlers_helm_test.go | 239 ++++++++++++++++++++ 3 files changed, 351 insertions(+) diff --git a/controllers/clustersummary_deployer_test.go b/controllers/clustersummary_deployer_test.go index 508192f5..8a2dcd79 100644 --- a/controllers/clustersummary_deployer_test.go +++ b/controllers/clustersummary_deployer_test.go @@ -333,6 +333,98 @@ var _ = Describe("ClustersummaryDeployer", func() { Expect(dep.IsKeyInProgress(key)).To(BeFalse()) }) + It("deployFeature does not revisit a stale Helm Conflict once the feature is Provisioned", func() { + // Documents the latch the Helm-side fix exists to prevent. A ClusterSummary that ends a + // pass Provisioned while still holding a Conflict entry never runs the Helm handler + // again, so the entry is never recomputed and the profile reports a conflict forever. + // shouldRedeploy returns false for a deployed feature whose hash has not changed, and + // helmConflictResolved is only consulted on a deployer error, so neither path recovers. + releaseName := randomString() + releaseNamespace := randomString() + clusterSummary.Spec.ClusterProfileSpec.HelmCharts = []configv1beta1.HelmChart{ + { + RepositoryURL: randomString(), + RepositoryName: randomString(), + ChartName: randomString(), + ChartVersion: randomString(), + ReleaseName: releaseName, + ReleaseNamespace: releaseNamespace, + }, + } + + initObjects := []client.Object{ + clusterSummary, + clusterProfile, + cluster, + } + + c := fake.NewClientBuilder().WithScheme(scheme).WithStatusSubresource(initObjects...).WithObjects(initObjects...).Build() + + clusterSummaryScope := getClusterSummaryScope(c, logger, clusterProfile, clusterSummary) + + // helmHash instantiates the chart values, and that resolves the Cluster through the + // management cluster client rather than the client passed in. So the Cluster has to + // exist there too, not only in the fake client driving the reconcile. + ns := &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: namespace}} + Expect(testEnv.Create(ctx, ns)).To(Succeed()) + Expect(waitForObject(ctx, testEnv.Client, ns)).To(Succeed()) + mgmtCluster := &clusterv1.Cluster{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: clusterSummary.Spec.ClusterNamespace, + Name: clusterSummary.Spec.ClusterName, + }, + } + Expect(testEnv.Create(ctx, mgmtCluster)).To(Succeed()) + Expect(waitForObject(ctx, testEnv.Client, mgmtCluster)).To(Succeed()) + + helmHash, err := controllers.HelmHash(ctx, c, clusterSummary, textlogger.NewLogger(textlogger.NewConfig())) + Expect(err).To(BeNil()) + + clusterSummary.Status.FeatureSummaries = []configv1beta1.FeatureSummary{ + { + FeatureID: libsveltosv1beta1.FeatureHelm, + Hash: helmHash, + Status: libsveltosv1beta1.FeatureStatusProvisioned, + }, + } + // The conflict is over. The ClusterSummary named here has been deleted, so this entry + // can only be corrected by running the Helm handler again. + clusterSummary.Status.HelmReleaseSummaries = []configv1beta1.HelmChartSummary{ + { + ReleaseName: releaseName, + ReleaseNamespace: releaseNamespace, + Status: configv1beta1.HelmChartStatusConflict, + ConflictMessage: fmt.Sprintf("ClusterSummary %s managing it", randomString()), + }, + } + + Expect(c.Status().Update(context.TODO(), clusterSummary)).To(Succeed()) + + dep := fakedeployer.GetClient(context.TODO(), textlogger.NewLogger(textlogger.NewConfig()), c) + + reconciler := getClusterSummaryReconciler(c, dep) + + f := controllers.GetHandlersForFeature(libsveltosv1beta1.FeatureHelm) + + err = controllers.DeployFeature(reconciler, context.TODO(), clusterSummaryScope, f, + textlogger.NewLogger(textlogger.NewConfig())) + Expect(err).To(BeNil()) + + // No deploy is submitted, so nothing recomputes HelmReleaseSummaries and the Conflict + // entry survives untouched. + key := deployer.GetKey(clusterSummary.Spec.ClusterNamespace, clusterSummary.Spec.ClusterName, + clusterSummary.Name, string(libsveltosv1beta1.FeatureHelm), libsveltosv1beta1.ClusterTypeCapi, false) + Expect(dep.IsKeyInProgress(key)).To(BeFalse()) + + currentClusterSummary := &configv1beta1.ClusterSummary{} + Expect(c.Get(context.TODO(), + types.NamespacedName{Namespace: clusterSummary.Namespace, Name: clusterSummary.Name}, + currentClusterSummary)).To(Succeed()) + Expect(currentClusterSummary.Status.HelmReleaseSummaries).To(HaveLen(1)) + Expect(currentClusterSummary.Status.HelmReleaseSummaries[0].Status).To( + Equal(configv1beta1.HelmChartStatusConflict)) + }) + It("deployFeature when feature is deployed and hash has changed, calls Deploy", func() { clusterRoleName := randomString() configMap := createConfigMapWithPolicy("default", randomString(), fmt.Sprintf(viewClusterRole, clusterRoleName)) diff --git a/controllers/handlers_helm.go b/controllers/handlers_helm.go index b15a4cf2..4711611f 100644 --- a/controllers/handlers_helm.go +++ b/controllers/handlers_helm.go @@ -4736,6 +4736,11 @@ func updateValueHashOnHelmChartSummary(ctx context.Context, requestedChart *conf return nil, err } + chartManager, err := chartmanager.GetChartManagerInstance(ctx, c) + if err != nil { + return nil, err + } + err = retry.RetryOnConflict(retry.DefaultRetry, func() error { currentClusterSummary := &configv1beta1.ClusterSummary{} err = c.Get(ctx, @@ -4744,6 +4749,17 @@ func updateValueHashOnHelmChartSummary(ctx context.Context, requestedChart *conf return err } + // Correct a Conflict left over from the start of this pass, but only while this + // ClusterSummary really is the chart's manager. deploySingleChart checks ownership + // before deploying; re-check it here, at the write, so the guarantee does not rest on + // that staying the only caller. Without the correction the entry latches: the feature + // ends Provisioned with an unchanged hash, so shouldRedeploy never runs the Helm + // handler again to recompute it. DryRun is excluded because it can pass the ownership + // check without ever registering, and it no-ops the two functions that otherwise + // maintain these entries. + isManager := dCtx.clusterSummary.Spec.ClusterProfileSpec.SyncMode != configv1beta1.SyncModeDryRun && + chartManager.CanManageChart(dCtx.clusterSummary, requestedChart) + for i := range currentClusterSummary.Status.HelmReleaseSummaries { rs := ¤tClusterSummary.Status.HelmReleaseSummaries[i] if rs.ReleaseName == requestedChart.ReleaseName && @@ -4752,6 +4768,10 @@ func updateValueHashOnHelmChartSummary(ctx context.Context, requestedChart *conf rs.ValuesHash = helmChartValuesHash rs.PatchesHash = helmChartPatchesHash rs.NeedsRedeploy = false + if isManager { + rs.Status = configv1beta1.HelmChartStatusManaging + rs.ConflictMessage = "" + } setResolvedHelmChartIdentity(ctx, c, dCtx.clusterSummary, rs, requestedChart, currentRelease, logger) } } diff --git a/controllers/handlers_helm_test.go b/controllers/handlers_helm_test.go index a56a54e6..73eaddcf 100644 --- a/controllers/handlers_helm_test.go +++ b/controllers/handlers_helm_test.go @@ -914,6 +914,204 @@ var _ = Describe("HandlersHelm", func() { Expect(bytes.Equal(updatedClusterSummary.Status.HelmReleaseSummaries[0].PatchesHash, stalePatchesHash)).To(BeTrue()) }) + It("UpdateValueHashOnHelmChartSummary clears a stale Conflict once this ClusterSummary owns the release", func() { + // Regression test for a latched conflict. buildReferencedHelmReleaseSummaries computes + // Status once, at the start of handleCharts. If the ClusterSummary holding the release + // unregisters after that write but before walkChartsAndDeploy re-checks ownership, this + // ClusterSummary deploys the chart while its entry still says Conflict. The pass then + // ends Provisioned with an unchanged hash, so shouldRedeploy never runs the Helm handler + // again and the stale entry stays forever. + nginxChart := &configv1beta1.HelmChart{ + RepositoryURL: testRepoURLNginxStable, + RepositoryName: testRepoNameNginxStable, + ChartName: testChartNameNginxIngress, + ChartVersion: testChartVersion100, + ReleaseName: testReleaseNameNginxLatest, + ReleaseNamespace: testNginxRepo, + HelmChartAction: configv1beta1.HelmChartActionInstall, + } + + clusterSummary.Spec.ClusterProfileSpec = configv1beta1.Spec{ + HelmCharts: []configv1beta1.HelmChart{*nginxChart}, + } + clusterSummary.Namespace = defaultNamespace + clusterSummary.Spec.ClusterNamespace = defaultNamespace + + Expect(testEnv.Create(context.TODO(), clusterSummary)).To(Succeed()) + Expect(waitForObject(context.TODO(), testEnv.Client, clusterSummary)).To(Succeed()) + + // The stale entry, naming a ClusterSummary that has since been deleted, plus a second + // release that is genuinely owned by someone else. Only the first may be touched. + otherConflictMessage := fmt.Sprintf("ClusterSummary %s managing it", randomString()) + clusterSummary.Status = configv1beta1.ClusterSummaryStatus{ + HelmReleaseSummaries: []configv1beta1.HelmChartSummary{ + { + ReleaseName: nginxChart.ReleaseName, + ReleaseNamespace: nginxChart.ReleaseNamespace, + Status: configv1beta1.HelmChartStatusConflict, + ConflictMessage: fmt.Sprintf("ClusterSummary %s managing it", randomString()), + }, + { + ReleaseName: testReleaseNameKyverno, + ReleaseNamespace: testReleaseNameKyverno, + Status: configv1beta1.HelmChartStatusConflict, + ConflictMessage: otherConflictMessage, + }, + }, + } + Expect(testEnv.Status().Update(context.TODO(), clusterSummary)).To(Succeed()) + + createClusterForClusterSummary(clusterSummary) + + // This ClusterSummary is the only registered manager, so it owns the release. + manager, err := chartmanager.GetChartManagerInstance(context.TODO(), testEnv.Client) + Expect(err).To(BeNil()) + manager.RegisterClusterSummaryForCharts(clusterSummary) + + dCtx := controllers.NewDeploymentContext(clusterSummary, nil, nil) + _, err = controllers.UpdateValueHashOnHelmChartSummary(context.TODO(), nginxChart, nil, dCtx, + textlogger.NewLogger(textlogger.NewConfig())) + Expect(err).To(BeNil()) + + current := readClusterSummaryUncached(clusterSummary) + + owned := findHelmChartSummary(current, nginxChart.ReleaseNamespace, nginxChart.ReleaseName) + Expect(owned).ToNot(BeNil()) + Expect(owned.Status).To(Equal(configv1beta1.HelmChartStatusManaging)) + Expect(owned.ConflictMessage).To(BeEmpty()) + + // The write must be scoped to the release that was deployed. + untouched := findHelmChartSummary(current, testReleaseNameKyverno, testReleaseNameKyverno) + Expect(untouched).ToNot(BeNil()) + Expect(untouched.Status).To(Equal(configv1beta1.HelmChartStatusConflict)) + Expect(untouched.ConflictMessage).To(Equal(otherConflictMessage)) + }) + + It("UpdateValueHashOnHelmChartSummary keeps the Conflict when another ClusterSummary owns the release", func() { + // The correction above is only safe while this ClusterSummary is the chart's manager. + // deploySingleChart gates on that before deploying, but the write re-checks it so the + // guarantee does not rest on that staying the only caller. + nginxChart := &configv1beta1.HelmChart{ + RepositoryURL: testRepoURLNginxStable, + RepositoryName: testRepoNameNginxStable, + ChartName: testChartNameNginxIngress, + ChartVersion: testChartVersion100, + ReleaseName: testReleaseNameNginxLatest, + ReleaseNamespace: testNginxRepo, + HelmChartAction: configv1beta1.HelmChartActionInstall, + } + + clusterSummary.Spec.ClusterProfileSpec = configv1beta1.Spec{ + HelmCharts: []configv1beta1.HelmChart{*nginxChart}, + } + clusterSummary.Namespace = defaultNamespace + clusterSummary.Spec.ClusterNamespace = defaultNamespace + + Expect(testEnv.Create(context.TODO(), clusterSummary)).To(Succeed()) + Expect(waitForObject(context.TODO(), testEnv.Client, clusterSummary)).To(Succeed()) + + conflictMessage := fmt.Sprintf("ClusterSummary %s managing it", randomString()) + clusterSummary.Status = configv1beta1.ClusterSummaryStatus{ + HelmReleaseSummaries: []configv1beta1.HelmChartSummary{ + { + ReleaseName: nginxChart.ReleaseName, + ReleaseNamespace: nginxChart.ReleaseNamespace, + Status: configv1beta1.HelmChartStatusConflict, + ConflictMessage: conflictMessage, + }, + }, + } + Expect(testEnv.Status().Update(context.TODO(), clusterSummary)).To(Succeed()) + + createClusterForClusterSummary(clusterSummary) + + // Register a different ClusterSummary first, so it holds the release and ours does not. + otherClusterSummary := &configv1beta1.ClusterSummary{ + ObjectMeta: metav1.ObjectMeta{ + Name: randomString(), + Namespace: clusterSummary.Namespace, + }, + Spec: configv1beta1.ClusterSummarySpec{ + ClusterNamespace: clusterSummary.Spec.ClusterNamespace, + ClusterName: clusterSummary.Spec.ClusterName, + ClusterType: clusterSummary.Spec.ClusterType, + ClusterProfileSpec: configv1beta1.Spec{HelmCharts: []configv1beta1.HelmChart{*nginxChart}}, + }, + } + manager, err := chartmanager.GetChartManagerInstance(context.TODO(), testEnv.Client) + Expect(err).To(BeNil()) + manager.RegisterClusterSummaryForCharts(otherClusterSummary) + manager.RegisterClusterSummaryForCharts(clusterSummary) + Expect(manager.CanManageChart(clusterSummary, nginxChart)).To(BeFalse()) + + dCtx := controllers.NewDeploymentContext(clusterSummary, nil, nil) + _, err = controllers.UpdateValueHashOnHelmChartSummary(context.TODO(), nginxChart, nil, dCtx, + textlogger.NewLogger(textlogger.NewConfig())) + Expect(err).To(BeNil()) + + current := readClusterSummaryUncached(clusterSummary) + summary := findHelmChartSummary(current, nginxChart.ReleaseNamespace, nginxChart.ReleaseName) + Expect(summary).ToNot(BeNil()) + Expect(summary.Status).To(Equal(configv1beta1.HelmChartStatusConflict)) + Expect(summary.ConflictMessage).To(Equal(conflictMessage)) + }) + + It("UpdateValueHashOnHelmChartSummary keeps the Conflict in DryRun mode", func() { + // DryRun can pass the ownership check without ever registering, and it no-ops the two + // functions that otherwise maintain these entries. It must change nothing here either. + nginxChart := &configv1beta1.HelmChart{ + RepositoryURL: testRepoURLNginxStable, + RepositoryName: testRepoNameNginxStable, + ChartName: testChartNameNginxIngress, + ChartVersion: testChartVersion100, + ReleaseName: testReleaseNameNginxLatest, + ReleaseNamespace: testNginxRepo, + HelmChartAction: configv1beta1.HelmChartActionInstall, + } + + clusterSummary.Spec.ClusterProfileSpec = configv1beta1.Spec{ + HelmCharts: []configv1beta1.HelmChart{*nginxChart}, + SyncMode: configv1beta1.SyncModeDryRun, + } + clusterSummary.Namespace = defaultNamespace + clusterSummary.Spec.ClusterNamespace = defaultNamespace + + Expect(testEnv.Create(context.TODO(), clusterSummary)).To(Succeed()) + Expect(waitForObject(context.TODO(), testEnv.Client, clusterSummary)).To(Succeed()) + + conflictMessage := fmt.Sprintf("ClusterSummary %s managing it", randomString()) + clusterSummary.Status = configv1beta1.ClusterSummaryStatus{ + HelmReleaseSummaries: []configv1beta1.HelmChartSummary{ + { + ReleaseName: nginxChart.ReleaseName, + ReleaseNamespace: nginxChart.ReleaseNamespace, + Status: configv1beta1.HelmChartStatusConflict, + ConflictMessage: conflictMessage, + }, + }, + } + Expect(testEnv.Status().Update(context.TODO(), clusterSummary)).To(Succeed()) + + createClusterForClusterSummary(clusterSummary) + + // Registered, so only the DryRun check can stop the write. + manager, err := chartmanager.GetChartManagerInstance(context.TODO(), testEnv.Client) + Expect(err).To(BeNil()) + manager.RegisterClusterSummaryForCharts(clusterSummary) + Expect(manager.CanManageChart(clusterSummary, nginxChart)).To(BeTrue()) + + dCtx := controllers.NewDeploymentContext(clusterSummary, nil, nil) + _, err = controllers.UpdateValueHashOnHelmChartSummary(context.TODO(), nginxChart, nil, dCtx, + textlogger.NewLogger(textlogger.NewConfig())) + Expect(err).To(BeNil()) + + current := readClusterSummaryUncached(clusterSummary) + summary := findHelmChartSummary(current, nginxChart.ReleaseNamespace, nginxChart.ReleaseName) + Expect(summary).ToNot(BeNil()) + Expect(summary.Status).To(Equal(configv1beta1.HelmChartStatusConflict)) + Expect(summary.ConflictMessage).To(Equal(conflictMessage)) + }) + It("updateStatusForeferencedHelmReleases is no-op in DryRun mode", func() { clusterSummary.Spec.ClusterProfileSpec = configv1beta1.Spec{ HelmCharts: []configv1beta1.HelmChart{ @@ -2836,3 +3034,44 @@ var _ = Describe("locateChartWithTimeout", func() { Expect(err).To(Equal(expectedErr)) }) }) + +// createClusterForClusterSummary creates the Cluster that clusterSummary points at. +// getHelmChartValuesHash resolves it via clusterproxy.GetCluster, so it must exist. +func createClusterForClusterSummary(clusterSummary *configv1beta1.ClusterSummary) { + cluster := &clusterv1.Cluster{ + ObjectMeta: metav1.ObjectMeta{ + Name: clusterSummary.Spec.ClusterName, + Namespace: clusterSummary.Spec.ClusterNamespace, + }, + } + Expect(testEnv.Create(context.TODO(), cluster)).To(Succeed()) + Expect(waitForObject(context.TODO(), testEnv.Client, cluster)).To(Succeed()) +} + +// readClusterSummaryUncached reads straight from the API server. testEnv.Client is cached and +// can still serve the pre-update object for a beat after a status write returns, which would +// make an assertion about a just-written value flaky. +func readClusterSummaryUncached(clusterSummary *configv1beta1.ClusterSummary) *configv1beta1.ClusterSummary { + uncached, err := client.New(testEnv.Config, client.Options{Scheme: scheme}) + Expect(err).To(BeNil()) + + current := &configv1beta1.ClusterSummary{} + Expect(uncached.Get(context.TODO(), + types.NamespacedName{Namespace: clusterSummary.Namespace, Name: clusterSummary.Name}, + current)).To(Succeed()) + + return current +} + +func findHelmChartSummary(clusterSummary *configv1beta1.ClusterSummary, + releaseNamespace, releaseName string) *configv1beta1.HelmChartSummary { + + for i := range clusterSummary.Status.HelmReleaseSummaries { + summary := &clusterSummary.Status.HelmReleaseSummaries[i] + if summary.ReleaseNamespace == releaseNamespace && summary.ReleaseName == releaseName { + return summary + } + } + + return nil +}