(fix) four ClusterSummary reconcile bugs - #1918
Merged
Merged
Conversation
gianlucam76
force-pushed
the
fixes
branch
2 times, most recently
from
August 16, 2026 07:46
6d5a51b to
bdc812e
Compare
…ting Four separate bug fixes: 1. isClusterPresent: transient GetCluster errors treated as "cluster present" isClusterPresent returned early on apierrors.IsNotFound, but any other error (a transient timeout, an API server hiccup) fell through to `return true, !cluster.GetDeletionTimestamp().IsZero(), err` with cluster still nil - reporting present=true alongside a non-nil error. The caller (reconcileDelete) then silently requeues forever on that error without ever reaching handleDeletedCluster/removeFinalizer, so a ClusterSummary whose SveltosCluster is actually gone can get stuck Terminating indefinitely if a single transient error hits at the wrong moment. Now returns (false, false, err) on any non-NotFound error instead. 2. removeStaleResourceSummary: one cluster's ResourceSummary blocked another's cleanup. The ResourceSummary List is scoped only by ClusterNameLabel/ClusterTypeLabel, not namespace so multiple clusters sharing the same name across different namespaces (a normal setup) all come back in one List. The function correctly filtered which ones to delete by namespace, but the "still present" gate checked the length of the unfiltered list, so an unrelated cluster's still-finalizing ResourceSummary permanently blocked this cluster's own cleanup. Now counts only the entries that actually match this cluster's namespace. 3. LocateChart: no timeout on the chart download call Unlike repo.ChartRepository.DownloadIndexFile a few lines away (already wrapped in a goroutine+timeout because the Helm SDK's repo APIs take no context.Context), action.Install/Upgrade's LocateChart was called directly, twice, with no timeout at all. A repository that accepts the connection but never completes the response wedges the deployer worker processing that request forever. The request only leaves the deployer's inProgress tracking once the handler call returns, so a call that never returns permanently occupies one of the worker pool's fixed slots, and its ClusterSummary/feature is stuck reporting "still being provisioned" with no further retries, indefinitely. Wrapped LocateChart in the same goroutine+channel+90s-timeout pattern as DownloadIndexFile. 4. dependsOn only protected deploy order, not deletion prepareForDeployment already blocks a dependent from deploying until its prerequisites are Provisioned (areDependenciesDeployed), but nothing enforced the reverse on delete: a prerequisite (e.g. cert-manager) and its dependent (e.g. kyverno, via dependsOn) would undeploy concurrently as soon as their own ClusterProfiles were deleted, in random order. Added areDependentsRemoved: a ClusterSummary's undeploy is now blocked while another ClusterSummary for the same cluster still exists and still lists this profile in its own dependsOn - dependsOn is treated as a two-way contract (gates deploy readiness AND protects against deletion while still needed), the same pattern Kubernetes itself uses for a CRD with live CRs or a PVC still mounted by a pod. The blocked ClusterSummary surfaces why via Status.Dependencies.
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
1 participant
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.
Four separate bug fixes:
isClusterPresent: transient GetCluster errors treated as "cluster present" isClusterPresent returned early on apierrors.IsNotFound, but any other error (a transient timeout, an API server hiccup) fell through to
return true, !cluster.GetDeletionTimestamp().IsZero(), errwith cluster still nil - reporting present=true alongside a non-nil error. The caller (reconcileDelete) then silently requeues forever on that error without ever reaching handleDeletedCluster/removeFinalizer, so a ClusterSummary whose SveltosCluster is actually gone can get stuck Terminating indefinitely if a single transient error hits at the wrong moment. Now returns (false, false, err) on any non-NotFound error instead.removeStaleResourceSummary: one cluster's ResourceSummary blocked another's cleanup. The ResourceSummary List is scoped only by ClusterNameLabel/ClusterTypeLabel, not namespace so multiple clusters sharing the same name across different namespaces (a normal setup) all come back in one List. The function correctly filtered which ones to delete by namespace, but the "still present" gate checked the length of the unfiltered list, so an unrelated cluster's still-finalizing ResourceSummary permanently blocked this cluster's own cleanup. Now counts only the entries that actually match this cluster's namespace.
LocateChart: no timeout on the chart download call Unlike repo.ChartRepository.DownloadIndexFile a few lines away (already wrapped in a goroutine+timeout because the Helm SDK's repo APIs take no context.Context), action.Install/Upgrade's LocateChart was called directly, twice, with no timeout at all. A repository that accepts the connection but never completes the response wedges the deployer worker processing that request forever. The request only leaves the deployer's inProgress tracking once the handler call returns, so a call that never returns permanently occupies one of the worker pool's fixed slots, and its ClusterSummary/feature is stuck reporting "still being provisioned" with no further retries, indefinitely. Wrapped LocateChart in the same goroutine+channel+90s-timeout pattern as DownloadIndexFile.
Deletion doesn't respect dependsOn ordering prepareForDeployment already blocks a dependent from deploying until its prerequisites are Provisioned (areDependenciesDeployed), but nothing enforced the reverse on delete: a prerequisite (e.g. cert-manager) and its dependent (e.g. kyverno, via dependsOn) undeploy concurrently as soon as their own ClusterProfiles are deleted, in random order - reproducible any time both ClusterProfiles independently match the same cluster