Merge pull request #5525 from fabriziopandini/Remove-template-cleanup-func-in-topology-controller

🌱 Remove template cleanup func in Topology controller

Author: k8s-ci-robot

controllers/topology/reconcile_state.go

@@ -24,7 +24,6 @@ import (
 	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/apiserver/pkg/storage/names"
 	"sigs.k8s.io/cluster-api/controllers/topology/internal/check"
 	"sigs.k8s.io/cluster-api/controllers/topology/internal/contract"
@@ -70,9 +69,6 @@ func (r *ClusterReconciler) reconcileInfrastructureCluster(ctx context.Context,
 // reconcileControlPlane works to bring the current state of a managed topology in line with the desired state. This involves
 // updating the cluster where needed.
 func (r *ClusterReconciler) reconcileControlPlane(ctx context.Context, s *scope.Scope) error {
-	// Set a default nil return function for the cleanup operation.
-	cleanup := func() error { return nil }
-
 	// If the clusterClass mandates the controlPlane has infrastructureMachines, reconcile it.
 	if s.Blueprint.HasControlPlaneInfrastructureMachine() {
 		ctx, _ := tlog.LoggerFrom(ctx).WithObject(s.Desired.ControlPlane.InfrastructureMachineTemplate).Into(ctx)
@@ -83,7 +79,7 @@ func (r *ClusterReconciler) reconcileControlPlane(ctx context.Context, s *scope.
 		}
 
 		// Create or update the MachineInfrastructureTemplate of the control plane.
-		cleanup, err = r.reconcileReferencedTemplate(ctx, reconcileReferencedTemplateInput{
+		err = r.reconcileReferencedTemplate(ctx, reconcileReferencedTemplateInput{
 			ref:                  cpInfraRef,
 			current:              s.Current.ControlPlane.InfrastructureMachineTemplate,
 			desired:              s.Desired.ControlPlane.InfrastructureMachineTemplate,
@@ -98,25 +94,17 @@ func (r *ClusterReconciler) reconcileControlPlane(ctx context.Context, s *scope.
 		// The controlPlaneObject.Spec.machineTemplate.infrastructureRef has to be updated in the desired object
 		err = contract.ControlPlane().MachineTemplate().InfrastructureRef().Set(s.Desired.ControlPlane.Object, refToUnstructured(cpInfraRef))
 		if err != nil {
-			return kerrors.NewAggregate([]error{
-				errors.Wrapf(err, "failed to update %s", tlog.KObj{Obj: s.Desired.ControlPlane.Object}),
-				cleanup(),
-			})
+			return errors.Wrapf(err, "failed to update %s", tlog.KObj{Obj: s.Desired.ControlPlane.Object})
 		}
 	}
 
 	// Create or update the ControlPlaneObject for the ControlPlaneState.
 	ctx, _ = tlog.LoggerFrom(ctx).WithObject(s.Desired.ControlPlane.Object).Into(ctx)
 	if err := r.reconcileReferencedObject(ctx, s.Current.ControlPlane.Object, s.Desired.ControlPlane.Object); err != nil {
-		return kerrors.NewAggregate([]error{
-			errors.Wrapf(err, "failed to update %s", tlog.KObj{Obj: s.Desired.ControlPlane.Object}),
-			cleanup(),
-		})
+		return errors.Wrapf(err, "failed to update %s", tlog.KObj{Obj: s.Desired.ControlPlane.Object})
 	}
 
-	// At this point we've updated the ControlPlane object and, where required, the ControlPlane InfrastructureMachineTemplate
-	// without error. Run the cleanup in order to delete the old InfrastructureMachineTemplate if template rotation was done during update.
-	return cleanup()
+	return nil
 }
 
 // reconcileCluster reconciles the desired state of the Cluster object.
@@ -180,14 +168,14 @@ func (r *ClusterReconciler) createMachineDeployment(ctx context.Context, md *sco
 	log := tlog.LoggerFrom(ctx).WithMachineDeployment(md.Object)
 
 	ctx, _ = log.WithObject(md.InfrastructureMachineTemplate).Into(ctx)
-	if _, err := r.reconcileReferencedTemplate(ctx, reconcileReferencedTemplateInput{
+	if err := r.reconcileReferencedTemplate(ctx, reconcileReferencedTemplateInput{
 		desired: md.InfrastructureMachineTemplate,
 	}); err != nil {
 		return errors.Wrapf(err, "failed to create %s", tlog.KObj{Obj: md.Object})
 	}
 
 	ctx, _ = log.WithObject(md.BootstrapTemplate).Into(ctx)
-	if _, err := r.reconcileReferencedTemplate(ctx, reconcileReferencedTemplateInput{
+	if err := r.reconcileReferencedTemplate(ctx, reconcileReferencedTemplateInput{
 		desired: md.BootstrapTemplate,
 	}); err != nil {
 		return errors.Wrapf(err, "failed to create %s", tlog.KObj{Obj: md.Object})
@@ -206,7 +194,7 @@ func (r *ClusterReconciler) updateMachineDeployment(ctx context.Context, cluster
 	log := tlog.LoggerFrom(ctx).WithMachineDeployment(desiredMD.Object)
 
 	ctx, _ = log.WithObject(desiredMD.InfrastructureMachineTemplate).Into(ctx)
-	if _, err := r.reconcileReferencedTemplate(ctx, reconcileReferencedTemplateInput{
+	if err := r.reconcileReferencedTemplate(ctx, reconcileReferencedTemplateInput{
 		ref:                  &desiredMD.Object.Spec.Template.Spec.InfrastructureRef,
 		current:              currentMD.InfrastructureMachineTemplate,
 		desired:              desiredMD.InfrastructureMachineTemplate,
@@ -217,7 +205,7 @@ func (r *ClusterReconciler) updateMachineDeployment(ctx context.Context, cluster
 	}
 
 	ctx, _ = log.WithObject(desiredMD.BootstrapTemplate).Into(ctx)
-	if _, err := r.reconcileReferencedTemplate(ctx, reconcileReferencedTemplateInput{
+	if err := r.reconcileReferencedTemplate(ctx, reconcileReferencedTemplateInput{
 		ref:                  desiredMD.Object.Spec.Template.Spec.Bootstrap.ConfigRef,
 		current:              currentMD.BootstrapTemplate,
 		desired:              desiredMD.BootstrapTemplate,
@@ -337,49 +325,47 @@ type reconcileReferencedTemplateInput struct {
 // This function specifically takes care of the first step and updates the reference locally. So the remaining steps
 // can be executed afterwards.
 // NOTE: This func has a side effect in case of template rotation, changing both the desired object and the object reference.
-func (r *ClusterReconciler) reconcileReferencedTemplate(ctx context.Context, in reconcileReferencedTemplateInput) (func() error, error) {
+func (r *ClusterReconciler) reconcileReferencedTemplate(ctx context.Context, in reconcileReferencedTemplateInput) error {
 	log := tlog.LoggerFrom(ctx)
 
-	cleanupFunc := func() error { return nil }
-
 	// If there is no current object, create the desired object.
 	if in.current == nil {
 		log.Infof("Creating %s", tlog.KObj{Obj: in.desired})
 		if err := r.Client.Create(ctx, in.desired.DeepCopy()); err != nil {
-			return nil, errors.Wrapf(err, "failed to create %s", tlog.KObj{Obj: in.desired})
+			return errors.Wrapf(err, "failed to create %s", tlog.KObj{Obj: in.desired})
 		}
-		return cleanupFunc, nil
+		return nil
 	}
 
 	if in.ref == nil {
-		return nil, errors.Errorf("failed to rotate %s: ref should not be nil", in.desired.GroupVersionKind())
+		return errors.Errorf("failed to rotate %s: ref should not be nil", in.desired.GroupVersionKind())
 	}
 
 	// Check if the current and desired referenced object are compatible.
 	if err := in.compatibilityChecker(in.current, in.desired); err != nil {
-		return nil, err
+		return err
 	}
 
 	// Check differences between current and desired objects, and if there are changes eventually start the template rotation.
 	patchHelper, err := mergepatch.NewHelper(in.current, in.desired, r.Client)
 	if err != nil {
-		return nil, errors.Wrapf(err, "failed to create patch helper for %s", tlog.KObj{Obj: in.current})
+		return errors.Wrapf(err, "failed to create patch helper for %s", tlog.KObj{Obj: in.current})
 	}
 
 	// Return if no changes are detected.
 	if !patchHelper.HasChanges() {
 		log.V(3).Infof("No changes for %s", tlog.KObj{Obj: in.desired})
-		return cleanupFunc, nil
+		return nil
 	}
 
 	// If there are no changes in the spec, and thus only changes in metadata, instead of doing a full template
 	// rotation we patch the object in place. This avoids recreating machines.
 	if !patchHelper.HasSpecChanges() {
 		log.Infof("Patching %s", tlog.KObj{Obj: in.desired})
 		if err := patchHelper.Patch(ctx); err != nil {
-			return nil, errors.Wrapf(err, "failed to patch %s", tlog.KObj{Obj: in.desired})
+			return errors.Wrapf(err, "failed to patch %s", tlog.KObj{Obj: in.desired})
 		}
-		return cleanupFunc, nil
+		return nil
 	}
 
 	// Create the new template.
@@ -392,21 +378,13 @@ func (r *ClusterReconciler) reconcileReferencedTemplate(ctx context.Context, in
 	log.Infof("Rotating %s, new name %s", tlog.KObj{Obj: in.current}, newName)
 	log.Infof("Creating %s", tlog.KObj{Obj: in.desired})
 	if err := r.Client.Create(ctx, in.desired.DeepCopy()); err != nil {
-		return nil, errors.Wrapf(err, "failed to create %s", tlog.KObj{Obj: in.desired})
+		return errors.Wrapf(err, "failed to create %s", tlog.KObj{Obj: in.desired})
 	}
 
 	// Update the reference with the new name.
 	// NOTE: Updating the object hosting reference to the template is executed outside this func.
 	// TODO: find a way to make side effect more explicit
 	in.ref.Name = newName
 
-	// Set up a cleanup func for removing the old template.
-	// NOTE: This function must be called after updating the object containing the reference to the Template.
-	return func() error {
-		log.Infof("Deleting %s", tlog.KObj{Obj: in.current})
-		if err := r.Client.Delete(ctx, in.current); err != nil {
-			return errors.Wrapf(err, "failed to delete %s", tlog.KObj{Obj: in.desired})
-		}
-		return nil
-	}, nil
+	return nil
 }