Skip to content

fix: clear stale Helm Conflict entry when ownership is won mid-deploy - #1975

Merged
gianlucam76 merged 1 commit into
projectsveltos:mainfrom
nscaledev:fix/stale-helm-conflict-latch
Sep 16, 2026
Merged

gianlucam76 merged 1 commit into
projectsveltos:mainfrom
nscaledev:fix/stale-helm-conflict-latch

Conversation

@prydie

@prydie prydie commented Sep 16, 2026

Copy link
Copy Markdown
Contributor

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.

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 Provisioned with a stale Conflict for more than 25 minutes. Both profiles were at the default tier: 100, and HasHigherOwnershipPriority requires 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, and postFinalizerCleanup reached deleteChartMap -> RemoveAllRegistrations.

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 there is no deployer error: proceedDeployingFeature returns on the Provisioned branch before reaching the deployerError != nil block that guards helmConflictResolved.

The fix

updateValueHashOnHelmChartSummary is the one status write reached per chart after deploySingleChart confirmed ownership. It now resets the entry to Managing and clears ConflictMessage.

The write is guarded by a fresh chartManager.CanManageChart check 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 both updateStatusForReferencedHelmReleases and updateStatusForNonReferencedHelmReleases.

The correction depends 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.

Alternatives considered. Correcting the entry in deploySingleChart costs an extra status write per chart, and an in-memory correction there is discarded by that same re-read. Re-running updateStatusForReferencedHelmReleases at the end of the pass would re-resolve ownership for every chart and can itself transfer ownership through determineChartOwnership.

Tests

controllers/handlers_helm_test.go:

  • clears a stale Conflict once this ClusterSummary owns the release, and leaves a second, genuinely conflicted release untouched
  • keeps the Conflict when another ClusterSummary owns the release
  • keeps the Conflict in DryRun mode

controllers/clustersummary_deployer_test.go: a ClusterSummary that is Provisioned with an unchanged hash submits no deploy, so a stale Conflict is 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

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
prydie force-pushed the fix/stale-helm-conflict-latch branch from 942b2eb to f980aa5 Compare September 16, 2026 13:48

@gianlucam76 gianlucam76 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. thank you

@gianlucam76
gianlucam76 merged commit 3b96646 into projectsveltos:main Sep 16, 2026
12 checks passed
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

BUG: ClusterSummary latches a stale Helm Conflict after ownership handover

2 participants