From 8a544ac47c3d56ca3993a14d014a79886eaee546 Mon Sep 17 00:00:00 2001 From: Gianluca Mardente Date: Tue, 18 Aug 2026 10:03:46 +0200 Subject: [PATCH] chore: Handle cluster not ready yet --- api/v1beta1/zz_generated.deepcopy.go | 5 +- controllers/clustersummary_controller.go | 131 +++++++++++------- controllers/clustersummary_controller_test.go | 42 ++++++ 3 files changed, 124 insertions(+), 54 deletions(-) diff --git a/api/v1beta1/zz_generated.deepcopy.go b/api/v1beta1/zz_generated.deepcopy.go index c6e84b84..ae5338bc 100644 --- a/api/v1beta1/zz_generated.deepcopy.go +++ b/api/v1beta1/zz_generated.deepcopy.go @@ -21,11 +21,12 @@ limitations under the License. package v1beta1 import ( - apiv1beta1 "github.com/projectsveltos/libsveltos/api/v1beta1" corev1 "k8s.io/api/core/v1" - "k8s.io/apimachinery/pkg/apis/meta/v1" + v1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/util/intstr" + + apiv1beta1 "github.com/projectsveltos/libsveltos/api/v1beta1" ) // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. diff --git a/controllers/clustersummary_controller.go b/controllers/clustersummary_controller.go index f2cac949..c5772ab8 100644 --- a/controllers/clustersummary_controller.go +++ b/controllers/clustersummary_controller.go @@ -338,58 +338,16 @@ func (r *ClusterSummaryReconciler) reconcileDelete( return reconcile.Result{Requeue: true, RequeueAfter: deleteRequeueAfter}, nil } if isPresent && isReady { // if cluster is not ready, do not try to clean up. It would fail. - // Cleanup - paused, err := r.isPaused(ctx, clusterSummaryScope.ClusterSummary) - if err != nil { - return reconcile.Result{}, err - } - if paused { - logger.V(logs.LogInfo).Info("cluster is paused. Do nothing.") - clusterSummaryScope.ClusterSummary.Status.ReconciliationSuspended = true - suspensionReason := clusterPausedMessage - clusterSummaryScope.ClusterSummary.Status.SuspensionReason = &suspensionReason - return reconcile.Result{}, nil - } - - clusterSummaryScope.ClusterSummary.Status.ReconciliationSuspended = false - clusterSummaryScope.ClusterSummary.Status.SuspensionReason = nil - - if !isDeleted { - // if cluster is marked for deletion do not try to remove ResourceSummaries. - // those are only deployed in the managed cluster so no need to cleanup on a deleted cluster - if r.anyFeatureNeedsResourceSummaryRemoval(clusterSummaryScope) { - err = r.removeResourceSummary(ctx, clusterSummaryScope, logger) - if err != nil { - logger.V(logs.LogInfo).Error(err, "failed to remove ResourceSummary.") - return reconcile.Result{Requeue: true, RequeueAfter: deleteRequeueAfter}, nil - } - r.markResourceSummaryRemovedForAllFeatures(clusterSummaryScope) - } - } - - // Mirror the dependsOn ordering enforced on deploy: do not undeploy a prerequisite - // while a dependent ClusterSummary for the same cluster still exists and still - // requires it. - allRemoved, dependentMsg, err := r.areDependentsRemoved(ctx, clusterSummaryScope, logger) - if err != nil { - return reconcile.Result{Requeue: true, RequeueAfter: deleteRequeueAfter}, nil - } - clusterSummaryScope.SetDependenciesMessage(&dependentMsg) - if !allRemoved { - return reconcile.Result{Requeue: true, RequeueAfter: deleteRequeueAfter}, nil - } - - // still call undeploy even if cluster is deleted. Sveltos might have deployed resources - // in the management cluster and those need to be removed. - err = r.undeploy(ctx, clusterSummaryScope, logger) - if err != nil { - return r.processUndeployError(clusterSummaryScope, err, logger) - } - - if !r.canRemoveFinalizer(ctx, clusterSummaryScope, logger) { - logger.V(logs.LogInfo).Error(err, "cannot remove finalizer yet") - return reconcile.Result{Requeue: true, RequeueAfter: deleteRequeueAfter}, nil - } + if result, cleanupErr, done := r.cleanupBeforeFinalizerRemoval(ctx, clusterSummaryScope, isDeleted, logger); done { + return result, cleanupErr + } + } else if isPresent && !isDeleted { + // Cluster exists, is not marked for deletion, but is not ready yet (e.g. a heartbeat + // gap). Cleanup was skipped above because it would fail against a cluster that is not + // ready. Wait for it rather than falling through to removeFinalizer below with nothing + // actually cleaned up - that would orphan whatever was previously deployed. + logger.V(logs.LogInfo).Info("cluster is not ready yet. Do not remove finalizer.") + return reconcile.Result{Requeue: true, RequeueAfter: deleteRequeueAfter}, nil } // If cluster is not present anymore or is it marked for deletion @@ -415,6 +373,75 @@ func (r *ClusterSummaryReconciler) reconcileDelete( return reconcile.Result{}, nil } +// cleanupBeforeFinalizerRemoval runs the delete-path cleanup for a cluster that is present and +// ready: honors a paused cluster, removes ResourceSummaries where appropriate, waits for +// dependent ClusterSummaries to be removed first, undeploys, and checks whether the finalizer +// can be removed yet. +// +// done is true whenever reconcileDelete should return (result, err) immediately as-is, whether +// that means stopping reconciliation (paused), requeuing to wait on some condition, or +// surfacing an undeploy error. done is false only once cleanup has fully succeeded, in which +// case reconcileDelete should continue on to remove the finalizer. +func (r *ClusterSummaryReconciler) cleanupBeforeFinalizerRemoval(ctx context.Context, + clusterSummaryScope *scope.ClusterSummaryScope, isDeleted bool, logger logr.Logger, +) (result reconcile.Result, err error, done bool) { + + paused, err := r.isPaused(ctx, clusterSummaryScope.ClusterSummary) + if err != nil { + return reconcile.Result{}, err, true + } + if paused { + logger.V(logs.LogInfo).Info("cluster is paused. Do nothing.") + clusterSummaryScope.ClusterSummary.Status.ReconciliationSuspended = true + suspensionReason := clusterPausedMessage + clusterSummaryScope.ClusterSummary.Status.SuspensionReason = &suspensionReason + return reconcile.Result{}, nil, true + } + + clusterSummaryScope.ClusterSummary.Status.ReconciliationSuspended = false + clusterSummaryScope.ClusterSummary.Status.SuspensionReason = nil + + if !isDeleted { + // if cluster is marked for deletion do not try to remove ResourceSummaries. + // those are only deployed in the managed cluster so no need to cleanup on a deleted cluster + if r.anyFeatureNeedsResourceSummaryRemoval(clusterSummaryScope) { + err = r.removeResourceSummary(ctx, clusterSummaryScope, logger) + if err != nil { + logger.V(logs.LogInfo).Error(err, "failed to remove ResourceSummary.") + return reconcile.Result{Requeue: true, RequeueAfter: deleteRequeueAfter}, nil, true + } + r.markResourceSummaryRemovedForAllFeatures(clusterSummaryScope) + } + } + + // Mirror the dependsOn ordering enforced on deploy: do not undeploy a prerequisite + // while a dependent ClusterSummary for the same cluster still exists and still + // requires it. + allRemoved, dependentMsg, err := r.areDependentsRemoved(ctx, clusterSummaryScope, logger) + if err != nil { + return reconcile.Result{Requeue: true, RequeueAfter: deleteRequeueAfter}, nil, true + } + clusterSummaryScope.SetDependenciesMessage(&dependentMsg) + if !allRemoved { + return reconcile.Result{Requeue: true, RequeueAfter: deleteRequeueAfter}, nil, true + } + + // still call undeploy even if cluster is deleted. Sveltos might have deployed resources + // in the management cluster and those need to be removed. + err = r.undeploy(ctx, clusterSummaryScope, logger) + if err != nil { + result, err = r.processUndeployError(clusterSummaryScope, err, logger) + return result, err, true + } + + if !r.canRemoveFinalizer(ctx, clusterSummaryScope, logger) { + logger.V(logs.LogInfo).Error(err, "cannot remove finalizer yet") + return reconcile.Result{Requeue: true, RequeueAfter: deleteRequeueAfter}, nil, true + } + + return reconcile.Result{}, nil, false +} + func (r *ClusterSummaryReconciler) postFinalizerCleanup(ctx context.Context, clusterSummaryScope *scope.ClusterSummaryScope, logger logr.Logger) error { diff --git a/controllers/clustersummary_controller_test.go b/controllers/clustersummary_controller_test.go index b85d3baa..64b9aecb 100644 --- a/controllers/clustersummary_controller_test.go +++ b/controllers/clustersummary_controller_test.go @@ -965,6 +965,48 @@ var _ = Describe("ClustersummaryController", func() { Expect(result.RequeueAfter).To(BeZero()) }) + It("reconcileDelete requeues and does not remove finalizer when cluster is present but not ready", func() { + controllerutil.AddFinalizer(clusterSummary, configv1beta1.ClusterSummaryFinalizer) + now := metav1.NewTime(time.Now()) + clusterSummary.DeletionTimestamp = &now + + // Cluster exists and is not marked for deletion, but is not ready yet. + notInitialized := false + cluster.Status.Initialization.ControlPlaneInitialized = ¬Initialized + + initObjects := []client.Object{ + clusterProfile, + clusterSummary, + cluster, + } + + c := fake.NewClientBuilder().WithScheme(scheme).WithStatusSubresource(initObjects...).WithObjects(initObjects...).Build() + + dep := fakedeployer.GetClient(context.TODO(), textlogger.NewLogger(textlogger.NewConfig()), c) + clusterSummaryReconciler := getClusterSummaryReconciler(c, dep) + + clusterSummaryScope, err := scope.NewClusterSummaryScope(&scope.ClusterSummaryScopeParams{ + Client: c, + Logger: textlogger.NewLogger(textlogger.NewConfig()), + ClusterSummary: clusterSummary, + ControllerName: testControllerNameSummary, + }) + Expect(err).To(BeNil()) + + result, err := controllers.ReconcileDelete(clusterSummaryReconciler, context.TODO(), clusterSummaryScope, + textlogger.NewLogger(textlogger.NewConfig())) + Expect(err).To(BeNil()) + Expect(result.RequeueAfter).ToNot(BeZero()) + + // Cleanup was skipped (cluster not ready) so the finalizer must still be there - removing + // it here would orphan whatever this ClusterSummary had deployed. + currentClusterSummary := &configv1beta1.ClusterSummary{} + Expect(c.Get(context.TODO(), + types.NamespacedName{Namespace: clusterSummary.Namespace, Name: clusterSummary.Name}, + currentClusterSummary)).To(Succeed()) + Expect(controllerutil.ContainsFinalizer(currentClusterSummary, configv1beta1.ClusterSummaryFinalizer)).To(BeTrue()) + }) + It("areDependenciesDeployed returns true when all dependencies are deployed", func() { clusterProfileAName := randomString() clusterSummaryAName := clusterops.GetClusterSummaryName(configv1beta1.ClusterProfileKind,