fix: clear stale Helm Conflict entry when ownership is won mid-deploy - #1975
Merged
gianlucam76 merged 1 commit intoSep 16, 2026
Merged
Conversation
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 projectsveltos#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.
prydie
force-pushed
the
fix/stale-helm-conflict-latch
branch
from
September 16, 2026 13:48
942b2eb to
f980aa5
Compare
gianlucam76
added a commit
that referenced
this pull request
Sep 17, 2026
…#1975) (#1980) 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. Co-authored-by: Andrew Pryde <andrew@rocketpod.co.uk>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
buildReferencedHelmReleaseSummariesis the only code that computes theStatusandConflictMessagefields ofStatus.HelmReleaseSummaries. It runs once per pass, fromupdateStatusForReferencedHelmReleasesat the start ofhandleCharts.walkChartsAndDeploythen checks ownership again, per chart, indeploySingleChart.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
Conflictand names a ClusterSummary that is already gone.Nothing later in the pass corrected that entry.
updateValueHashOnHelmChartSummarywroteValuesHash,PatchesHashandNeedsRedeploy, but notStatus.updateStatusForNonReferencedHelmReleasesre-reads the ClusterSummary and copies each referenced entry verbatim, overriding onlyFailureMessage.The pass ends with no error, so the feature goes to
Provisioned. The state is then permanent:shouldRedeployreturnsfalsefor a deployed feature whose hash has not changed, soproceedDeployingFeatureis never reached and the Helm handler never runs again to recompute the entry. The profile reports aConflictagainst a ClusterSummary that no longer exists, until an operator clears the feature summary by hand.We hit this in production on v1.13.0. A cluster moved between two Profiles that both declare the same chart. The chart installed correctly, but the new ClusterSummary sat
Provisionedwith a staleConflictfor more than 25 minutes. Both profiles were at the defaulttier: 100, andHasHigherOwnershipPriorityrequires a strictly lower tier to win, so the tier path could not have transferred ownership. Deregistration of the departing ClusterSummary was the only mechanism available: the profile stopped matching, the ClusterSummary was deleted, andpostFinalizerCleanupreacheddeleteChartMap->RemoveAllRegistrations.This is the sibling case of the race fixed in #1741. There the deploy fails with a
NonRetriableError, andhelmConflictResolvedrecovers it. Here the deploy succeeds, so there is no deployer error:proceedDeployingFeaturereturns on theProvisionedbranch before reaching thedeployerError != nilblock that guardshelmConflictResolved.The fix
updateValueHashOnHelmChartSummaryis the one status write reached per chart afterdeploySingleChartconfirmed ownership. It now resets the entry toManagingand clearsConflictMessage.The write is guarded by a fresh
chartManager.CanManageChartcheck rather than by the call site alone, so the correction cannot happen for a release this ClusterSummary does not own even if the function gains another caller. DryRun is excluded separately, because a DryRun ClusterSummary can pass the ownership check without ever registering, and DryRun no-ops bothupdateStatusForReferencedHelmReleasesandupdateStatusForNonReferencedHelmReleases.The correction depends on
updateStatusForNonReferencedHelmReleases, which runs afterwards and does its ownStatus().Update, to carry the value forward. It does, because it re-reads from the API server rather than using the in-memory copy.Alternatives considered. Correcting the entry in
deploySingleChartcosts an extra status write per chart, and an in-memory correction there is discarded by that same re-read. Re-runningupdateStatusForReferencedHelmReleasesat the end of the pass would re-resolve ownership for every chart and can itself transfer ownership throughdetermineChartOwnership.Tests
controllers/handlers_helm_test.go:Conflictonce this ClusterSummary owns the release, and leaves a second, genuinely conflicted release untouchedConflictwhen another ClusterSummary owns the releaseConflictin DryRun modecontrollers/clustersummary_deployer_test.go: a ClusterSummary that isProvisionedwith an unchanged hash submits no deploy, so a staleConflictis never recomputed. That test passes before and after this change. It is here to pin the latch itself, which is what makes a wrong entry at the end of a pass permanent rather than temporary.Fixes #1974