From 8d9af52bb982d36a9c6fd4447b7350e7dcbff0a5 Mon Sep 17 00:00:00 2001 From: Dhairya-Arora01 Date: Mon, 24 Jun 2024 22:53:49 +0530 Subject: [PATCH 01/17] foreground deletion for machine deployments --- api/v1beta1/machinedeployment_types.go | 4 + api/v1beta1/machineset_types.go | 4 + .../machinedeployment_controller.go | 44 +++++- .../machineset/machineset_controller.go | 136 ++++++++++++------ 4 files changed, 141 insertions(+), 47 deletions(-) diff --git a/api/v1beta1/machinedeployment_types.go b/api/v1beta1/machinedeployment_types.go index 1e4b1c6b0ded..b67f48d5dd59 100644 --- a/api/v1beta1/machinedeployment_types.go +++ b/api/v1beta1/machinedeployment_types.go @@ -25,6 +25,10 @@ const ( // MachineDeploymentTopologyFinalizer is the finalizer used by the topology MachineDeployment controller to // clean up referenced template resources if necessary when a MachineDeployment is being deleted. MachineDeploymentTopologyFinalizer = "machinedeployment.topology.cluster.x-k8s.io" + + // MachineDeploymentFinalizer is the finalizer used by the MachineDeployment controller to + // cleanup the MachineDeployment descendant MachineSets when a MachineDeployment is being deleted. + MachineDeploymentFinalizer = "machinedeployment.cluster.x-k8s.io" ) // MachineDeploymentStrategyType defines the type of MachineDeployment rollout strategies. diff --git a/api/v1beta1/machineset_types.go b/api/v1beta1/machineset_types.go index f10e44f4f28d..d579168c9252 100644 --- a/api/v1beta1/machineset_types.go +++ b/api/v1beta1/machineset_types.go @@ -29,6 +29,10 @@ const ( // MachineSetTopologyFinalizer is the finalizer used by the topology MachineDeployment controller to // clean up referenced template resources if necessary when a MachineSet is being deleted. MachineSetTopologyFinalizer = "machineset.topology.cluster.x-k8s.io" + + // MachineSetFinalizer is the finalizer used by the MachineSet controller to + // cleanup the MachineSet descendant Machines when a Machineset is being deleted. + MachineSetFinalizer = "machineset.cluster.x-k8s.io" ) // ANCHOR: MachineSetSpec diff --git a/internal/controllers/machinedeployment/machinedeployment_controller.go b/internal/controllers/machinedeployment/machinedeployment_controller.go index f9b55db02c4c..aafa17398470 100644 --- a/internal/controllers/machinedeployment/machinedeployment_controller.go +++ b/internal/controllers/machinedeployment/machinedeployment_controller.go @@ -20,6 +20,7 @@ import ( "context" "fmt" "strings" + "time" "github.com/pkg/errors" corev1 "k8s.io/api/core/v1" @@ -33,7 +34,9 @@ import ( "sigs.k8s.io/controller-runtime/pkg/builder" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/controller" + "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" "sigs.k8s.io/controller-runtime/pkg/handler" + "sigs.k8s.io/controller-runtime/pkg/reconcile" clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" "sigs.k8s.io/cluster-api/controllers/external" @@ -51,6 +54,10 @@ var ( machineDeploymentKind = clusterv1.GroupVersion.WithKind("MachineDeployment") ) +// deleteRequeueAfter is how long to wait before checking again to see if the MachineDeployment +// still has owned MachineSets. +const deleteRequeueAfter = 5 * time.Second + // machineDeploymentManagerName is the manager name used for Server-Side-Apply (SSA) operations // in the MachineDeployment controller. const machineDeploymentManagerName = "capi-machinedeployment" @@ -158,9 +165,15 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (_ ctrl.Re } }() - // Ignore deleted MachineDeployments, this can happen when foregroundDeletion - // is enabled + // Handle deletion reconciliation loop. if !deployment.DeletionTimestamp.IsZero() { + return r.reconcileDelete(ctx, deployment) + } + + // Add finalizer first if not set to avoid the race condition between init and delete. + // Note: Finalizers in general can only be added when the deletionTimestamp is not set. + if !controllerutil.ContainsFinalizer(deployment, clusterv1.MachineDeploymentFinalizer) { + controllerutil.AddFinalizer(deployment, clusterv1.MachineDeploymentFinalizer) return ctrl.Result{}, nil } @@ -286,6 +299,33 @@ func (r *Reconciler) reconcile(ctx context.Context, cluster *clusterv1.Cluster, return errors.Errorf("unexpected deployment strategy type: %s", md.Spec.Strategy.Type) } +func (r *Reconciler) reconcileDelete(ctx context.Context, md *clusterv1.MachineDeployment) (reconcile.Result, error) { + log := ctrl.LoggerFrom(ctx) + msList, err := r.getMachineSetsForDeployment(ctx, md) + if err != nil { + return ctrl.Result{}, err + } + + // If all the descendant machinesets are deleted, then remove the machinedeployment's finalizer. + if len(msList) == 0 { + controllerutil.RemoveFinalizer(md, clusterv1.MachineDeploymentFinalizer) + return ctrl.Result{}, nil + } + + // else delete owned machinesets. + log.Info("MachineDeployment still has owned MachineSets, deleting them first") + for _, ms := range msList { + if ms.DeletionTimestamp.IsZero() { + log.Info("Deleting MachineSet", "MachineSet", klog.KObj(ms)) + if err := r.Client.Delete(ctx, ms); err != nil { + return ctrl.Result{}, errors.Wrapf(err, "failed to delete MachineSet %s", klog.KObj(ms)) + } + } + } + + return ctrl.Result{RequeueAfter: deleteRequeueAfter}, nil +} + // getMachineSetsForDeployment returns a list of MachineSets associated with a MachineDeployment. func (r *Reconciler) getMachineSetsForDeployment(ctx context.Context, md *clusterv1.MachineDeployment) ([]*clusterv1.MachineSet, error) { log := ctrl.LoggerFrom(ctx) diff --git a/internal/controllers/machineset/machineset_controller.go b/internal/controllers/machineset/machineset_controller.go index 360616e58412..8a34006b772b 100644 --- a/internal/controllers/machineset/machineset_controller.go +++ b/internal/controllers/machineset/machineset_controller.go @@ -41,7 +41,9 @@ import ( "sigs.k8s.io/controller-runtime/pkg/builder" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/controller" + "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" "sigs.k8s.io/controller-runtime/pkg/handler" + "sigs.k8s.io/controller-runtime/pkg/reconcile" clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" "sigs.k8s.io/cluster-api/controllers/external" @@ -73,6 +75,10 @@ var ( stateConfirmationInterval = 100 * time.Millisecond ) +// deleteRequeueAfter is how long to wait before checking again to see if the MachineDeployment +// still has owned MachineSets. +const deleteRequeueAfter = 5 * time.Second + const machineSetManagerName = "capi-machineset" // Update permissions on /finalizers subresrouce is required on management clusters with 'OwnerReferencesPermissionEnforcement' plugin enabled. @@ -182,9 +188,15 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (_ ctrl.Re } }() - // Ignore deleted MachineSets, this can happen when foregroundDeletion - // is enabled + // Handle deletion reconciliation loop. if !machineSet.DeletionTimestamp.IsZero() { + return r.reconcileDelete(ctx, machineSet) + } + + // Add finalizer first if not set to avoid the race condition between init and delete. + // Note: Finalizers in general can only be added when the deletionTimestamp is not set. + if !controllerutil.ContainsFinalizer(machineSet, clusterv1.MachineSetFinalizer) { + controllerutil.AddFinalizer(machineSet, clusterv1.MachineSetFinalizer) return ctrl.Result{}, nil } @@ -224,8 +236,6 @@ func patchMachineSet(ctx context.Context, patchHelper *patch.Helper, machineSet } func (r *Reconciler) reconcile(ctx context.Context, cluster *clusterv1.Cluster, machineSet *clusterv1.MachineSet) (ctrl.Result, error) { - log := ctrl.LoggerFrom(ctx) - // Reconcile and retrieve the Cluster object. if machineSet.Labels == nil { machineSet.Labels = make(map[string]string) @@ -266,63 +276,28 @@ func (r *Reconciler) reconcile(ctx context.Context, cluster *clusterv1.Cluster, machineSet.Spec.Selector.MatchLabels[clusterv1.ClusterNameLabel] = machineSet.Spec.ClusterName machineSet.Spec.Template.Labels[clusterv1.ClusterNameLabel] = machineSet.Spec.ClusterName - selectorMap, err := metav1.LabelSelectorAsMap(&machineSet.Spec.Selector) - if err != nil { - return ctrl.Result{}, errors.Wrapf(err, "failed to convert MachineSet %q label selector to a map", machineSet.Name) - } - - // Get all Machines linked to this MachineSet. - allMachines := &clusterv1.MachineList{} - err = r.Client.List(ctx, - allMachines, - client.InNamespace(machineSet.Namespace), - client.MatchingLabels(selectorMap), - ) + machines, err := r.getMachinesForMachineSet(ctx, machineSet) if err != nil { - return ctrl.Result{}, errors.Wrap(err, "failed to list machines") - } - - // Filter out irrelevant machines (i.e. IsControlledBy something else) and claim orphaned machines. - // Machines in deleted state are deliberately not excluded /~https://github.com/kubernetes-sigs/cluster-api/pull/3434. - filteredMachines := make([]*clusterv1.Machine, 0, len(allMachines.Items)) - for idx := range allMachines.Items { - machine := &allMachines.Items[idx] - log := log.WithValues("Machine", klog.KObj(machine)) - if shouldExcludeMachine(machineSet, machine) { - continue - } - - // Attempt to adopt machine if it meets previous conditions and it has no controller references. - if metav1.GetControllerOf(machine) == nil { - if err := r.adoptOrphan(ctx, machineSet, machine); err != nil { - log.Error(err, "Failed to adopt Machine") - r.recorder.Eventf(machineSet, corev1.EventTypeWarning, "FailedAdopt", "Failed to adopt Machine %q: %v", machine.Name, err) - continue - } - log.Info("Adopted Machine") - r.recorder.Eventf(machineSet, corev1.EventTypeNormal, "SuccessfulAdopt", "Adopted Machine %q", machine.Name) - } - - filteredMachines = append(filteredMachines, machine) + return ctrl.Result{}, errors.Wrap(err, "failed to list Machines") } result := ctrl.Result{} - reconcileUnhealthyMachinesResult, err := r.reconcileUnhealthyMachines(ctx, cluster, machineSet, filteredMachines) + reconcileUnhealthyMachinesResult, err := r.reconcileUnhealthyMachines(ctx, cluster, machineSet, machines) if err != nil { return ctrl.Result{}, errors.Wrap(err, "failed to reconcile unhealthy machines") } result = util.LowestNonZeroResult(result, reconcileUnhealthyMachinesResult) - if err := r.syncMachines(ctx, machineSet, filteredMachines); err != nil { + if err := r.syncMachines(ctx, machineSet, machines); err != nil { return ctrl.Result{}, errors.Wrap(err, "failed to update Machines") } - syncReplicasResult, syncErr := r.syncReplicas(ctx, cluster, machineSet, filteredMachines) + syncReplicasResult, syncErr := r.syncReplicas(ctx, cluster, machineSet, machines) result = util.LowestNonZeroResult(result, syncReplicasResult) // Always updates status as machines come up or die. - if err := r.updateStatus(ctx, cluster, machineSet, filteredMachines); err != nil { + if err := r.updateStatus(ctx, cluster, machineSet, machines); err != nil { return ctrl.Result{}, errors.Wrapf(kerrors.NewAggregate([]error{err, syncErr}), "failed to update MachineSet's Status") } @@ -359,6 +334,77 @@ func (r *Reconciler) reconcile(ctx context.Context, cluster *clusterv1.Cluster, return result, nil } +func (r *Reconciler) reconcileDelete(ctx context.Context, machineSet *clusterv1.MachineSet) (reconcile.Result, error) { + log := ctrl.LoggerFrom(ctx) + machineList, err := r.getMachinesForMachineSet(ctx, machineSet) + if err != nil { + return ctrl.Result{}, err + } + + // If all the descendant machines are deleted, then remove the machinedeployment's finalizer. + if len(machineList) == 0 { + controllerutil.RemoveFinalizer(machineSet, clusterv1.MachineSetFinalizer) + return ctrl.Result{}, nil + } + + log.Info("MachineSet still has owned Machines, deleting them first") + for _, machine := range machineList { + if machine.DeletionTimestamp.IsZero() { + log.Info("Deleting Machine", "Machine", klog.KObj(machine)) + if err := r.Client.Delete(ctx, machine); err != nil { + return ctrl.Result{}, errors.Wrapf(err, "failed to delete Machine %s", klog.KObj(machine)) + } + } + } + + return ctrl.Result{RequeueAfter: deleteRequeueAfter}, nil +} + +func (r *Reconciler) getMachinesForMachineSet(ctx context.Context, machineSet *clusterv1.MachineSet) ([]*clusterv1.Machine, error) { + log := ctrl.LoggerFrom(ctx) + selectorMap, err := metav1.LabelSelectorAsMap(&machineSet.Spec.Selector) + if err != nil { + return nil, errors.Wrapf(err, "failed to convert MachineSet %q label selector to a map", machineSet.Name) + } + + // Get all Machines linked to this MachineSet. + allMachines := &clusterv1.MachineList{} + err = r.Client.List(ctx, + allMachines, + client.InNamespace(machineSet.Namespace), + client.MatchingLabels(selectorMap), + ) + if err != nil { + return nil, errors.Wrap(err, "failed to list machines") + } + + // Filter out irrelevant machines (i.e. IsControlledBy something else) and claim orphaned machines. + // Machines in deleted state are deliberately not excluded /~https://github.com/kubernetes-sigs/cluster-api/pull/3434. + filteredMachines := make([]*clusterv1.Machine, 0, len(allMachines.Items)) + for idx := range allMachines.Items { + machine := &allMachines.Items[idx] + log := log.WithValues("Machine", klog.KObj(machine)) + if shouldExcludeMachine(machineSet, machine) { + continue + } + + // Attempt to adopt machine if it meets previous conditions and it has no controller references. + if metav1.GetControllerOf(machine) == nil { + if err := r.adoptOrphan(ctx, machineSet, machine); err != nil { + log.Error(err, "Failed to adopt Machine") + r.recorder.Eventf(machineSet, corev1.EventTypeWarning, "FailedAdopt", "Failed to adopt Machine %q: %v", machine.Name, err) + continue + } + log.Info("Adopted Machine") + r.recorder.Eventf(machineSet, corev1.EventTypeNormal, "SuccessfulAdopt", "Adopted Machine %q", machine.Name) + } + + filteredMachines = append(filteredMachines, machine) + } + + return filteredMachines, nil +} + // syncMachines updates Machines, InfrastructureMachine and BootstrapConfig to propagate in-place mutable fields // from the MachineSet. // Note: It also cleans up managed fields of all Machines so that Machines that were From d28ff9dd1fb3570d0e9fde5096cb7224776445f1 Mon Sep 17 00:00:00 2001 From: Christian Schlotter Date: Wed, 11 Sep 2024 17:21:47 +0200 Subject: [PATCH 02/17] review fixes --- api/v1beta1/machinedeployment_types.go | 2 +- api/v1beta1/machineset_types.go | 2 +- .../machinedeployment_controller.go | 34 +++++++++------ .../machineset/machineset_controller.go | 42 ++++++++++++------- 4 files changed, 49 insertions(+), 31 deletions(-) diff --git a/api/v1beta1/machinedeployment_types.go b/api/v1beta1/machinedeployment_types.go index b67f48d5dd59..c2f1d38bf8a5 100644 --- a/api/v1beta1/machinedeployment_types.go +++ b/api/v1beta1/machinedeployment_types.go @@ -28,7 +28,7 @@ const ( // MachineDeploymentFinalizer is the finalizer used by the MachineDeployment controller to // cleanup the MachineDeployment descendant MachineSets when a MachineDeployment is being deleted. - MachineDeploymentFinalizer = "machinedeployment.cluster.x-k8s.io" + MachineDeploymentFinalizer = "cluster.x-k8s.io/machinedeployment" ) // MachineDeploymentStrategyType defines the type of MachineDeployment rollout strategies. diff --git a/api/v1beta1/machineset_types.go b/api/v1beta1/machineset_types.go index d579168c9252..37ce00076904 100644 --- a/api/v1beta1/machineset_types.go +++ b/api/v1beta1/machineset_types.go @@ -32,7 +32,7 @@ const ( // MachineSetFinalizer is the finalizer used by the MachineSet controller to // cleanup the MachineSet descendant Machines when a Machineset is being deleted. - MachineSetFinalizer = "machineset.cluster.x-k8s.io" + MachineSetFinalizer = "cluster.x-k8s.io/machineset" ) // ANCHOR: MachineSetSpec diff --git a/internal/controllers/machinedeployment/machinedeployment_controller.go b/internal/controllers/machinedeployment/machinedeployment_controller.go index aafa17398470..ee8ba0a67bcb 100644 --- a/internal/controllers/machinedeployment/machinedeployment_controller.go +++ b/internal/controllers/machinedeployment/machinedeployment_controller.go @@ -20,7 +20,6 @@ import ( "context" "fmt" "strings" - "time" "github.com/pkg/errors" corev1 "k8s.io/api/core/v1" @@ -36,7 +35,6 @@ import ( "sigs.k8s.io/controller-runtime/pkg/controller" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" "sigs.k8s.io/controller-runtime/pkg/handler" - "sigs.k8s.io/controller-runtime/pkg/reconcile" clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" "sigs.k8s.io/cluster-api/controllers/external" @@ -54,10 +52,6 @@ var ( machineDeploymentKind = clusterv1.GroupVersion.WithKind("MachineDeployment") ) -// deleteRequeueAfter is how long to wait before checking again to see if the MachineDeployment -// still has owned MachineSets. -const deleteRequeueAfter = 5 * time.Second - // machineDeploymentManagerName is the manager name used for Server-Side-Apply (SSA) operations // in the MachineDeployment controller. const machineDeploymentManagerName = "capi-machinedeployment" @@ -167,7 +161,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (_ ctrl.Re // Handle deletion reconciliation loop. if !deployment.DeletionTimestamp.IsZero() { - return r.reconcileDelete(ctx, deployment) + return ctrl.Result{}, r.reconcileDelete(ctx, deployment) } // Add finalizer first if not set to avoid the race condition between init and delete. @@ -299,31 +293,32 @@ func (r *Reconciler) reconcile(ctx context.Context, cluster *clusterv1.Cluster, return errors.Errorf("unexpected deployment strategy type: %s", md.Spec.Strategy.Type) } -func (r *Reconciler) reconcileDelete(ctx context.Context, md *clusterv1.MachineDeployment) (reconcile.Result, error) { +func (r *Reconciler) reconcileDelete(ctx context.Context, md *clusterv1.MachineDeployment) error { log := ctrl.LoggerFrom(ctx) msList, err := r.getMachineSetsForDeployment(ctx, md) if err != nil { - return ctrl.Result{}, err + return err } // If all the descendant machinesets are deleted, then remove the machinedeployment's finalizer. if len(msList) == 0 { controllerutil.RemoveFinalizer(md, clusterv1.MachineDeploymentFinalizer) - return ctrl.Result{}, nil + return nil } + log.Info("MachineDeployment still has descendant MachineSets - deleting them first", "count", len(msList), "descendants", descendantMachineSets(msList)) + // else delete owned machinesets. - log.Info("MachineDeployment still has owned MachineSets, deleting them first") for _, ms := range msList { if ms.DeletionTimestamp.IsZero() { log.Info("Deleting MachineSet", "MachineSet", klog.KObj(ms)) if err := r.Client.Delete(ctx, ms); err != nil { - return ctrl.Result{}, errors.Wrapf(err, "failed to delete MachineSet %s", klog.KObj(ms)) + return errors.Wrapf(err, "failed to delete MachineSet %s", klog.KObj(ms)) } } } - return ctrl.Result{RequeueAfter: deleteRequeueAfter}, nil + return nil } // getMachineSetsForDeployment returns a list of MachineSets associated with a MachineDeployment. @@ -479,3 +474,16 @@ func reconcileExternalTemplateReference(ctx context.Context, c client.Client, cl return patchHelper.Patch(ctx, obj) } + +func descendantMachineSets(objs []*clusterv1.MachineSet) string { + objNames := make([]string, len(objs)) + for _, obj := range objs { + objNames = append(objNames, obj.GetName()) + } + + if len(objNames) > 10 { + objNames = append(objNames[:10], "...") + } + + return strings.Join(objNames, ",") +} diff --git a/internal/controllers/machineset/machineset_controller.go b/internal/controllers/machineset/machineset_controller.go index 8a34006b772b..837582850fc7 100644 --- a/internal/controllers/machineset/machineset_controller.go +++ b/internal/controllers/machineset/machineset_controller.go @@ -43,7 +43,6 @@ import ( "sigs.k8s.io/controller-runtime/pkg/controller" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" "sigs.k8s.io/controller-runtime/pkg/handler" - "sigs.k8s.io/controller-runtime/pkg/reconcile" clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" "sigs.k8s.io/cluster-api/controllers/external" @@ -75,10 +74,6 @@ var ( stateConfirmationInterval = 100 * time.Millisecond ) -// deleteRequeueAfter is how long to wait before checking again to see if the MachineDeployment -// still has owned MachineSets. -const deleteRequeueAfter = 5 * time.Second - const machineSetManagerName = "capi-machineset" // Update permissions on /finalizers subresrouce is required on management clusters with 'OwnerReferencesPermissionEnforcement' plugin enabled. @@ -190,7 +185,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (_ ctrl.Re // Handle deletion reconciliation loop. if !machineSet.DeletionTimestamp.IsZero() { - return r.reconcileDelete(ctx, machineSet) + return ctrl.Result{}, r.reconcileDelete(ctx, machineSet) } // Add finalizer first if not set to avoid the race condition between init and delete. @@ -276,7 +271,7 @@ func (r *Reconciler) reconcile(ctx context.Context, cluster *clusterv1.Cluster, machineSet.Spec.Selector.MatchLabels[clusterv1.ClusterNameLabel] = machineSet.Spec.ClusterName machineSet.Spec.Template.Labels[clusterv1.ClusterNameLabel] = machineSet.Spec.ClusterName - machines, err := r.getMachinesForMachineSet(ctx, machineSet) + machines, err := r.getAndAdoptMachinesForMachineSet(ctx, machineSet) if err != nil { return ctrl.Result{}, errors.Wrap(err, "failed to list Machines") } @@ -334,33 +329,35 @@ func (r *Reconciler) reconcile(ctx context.Context, cluster *clusterv1.Cluster, return result, nil } -func (r *Reconciler) reconcileDelete(ctx context.Context, machineSet *clusterv1.MachineSet) (reconcile.Result, error) { +func (r *Reconciler) reconcileDelete(ctx context.Context, machineSet *clusterv1.MachineSet) error { log := ctrl.LoggerFrom(ctx) - machineList, err := r.getMachinesForMachineSet(ctx, machineSet) + machineList, err := r.getAndAdoptMachinesForMachineSet(ctx, machineSet) if err != nil { - return ctrl.Result{}, err + return err } - // If all the descendant machines are deleted, then remove the machinedeployment's finalizer. + // If all the descendant machines are deleted, then remove the machineset's finalizer. if len(machineList) == 0 { controllerutil.RemoveFinalizer(machineSet, clusterv1.MachineSetFinalizer) - return ctrl.Result{}, nil + return nil } - log.Info("MachineSet still has owned Machines, deleting them first") + log.Info("MachineSet still has descendant Machines - deleting them first", "count", len(machineList), "descendants", descendantMachines(machineList)) + + // else delete owned machines. for _, machine := range machineList { if machine.DeletionTimestamp.IsZero() { log.Info("Deleting Machine", "Machine", klog.KObj(machine)) if err := r.Client.Delete(ctx, machine); err != nil { - return ctrl.Result{}, errors.Wrapf(err, "failed to delete Machine %s", klog.KObj(machine)) + return errors.Wrapf(err, "failed to delete Machine %s", klog.KObj(machine)) } } } - return ctrl.Result{RequeueAfter: deleteRequeueAfter}, nil + return nil } -func (r *Reconciler) getMachinesForMachineSet(ctx context.Context, machineSet *clusterv1.MachineSet) ([]*clusterv1.Machine, error) { +func (r *Reconciler) getAndAdoptMachinesForMachineSet(ctx context.Context, machineSet *clusterv1.MachineSet) ([]*clusterv1.Machine, error) { log := ctrl.LoggerFrom(ctx) selectorMap, err := metav1.LabelSelectorAsMap(&machineSet.Spec.Selector) if err != nil { @@ -1230,3 +1227,16 @@ func (r *Reconciler) reconcileExternalTemplateReference(ctx context.Context, clu return patchHelper.Patch(ctx, obj) } + +func descendantMachines(objs []*clusterv1.Machine) string { + objNames := make([]string, len(objs)) + for _, obj := range objs { + objNames = append(objNames, obj.GetName()) + } + + if len(objNames) > 10 { + objNames = append(objNames[:10], "...") + } + + return strings.Join(objNames, ",") +} From 1c47843ba8ee6cfd8c4dd8f8121b0c47aea85b5f Mon Sep 17 00:00:00 2001 From: Christian Schlotter Date: Wed, 11 Sep 2024 17:22:05 +0200 Subject: [PATCH 03/17] fix unit tests --- .../controllers/machineset/machineset_controller_test.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/internal/controllers/machineset/machineset_controller_test.go b/internal/controllers/machineset/machineset_controller_test.go index 4656d208f368..484f8449c422 100644 --- a/internal/controllers/machineset/machineset_controller_test.go +++ b/internal/controllers/machineset/machineset_controller_test.go @@ -890,6 +890,9 @@ func newMachineSet(name, cluster string, replicas int32) *clusterv1.MachineSet { ObjectMeta: metav1.ObjectMeta{ Name: name, Namespace: metav1.NamespaceDefault, + Finalizers: []string{ + clusterv1.MachineSetFinalizer, + }, Labels: map[string]string{ clusterv1.ClusterNameLabel: cluster, }, @@ -931,6 +934,9 @@ func TestMachineSetReconcile_MachinesCreatedConditionFalseOnBadInfraRef(t *testi Labels: map[string]string{ clusterv1.ClusterNameLabel: cluster.Name, }, + Finalizers: []string{ + clusterv1.MachineSetFinalizer, + }, }, Spec: clusterv1.MachineSetSpec{ ClusterName: cluster.ObjectMeta.Name, From cf23f4c8714a66d7ae35f19fa603cb77852b6cf9 Mon Sep 17 00:00:00 2001 From: Christian Schlotter Date: Wed, 11 Sep 2024 17:53:06 +0200 Subject: [PATCH 04/17] fix e2e finalizer assertions --- test/framework/finalizers_helpers.go | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/test/framework/finalizers_helpers.go b/test/framework/finalizers_helpers.go index 946ec36ea3b3..0cfd61c29f86 100644 --- a/test/framework/finalizers_helpers.go +++ b/test/framework/finalizers_helpers.go @@ -42,8 +42,10 @@ import ( // CoreFinalizersAssertionWithLegacyClusters maps Cluster API core types to their expected finalizers for legacy Clusters. var CoreFinalizersAssertionWithLegacyClusters = map[string]func(types.NamespacedName) []string{ - clusterKind: func(_ types.NamespacedName) []string { return []string{clusterv1.ClusterFinalizer} }, - machineKind: func(_ types.NamespacedName) []string { return []string{clusterv1.MachineFinalizer} }, + clusterKind: func(_ types.NamespacedName) []string { return []string{clusterv1.ClusterFinalizer} }, + machineKind: func(_ types.NamespacedName) []string { return []string{clusterv1.MachineFinalizer} }, + machineSetKind: func(_ types.NamespacedName) []string { return []string{clusterv1.MachineSetFinalizer} }, + machineDeploymentKind: func(_ types.NamespacedName) []string { return []string{clusterv1.MachineDeploymentFinalizer} }, } // CoreFinalizersAssertionWithClassyClusters maps Cluster API core types to their expected finalizers for classy Clusters. @@ -52,8 +54,12 @@ var CoreFinalizersAssertionWithClassyClusters = func() map[string]func(types.Nam for k, v := range CoreFinalizersAssertionWithLegacyClusters { r[k] = v } - r[machineSetKind] = func(_ types.NamespacedName) []string { return []string{clusterv1.MachineSetTopologyFinalizer} } - r[machineDeploymentKind] = func(_ types.NamespacedName) []string { return []string{clusterv1.MachineDeploymentTopologyFinalizer} } + r[machineSetKind] = func(_ types.NamespacedName) []string { + return []string{clusterv1.MachineSetTopologyFinalizer, clusterv1.MachineSetFinalizer} + } + r[machineDeploymentKind] = func(_ types.NamespacedName) []string { + return []string{clusterv1.MachineDeploymentTopologyFinalizer, clusterv1.MachineDeploymentFinalizer} + } return r }() From 8824235e947554de45bbf3fdac9b1e071dfee63d Mon Sep 17 00:00:00 2001 From: Christian Schlotter Date: Thu, 12 Sep 2024 08:52:56 +0200 Subject: [PATCH 05/17] machinedeployment: rename helper function --- .../machinedeployment/machinedeployment_controller.go | 8 ++++---- .../machinedeployment_controller_test.go | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/internal/controllers/machinedeployment/machinedeployment_controller.go b/internal/controllers/machinedeployment/machinedeployment_controller.go index ee8ba0a67bcb..8a873dd60680 100644 --- a/internal/controllers/machinedeployment/machinedeployment_controller.go +++ b/internal/controllers/machinedeployment/machinedeployment_controller.go @@ -232,7 +232,7 @@ func (r *Reconciler) reconcile(ctx context.Context, cluster *clusterv1.Cluster, } } - msList, err := r.getMachineSetsForDeployment(ctx, md) + msList, err := r.getAndAdoptMachineSetsForDeployment(ctx, md) if err != nil { return err } @@ -295,7 +295,7 @@ func (r *Reconciler) reconcile(ctx context.Context, cluster *clusterv1.Cluster, func (r *Reconciler) reconcileDelete(ctx context.Context, md *clusterv1.MachineDeployment) error { log := ctrl.LoggerFrom(ctx) - msList, err := r.getMachineSetsForDeployment(ctx, md) + msList, err := r.getAndAdoptMachineSetsForDeployment(ctx, md) if err != nil { return err } @@ -321,8 +321,8 @@ func (r *Reconciler) reconcileDelete(ctx context.Context, md *clusterv1.MachineD return nil } -// getMachineSetsForDeployment returns a list of MachineSets associated with a MachineDeployment. -func (r *Reconciler) getMachineSetsForDeployment(ctx context.Context, md *clusterv1.MachineDeployment) ([]*clusterv1.MachineSet, error) { +// getAndAdoptMachineSetsForDeployment returns a list of MachineSets associated with a MachineDeployment. +func (r *Reconciler) getAndAdoptMachineSetsForDeployment(ctx context.Context, md *clusterv1.MachineDeployment) ([]*clusterv1.MachineSet, error) { log := ctrl.LoggerFrom(ctx) // List all MachineSets to find those we own but that no longer match our selector. diff --git a/internal/controllers/machinedeployment/machinedeployment_controller_test.go b/internal/controllers/machinedeployment/machinedeployment_controller_test.go index a82768f2abee..97a2ab079f97 100644 --- a/internal/controllers/machinedeployment/machinedeployment_controller_test.go +++ b/internal/controllers/machinedeployment/machinedeployment_controller_test.go @@ -962,7 +962,7 @@ func TestGetMachineSetsForDeployment(t *testing.T) { recorder: record.NewFakeRecorder(32), } - got, err := r.getMachineSetsForDeployment(ctx, &tc.machineDeployment) + got, err := r.getAndAdoptMachineSetsForDeployment(ctx, &tc.machineDeployment) g.Expect(err).ToNot(HaveOccurred()) g.Expect(got).To(HaveLen(len(tc.expected))) From 4d72a0f8f514aa5d3925b9d933dafafa2318d428 Mon Sep 17 00:00:00 2001 From: Christian Schlotter Date: Fri, 13 Sep 2024 11:02:51 +0200 Subject: [PATCH 06/17] review fixes --- api/v1beta1/machinedeployment_types.go | 2 +- api/v1beta1/machineset_types.go | 2 +- .../machinedeployment_controller.go | 16 ++-------------- .../machineset/machineset_controller.go | 15 +-------------- util/log/log.go | 18 ++++++++++++++++++ 5 files changed, 23 insertions(+), 30 deletions(-) diff --git a/api/v1beta1/machinedeployment_types.go b/api/v1beta1/machinedeployment_types.go index c2f1d38bf8a5..bda8e7d92e15 100644 --- a/api/v1beta1/machinedeployment_types.go +++ b/api/v1beta1/machinedeployment_types.go @@ -27,7 +27,7 @@ const ( MachineDeploymentTopologyFinalizer = "machinedeployment.topology.cluster.x-k8s.io" // MachineDeploymentFinalizer is the finalizer used by the MachineDeployment controller to - // cleanup the MachineDeployment descendant MachineSets when a MachineDeployment is being deleted. + // ensure ordered cleanup of corresponding MachineSets when a MachineDeployment is being deleted. MachineDeploymentFinalizer = "cluster.x-k8s.io/machinedeployment" ) diff --git a/api/v1beta1/machineset_types.go b/api/v1beta1/machineset_types.go index 37ce00076904..f0899d48e73d 100644 --- a/api/v1beta1/machineset_types.go +++ b/api/v1beta1/machineset_types.go @@ -31,7 +31,7 @@ const ( MachineSetTopologyFinalizer = "machineset.topology.cluster.x-k8s.io" // MachineSetFinalizer is the finalizer used by the MachineSet controller to - // cleanup the MachineSet descendant Machines when a Machineset is being deleted. + // ensure ordered cleanup of corresponding Machines when a Machineset is being deleted. MachineSetFinalizer = "cluster.x-k8s.io/machineset" ) diff --git a/internal/controllers/machinedeployment/machinedeployment_controller.go b/internal/controllers/machinedeployment/machinedeployment_controller.go index 8a873dd60680..d7e26a78989f 100644 --- a/internal/controllers/machinedeployment/machinedeployment_controller.go +++ b/internal/controllers/machinedeployment/machinedeployment_controller.go @@ -43,6 +43,7 @@ import ( "sigs.k8s.io/cluster-api/util/annotations" "sigs.k8s.io/cluster-api/util/conditions" utilconversion "sigs.k8s.io/cluster-api/util/conversion" + clog "sigs.k8s.io/cluster-api/util/log" "sigs.k8s.io/cluster-api/util/patch" "sigs.k8s.io/cluster-api/util/predicates" ) @@ -306,7 +307,7 @@ func (r *Reconciler) reconcileDelete(ctx context.Context, md *clusterv1.MachineD return nil } - log.Info("MachineDeployment still has descendant MachineSets - deleting them first", "count", len(msList), "descendants", descendantMachineSets(msList)) + log.Info("Waiting for MachineSets to be deleted", "MachineSets", clog.ObjNamesString(msList)) // else delete owned machinesets. for _, ms := range msList { @@ -474,16 +475,3 @@ func reconcileExternalTemplateReference(ctx context.Context, c client.Client, cl return patchHelper.Patch(ctx, obj) } - -func descendantMachineSets(objs []*clusterv1.MachineSet) string { - objNames := make([]string, len(objs)) - for _, obj := range objs { - objNames = append(objNames, obj.GetName()) - } - - if len(objNames) > 10 { - objNames = append(objNames[:10], "...") - } - - return strings.Join(objNames, ",") -} diff --git a/internal/controllers/machineset/machineset_controller.go b/internal/controllers/machineset/machineset_controller.go index 837582850fc7..a221750b010e 100644 --- a/internal/controllers/machineset/machineset_controller.go +++ b/internal/controllers/machineset/machineset_controller.go @@ -342,7 +342,7 @@ func (r *Reconciler) reconcileDelete(ctx context.Context, machineSet *clusterv1. return nil } - log.Info("MachineSet still has descendant Machines - deleting them first", "count", len(machineList), "descendants", descendantMachines(machineList)) + log.Info("Waiting for Machines to be deleted", "Machines", clog.ObjNamesString(machineList)) // else delete owned machines. for _, machine := range machineList { @@ -1227,16 +1227,3 @@ func (r *Reconciler) reconcileExternalTemplateReference(ctx context.Context, clu return patchHelper.Patch(ctx, obj) } - -func descendantMachines(objs []*clusterv1.Machine) string { - objNames := make([]string, len(objs)) - for _, obj := range objs { - objNames = append(objNames, obj.GetName()) - } - - if len(objNames) > 10 { - objNames = append(objNames[:10], "...") - } - - return strings.Join(objNames, ",") -} diff --git a/util/log/log.go b/util/log/log.go index 8a8e0354ff60..45678cb6bf37 100644 --- a/util/log/log.go +++ b/util/log/log.go @@ -19,6 +19,8 @@ package log import ( "context" + "fmt" + "strings" "github.com/go-logr/logr" "github.com/pkg/errors" @@ -104,3 +106,19 @@ func getOwners(ctx context.Context, c client.Client, obj metav1.Object) ([]owner return owners, nil } + +// ObjNamesString returns a comma separated list of the object names, limited to +// five objects. On more than five objects it outputs the first five objects and +// adds information about how much more are in the given list. +func ObjNamesString[T client.Object](objs []T) string { + objNames := make([]string, len(objs)) + for _, obj := range objs { + objNames = append(objNames, obj.GetName()) + } + + if len(objNames) <= 5 { + return strings.Join(objNames, ", ") + } + + return fmt.Sprintf("%s and %d more", strings.Join(objNames[:5], ", "), len(objNames)-5) +} From c83c6283438b0d2b34068f180e2e53eee7fcd707 Mon Sep 17 00:00:00 2001 From: Christian Schlotter Date: Fri, 13 Sep 2024 11:05:30 +0200 Subject: [PATCH 07/17] add unit tests for reconcileDelete functions --- .../machinedeployment_controller_test.go | 95 +++++++++++++++++++ .../machineset/machineset_controller_test.go | 94 ++++++++++++++++++ 2 files changed, 189 insertions(+) diff --git a/internal/controllers/machinedeployment/machinedeployment_controller_test.go b/internal/controllers/machinedeployment/machinedeployment_controller_test.go index 97a2ab079f97..ca9bfd99ad33 100644 --- a/internal/controllers/machinedeployment/machinedeployment_controller_test.go +++ b/internal/controllers/machinedeployment/machinedeployment_controller_test.go @@ -18,6 +18,7 @@ package machinedeployment import ( "context" + "sort" "testing" "time" @@ -34,6 +35,7 @@ import ( clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" "sigs.k8s.io/cluster-api/controllers/external" + "sigs.k8s.io/cluster-api/internal/test/builder" "sigs.k8s.io/cluster-api/internal/util/ssa" "sigs.k8s.io/cluster-api/util" "sigs.k8s.io/cluster-api/util/conditions" @@ -993,3 +995,96 @@ func updateMachineDeployment(ctx context.Context, c client.Client, md *clusterv1 return patchHelper.Patch(ctx, md) }) } + +func TestReconciler_reconcileDelete(t *testing.T) { + labels := map[string]string{ + "some": "labelselector", + } + md := builder.MachineDeployment("default", "md0").WithClusterName("test").Build() + md.DeletionTimestamp = ptr.To(metav1.Now()) + md.Spec.Selector = metav1.LabelSelector{ + MatchLabels: labels, + } + mdWithoutFinalizer := md.DeepCopy() + mdWithoutFinalizer.Finalizers = nil + tests := []struct { + name string + machineDeployment *clusterv1.MachineDeployment + want *clusterv1.MachineDeployment + objs []client.Object + wantMachineSets []clusterv1.MachineSet + expectError bool + }{ + { + name: "Should do nothing when no descendant MachineSets exist and finalizer is already gone", + machineDeployment: mdWithoutFinalizer.DeepCopy(), + want: mdWithoutFinalizer.DeepCopy(), + objs: nil, + wantMachineSets: nil, + expectError: false, + }, + { + name: "Should remove finalizer when no descendant MachineSets exist", + machineDeployment: md.DeepCopy(), + want: mdWithoutFinalizer.DeepCopy(), + objs: nil, + wantMachineSets: nil, + expectError: false, + }, + { + name: "Should keep finalizer when descendant MachineSets exist and trigger deletion only for descendant MachineSets", + machineDeployment: md.DeepCopy(), + want: md.DeepCopy(), + objs: []client.Object{ + builder.MachineSet("default", "ms0").WithClusterName("test").WithLabels(labels).Build(), + builder.MachineSet("default", "ms1").WithClusterName("test").WithLabels(labels).Build(), + builder.MachineSet("default", "ms2-not-part-of-md").WithClusterName("test").Build(), + builder.MachineSet("default", "ms3-not-part-of-md").WithClusterName("test").Build(), + }, + wantMachineSets: []clusterv1.MachineSet{ + *builder.MachineSet("default", "ms2-not-part-of-md").WithClusterName("test").Build(), + *builder.MachineSet("default", "ms3-not-part-of-md").WithClusterName("test").Build(), + }, + expectError: false, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + + c := fake.NewClientBuilder().WithObjects(tt.objs...).Build() + r := &Reconciler{ + Client: c, + recorder: record.NewFakeRecorder(32), + } + // Mark machineDeployment to be in deletion. + + err := r.reconcileDelete(ctx, tt.machineDeployment) + if tt.expectError { + g.Expect(err).To(HaveOccurred()) + } else { + g.Expect(err).ToNot(HaveOccurred()) + } + + g.Expect(tt.machineDeployment).To(BeComparableTo(tt.want)) + + machineSetList := &clusterv1.MachineSetList{} + g.Expect(c.List(ctx, machineSetList, client.InNamespace("default"))).ToNot(HaveOccurred()) + + // Remove ResourceVersion so we can actually compare. + for i, m := range machineSetList.Items { + m.ResourceVersion = "" + machineSetList.Items[i] = m + } + + sort.Slice(machineSetList.Items, func(i, j int) bool { + return machineSetList.Items[i].GetName() < machineSetList.Items[j].GetName() + }) + sort.Slice(tt.wantMachineSets, func(i, j int) bool { + return tt.wantMachineSets[i].GetName() < tt.wantMachineSets[j].GetName() + }) + + g.Expect(machineSetList.Items).To(BeComparableTo(tt.wantMachineSets)) + }) + } +} diff --git a/internal/controllers/machineset/machineset_controller_test.go b/internal/controllers/machineset/machineset_controller_test.go index 484f8449c422..3f9352bd3f67 100644 --- a/internal/controllers/machineset/machineset_controller_test.go +++ b/internal/controllers/machineset/machineset_controller_test.go @@ -18,6 +18,7 @@ package machineset import ( "fmt" + "sort" "testing" "time" @@ -2129,3 +2130,96 @@ func assertMachine(g *WithT, actualMachine *clusterv1.Machine, expectedMachine * g.Expect(actualMachine.Finalizers).Should(Equal(expectedMachine.Finalizers)) } } + +func TestReconciler_reconcileDelete(t *testing.T) { + labels := map[string]string{ + "some": "labelselector", + } + ms := builder.MachineSet("default", "ms0").WithClusterName("test").Build() + ms.DeletionTimestamp = ptr.To(metav1.Now()) + ms.Spec.Selector = metav1.LabelSelector{ + MatchLabels: labels, + } + msWithoutFinalizer := ms.DeepCopy() + msWithoutFinalizer.Finalizers = nil + tests := []struct { + name string + machineSet *clusterv1.MachineSet + want *clusterv1.MachineSet + objs []client.Object + wantMachines []clusterv1.Machine + expectError bool + }{ + { + name: "Should do nothing when no descendant Machines exist and finalizer is already gone", + machineSet: msWithoutFinalizer.DeepCopy(), + want: msWithoutFinalizer.DeepCopy(), + objs: nil, + wantMachines: nil, + expectError: false, + }, + { + name: "Should remove finalizer when no descendant Machines exist", + machineSet: ms.DeepCopy(), + want: msWithoutFinalizer.DeepCopy(), + objs: nil, + wantMachines: nil, + expectError: false, + }, + { + name: "Should keep finalizer when descendant Machines exist and trigger deletion only for descendant Machines", + machineSet: ms.DeepCopy(), + want: ms.DeepCopy(), + objs: []client.Object{ + builder.Machine("default", "m0").WithClusterName("test").WithLabels(labels).Build(), + builder.Machine("default", "m1").WithClusterName("test").WithLabels(labels).Build(), + builder.Machine("default", "m2-not-part-of-ms").WithClusterName("test").Build(), + builder.Machine("default", "m3-not-part-of-ms").WithClusterName("test").Build(), + }, + wantMachines: []clusterv1.Machine{ + *builder.Machine("default", "m2-not-part-of-ms").WithClusterName("test").Build(), + *builder.Machine("default", "m3-not-part-of-ms").WithClusterName("test").Build(), + }, + expectError: false, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + + c := fake.NewClientBuilder().WithObjects(tt.objs...).Build() + r := &Reconciler{ + Client: c, + recorder: record.NewFakeRecorder(32), + } + // Mark machineSet to be in deletion. + + err := r.reconcileDelete(ctx, tt.machineSet) + if tt.expectError { + g.Expect(err).To(HaveOccurred()) + } else { + g.Expect(err).ToNot(HaveOccurred()) + } + + g.Expect(tt.machineSet).To(BeComparableTo(tt.want)) + + machineList := &clusterv1.MachineList{} + g.Expect(c.List(ctx, machineList, client.InNamespace("default"))).ToNot(HaveOccurred()) + + // Remove ResourceVersion so we can actually compare. + for i, m := range machineList.Items { + m.ResourceVersion = "" + machineList.Items[i] = m + } + + sort.Slice(machineList.Items, func(i, j int) bool { + return machineList.Items[i].GetName() < machineList.Items[j].GetName() + }) + sort.Slice(tt.wantMachines, func(i, j int) bool { + return tt.wantMachines[i].GetName() < tt.wantMachines[j].GetName() + }) + + g.Expect(machineList.Items).To(BeComparableTo(tt.wantMachines)) + }) + } +} From 3d27e600169fda10ddf680a75909c308faf79b6c Mon Sep 17 00:00:00 2001 From: Christian Schlotter Date: Mon, 16 Sep 2024 12:01:04 +0200 Subject: [PATCH 08/17] review fixes --- .../machinedeployment_controller.go | 5 +++-- .../machinedeployment_controller_test.go | 21 +++++++------------ .../machineset/machineset_controller.go | 3 ++- .../machineset/machineset_controller_test.go | 20 +++++++----------- util/log/log.go | 16 +++++++++----- 5 files changed, 30 insertions(+), 35 deletions(-) diff --git a/internal/controllers/machinedeployment/machinedeployment_controller.go b/internal/controllers/machinedeployment/machinedeployment_controller.go index d7e26a78989f..fe9e663b442f 100644 --- a/internal/controllers/machinedeployment/machinedeployment_controller.go +++ b/internal/controllers/machinedeployment/machinedeployment_controller.go @@ -313,7 +313,7 @@ func (r *Reconciler) reconcileDelete(ctx context.Context, md *clusterv1.MachineD for _, ms := range msList { if ms.DeletionTimestamp.IsZero() { log.Info("Deleting MachineSet", "MachineSet", klog.KObj(ms)) - if err := r.Client.Delete(ctx, ms); err != nil { + if err := r.Client.Delete(ctx, ms); err != nil && !apierrors.IsNotFound(err) { return errors.Wrapf(err, "failed to delete MachineSet %s", klog.KObj(ms)) } } @@ -335,7 +335,8 @@ func (r *Reconciler) getAndAdoptMachineSetsForDeployment(ctx context.Context, md filtered := make([]*clusterv1.MachineSet, 0, len(machineSets.Items)) for idx := range machineSets.Items { ms := &machineSets.Items[idx] - log.WithValues("MachineSet", klog.KObj(ms)) + log := log.WithValues("MachineSet", klog.KObj(ms)) + ctx := ctrl.LoggerInto(ctx, log) selector, err := metav1.LabelSelectorAsSelector(&md.Spec.Selector) if err != nil { log.Error(err, "Skipping MachineSet, failed to get label selector from spec selector") diff --git a/internal/controllers/machinedeployment/machinedeployment_controller_test.go b/internal/controllers/machinedeployment/machinedeployment_controller_test.go index ca9bfd99ad33..51e14f39d82c 100644 --- a/internal/controllers/machinedeployment/machinedeployment_controller_test.go +++ b/internal/controllers/machinedeployment/machinedeployment_controller_test.go @@ -18,7 +18,6 @@ package machinedeployment import ( "context" - "sort" "testing" "time" @@ -1001,12 +1000,15 @@ func TestReconciler_reconcileDelete(t *testing.T) { "some": "labelselector", } md := builder.MachineDeployment("default", "md0").WithClusterName("test").Build() + md.Finalizers = []string{ + clusterv1.MachineDeploymentFinalizer, + } md.DeletionTimestamp = ptr.To(metav1.Now()) md.Spec.Selector = metav1.LabelSelector{ MatchLabels: labels, } mdWithoutFinalizer := md.DeepCopy() - mdWithoutFinalizer.Finalizers = nil + mdWithoutFinalizer.Finalizers = []string{} tests := []struct { name string machineDeployment *clusterv1.MachineDeployment @@ -1057,7 +1059,6 @@ func TestReconciler_reconcileDelete(t *testing.T) { Client: c, recorder: record.NewFakeRecorder(32), } - // Mark machineDeployment to be in deletion. err := r.reconcileDelete(ctx, tt.machineDeployment) if tt.expectError { @@ -1072,19 +1073,11 @@ func TestReconciler_reconcileDelete(t *testing.T) { g.Expect(c.List(ctx, machineSetList, client.InNamespace("default"))).ToNot(HaveOccurred()) // Remove ResourceVersion so we can actually compare. - for i, m := range machineSetList.Items { - m.ResourceVersion = "" - machineSetList.Items[i] = m + for i := range machineSetList.Items { + machineSetList.Items[i].ResourceVersion = "" } - sort.Slice(machineSetList.Items, func(i, j int) bool { - return machineSetList.Items[i].GetName() < machineSetList.Items[j].GetName() - }) - sort.Slice(tt.wantMachineSets, func(i, j int) bool { - return tt.wantMachineSets[i].GetName() < tt.wantMachineSets[j].GetName() - }) - - g.Expect(machineSetList.Items).To(BeComparableTo(tt.wantMachineSets)) + g.Expect(machineSetList.Items).To(ConsistOf(tt.wantMachineSets)) }) } } diff --git a/internal/controllers/machineset/machineset_controller.go b/internal/controllers/machineset/machineset_controller.go index a221750b010e..27b9682adf9e 100644 --- a/internal/controllers/machineset/machineset_controller.go +++ b/internal/controllers/machineset/machineset_controller.go @@ -348,7 +348,7 @@ func (r *Reconciler) reconcileDelete(ctx context.Context, machineSet *clusterv1. for _, machine := range machineList { if machine.DeletionTimestamp.IsZero() { log.Info("Deleting Machine", "Machine", klog.KObj(machine)) - if err := r.Client.Delete(ctx, machine); err != nil { + if err := r.Client.Delete(ctx, machine); err != nil && !apierrors.IsNotFound(err) { return errors.Wrapf(err, "failed to delete Machine %s", klog.KObj(machine)) } } @@ -381,6 +381,7 @@ func (r *Reconciler) getAndAdoptMachinesForMachineSet(ctx context.Context, machi for idx := range allMachines.Items { machine := &allMachines.Items[idx] log := log.WithValues("Machine", klog.KObj(machine)) + ctx := ctrl.LoggerInto(ctx, log) if shouldExcludeMachine(machineSet, machine) { continue } diff --git a/internal/controllers/machineset/machineset_controller_test.go b/internal/controllers/machineset/machineset_controller_test.go index 3f9352bd3f67..f1c866c42740 100644 --- a/internal/controllers/machineset/machineset_controller_test.go +++ b/internal/controllers/machineset/machineset_controller_test.go @@ -18,7 +18,6 @@ package machineset import ( "fmt" - "sort" "testing" "time" @@ -2136,12 +2135,15 @@ func TestReconciler_reconcileDelete(t *testing.T) { "some": "labelselector", } ms := builder.MachineSet("default", "ms0").WithClusterName("test").Build() + ms.Finalizers = []string{ + clusterv1.MachineSetFinalizer, + } ms.DeletionTimestamp = ptr.To(metav1.Now()) ms.Spec.Selector = metav1.LabelSelector{ MatchLabels: labels, } msWithoutFinalizer := ms.DeepCopy() - msWithoutFinalizer.Finalizers = nil + msWithoutFinalizer.Finalizers = []string{} tests := []struct { name string machineSet *clusterv1.MachineSet @@ -2207,19 +2209,11 @@ func TestReconciler_reconcileDelete(t *testing.T) { g.Expect(c.List(ctx, machineList, client.InNamespace("default"))).ToNot(HaveOccurred()) // Remove ResourceVersion so we can actually compare. - for i, m := range machineList.Items { - m.ResourceVersion = "" - machineList.Items[i] = m + for i := range machineList.Items { + machineList.Items[i].ResourceVersion = "" } - sort.Slice(machineList.Items, func(i, j int) bool { - return machineList.Items[i].GetName() < machineList.Items[j].GetName() - }) - sort.Slice(tt.wantMachines, func(i, j int) bool { - return tt.wantMachines[i].GetName() < tt.wantMachines[j].GetName() - }) - - g.Expect(machineList.Items).To(BeComparableTo(tt.wantMachines)) + g.Expect(machineList.Items).To(ConsistOf(tt.wantMachines)) }) } } diff --git a/util/log/log.go b/util/log/log.go index 45678cb6bf37..11a41c460361 100644 --- a/util/log/log.go +++ b/util/log/log.go @@ -111,14 +111,20 @@ func getOwners(ctx context.Context, c client.Client, obj metav1.Object) ([]owner // five objects. On more than five objects it outputs the first five objects and // adds information about how much more are in the given list. func ObjNamesString[T client.Object](objs []T) string { - objNames := make([]string, len(objs)) + shortenedBy := 0 + if len(objs) > 5 { + shortenedBy = len(objs) - 5 + objs = objs[:5] + } + + stringList := []string{} for _, obj := range objs { - objNames = append(objNames, obj.GetName()) + stringList = append(stringList, obj.GetName()) } - if len(objNames) <= 5 { - return strings.Join(objNames, ", ") + if shortenedBy > 0 { + stringList = append(stringList, fmt.Sprintf("... (%d more)", shortenedBy)) } - return fmt.Sprintf("%s and %d more", strings.Join(objNames[:5], ", "), len(objNames)-5) + return strings.Join(stringList, ", ") } From 8859da582833b5159ca958557b37a52af8a3a192 Mon Sep 17 00:00:00 2001 From: Christian Schlotter Date: Mon, 16 Sep 2024 15:47:14 +0200 Subject: [PATCH 09/17] review fix --- internal/controllers/machineset/machineset_controller_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/internal/controllers/machineset/machineset_controller_test.go b/internal/controllers/machineset/machineset_controller_test.go index f1c866c42740..757f8447995f 100644 --- a/internal/controllers/machineset/machineset_controller_test.go +++ b/internal/controllers/machineset/machineset_controller_test.go @@ -2194,7 +2194,6 @@ func TestReconciler_reconcileDelete(t *testing.T) { Client: c, recorder: record.NewFakeRecorder(32), } - // Mark machineSet to be in deletion. err := r.reconcileDelete(ctx, tt.machineSet) if tt.expectError { From 88733b2449a068d629bf904024594c243934a8c9 Mon Sep 17 00:00:00 2001 From: Christian Schlotter Date: Fri, 13 Sep 2024 12:14:58 +0200 Subject: [PATCH 10/17] test: implement e2e test for cluster deletion --- test/e2e/cluster_deletion.go | 357 ++++++++++++++++++++++++++++++ test/e2e/cluster_deletion_test.go | 61 +++++ 2 files changed, 418 insertions(+) create mode 100644 test/e2e/cluster_deletion.go create mode 100644 test/e2e/cluster_deletion_test.go diff --git a/test/e2e/cluster_deletion.go b/test/e2e/cluster_deletion.go new file mode 100644 index 000000000000..204fc3efc6a1 --- /dev/null +++ b/test/e2e/cluster_deletion.go @@ -0,0 +1,357 @@ +/* +Copyright 2024 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package e2e + +import ( + "context" + "fmt" + "os" + "path/filepath" + "time" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/util/sets" + "k8s.io/klog/v2" + "k8s.io/utils/ptr" + "sigs.k8s.io/controller-runtime/pkg/client" + + clusterctlcluster "sigs.k8s.io/cluster-api/cmd/clusterctl/client/cluster" + "sigs.k8s.io/cluster-api/test/e2e/internal/log" + "sigs.k8s.io/cluster-api/test/framework" + "sigs.k8s.io/cluster-api/test/framework/clusterctl" + "sigs.k8s.io/cluster-api/util" + "sigs.k8s.io/cluster-api/util/patch" +) + +// ClusterDeletionSpecInput is the input for ClusterDeletionSpec. +type ClusterDeletionSpecInput struct { + E2EConfig *clusterctl.E2EConfig + ClusterctlConfigPath string + BootstrapClusterProxy framework.ClusterProxy + ArtifactFolder string + SkipCleanup bool + + // Cluster name allows to specify a deterministic clusterName. + // If not set, a random one will be generated. + ClusterName *string + + // InfrastructureProvider allows to specify the infrastructure provider to be used when looking for + // cluster templates. + // If not set, clusterctl will look at the infrastructure provider installed in the management cluster; + // if only one infrastructure provider exists, it will be used, otherwise the operation will fail if more than one exists. + InfrastructureProvider *string + + // Flavor, if specified is the template flavor used to create the cluster for testing. + // If not specified, the default flavor for the selected infrastructure provider is used. + Flavor *string + + // ControlPlaneMachineCount defines the number of control plane machines to be added to the workload cluster. + // If not specified, 1 will be used. + ControlPlaneMachineCount *int64 + + // WorkerMachineCount defines number of worker machines to be added to the workload cluster. + // If not specified, 1 will be used. + WorkerMachineCount *int64 + + // Allows to inject functions to be run while waiting for the control plane to be initialized, + // which unblocks CNI installation, and for the control plane machines to be ready (after CNI installation). + ControlPlaneWaiters clusterctl.ControlPlaneWaiters + + // Allows to inject a function to be run after test namespace is created. + // If not specified, this is a no-op. + PostNamespaceCreated func(managementClusterProxy framework.ClusterProxy, workloadClusterNamespace string) + + // Allows to inject a function to be run after machines are provisioned. + // If not specified, this is a no-op. + PostMachinesProvisioned func(managementClusterProxy framework.ClusterProxy, workloadClusterNamespace, workloadClusterName string) + + // ClusterDeletionPhases define kinds which are expected to get deleted in phases. + ClusterDeletionPhases []ClusterDeletionPhase +} + +type ClusterDeletionPhase struct { + // Kinds are the Kinds which are supposed to be deleted in this phase. + Kinds sets.Set[string] + // KindsWithFinalizer are the kinds which are considered to be deleted in this phase, but need to be blocking. + KindsWithFinalizer sets.Set[string] + // objects will be filled later by objects which match a kind given in one of the above sets. + objects []client.Object +} + +// ClusterDeletionSpec implements a test that verifies that MachineDeployment rolling updates are successful. +func ClusterDeletionSpec(ctx context.Context, inputGetter func() ClusterDeletionSpecInput) { + var ( + specName = "cluster-deletion" + input ClusterDeletionSpecInput + namespace *corev1.Namespace + cancelWatches context.CancelFunc + clusterResources *clusterctl.ApplyClusterTemplateAndWaitResult + finalizer = "test.cluster.x-k8s.io/cluster-deletion" + objectsWithFinalizer []client.Object + ) + + BeforeEach(func() { + Expect(ctx).NotTo(BeNil(), "ctx is required for %s spec", specName) + input = inputGetter() + Expect(input.E2EConfig).ToNot(BeNil(), "Invalid argument. input.E2EConfig can't be nil when calling %s spec", specName) + Expect(input.ClusterctlConfigPath).To(BeAnExistingFile(), "Invalid argument. input.ClusterctlConfigPath must be an existing file when calling %s spec", specName) + Expect(input.BootstrapClusterProxy).ToNot(BeNil(), "Invalid argument. input.BootstrapClusterProxy can't be nil when calling %s spec", specName) + Expect(os.MkdirAll(input.ArtifactFolder, 0750)).To(Succeed(), "Invalid argument. input.ArtifactFolder can't be created for %s spec", specName) + + Expect(input.E2EConfig.Variables).To(HaveKey(KubernetesVersion)) + + // Setup a Namespace where to host objects for this spec and create a watcher for the namespace events. + namespace, cancelWatches = framework.SetupSpecNamespace(ctx, specName, input.BootstrapClusterProxy, input.ArtifactFolder, input.PostNamespaceCreated) + clusterResources = new(clusterctl.ApplyClusterTemplateAndWaitResult) + }) + + It("Should successfully create and delete a Cluster", func() { + infrastructureProvider := clusterctl.DefaultInfrastructureProvider + if input.InfrastructureProvider != nil { + infrastructureProvider = *input.InfrastructureProvider + } + + flavor := clusterctl.DefaultFlavor + if input.Flavor != nil { + flavor = *input.Flavor + } + + controlPlaneMachineCount := ptr.To[int64](1) + if input.ControlPlaneMachineCount != nil { + controlPlaneMachineCount = input.ControlPlaneMachineCount + } + + workerMachineCount := ptr.To[int64](1) + if input.WorkerMachineCount != nil { + workerMachineCount = input.WorkerMachineCount + } + + clusterName := fmt.Sprintf("%s-%s", specName, util.RandomString(6)) + if input.ClusterName != nil { + clusterName = *input.ClusterName + } + clusterctl.ApplyClusterTemplateAndWait(ctx, clusterctl.ApplyClusterTemplateAndWaitInput{ + ClusterProxy: input.BootstrapClusterProxy, + ConfigCluster: clusterctl.ConfigClusterInput{ + LogFolder: filepath.Join(input.ArtifactFolder, "clusters", input.BootstrapClusterProxy.GetName()), + ClusterctlConfigPath: input.ClusterctlConfigPath, + KubeconfigPath: input.BootstrapClusterProxy.GetKubeconfigPath(), + InfrastructureProvider: infrastructureProvider, + Flavor: flavor, + Namespace: namespace.Name, + ClusterName: clusterName, + KubernetesVersion: input.E2EConfig.GetVariable(KubernetesVersion), + ControlPlaneMachineCount: controlPlaneMachineCount, + WorkerMachineCount: workerMachineCount, + }, + ControlPlaneWaiters: input.ControlPlaneWaiters, + WaitForClusterIntervals: input.E2EConfig.GetIntervals(specName, "wait-cluster"), + WaitForControlPlaneIntervals: input.E2EConfig.GetIntervals(specName, "wait-control-plane"), + WaitForMachineDeployments: input.E2EConfig.GetIntervals(specName, "wait-worker-nodes"), + PostMachinesProvisioned: func() { + if input.PostMachinesProvisioned != nil { + input.PostMachinesProvisioned(input.BootstrapClusterProxy, namespace.Name, clusterName) + } + }, + }, clusterResources) + + relevantKinds := sets.Set[string]{} + + for _, phaseInput := range input.ClusterDeletionPhases { + relevantKinds = relevantKinds. + Union(phaseInput.KindsWithFinalizer). + Union(phaseInput.Kinds) + } + + // Get all objects relevant to the test by filtering for the kinds of the phases. + graph, err := clusterctlcluster.GetOwnerGraph(ctx, namespace.GetName(), input.BootstrapClusterProxy.GetKubeconfigPath(), func(u unstructured.Unstructured) bool { + return relevantKinds.Has(u.GetKind()) + }) + Expect(err).ToNot(HaveOccurred()) + + for i, phase := range input.ClusterDeletionPhases { + // Iterate over the graph and find all objects relevant for the phase. + // * Objects matching the kind except for Machines (see below) + // * Machines which are owned by one of the kinds which are part of the phase + allKinds := phase.KindsWithFinalizer.Union(phase.Kinds) + allKindsWithoutMachine := allKinds.Clone().Delete("Machine") + + for _, node := range graph { + // Only add objects which match a defined kind for the phase. + if !allKinds.Has(node.Object.Kind) { + continue + } + + // Special handling for machines: only add machines which are owned by one of the given kinds. + if node.Object.Kind == "Machine" { + isOwnedByKind := false + for _, owner := range node.Owners { + if allKindsWithoutMachine.Has(owner.Kind) { + isOwnedByKind = true + break + } + } + // Ignore Machines which are not owned by one of the given kinds. + if !isOwnedByKind { + continue + } + } + + obj := new(unstructured.Unstructured) + obj.SetAPIVersion(node.Object.APIVersion) + obj.SetKind(node.Object.Kind) + obj.SetName(node.Object.Name) + obj.SetNamespace(namespace.GetName()) + + phase.objects = append(phase.objects, obj) + + // If the object's kind is part of kindsWithFinalizer, then additionally the object to the finalizedObjs slice. + if phase.KindsWithFinalizer.Has(obj.GetKind()) { + objectsWithFinalizer = append(objectsWithFinalizer, obj) + } + } + + // Update the phase in input.ClusterDeletionPhases + input.ClusterDeletionPhases[i] = phase + } + + // Update all objects in objectsWithFinalizers and add the finalizer for this test to control when these objects vanish. + addFinalizer(ctx, input.BootstrapClusterProxy.GetClient(), finalizer, objectsWithFinalizer...) + + // Trigger the deletion of the Cluster. + framework.DeleteCluster(ctx, framework.DeleteClusterInput{ + Cluster: clusterResources.Cluster, + Deleter: input.BootstrapClusterProxy.GetClient(), + }) + + var objectsDeleted, objectsInDeletion []client.Object + + for i, phase := range input.ClusterDeletionPhases { + // Objects which previously were in deletion because they waited for our finalizer being removed are now checked to actually had been removed. + objectsDeleted = objectsInDeletion + // All objects for this phase are expected to get into deletion (still exist and have deletionTimestamp set). + objectsInDeletion = phase.objects + // All objects which are part of future phases are expected to not yet be in deletion. + objectsNotInDeletion := []client.Object{} + for j := i + 1; j < len(input.ClusterDeletionPhases); j++ { + objectsNotInDeletion = append(objectsNotInDeletion, input.ClusterDeletionPhases[j].objects...) + } + + Eventually(func(g Gomega) { + // Ensure expected objects are in deletion + for _, obj := range objectsInDeletion { + g.Expect(input.BootstrapClusterProxy.GetClient().Get(ctx, client.ObjectKeyFromObject(obj), obj)).ToNot(HaveOccurred()) + g.Expect(obj.GetDeletionTimestamp().IsZero()).To(BeFalse()) + } + + // Ensure deleted objects are gone + for _, obj := range objectsDeleted { + err := input.BootstrapClusterProxy.GetClient().Get(ctx, client.ObjectKeyFromObject(obj), obj) + g.Expect(err).To(HaveOccurred()) + g.Expect(apierrors.IsNotFound(err)).To(BeTrue()) + } + + // Ensure other objects are not in deletion. + for _, obj := range objectsNotInDeletion { + g.Expect(input.BootstrapClusterProxy.GetClient().Get(ctx, client.ObjectKeyFromObject(obj), obj)).ToNot(HaveOccurred()) + g.Expect(obj.GetDeletionTimestamp().IsZero()).To(BeTrue()) + } + }).WithTimeout(5 * time.Minute).WithPolling(10 * time.Second).Should(Succeed()) + + // Remove the test's finalizer from all objects which had been expected to be in deletion to unblock the next phase. + removeFinalizer(ctx, input.BootstrapClusterProxy.GetClient(), finalizer, objectsInDeletion...) + } + + // The last phase should unblock the cluster to actually have been removed. + log.Logf("Waiting for the Cluster %s to be deleted", klog.KObj(clusterResources.Cluster)) + framework.WaitForClusterDeleted(ctx, framework.WaitForClusterDeletedInput{ + Client: input.BootstrapClusterProxy.GetClient(), + Cluster: clusterResources.Cluster, + ArtifactFolder: input.ArtifactFolder, + }, input.E2EConfig.GetIntervals(specName, "wait-delete-cluster")...) + + By("PASSED!") + }) + + AfterEach(func() { + // Dump all the resources in the spec namespace and the workload cluster. + framework.DumpAllResourcesAndLogs(ctx, input.BootstrapClusterProxy, input.ArtifactFolder, namespace, clusterResources.Cluster) + + if !input.SkipCleanup { + // Remove finalizers we added to block normal deletion. + removeFinalizer(ctx, input.BootstrapClusterProxy.GetClient(), finalizer, objectsWithFinalizer...) + + By(fmt.Sprintf("Deleting cluster %s", klog.KObj(clusterResources.Cluster))) + // While /~https://github.com/kubernetes-sigs/cluster-api/issues/2955 is addressed in future iterations, there is a chance + // that cluster variable is not set even if the cluster exists, so we are calling DeleteAllClustersAndWait + // instead of DeleteClusterAndWait + framework.DeleteAllClustersAndWait(ctx, framework.DeleteAllClustersAndWaitInput{ + Client: input.BootstrapClusterProxy.GetClient(), + Namespace: namespace.Name, + ArtifactFolder: input.ArtifactFolder, + }, input.E2EConfig.GetIntervals(specName, "wait-delete-cluster")...) + + By(fmt.Sprintf("Deleting namespace used for hosting the %q test spec", specName)) + framework.DeleteNamespace(ctx, framework.DeleteNamespaceInput{ + Deleter: input.BootstrapClusterProxy.GetClient(), + Name: namespace.Name, + }) + } + cancelWatches() + }) +} + +func addFinalizer(ctx context.Context, c client.Client, finalizer string, objs ...client.Object) { + for _, obj := range objs { + Expect(c.Get(ctx, client.ObjectKeyFromObject(obj), obj)).To(Succeed()) + + helper, err := patch.NewHelper(obj, c) + Expect(err).ToNot(HaveOccurred()) + + obj.SetFinalizers(append(obj.GetFinalizers(), finalizer)) + + Expect(helper.Patch(ctx, obj)).ToNot(HaveOccurred()) + } +} + +func removeFinalizer(ctx context.Context, c client.Client, finalizer string, objs ...client.Object) { + for _, obj := range objs { + err := c.Get(ctx, client.ObjectKeyFromObject(obj), obj) + if apierrors.IsNotFound(err) { + continue + } + Expect(err).ToNot(HaveOccurred()) + + helper, err := patch.NewHelper(obj, c) + Expect(err).ToNot(HaveOccurred()) + + var finalizers []string + for _, f := range obj.GetFinalizers() { + if f == finalizer { + continue + } + finalizers = append(finalizers, f) + } + + obj.SetFinalizers(finalizers) + Expect(helper.Patch(ctx, obj)).ToNot(HaveOccurred()) + } +} diff --git a/test/e2e/cluster_deletion_test.go b/test/e2e/cluster_deletion_test.go new file mode 100644 index 000000000000..7c70d9dc191c --- /dev/null +++ b/test/e2e/cluster_deletion_test.go @@ -0,0 +1,61 @@ +//go:build e2e +// +build e2e + +/* +Copyright 2024 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package e2e + +import ( + . "github.com/onsi/ginkgo/v2" + "k8s.io/apimachinery/pkg/util/sets" + "k8s.io/utils/ptr" +) + +var _ = Describe("When following the Cluster API quick-start with ClusterClass [PR-Blocking] [ClusterClass]", func() { + ClusterDeletionSpec(ctx, func() ClusterDeletionSpecInput { + return ClusterDeletionSpecInput{ + E2EConfig: e2eConfig, + ClusterctlConfigPath: clusterctlConfigPath, + BootstrapClusterProxy: bootstrapClusterProxy, + ArtifactFolder: artifactFolder, + SkipCleanup: skipCleanup, + ControlPlaneMachineCount: ptr.To[int64](3), + Flavor: ptr.To("topology"), + InfrastructureProvider: ptr.To("docker"), + + ClusterDeletionPhases: []ClusterDeletionPhase{ + // The first phase deletes all worker related machines. + // Note: only setting the finalizer on Machines results in properly testing foreground deletion. + { + Kinds: sets.New[string]("MachineDeployment", "MachineSet"), + KindsWithFinalizer: sets.New[string]("Machine"), + }, + // The second phase deletes all control plane related machines. + // Note: only setting the finalizer on Machines results in properly testing foreground deletion. + { + Kinds: sets.New[string]("KubeadmControlPlane"), + KindsWithFinalizer: sets.New[string]("Machine"), + }, + // The third phase deletes the infrastructure cluster. + { + Kinds: sets.New[string](), + KindsWithFinalizer: sets.New[string]("DockerCluster"), + }, + }, + } + }) +}) From 269815593848445d8d37acbfeb93517710bb4ec1 Mon Sep 17 00:00:00 2001 From: Christian Schlotter Date: Thu, 19 Sep 2024 17:05:05 +0200 Subject: [PATCH 11/17] review fixes --- test/e2e/cluster_deletion.go | 88 +++++++++++++++++++++++-------- test/e2e/cluster_deletion_test.go | 14 ++--- 2 files changed, 72 insertions(+), 30 deletions(-) diff --git a/test/e2e/cluster_deletion.go b/test/e2e/cluster_deletion.go index 204fc3efc6a1..5bd9a9cdda2d 100644 --- a/test/e2e/cluster_deletion.go +++ b/test/e2e/cluster_deletion.go @@ -25,9 +25,11 @@ import ( . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" + "github.com/pkg/errors" corev1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + kerrors "k8s.io/apimachinery/pkg/util/errors" "k8s.io/apimachinery/pkg/util/sets" "k8s.io/klog/v2" "k8s.io/utils/ptr" @@ -88,15 +90,28 @@ type ClusterDeletionSpecInput struct { } type ClusterDeletionPhase struct { - // Kinds are the Kinds which are supposed to be deleted in this phase. - Kinds sets.Set[string] - // KindsWithFinalizer are the kinds which are considered to be deleted in this phase, but need to be blocking. - KindsWithFinalizer sets.Set[string] + // DeleteKinds are the DeleteKinds which are supposed to be deleted in this phase. + // Note: If Machines is added here, the Machine must have an owner with a kind from DeleteBlockKinds or DeleteKinds. + DeleteKinds sets.Set[string] + // DeleteBlockKinds are the kinds which are considered to be deleted in this phase, but need to be blocking. + // Note: If Machines is added here, the Machine must have an owner with a kind from DeleteBlockKinds or DeleteKinds. + DeleteBlockKinds sets.Set[string] // objects will be filled later by objects which match a kind given in one of the above sets. objects []client.Object } // ClusterDeletionSpec implements a test that verifies that MachineDeployment rolling updates are successful. +// ClusterDeletionSpec goes through the following steps: +// * Create a cluster. +// * Add finalizer to objects defined in input.ClusterDeletionPhases.DeleteBlockKinds. +// * Trigger deletion for the cluster. +// * For each phase: +// - Verify objects expected to get deleted in previous phases are gone. +// - Verify objects expected to be in deletion have a deletionTimestamp set but still exist. +// - Verify objects expected to be deleted in a later phase don't have a deletionTimestamp set. +// - Remove finalizers for objects in deletion. +// +// * Verify the Cluster object is gone. func ClusterDeletionSpec(ctx context.Context, inputGetter func() ClusterDeletionSpecInput) { var ( specName = "cluster-deletion" @@ -177,8 +192,8 @@ func ClusterDeletionSpec(ctx context.Context, inputGetter func() ClusterDeletion for _, phaseInput := range input.ClusterDeletionPhases { relevantKinds = relevantKinds. - Union(phaseInput.KindsWithFinalizer). - Union(phaseInput.Kinds) + Union(phaseInput.DeleteBlockKinds). + Union(phaseInput.DeleteKinds) } // Get all objects relevant to the test by filtering for the kinds of the phases. @@ -191,7 +206,7 @@ func ClusterDeletionSpec(ctx context.Context, inputGetter func() ClusterDeletion // Iterate over the graph and find all objects relevant for the phase. // * Objects matching the kind except for Machines (see below) // * Machines which are owned by one of the kinds which are part of the phase - allKinds := phase.KindsWithFinalizer.Union(phase.Kinds) + allKinds := phase.DeleteBlockKinds.Union(phase.DeleteKinds) allKindsWithoutMachine := allKinds.Clone().Delete("Machine") for _, node := range graph { @@ -224,7 +239,7 @@ func ClusterDeletionSpec(ctx context.Context, inputGetter func() ClusterDeletion phase.objects = append(phase.objects, obj) // If the object's kind is part of kindsWithFinalizer, then additionally the object to the finalizedObjs slice. - if phase.KindsWithFinalizer.Has(obj.GetKind()) { + if phase.DeleteBlockKinds.Has(obj.GetKind()) { objectsWithFinalizer = append(objectsWithFinalizer, obj) } } @@ -234,6 +249,7 @@ func ClusterDeletionSpec(ctx context.Context, inputGetter func() ClusterDeletion } // Update all objects in objectsWithFinalizers and add the finalizer for this test to control when these objects vanish. + By(fmt.Sprintf("Adding finalizer %s on objects to block during cluster deletion", finalizer)) addFinalizer(ctx, input.BootstrapClusterProxy.GetClient(), finalizer, objectsWithFinalizer...) // Trigger the deletion of the Cluster. @@ -245,6 +261,7 @@ func ClusterDeletionSpec(ctx context.Context, inputGetter func() ClusterDeletion var objectsDeleted, objectsInDeletion []client.Object for i, phase := range input.ClusterDeletionPhases { + By(fmt.Sprintf("Verify deletion phase %d/%d", i+1, len(input.ClusterDeletionPhases))) // Objects which previously were in deletion because they waited for our finalizer being removed are now checked to actually had been removed. objectsDeleted = objectsInDeletion // All objects for this phase are expected to get into deletion (still exist and have deletionTimestamp set). @@ -255,28 +272,51 @@ func ClusterDeletionSpec(ctx context.Context, inputGetter func() ClusterDeletion objectsNotInDeletion = append(objectsNotInDeletion, input.ClusterDeletionPhases[j].objects...) } - Eventually(func(g Gomega) { - // Ensure expected objects are in deletion - for _, obj := range objectsInDeletion { - g.Expect(input.BootstrapClusterProxy.GetClient().Get(ctx, client.ObjectKeyFromObject(obj), obj)).ToNot(HaveOccurred()) - g.Expect(obj.GetDeletionTimestamp().IsZero()).To(BeFalse()) - } - + Eventually(func() error { + var errs []error // Ensure deleted objects are gone for _, obj := range objectsDeleted { err := input.BootstrapClusterProxy.GetClient().Get(ctx, client.ObjectKeyFromObject(obj), obj) - g.Expect(err).To(HaveOccurred()) - g.Expect(apierrors.IsNotFound(err)).To(BeTrue()) + if err != nil { + if apierrors.IsNotFound(err) { + continue + } + errs = append(errs, errors.Wrapf(err, "expected %s %s to be deleted", obj.GetObjectKind().GroupVersionKind().Kind, klog.KObj(obj))) + continue + } + errs = append(errs, errors.Wrapf(err, "expected %s %s to be deleted but it still exists", obj.GetObjectKind().GroupVersionKind().Kind, klog.KObj(obj))) + } + + // Ensure expected objects are in deletion + for _, obj := range objectsInDeletion { + if err := input.BootstrapClusterProxy.GetClient().Get(ctx, client.ObjectKeyFromObject(obj), obj); err != nil { + errs = append(errs, errors.Wrapf(err, "checking %s %s is in deletion", obj.GetObjectKind().GroupVersionKind().Kind, klog.KObj(obj))) + continue + } + if obj.GetDeletionTimestamp().IsZero() { + errs = append(errs, errors.Wrapf(err, "expected %s %s to be in deletion", obj.GetObjectKind().GroupVersionKind().Kind, klog.KObj(obj))) + continue + } } // Ensure other objects are not in deletion. for _, obj := range objectsNotInDeletion { - g.Expect(input.BootstrapClusterProxy.GetClient().Get(ctx, client.ObjectKeyFromObject(obj), obj)).ToNot(HaveOccurred()) - g.Expect(obj.GetDeletionTimestamp().IsZero()).To(BeTrue()) + if err := input.BootstrapClusterProxy.GetClient().Get(ctx, client.ObjectKeyFromObject(obj), obj); err != nil { + errs = append(errs, errors.Wrapf(err, "checking %s %s is not in deletion", obj.GetObjectKind().GroupVersionKind().Kind, klog.KObj(obj))) + continue + } + + if !obj.GetDeletionTimestamp().IsZero() { + errs = append(errs, errors.Wrapf(err, "expected %s %s to not be in deletion", obj.GetObjectKind().GroupVersionKind().Kind, klog.KObj(obj))) + continue + } } + + return kerrors.NewAggregate(errs) }).WithTimeout(5 * time.Minute).WithPolling(10 * time.Second).Should(Succeed()) // Remove the test's finalizer from all objects which had been expected to be in deletion to unblock the next phase. + By(fmt.Sprintf("Removing finalizers for phase %d/%d", i+1, len(input.ClusterDeletionPhases))) removeFinalizer(ctx, input.BootstrapClusterProxy.GetClient(), finalizer, objectsInDeletion...) } @@ -321,24 +361,26 @@ func ClusterDeletionSpec(ctx context.Context, inputGetter func() ClusterDeletion func addFinalizer(ctx context.Context, c client.Client, finalizer string, objs ...client.Object) { for _, obj := range objs { - Expect(c.Get(ctx, client.ObjectKeyFromObject(obj), obj)).To(Succeed()) + log.Logf("Adding finalizer for %s %s", obj.GetObjectKind().GroupVersionKind().Kind, klog.KObj(obj)) + Expect(c.Get(ctx, client.ObjectKeyFromObject(obj), obj)).To(Succeed(), fmt.Sprintf("Failed to get %s %s", obj.GetObjectKind().GroupVersionKind().Kind, klog.KObj(obj))) helper, err := patch.NewHelper(obj, c) Expect(err).ToNot(HaveOccurred()) obj.SetFinalizers(append(obj.GetFinalizers(), finalizer)) - Expect(helper.Patch(ctx, obj)).ToNot(HaveOccurred()) + Expect(helper.Patch(ctx, obj)).ToNot(HaveOccurred(), fmt.Sprintf("Failed to add finalizer to %s %s", obj.GetObjectKind().GroupVersionKind().Kind, klog.KObj(obj))) } } func removeFinalizer(ctx context.Context, c client.Client, finalizer string, objs ...client.Object) { for _, obj := range objs { + log.Logf("Removing finalizer for %s %s", obj.GetObjectKind().GroupVersionKind().Kind, klog.KObj(obj)) err := c.Get(ctx, client.ObjectKeyFromObject(obj), obj) if apierrors.IsNotFound(err) { continue } - Expect(err).ToNot(HaveOccurred()) + Expect(err).ToNot(HaveOccurred(), fmt.Sprintf("Failed to get %s %s", obj.GetObjectKind().GroupVersionKind().Kind, klog.KObj(obj))) helper, err := patch.NewHelper(obj, c) Expect(err).ToNot(HaveOccurred()) @@ -352,6 +394,6 @@ func removeFinalizer(ctx context.Context, c client.Client, finalizer string, obj } obj.SetFinalizers(finalizers) - Expect(helper.Patch(ctx, obj)).ToNot(HaveOccurred()) + Expect(helper.Patch(ctx, obj)).ToNot(HaveOccurred(), fmt.Sprintf("Failed to remove finalizer from %s %s", obj.GetObjectKind().GroupVersionKind().Kind, klog.KObj(obj))) } } diff --git a/test/e2e/cluster_deletion_test.go b/test/e2e/cluster_deletion_test.go index 7c70d9dc191c..42ee4976b30c 100644 --- a/test/e2e/cluster_deletion_test.go +++ b/test/e2e/cluster_deletion_test.go @@ -25,7 +25,7 @@ import ( "k8s.io/utils/ptr" ) -var _ = Describe("When following the Cluster API quick-start with ClusterClass [PR-Blocking] [ClusterClass]", func() { +var _ = Describe("When performing cluster deletion with ClusterClass [PR-Blocking] [ClusterClass]", func() { ClusterDeletionSpec(ctx, func() ClusterDeletionSpecInput { return ClusterDeletionSpecInput{ E2EConfig: e2eConfig, @@ -41,19 +41,19 @@ var _ = Describe("When following the Cluster API quick-start with ClusterClass [ // The first phase deletes all worker related machines. // Note: only setting the finalizer on Machines results in properly testing foreground deletion. { - Kinds: sets.New[string]("MachineDeployment", "MachineSet"), - KindsWithFinalizer: sets.New[string]("Machine"), + DeleteKinds: sets.New[string]("MachineDeployment", "MachineSet"), + DeleteBlockKinds: sets.New[string]("Machine"), }, // The second phase deletes all control plane related machines. // Note: only setting the finalizer on Machines results in properly testing foreground deletion. { - Kinds: sets.New[string]("KubeadmControlPlane"), - KindsWithFinalizer: sets.New[string]("Machine"), + DeleteKinds: sets.New[string]("KubeadmControlPlane"), + DeleteBlockKinds: sets.New[string]("Machine"), }, // The third phase deletes the infrastructure cluster. { - Kinds: sets.New[string](), - KindsWithFinalizer: sets.New[string]("DockerCluster"), + DeleteKinds: sets.New[string](), + DeleteBlockKinds: sets.New[string]("DockerCluster"), }, }, } From d5a93aef8fbe78259ee8b04d4fb15f3b2d648c70 Mon Sep 17 00:00:00 2001 From: Christian Schlotter Date: Mon, 23 Sep 2024 20:10:41 +0200 Subject: [PATCH 12/17] review fixes --- test/e2e/cluster_deletion.go | 74 +++++++++---------------------- test/e2e/cluster_deletion_test.go | 53 ++++++++++++++++++---- 2 files changed, 66 insertions(+), 61 deletions(-) diff --git a/test/e2e/cluster_deletion.go b/test/e2e/cluster_deletion.go index 5bd9a9cdda2d..9ea056b81717 100644 --- a/test/e2e/cluster_deletion.go +++ b/test/e2e/cluster_deletion.go @@ -30,7 +30,6 @@ import ( apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" kerrors "k8s.io/apimachinery/pkg/util/errors" - "k8s.io/apimachinery/pkg/util/sets" "k8s.io/klog/v2" "k8s.io/utils/ptr" "sigs.k8s.io/controller-runtime/pkg/client" @@ -90,17 +89,17 @@ type ClusterDeletionSpecInput struct { } type ClusterDeletionPhase struct { - // DeleteKinds are the DeleteKinds which are supposed to be deleted in this phase. - // Note: If Machines is added here, the Machine must have an owner with a kind from DeleteBlockKinds or DeleteKinds. - DeleteKinds sets.Set[string] - // DeleteBlockKinds are the kinds which are considered to be deleted in this phase, but need to be blocking. - // Note: If Machines is added here, the Machine must have an owner with a kind from DeleteBlockKinds or DeleteKinds. - DeleteBlockKinds sets.Set[string] - // objects will be filled later by objects which match a kind given in one of the above sets. + // DeletionSelector identifies if a node should be considered to get deleted during this phase. + DeletionSelector func(node clusterctlcluster.OwnerGraphNode) bool + // IsBlocking is a filter on top of the DeletionSelector to identify which nodes should block the deletion in this phase. + // The deletion of all objects in this phase gets blocked by adding a finalizer to this nodes to ensure the test can + // assert when the objects should be in deletion and actually get removed by control the removal of the finalizer. + IsBlocking func(node clusterctlcluster.OwnerGraphNode) bool + + // objects will be filled later by objects which matched the DeletionSelector objects []client.Object } -// ClusterDeletionSpec implements a test that verifies that MachineDeployment rolling updates are successful. // ClusterDeletionSpec goes through the following steps: // * Create a cluster. // * Add finalizer to objects defined in input.ClusterDeletionPhases.DeleteBlockKinds. @@ -188,48 +187,19 @@ func ClusterDeletionSpec(ctx context.Context, inputGetter func() ClusterDeletion }, }, clusterResources) - relevantKinds := sets.Set[string]{} - - for _, phaseInput := range input.ClusterDeletionPhases { - relevantKinds = relevantKinds. - Union(phaseInput.DeleteBlockKinds). - Union(phaseInput.DeleteKinds) - } - // Get all objects relevant to the test by filtering for the kinds of the phases. - graph, err := clusterctlcluster.GetOwnerGraph(ctx, namespace.GetName(), input.BootstrapClusterProxy.GetKubeconfigPath(), func(u unstructured.Unstructured) bool { - return relevantKinds.Has(u.GetKind()) - }) + graph, err := clusterctlcluster.GetOwnerGraph(ctx, namespace.GetName(), input.BootstrapClusterProxy.GetKubeconfigPath(), clusterctlcluster.FilterClusterObjectsWithNameFilter(clusterName)) Expect(err).ToNot(HaveOccurred()) for i, phase := range input.ClusterDeletionPhases { // Iterate over the graph and find all objects relevant for the phase. // * Objects matching the kind except for Machines (see below) // * Machines which are owned by one of the kinds which are part of the phase - allKinds := phase.DeleteBlockKinds.Union(phase.DeleteKinds) - allKindsWithoutMachine := allKinds.Clone().Delete("Machine") - for _, node := range graph { - // Only add objects which match a defined kind for the phase. - if !allKinds.Has(node.Object.Kind) { + if !phase.DeletionSelector(node) { continue } - // Special handling for machines: only add machines which are owned by one of the given kinds. - if node.Object.Kind == "Machine" { - isOwnedByKind := false - for _, owner := range node.Owners { - if allKindsWithoutMachine.Has(owner.Kind) { - isOwnedByKind = true - break - } - } - // Ignore Machines which are not owned by one of the given kinds. - if !isOwnedByKind { - continue - } - } - obj := new(unstructured.Unstructured) obj.SetAPIVersion(node.Object.APIVersion) obj.SetKind(node.Object.Kind) @@ -238,8 +208,8 @@ func ClusterDeletionSpec(ctx context.Context, inputGetter func() ClusterDeletion phase.objects = append(phase.objects, obj) - // If the object's kind is part of kindsWithFinalizer, then additionally the object to the finalizedObjs slice. - if phase.DeleteBlockKinds.Has(obj.GetKind()) { + // Add the object to the objectsWithFinalizer array if it should be blocking. + if phase.IsBlocking(node) { objectsWithFinalizer = append(objectsWithFinalizer, obj) } } @@ -258,24 +228,24 @@ func ClusterDeletionSpec(ctx context.Context, inputGetter func() ClusterDeletion Deleter: input.BootstrapClusterProxy.GetClient(), }) - var objectsDeleted, objectsInDeletion []client.Object + var expectedObjectsDeleted, expectedObjectsInDeletion []client.Object for i, phase := range input.ClusterDeletionPhases { By(fmt.Sprintf("Verify deletion phase %d/%d", i+1, len(input.ClusterDeletionPhases))) // Objects which previously were in deletion because they waited for our finalizer being removed are now checked to actually had been removed. - objectsDeleted = objectsInDeletion + expectedObjectsDeleted = expectedObjectsInDeletion // All objects for this phase are expected to get into deletion (still exist and have deletionTimestamp set). - objectsInDeletion = phase.objects + expectedObjectsInDeletion = phase.objects // All objects which are part of future phases are expected to not yet be in deletion. - objectsNotInDeletion := []client.Object{} + expectedObjectsNotInDeletion := []client.Object{} for j := i + 1; j < len(input.ClusterDeletionPhases); j++ { - objectsNotInDeletion = append(objectsNotInDeletion, input.ClusterDeletionPhases[j].objects...) + expectedObjectsNotInDeletion = append(expectedObjectsNotInDeletion, input.ClusterDeletionPhases[j].objects...) } Eventually(func() error { var errs []error // Ensure deleted objects are gone - for _, obj := range objectsDeleted { + for _, obj := range expectedObjectsDeleted { err := input.BootstrapClusterProxy.GetClient().Get(ctx, client.ObjectKeyFromObject(obj), obj) if err != nil { if apierrors.IsNotFound(err) { @@ -284,11 +254,11 @@ func ClusterDeletionSpec(ctx context.Context, inputGetter func() ClusterDeletion errs = append(errs, errors.Wrapf(err, "expected %s %s to be deleted", obj.GetObjectKind().GroupVersionKind().Kind, klog.KObj(obj))) continue } - errs = append(errs, errors.Wrapf(err, "expected %s %s to be deleted but it still exists", obj.GetObjectKind().GroupVersionKind().Kind, klog.KObj(obj))) + errs = append(errs, fmt.Errorf("expected %s %s to be deleted but it still exists", obj.GetObjectKind().GroupVersionKind().Kind, klog.KObj(obj))) } // Ensure expected objects are in deletion - for _, obj := range objectsInDeletion { + for _, obj := range expectedObjectsInDeletion { if err := input.BootstrapClusterProxy.GetClient().Get(ctx, client.ObjectKeyFromObject(obj), obj); err != nil { errs = append(errs, errors.Wrapf(err, "checking %s %s is in deletion", obj.GetObjectKind().GroupVersionKind().Kind, klog.KObj(obj))) continue @@ -300,7 +270,7 @@ func ClusterDeletionSpec(ctx context.Context, inputGetter func() ClusterDeletion } // Ensure other objects are not in deletion. - for _, obj := range objectsNotInDeletion { + for _, obj := range expectedObjectsNotInDeletion { if err := input.BootstrapClusterProxy.GetClient().Get(ctx, client.ObjectKeyFromObject(obj), obj); err != nil { errs = append(errs, errors.Wrapf(err, "checking %s %s is not in deletion", obj.GetObjectKind().GroupVersionKind().Kind, klog.KObj(obj))) continue @@ -317,7 +287,7 @@ func ClusterDeletionSpec(ctx context.Context, inputGetter func() ClusterDeletion // Remove the test's finalizer from all objects which had been expected to be in deletion to unblock the next phase. By(fmt.Sprintf("Removing finalizers for phase %d/%d", i+1, len(input.ClusterDeletionPhases))) - removeFinalizer(ctx, input.BootstrapClusterProxy.GetClient(), finalizer, objectsInDeletion...) + removeFinalizer(ctx, input.BootstrapClusterProxy.GetClient(), finalizer, expectedObjectsInDeletion...) } // The last phase should unblock the cluster to actually have been removed. diff --git a/test/e2e/cluster_deletion_test.go b/test/e2e/cluster_deletion_test.go index 42ee4976b30c..f0b53533a0db 100644 --- a/test/e2e/cluster_deletion_test.go +++ b/test/e2e/cluster_deletion_test.go @@ -21,11 +21,14 @@ package e2e import ( . "github.com/onsi/ginkgo/v2" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/sets" "k8s.io/utils/ptr" + + clusterctlcluster "sigs.k8s.io/cluster-api/cmd/clusterctl/client/cluster" ) -var _ = Describe("When performing cluster deletion with ClusterClass [PR-Blocking] [ClusterClass]", func() { +var _ = Describe("When performing cluster deletion with ClusterClass [ClusterClass]", func() { ClusterDeletionSpec(ctx, func() ClusterDeletionSpecInput { return ClusterDeletionSpecInput{ E2EConfig: e2eConfig, @@ -39,23 +42,55 @@ var _ = Describe("When performing cluster deletion with ClusterClass [PR-Blockin ClusterDeletionPhases: []ClusterDeletionPhase{ // The first phase deletes all worker related machines. - // Note: only setting the finalizer on Machines results in properly testing foreground deletion. { - DeleteKinds: sets.New[string]("MachineDeployment", "MachineSet"), - DeleteBlockKinds: sets.New[string]("Machine"), + // All Machines owned by MachineDeployments or MachineSets and themselves should be deleted in this phase. + DeletionSelector: func(node clusterctlcluster.OwnerGraphNode) bool { + if node.Object.Kind == "Machine" && hasOwner(node.Owners, "MachineDeployment", "MachineSet") { + return true + } + return sets.New[string]("MachineDeployment", "MachineSet").Has(node.Object.Kind) + }, + // Of the above, only Machines should be considered to be blocking to test foreground deletion. + IsBlocking: func(node clusterctlcluster.OwnerGraphNode) bool { + return node.Object.Kind == "Machine" + }, }, // The second phase deletes all control plane related machines. - // Note: only setting the finalizer on Machines results in properly testing foreground deletion. { - DeleteKinds: sets.New[string]("KubeadmControlPlane"), - DeleteBlockKinds: sets.New[string]("Machine"), + // All Machines owned by KubeadmControlPlane and themselves should be deleted in this phase. + DeletionSelector: func(node clusterctlcluster.OwnerGraphNode) bool { + if node.Object.Kind == "Machine" && hasOwner(node.Owners, "KubeadmControlPlane") { + return true + } + return node.Object.Kind == "KubeadmControlPlane" + }, + // Of the above, only Machines should be considered to be blocking to test foreground deletion. + IsBlocking: func(node clusterctlcluster.OwnerGraphNode) bool { + return node.Object.Kind == "Machine" + }, }, // The third phase deletes the infrastructure cluster. { - DeleteKinds: sets.New[string](), - DeleteBlockKinds: sets.New[string]("DockerCluster"), + // The DockerCluster should be deleted in this phase. + DeletionSelector: func(node clusterctlcluster.OwnerGraphNode) bool { + return node.Object.Kind == "DockerCluster" + }, + // Of the above, the DockerCluster should be considered to be blocking. + IsBlocking: func(node clusterctlcluster.OwnerGraphNode) bool { + return node.Object.Kind == "DockerCluster" + }, }, }, } }) }) + +func hasOwner(ownerReferences []metav1.OwnerReference, owners ...string) bool { + ownersSet := sets.New[string](owners...) + for _, owner := range ownerReferences { + if ownersSet.Has(owner.Kind) { + return true + } + } + return false +} From 6032cf211b05a64190881dfb2f3bf4de57042e75 Mon Sep 17 00:00:00 2001 From: Christian Schlotter Date: Tue, 24 Sep 2024 15:36:47 +0200 Subject: [PATCH 13/17] Review Fixes --- test/e2e/cluster_deletion.go | 249 +++++++++++++++++------------- test/e2e/cluster_deletion_test.go | 36 ++--- 2 files changed, 161 insertions(+), 124 deletions(-) diff --git a/test/e2e/cluster_deletion.go b/test/e2e/cluster_deletion.go index 9ea056b81717..b9cdb8a87d66 100644 --- a/test/e2e/cluster_deletion.go +++ b/test/e2e/cluster_deletion.go @@ -28,12 +28,14 @@ import ( "github.com/pkg/errors" corev1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" kerrors "k8s.io/apimachinery/pkg/util/errors" "k8s.io/klog/v2" "k8s.io/utils/ptr" "sigs.k8s.io/controller-runtime/pkg/client" + clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" clusterctlcluster "sigs.k8s.io/cluster-api/cmd/clusterctl/client/cluster" "sigs.k8s.io/cluster-api/test/e2e/internal/log" "sigs.k8s.io/cluster-api/test/framework" @@ -89,37 +91,43 @@ type ClusterDeletionSpecInput struct { } type ClusterDeletionPhase struct { - // DeletionSelector identifies if a node should be considered to get deleted during this phase. - DeletionSelector func(node clusterctlcluster.OwnerGraphNode) bool - // IsBlocking is a filter on top of the DeletionSelector to identify which nodes should block the deletion in this phase. - // The deletion of all objects in this phase gets blocked by adding a finalizer to this nodes to ensure the test can - // assert when the objects should be in deletion and actually get removed by control the removal of the finalizer. - IsBlocking func(node clusterctlcluster.OwnerGraphNode) bool - - // objects will be filled later by objects which matched the DeletionSelector - objects []client.Object + // ObjectFilter identifies if an object should be considered to get deleted during this phase. + // During the test the identified objects are expected to only have an deletionTimestamp when this phase is tested + // and be gone after unblocking this phases blocking objects. + ObjectFilter func(objectReference corev1.ObjectReference, objectOwnerReferences []metav1.OwnerReference) bool + // BlockingObjectFilter is a filter on top of the ObjectFilter to identify which objects should block the deletion in + // this phase. The identified objects will get a finalizer added before the deletion of the cluster starts. + // After a successful verification that all objects are in the correct state the finalizer gets removed to unblock + // the deletion and go to the next phase. + BlockingObjectFilter func(objectReference corev1.ObjectReference, objectOwnerReferences []metav1.OwnerReference) bool } // ClusterDeletionSpec goes through the following steps: // * Create a cluster. -// * Add finalizer to objects defined in input.ClusterDeletionPhases.DeleteBlockKinds. +// * Add a finalizer to the Cluster object. +// * Add a finalizer to objects identified by input.ClusterDeletionPhases.BlockingObjectFilter. // * Trigger deletion for the cluster. // * For each phase: -// - Verify objects expected to get deleted in previous phases are gone. -// - Verify objects expected to be in deletion have a deletionTimestamp set but still exist. +// - Verify objects of the previous phase are gone. +// - Verify objects to be deleted in this phase have a deletionTimestamp set but still exist. // - Verify objects expected to be deleted in a later phase don't have a deletionTimestamp set. -// - Remove finalizers for objects in deletion. +// - Remove finalizers for this phase's BlockingObjects to unblock deletion of them. +// +// * Do a final verification: +// - Verify objects expected to get deleted in the last phase are gone. +// - Verify the Cluster object has a deletionTimestamp set but still exists. +// - Remove finalizer for the Cluster object. // // * Verify the Cluster object is gone. func ClusterDeletionSpec(ctx context.Context, inputGetter func() ClusterDeletionSpecInput) { var ( - specName = "cluster-deletion" - input ClusterDeletionSpecInput - namespace *corev1.Namespace - cancelWatches context.CancelFunc - clusterResources *clusterctl.ApplyClusterTemplateAndWaitResult - finalizer = "test.cluster.x-k8s.io/cluster-deletion" - objectsWithFinalizer []client.Object + specName = "cluster-deletion" + input ClusterDeletionSpecInput + namespace *corev1.Namespace + cancelWatches context.CancelFunc + clusterResources *clusterctl.ApplyClusterTemplateAndWaitResult + finalizer = "test.cluster.x-k8s.io/cluster-deletion" + blockingObjects []client.Object ) BeforeEach(func() { @@ -187,40 +195,13 @@ func ClusterDeletionSpec(ctx context.Context, inputGetter func() ClusterDeletion }, }, clusterResources) - // Get all objects relevant to the test by filtering for the kinds of the phases. - graph, err := clusterctlcluster.GetOwnerGraph(ctx, namespace.GetName(), input.BootstrapClusterProxy.GetKubeconfigPath(), clusterctlcluster.FilterClusterObjectsWithNameFilter(clusterName)) - Expect(err).ToNot(HaveOccurred()) - - for i, phase := range input.ClusterDeletionPhases { - // Iterate over the graph and find all objects relevant for the phase. - // * Objects matching the kind except for Machines (see below) - // * Machines which are owned by one of the kinds which are part of the phase - for _, node := range graph { - if !phase.DeletionSelector(node) { - continue - } - - obj := new(unstructured.Unstructured) - obj.SetAPIVersion(node.Object.APIVersion) - obj.SetKind(node.Object.Kind) - obj.SetName(node.Object.Name) - obj.SetNamespace(namespace.GetName()) - - phase.objects = append(phase.objects, obj) + // Get all objects per deletion phase and the list of blocking objects. + var objectsPerPhase [][]client.Object + objectsPerPhase, blockingObjects = getDeletionPhaseObjects(ctx, input.BootstrapClusterProxy, clusterResources.Cluster, input.ClusterDeletionPhases) - // Add the object to the objectsWithFinalizer array if it should be blocking. - if phase.IsBlocking(node) { - objectsWithFinalizer = append(objectsWithFinalizer, obj) - } - } - - // Update the phase in input.ClusterDeletionPhases - input.ClusterDeletionPhases[i] = phase - } - - // Update all objects in objectsWithFinalizers and add the finalizer for this test to control when these objects vanish. + // Add a finalizer to all objects in blockingObjects to control when these objects are gone. By(fmt.Sprintf("Adding finalizer %s on objects to block during cluster deletion", finalizer)) - addFinalizer(ctx, input.BootstrapClusterProxy.GetClient(), finalizer, objectsWithFinalizer...) + addFinalizer(ctx, input.BootstrapClusterProxy.GetClient(), finalizer, blockingObjects...) // Trigger the deletion of the Cluster. framework.DeleteCluster(ctx, framework.DeleteClusterInput{ @@ -230,67 +211,43 @@ func ClusterDeletionSpec(ctx context.Context, inputGetter func() ClusterDeletion var expectedObjectsDeleted, expectedObjectsInDeletion []client.Object - for i, phase := range input.ClusterDeletionPhases { - By(fmt.Sprintf("Verify deletion phase %d/%d", i+1, len(input.ClusterDeletionPhases))) - // Objects which previously were in deletion because they waited for our finalizer being removed are now checked to actually had been removed. + // Verify deletion for each phase. + for i, phaseObjects := range objectsPerPhase { + By(fmt.Sprintf("Verify deletion phase %d/%d", i+1, len(objectsPerPhase))) + // Expect the objects of the previous phase to be deleted. expectedObjectsDeleted = expectedObjectsInDeletion - // All objects for this phase are expected to get into deletion (still exist and have deletionTimestamp set). - expectedObjectsInDeletion = phase.objects - // All objects which are part of future phases are expected to not yet be in deletion. + // Expect the objects of this phase to have a deletionTimestamp set, but still exist due to the blocking objects having the finalizer. + expectedObjectsInDeletion = phaseObjects + // Expect the objects of upcoming phases to not yet have a deletionTimestamp set. expectedObjectsNotInDeletion := []client.Object{} - for j := i + 1; j < len(input.ClusterDeletionPhases); j++ { - expectedObjectsNotInDeletion = append(expectedObjectsNotInDeletion, input.ClusterDeletionPhases[j].objects...) + for j := i + 1; j < len(objectsPerPhase); j++ { + expectedObjectsNotInDeletion = append(expectedObjectsNotInDeletion, objectsPerPhase[j]...) } - Eventually(func() error { - var errs []error - // Ensure deleted objects are gone - for _, obj := range expectedObjectsDeleted { - err := input.BootstrapClusterProxy.GetClient().Get(ctx, client.ObjectKeyFromObject(obj), obj) - if err != nil { - if apierrors.IsNotFound(err) { - continue - } - errs = append(errs, errors.Wrapf(err, "expected %s %s to be deleted", obj.GetObjectKind().GroupVersionKind().Kind, klog.KObj(obj))) - continue - } - errs = append(errs, fmt.Errorf("expected %s %s to be deleted but it still exists", obj.GetObjectKind().GroupVersionKind().Kind, klog.KObj(obj))) - } - - // Ensure expected objects are in deletion - for _, obj := range expectedObjectsInDeletion { - if err := input.BootstrapClusterProxy.GetClient().Get(ctx, client.ObjectKeyFromObject(obj), obj); err != nil { - errs = append(errs, errors.Wrapf(err, "checking %s %s is in deletion", obj.GetObjectKind().GroupVersionKind().Kind, klog.KObj(obj))) - continue - } - if obj.GetDeletionTimestamp().IsZero() { - errs = append(errs, errors.Wrapf(err, "expected %s %s to be in deletion", obj.GetObjectKind().GroupVersionKind().Kind, klog.KObj(obj))) - continue - } - } - - // Ensure other objects are not in deletion. - for _, obj := range expectedObjectsNotInDeletion { - if err := input.BootstrapClusterProxy.GetClient().Get(ctx, client.ObjectKeyFromObject(obj), obj); err != nil { - errs = append(errs, errors.Wrapf(err, "checking %s %s is not in deletion", obj.GetObjectKind().GroupVersionKind().Kind, klog.KObj(obj))) - continue - } - - if !obj.GetDeletionTimestamp().IsZero() { - errs = append(errs, errors.Wrapf(err, "expected %s %s to not be in deletion", obj.GetObjectKind().GroupVersionKind().Kind, klog.KObj(obj))) - continue - } - } - - return kerrors.NewAggregate(errs) - }).WithTimeout(5 * time.Minute).WithPolling(10 * time.Second).Should(Succeed()) + assertDeletionPhase(ctx, input.BootstrapClusterProxy.GetClient(), + expectedObjectsDeleted, + expectedObjectsInDeletion, + expectedObjectsNotInDeletion, + ) // Remove the test's finalizer from all objects which had been expected to be in deletion to unblock the next phase. By(fmt.Sprintf("Removing finalizers for phase %d/%d", i+1, len(input.ClusterDeletionPhases))) removeFinalizer(ctx, input.BootstrapClusterProxy.GetClient(), finalizer, expectedObjectsInDeletion...) } - // The last phase should unblock the cluster to actually have been removed. + By("Final deletion verification") + // Verify that all objects of the last phase are gone and the cluster does still exist. + expectedObjectsDeleted = expectedObjectsInDeletion + assertDeletionPhase(ctx, input.BootstrapClusterProxy.GetClient(), + expectedObjectsDeleted, + []client.Object{clusterResources.Cluster}, + nil, + ) + + // Remove the test's finalizer from the cluster object. + By("Removing finalizers for Cluster") + removeFinalizer(ctx, input.BootstrapClusterProxy.GetClient(), finalizer, clusterResources.Cluster) + log.Logf("Waiting for the Cluster %s to be deleted", klog.KObj(clusterResources.Cluster)) framework.WaitForClusterDeleted(ctx, framework.WaitForClusterDeletedInput{ Client: input.BootstrapClusterProxy.GetClient(), @@ -307,7 +264,7 @@ func ClusterDeletionSpec(ctx context.Context, inputGetter func() ClusterDeletion if !input.SkipCleanup { // Remove finalizers we added to block normal deletion. - removeFinalizer(ctx, input.BootstrapClusterProxy.GetClient(), finalizer, objectsWithFinalizer...) + removeFinalizer(ctx, input.BootstrapClusterProxy.GetClient(), finalizer, blockingObjects...) By(fmt.Sprintf("Deleting cluster %s", klog.KObj(clusterResources.Cluster))) // While /~https://github.com/kubernetes-sigs/cluster-api/issues/2955 is addressed in future iterations, there is a chance @@ -329,6 +286,45 @@ func ClusterDeletionSpec(ctx context.Context, inputGetter func() ClusterDeletion }) } +// getDeletionPhaseObjects gets the OwnerGraph for the cluster and returns all objects per phase and an additional array +// with all objects which should block during the cluster deletion. +func getDeletionPhaseObjects(ctx context.Context, bootstrapClusterProxy framework.ClusterProxy, cluster *clusterv1.Cluster, phases []ClusterDeletionPhase) (objectsPerPhase [][]client.Object, blockingObjects []client.Object) { + // Add Cluster object to blockingObjects to control when it gets actually removed during the test + // and to be able to cleanup the Cluster during Teardown on failures. + blockingObjects = append(blockingObjects, cluster) + + // Get all objects relevant to the test by filtering for the kinds of the phases. + graph, err := clusterctlcluster.GetOwnerGraph(ctx, cluster.GetNamespace(), bootstrapClusterProxy.GetKubeconfigPath(), clusterctlcluster.FilterClusterObjectsWithNameFilter(cluster.GetName())) + Expect(err).ToNot(HaveOccurred()) + + for _, phase := range phases { + objects := []client.Object{} + // Iterate over the graph and find all objects relevant for the phase and which of them should be blocking. + for _, node := range graph { + // Check if the node should be part of the phase. + if !phase.ObjectFilter(node.Object, node.Owners) { + continue + } + + obj := new(unstructured.Unstructured) + obj.SetAPIVersion(node.Object.APIVersion) + obj.SetKind(node.Object.Kind) + obj.SetName(node.Object.Name) + obj.SetNamespace(cluster.GetNamespace()) + + objects = append(objects, obj) + + // Add the object to the objectsWithFinalizer array if it should be blocking. + if phase.BlockingObjectFilter(node.Object, node.Owners) { + blockingObjects = append(blockingObjects, obj) + } + } + + objectsPerPhase = append(objectsPerPhase, objects) + } + return +} + func addFinalizer(ctx context.Context, c client.Client, finalizer string, objs ...client.Object) { for _, obj := range objs { log.Logf("Adding finalizer for %s %s", obj.GetObjectKind().GroupVersionKind().Kind, klog.KObj(obj)) @@ -367,3 +363,48 @@ func removeFinalizer(ctx context.Context, c client.Client, finalizer string, obj Expect(helper.Patch(ctx, obj)).ToNot(HaveOccurred(), fmt.Sprintf("Failed to remove finalizer from %s %s", obj.GetObjectKind().GroupVersionKind().Kind, klog.KObj(obj))) } } + +func assertDeletionPhase(ctx context.Context, c client.Client, expectedObjectsDeleted, expectedObjectsInDeletion, expectedObjectsNotInDeletion []client.Object) { + Eventually(func() error { + var errs []error + // Ensure deleted objects are gone + for _, obj := range expectedObjectsDeleted { + err := c.Get(ctx, client.ObjectKeyFromObject(obj), obj) + if err != nil { + if apierrors.IsNotFound(err) { + continue + } + errs = append(errs, errors.Wrapf(err, "expected %s %s to be deleted", obj.GetObjectKind().GroupVersionKind().Kind, klog.KObj(obj))) + continue + } + errs = append(errs, fmt.Errorf("expected %s %s to be deleted but it still exists", obj.GetObjectKind().GroupVersionKind().Kind, klog.KObj(obj))) + } + + // Ensure expected objects are in deletion + for _, obj := range expectedObjectsInDeletion { + if err := c.Get(ctx, client.ObjectKeyFromObject(obj), obj); err != nil { + errs = append(errs, errors.Wrapf(err, "checking %s %s is in deletion", obj.GetObjectKind().GroupVersionKind().Kind, klog.KObj(obj))) + continue + } + if obj.GetDeletionTimestamp().IsZero() { + errs = append(errs, errors.Errorf("expected %s %s to be in deletion but deletionTimestamp is not set", obj.GetObjectKind().GroupVersionKind().Kind, klog.KObj(obj))) + continue + } + } + + // Ensure other objects are not in deletion. + for _, obj := range expectedObjectsNotInDeletion { + if err := c.Get(ctx, client.ObjectKeyFromObject(obj), obj); err != nil { + errs = append(errs, errors.Wrapf(err, "checking %s %s is not in deletion", obj.GetObjectKind().GroupVersionKind().Kind, klog.KObj(obj))) + continue + } + + if !obj.GetDeletionTimestamp().IsZero() { + errs = append(errs, errors.Errorf("expected %s %s to not be in deletion but deletionTimestamp is set", obj.GetObjectKind().GroupVersionKind().Kind, klog.KObj(obj))) + continue + } + } + + return kerrors.NewAggregate(errs) + }).WithTimeout(5 * time.Minute).WithPolling(10 * time.Second).Should(Succeed()) +} diff --git a/test/e2e/cluster_deletion_test.go b/test/e2e/cluster_deletion_test.go index f0b53533a0db..7a4b8bde6555 100644 --- a/test/e2e/cluster_deletion_test.go +++ b/test/e2e/cluster_deletion_test.go @@ -21,11 +21,10 @@ package e2e import ( . "github.com/onsi/ginkgo/v2" + corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/sets" "k8s.io/utils/ptr" - - clusterctlcluster "sigs.k8s.io/cluster-api/cmd/clusterctl/client/cluster" ) var _ = Describe("When performing cluster deletion with ClusterClass [ClusterClass]", func() { @@ -41,43 +40,40 @@ var _ = Describe("When performing cluster deletion with ClusterClass [ClusterCla InfrastructureProvider: ptr.To("docker"), ClusterDeletionPhases: []ClusterDeletionPhase{ - // The first phase deletes all worker related machines. { // All Machines owned by MachineDeployments or MachineSets and themselves should be deleted in this phase. - DeletionSelector: func(node clusterctlcluster.OwnerGraphNode) bool { - if node.Object.Kind == "Machine" && hasOwner(node.Owners, "MachineDeployment", "MachineSet") { + ObjectFilter: func(objectReference corev1.ObjectReference, objectOwnerReferences []metav1.OwnerReference) bool { + if objectReference.Kind == "Machine" && hasOwner(objectOwnerReferences, "MachineDeployment", "MachineSet") { return true } - return sets.New[string]("MachineDeployment", "MachineSet").Has(node.Object.Kind) + return sets.New[string]("MachineDeployment", "MachineSet").Has(objectReference.Kind) }, // Of the above, only Machines should be considered to be blocking to test foreground deletion. - IsBlocking: func(node clusterctlcluster.OwnerGraphNode) bool { - return node.Object.Kind == "Machine" + BlockingObjectFilter: func(objectReference corev1.ObjectReference, objectOwnerReferences []metav1.OwnerReference) bool { + return objectReference.Kind == "Machine" }, }, - // The second phase deletes all control plane related machines. { - // All Machines owned by KubeadmControlPlane and themselves should be deleted in this phase. - DeletionSelector: func(node clusterctlcluster.OwnerGraphNode) bool { - if node.Object.Kind == "Machine" && hasOwner(node.Owners, "KubeadmControlPlane") { + // All Machines owned by KubeadmControlPlane and itself should be deleted in this phase. + ObjectFilter: func(objectReference corev1.ObjectReference, objectOwnerReferences []metav1.OwnerReference) bool { + if objectReference.Kind == "Machine" && hasOwner(objectOwnerReferences, "KubeadmControlPlane") { return true } - return node.Object.Kind == "KubeadmControlPlane" + return objectReference.Kind == "KubeadmControlPlane" }, // Of the above, only Machines should be considered to be blocking to test foreground deletion. - IsBlocking: func(node clusterctlcluster.OwnerGraphNode) bool { - return node.Object.Kind == "Machine" + BlockingObjectFilter: func(objectReference corev1.ObjectReference, objectOwnerReferences []metav1.OwnerReference) bool { + return objectReference.Kind == "Machine" }, }, - // The third phase deletes the infrastructure cluster. { // The DockerCluster should be deleted in this phase. - DeletionSelector: func(node clusterctlcluster.OwnerGraphNode) bool { - return node.Object.Kind == "DockerCluster" + ObjectFilter: func(objectReference corev1.ObjectReference, objectOwnerReferences []metav1.OwnerReference) bool { + return objectReference.Kind == "DockerCluster" }, // Of the above, the DockerCluster should be considered to be blocking. - IsBlocking: func(node clusterctlcluster.OwnerGraphNode) bool { - return node.Object.Kind == "DockerCluster" + BlockingObjectFilter: func(objectReference corev1.ObjectReference, objectOwnerReferences []metav1.OwnerReference) bool { + return objectReference.Kind == "DockerCluster" }, }, }, From 6c46d1ffa4b83ae2701512dec9ecbbc710c4f3d5 Mon Sep 17 00:00:00 2001 From: Christian Schlotter Date: Tue, 24 Sep 2024 16:40:39 +0200 Subject: [PATCH 14/17] lint fixes --- test/e2e/cluster_deletion.go | 2 +- test/e2e/cluster_deletion_test.go | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/test/e2e/cluster_deletion.go b/test/e2e/cluster_deletion.go index b9cdb8a87d66..b9c27ba55e6f 100644 --- a/test/e2e/cluster_deletion.go +++ b/test/e2e/cluster_deletion.go @@ -322,7 +322,7 @@ func getDeletionPhaseObjects(ctx context.Context, bootstrapClusterProxy framewor objectsPerPhase = append(objectsPerPhase, objects) } - return + return objectsPerPhase, blockingObjects } func addFinalizer(ctx context.Context, c client.Client, finalizer string, objs ...client.Object) { diff --git a/test/e2e/cluster_deletion_test.go b/test/e2e/cluster_deletion_test.go index 7a4b8bde6555..ec11d1926c6c 100644 --- a/test/e2e/cluster_deletion_test.go +++ b/test/e2e/cluster_deletion_test.go @@ -49,7 +49,7 @@ var _ = Describe("When performing cluster deletion with ClusterClass [ClusterCla return sets.New[string]("MachineDeployment", "MachineSet").Has(objectReference.Kind) }, // Of the above, only Machines should be considered to be blocking to test foreground deletion. - BlockingObjectFilter: func(objectReference corev1.ObjectReference, objectOwnerReferences []metav1.OwnerReference) bool { + BlockingObjectFilter: func(objectReference corev1.ObjectReference, _ []metav1.OwnerReference) bool { return objectReference.Kind == "Machine" }, }, @@ -62,17 +62,17 @@ var _ = Describe("When performing cluster deletion with ClusterClass [ClusterCla return objectReference.Kind == "KubeadmControlPlane" }, // Of the above, only Machines should be considered to be blocking to test foreground deletion. - BlockingObjectFilter: func(objectReference corev1.ObjectReference, objectOwnerReferences []metav1.OwnerReference) bool { + BlockingObjectFilter: func(objectReference corev1.ObjectReference, _ []metav1.OwnerReference) bool { return objectReference.Kind == "Machine" }, }, { // The DockerCluster should be deleted in this phase. - ObjectFilter: func(objectReference corev1.ObjectReference, objectOwnerReferences []metav1.OwnerReference) bool { + ObjectFilter: func(objectReference corev1.ObjectReference, _ []metav1.OwnerReference) bool { return objectReference.Kind == "DockerCluster" }, // Of the above, the DockerCluster should be considered to be blocking. - BlockingObjectFilter: func(objectReference corev1.ObjectReference, objectOwnerReferences []metav1.OwnerReference) bool { + BlockingObjectFilter: func(objectReference corev1.ObjectReference, _ []metav1.OwnerReference) bool { return objectReference.Kind == "DockerCluster" }, }, From 37f30cd1067ea13e7081b5b84572fe59313b95d2 Mon Sep 17 00:00:00 2001 From: Christian Schlotter Date: Tue, 24 Sep 2024 18:40:29 +0200 Subject: [PATCH 15/17] fix e2e test --- test/e2e/cluster_deletion.go | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-) diff --git a/test/e2e/cluster_deletion.go b/test/e2e/cluster_deletion.go index b9c27ba55e6f..8dd626f77913 100644 --- a/test/e2e/cluster_deletion.go +++ b/test/e2e/cluster_deletion.go @@ -21,6 +21,7 @@ import ( "fmt" "os" "path/filepath" + "strings" "time" . "github.com/onsi/ginkgo/v2" @@ -31,6 +32,7 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" kerrors "k8s.io/apimachinery/pkg/util/errors" + "k8s.io/apimachinery/pkg/util/sets" "k8s.io/klog/v2" "k8s.io/utils/ptr" "sigs.k8s.io/controller-runtime/pkg/client" @@ -224,7 +226,7 @@ func ClusterDeletionSpec(ctx context.Context, inputGetter func() ClusterDeletion expectedObjectsNotInDeletion = append(expectedObjectsNotInDeletion, objectsPerPhase[j]...) } - assertDeletionPhase(ctx, input.BootstrapClusterProxy.GetClient(), + assertDeletionPhase(ctx, input.BootstrapClusterProxy.GetClient(), finalizer, expectedObjectsDeleted, expectedObjectsInDeletion, expectedObjectsNotInDeletion, @@ -238,7 +240,7 @@ func ClusterDeletionSpec(ctx context.Context, inputGetter func() ClusterDeletion By("Final deletion verification") // Verify that all objects of the last phase are gone and the cluster does still exist. expectedObjectsDeleted = expectedObjectsInDeletion - assertDeletionPhase(ctx, input.BootstrapClusterProxy.GetClient(), + assertDeletionPhase(ctx, input.BootstrapClusterProxy.GetClient(), finalizer, expectedObjectsDeleted, []client.Object{clusterResources.Cluster}, nil, @@ -364,7 +366,7 @@ func removeFinalizer(ctx context.Context, c client.Client, finalizer string, obj } } -func assertDeletionPhase(ctx context.Context, c client.Client, expectedObjectsDeleted, expectedObjectsInDeletion, expectedObjectsNotInDeletion []client.Object) { +func assertDeletionPhase(ctx context.Context, c client.Client, finalizer string, expectedObjectsDeleted, expectedObjectsInDeletion, expectedObjectsNotInDeletion []client.Object) { Eventually(func() error { var errs []error // Ensure deleted objects are gone @@ -390,6 +392,13 @@ func assertDeletionPhase(ctx context.Context, c client.Client, expectedObjectsDe errs = append(errs, errors.Errorf("expected %s %s to be in deletion but deletionTimestamp is not set", obj.GetObjectKind().GroupVersionKind().Kind, klog.KObj(obj))) continue } + + // Blocking objects have our finalizer set. When being excepted to be in deletion, + // blocking objects should only have our finalizer, otherwise they are getting blocked by something else. + if sets.New[string](obj.GetFinalizers()...).Has(finalizer) && len(obj.GetFinalizers()) > 1 { + errs = append(errs, errors.Errorf("expected blocking %s %s to only have %s as finalizer, got [%s]", obj.GetObjectKind().GroupVersionKind().Kind, klog.KObj(obj), finalizer, strings.Join(obj.GetFinalizers(), ", "))) + continue + } } // Ensure other objects are not in deletion. @@ -408,3 +417,7 @@ func assertDeletionPhase(ctx context.Context, c client.Client, expectedObjectsDe return kerrors.NewAggregate(errs) }).WithTimeout(5 * time.Minute).WithPolling(10 * time.Second).Should(Succeed()) } + +func hasFinalizer(wantFinalizer string, finalizers []string) bool { + return sets.New[string](finalizers...).Has(wantFinalizer) +} From edfa365fb69bc0095e3bb87dbc62e443915d30b5 Mon Sep 17 00:00:00 2001 From: Christian Schlotter Date: Tue, 24 Sep 2024 18:52:33 +0200 Subject: [PATCH 16/17] lint fix --- test/e2e/cluster_deletion.go | 4 ---- 1 file changed, 4 deletions(-) diff --git a/test/e2e/cluster_deletion.go b/test/e2e/cluster_deletion.go index 8dd626f77913..a05193a352a3 100644 --- a/test/e2e/cluster_deletion.go +++ b/test/e2e/cluster_deletion.go @@ -417,7 +417,3 @@ func assertDeletionPhase(ctx context.Context, c client.Client, finalizer string, return kerrors.NewAggregate(errs) }).WithTimeout(5 * time.Minute).WithPolling(10 * time.Second).Should(Succeed()) } - -func hasFinalizer(wantFinalizer string, finalizers []string) bool { - return sets.New[string](finalizers...).Has(wantFinalizer) -} From 50647997200f2e1b0060310f6a1da776edd3174b Mon Sep 17 00:00:00 2001 From: Christian Schlotter Date: Wed, 25 Sep 2024 12:45:01 +0200 Subject: [PATCH 17/17] Review Fixes --- test/e2e/cluster_deletion.go | 17 ++++++++--------- test/e2e/cluster_deletion_test.go | 6 +++--- 2 files changed, 11 insertions(+), 12 deletions(-) diff --git a/test/e2e/cluster_deletion.go b/test/e2e/cluster_deletion.go index a05193a352a3..2cec799ad25b 100644 --- a/test/e2e/cluster_deletion.go +++ b/test/e2e/cluster_deletion.go @@ -36,6 +36,7 @@ import ( "k8s.io/klog/v2" "k8s.io/utils/ptr" "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" clusterctlcluster "sigs.k8s.io/cluster-api/cmd/clusterctl/client/cluster" @@ -233,6 +234,7 @@ func ClusterDeletionSpec(ctx context.Context, inputGetter func() ClusterDeletion ) // Remove the test's finalizer from all objects which had been expected to be in deletion to unblock the next phase. + // Note: this only removes the finalizer from objects which have it set. By(fmt.Sprintf("Removing finalizers for phase %d/%d", i+1, len(input.ClusterDeletionPhases))) removeFinalizer(ctx, input.BootstrapClusterProxy.GetClient(), finalizer, expectedObjectsInDeletion...) } @@ -335,7 +337,9 @@ func addFinalizer(ctx context.Context, c client.Client, finalizer string, objs . helper, err := patch.NewHelper(obj, c) Expect(err).ToNot(HaveOccurred()) - obj.SetFinalizers(append(obj.GetFinalizers(), finalizer)) + if updated := controllerutil.AddFinalizer(obj, finalizer); !updated { + continue + } Expect(helper.Patch(ctx, obj)).ToNot(HaveOccurred(), fmt.Sprintf("Failed to add finalizer to %s %s", obj.GetObjectKind().GroupVersionKind().Kind, klog.KObj(obj))) } @@ -353,15 +357,10 @@ func removeFinalizer(ctx context.Context, c client.Client, finalizer string, obj helper, err := patch.NewHelper(obj, c) Expect(err).ToNot(HaveOccurred()) - var finalizers []string - for _, f := range obj.GetFinalizers() { - if f == finalizer { - continue - } - finalizers = append(finalizers, f) + if updated := controllerutil.RemoveFinalizer(obj, finalizer); !updated { + continue } - obj.SetFinalizers(finalizers) Expect(helper.Patch(ctx, obj)).ToNot(HaveOccurred(), fmt.Sprintf("Failed to remove finalizer from %s %s", obj.GetObjectKind().GroupVersionKind().Kind, klog.KObj(obj))) } } @@ -379,7 +378,7 @@ func assertDeletionPhase(ctx context.Context, c client.Client, finalizer string, errs = append(errs, errors.Wrapf(err, "expected %s %s to be deleted", obj.GetObjectKind().GroupVersionKind().Kind, klog.KObj(obj))) continue } - errs = append(errs, fmt.Errorf("expected %s %s to be deleted but it still exists", obj.GetObjectKind().GroupVersionKind().Kind, klog.KObj(obj))) + errs = append(errs, errors.Errorf("expected %s %s to be deleted but it still exists", obj.GetObjectKind().GroupVersionKind().Kind, klog.KObj(obj))) } // Ensure expected objects are in deletion diff --git a/test/e2e/cluster_deletion_test.go b/test/e2e/cluster_deletion_test.go index ec11d1926c6c..801a03cf9897 100644 --- a/test/e2e/cluster_deletion_test.go +++ b/test/e2e/cluster_deletion_test.go @@ -41,12 +41,12 @@ var _ = Describe("When performing cluster deletion with ClusterClass [ClusterCla ClusterDeletionPhases: []ClusterDeletionPhase{ { - // All Machines owned by MachineDeployments or MachineSets and themselves should be deleted in this phase. + // All Machines owned by MachineDeployments or MachineSets, MachineDeployments and MachineSets should be deleted in this phase. ObjectFilter: func(objectReference corev1.ObjectReference, objectOwnerReferences []metav1.OwnerReference) bool { if objectReference.Kind == "Machine" && hasOwner(objectOwnerReferences, "MachineDeployment", "MachineSet") { return true } - return sets.New[string]("MachineDeployment", "MachineSet").Has(objectReference.Kind) + return objectReference.Kind == "MachineDeployment" || objectReference.Kind == "MachineSet" }, // Of the above, only Machines should be considered to be blocking to test foreground deletion. BlockingObjectFilter: func(objectReference corev1.ObjectReference, _ []metav1.OwnerReference) bool { @@ -54,7 +54,7 @@ var _ = Describe("When performing cluster deletion with ClusterClass [ClusterCla }, }, { - // All Machines owned by KubeadmControlPlane and itself should be deleted in this phase. + // All Machines owned by KubeadmControlPlane and KubeadmControlPlane should be deleted in this phase. ObjectFilter: func(objectReference corev1.ObjectReference, objectOwnerReferences []metav1.OwnerReference) bool { if objectReference.Kind == "Machine" && hasOwner(objectOwnerReferences, "KubeadmControlPlane") { return true