Skip to content

(fix) four ClusterSummary reconcile bugs - #1918

Open
gianlucam76 wants to merge 1 commit into
projectsveltos:mainfrom
gianlucam76:fixes
Open

(fix) four ClusterSummary reconcile bugs#1918
gianlucam76 wants to merge 1 commit into
projectsveltos:mainfrom
gianlucam76:fixes

Conversation

@gianlucam76

Copy link
Copy Markdown
Member

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. 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

…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. 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
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.

1 participant