🐛 Implement template deletion for topology-owned MD and MS

api/v1alpha4/machinedeployment_types.go

@@ -21,6 +21,12 @@ import (
 	"k8s.io/apimachinery/pkg/util/intstr"
 )
 
+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"
+)
+
 // MachineDeploymentStrategyType defines the type of MachineDeployment rollout strategies.
 type MachineDeploymentStrategyType string
 

api/v1alpha4/machineset_types.go

@@ -24,6 +24,12 @@ import (
 	capierrors "sigs.k8s.io/cluster-api/errors"
 )
 
+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"
+)
+
 // ANCHOR: MachineSetSpec
 
 // MachineSetSpec defines the desired state of MachineSet.

config/rbac/role.yaml

@@ -66,7 +66,14 @@ rules:
   - cluster.x-k8s.io
   resources:
   - clusterclasses
-  - machinedeployments
+  verbs:
+  - get
+  - list
+  - watch
+- apiGroups:
+  - cluster.x-k8s.io
+  resources:
+  - clusters
   verbs:
   - get
   - list
@@ -98,6 +105,29 @@ rules:
   - patch
   - update
   - watch
+- apiGroups:
+  - cluster.x-k8s.io
+  resources:
+  - machinedeployments
+  verbs:
+  - create
+  - delete
+  - get
+  - list
+  - patch
+  - update
+  - watch
+- apiGroups:
+  - cluster.x-k8s.io
+  resources:
+  - machinedeployments
+  - machinedeployments/finalizers
+  verbs:
+  - get
+  - list
+  - patch
+  - update
+  - watch
 - apiGroups:
   - cluster.x-k8s.io
   resources:
@@ -162,6 +192,25 @@ rules:
   - get
   - list
   - watch
+- apiGroups:
+  - cluster.x-k8s.io
+  resources:
+  - machinesets
+  verbs:
+  - get
+  - list
+  - watch
+- apiGroups:
+  - cluster.x-k8s.io
+  resources:
+  - machinesets
+  - machinesets/finalizers
+  verbs:
+  - get
+  - list
+  - patch
+  - update
+  - watch
 - apiGroups:
   - cluster.x-k8s.io
   resources:

controllers/external/util.go

@@ -50,6 +50,19 @@ func Get(ctx context.Context, c client.Client, ref *corev1.ObjectReference, name
 	return obj, nil
 }
 
+// Delete uses the client and reference to delete an external, unstructured object.
+func Delete(ctx context.Context, c client.Client, ref *corev1.ObjectReference) error {
+	obj := new(unstructured.Unstructured)
+	obj.SetAPIVersion(ref.APIVersion)
+	obj.SetKind(ref.Kind)
+	obj.SetName(ref.Name)
+	obj.SetNamespace(ref.Namespace)
+	if err := c.Delete(ctx, obj); err != nil {
+		return errors.Wrapf(err, "failed to delete %s external object %q/%q", obj.GetKind(), obj.GetNamespace(), obj.GetName())
+	}
+	return nil
+}
+
 // CloneTemplateInput is the input to CloneTemplate.
 type CloneTemplateInput struct {
 	// Client is the controller runtime client.

controllers/machinedeployment_sync.go

@@ -31,11 +31,14 @@ import (
 	"k8s.io/client-go/util/retry"
 	clusterv1 "sigs.k8s.io/cluster-api/api/v1alpha4"
 	"sigs.k8s.io/cluster-api/controllers/mdutil"
+	"sigs.k8s.io/cluster-api/feature"
 	"sigs.k8s.io/cluster-api/util"
 	"sigs.k8s.io/cluster-api/util/conditions"
+	"sigs.k8s.io/cluster-api/util/labels"
 	"sigs.k8s.io/cluster-api/util/patch"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
+	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 )
 
 // sync is responsible for reconciling deployments on scaling events or when they
@@ -173,13 +176,24 @@ func (r *MachineDeploymentReconciler) getNewMachineSet(ctx context.Context, d *c
 		},
 	}
 
+	if feature.Gates.Enabled(feature.ClusterTopology) {
+		// If the MachineDeployment is owned by a Cluster Topology,
+		// add the finalizer to allow the topology controller to
+		// clean up resources when the MachineSet is deleted.
+		// MachineSets are deleted during rollout (e.g. template rotation) and
+		// after MachineDeployment deletion.
+		if labels.IsTopologyOwned(d) {
+			controllerutil.AddFinalizer(&newMS, clusterv1.MachineSetTopologyFinalizer)
+		}
+	}
+
 	if d.Spec.Strategy.RollingUpdate.DeletePolicy != nil {
 		newMS.Spec.DeletePolicy = *d.Spec.Strategy.RollingUpdate.DeletePolicy
 	}
 
 	// Add foregroundDeletion finalizer to MachineSet if the MachineDeployment has it
 	if sets.NewString(d.Finalizers...).Has(metav1.FinalizerDeleteDependents) {
-		newMS.Finalizers = []string{metav1.FinalizerDeleteDependents}
+		controllerutil.AddFinalizer(&newMS, metav1.FinalizerDeleteDependents)
 	}
 
 	allMSs := append(oldMSs, &newMS)

controllers/topology/controller.go → controllers/topology/cluster_controller.go

@@ -39,7 +39,8 @@ import (
 
 // +kubebuilder:rbac:groups=infrastructure.cluster.x-k8s.io;bootstrap.cluster.x-k8s.io;controlplane.cluster.x-k8s.io,resources=*,verbs=get;list;watch;create;update;patch;delete
 // +kubebuilder:rbac:groups=cluster.x-k8s.io,resources=clusters;clusters/status,verbs=get;list;watch;create;update;patch;delete
-// +kubebuilder:rbac:groups=cluster.x-k8s.io,resources=clusterclasses;machinedeployments,verbs=get;list;watch
+// +kubebuilder:rbac:groups=cluster.x-k8s.io,resources=clusterclasses,verbs=get;list;watch
+// +kubebuilder:rbac:groups=cluster.x-k8s.io,resources=machinedeployments,verbs=get;list;watch;create;update;patch;delete
 // +kubebuilder:rbac:groups=apiextensions.k8s.io,resources=customresourcedefinitions,verbs=get;list;watch
 
 // ClusterReconciler reconciles a managed topology for a Cluster object.
@@ -57,7 +58,7 @@ type ClusterReconciler struct {
 func (r *ClusterReconciler) SetupWithManager(ctx context.Context, mgr ctrl.Manager, options controller.Options) error {
 	c, err := ctrl.NewControllerManagedBy(mgr).
 		For(&clusterv1.Cluster{}).
-		Named("cluster/topology").
+		Named("topology/cluster").
 		Watches(
 			&source.Kind{Type: &clusterv1.ClusterClass{}},
 			handler.EnqueueRequestsFromMapFunc(r.clusterClassToCluster),

controllers/topology/desired_state.go

@@ -31,6 +31,7 @@ import (
 	"sigs.k8s.io/cluster-api/controllers/topology/internal/contract"
 	tlog "sigs.k8s.io/cluster-api/controllers/topology/internal/log"
 	"sigs.k8s.io/cluster-api/controllers/topology/internal/scope"
+	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 )
 
 // computeDesiredState computes the desired state of the cluster topology.
@@ -301,6 +302,13 @@ func computeMachineDeployment(_ context.Context, s *scope.Scope, machineDeployme
 		},
 	}
 
+	// If it's a new MachineDeployment, set the finalizer.
+	// Note: we only add it on creation to avoid race conditions later on when
+	// the MachineDeployment topology controller removes the finalizer.
+	if currentMachineDeployment == nil {
+		controllerutil.AddFinalizer(desiredMachineDeploymentObj, clusterv1.MachineDeploymentTopologyFinalizer)
+	}
+
 	// If an existing MachineDeployment is present, override the MachineDeployment generate name
 	// re-using the existing name (this will help in reconcile).
 	if currentMachineDeployment != nil && currentMachineDeployment.Object != nil {

controllers/topology/desired_state_test.go

@@ -21,6 +21,7 @@ import (
 	"testing"
 
 	"sigs.k8s.io/cluster-api/internal/testtypes"
+	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 
 	. "github.com/onsi/gomega"
 	corev1 "k8s.io/api/core/v1"
@@ -512,6 +513,7 @@ func TestComputeMachineDeployment(t *testing.T) {
 
 		g.Expect(actualMd.Labels).To(HaveKeyWithValue(clusterv1.ClusterTopologyMachineDeploymentLabelName, "big-pool-of-machines"))
 		g.Expect(actualMd.Labels).To(HaveKey(clusterv1.ClusterTopologyOwnedLabel))
+		g.Expect(controllerutil.ContainsFinalizer(actualMd, clusterv1.MachineDeploymentTopologyFinalizer)).To(BeTrue())
 
 		g.Expect(actualMd.Spec.Template.ObjectMeta.Labels).To(HaveKeyWithValue("foo", "baz"))
 		g.Expect(actualMd.Spec.Template.ObjectMeta.Labels).To(HaveKeyWithValue("fizz", "buzz"))
@@ -559,6 +561,7 @@ func TestComputeMachineDeployment(t *testing.T) {
 
 		g.Expect(actualMd.Labels).To(HaveKeyWithValue(clusterv1.ClusterTopologyMachineDeploymentLabelName, "big-pool-of-machines"))
 		g.Expect(actualMd.Labels).To(HaveKey(clusterv1.ClusterTopologyOwnedLabel))
+		g.Expect(controllerutil.ContainsFinalizer(actualMd, clusterv1.MachineDeploymentTopologyFinalizer)).To(BeFalse())
 
 		g.Expect(actualMd.Spec.Template.ObjectMeta.Labels).To(HaveKeyWithValue("foo", "baz"))
 		g.Expect(actualMd.Spec.Template.ObjectMeta.Labels).To(HaveKeyWithValue("fizz", "buzz"))

controllers/topology/internal/log/log.go

@@ -47,6 +47,10 @@ type Logger interface {
 	// a resources being part of the Cluster by reconciled.
 	WithObject(obj client.Object) Logger
 
+	// WithRef adds to the logger information about the object ref being modified by reconcile, which in most case it is
+	// a resources being part of the Cluster by reconciled.
+	WithRef(ref *corev1.ObjectReference) Logger
+
 	// WithMachineDeployment adds to the logger information about the MachineDeployment object being processed.
 	WithMachineDeployment(md *clusterv1.MachineDeployment) Logger
 
@@ -80,6 +84,17 @@ func (l *topologyReconcileLogger) WithObject(obj client.Object) Logger {
 	return l
 }
 
+// WithRef adds to the logger information about the object ref being modified by reconcile, which in most case it is
+// a resources being part of the Cluster by reconciled.
+func (l *topologyReconcileLogger) WithRef(ref *corev1.ObjectReference) Logger {
+	l.Logger = l.Logger.WithValues(
+		"object groupVersion", ref.APIVersion,
+		"object kind", ref.Kind,
+		"object", ref.Name,
+	)
+	return l
+}
+
 // WithMachineDeployment adds to the logger information about the MachineDeployment object being processed.
 func (l *topologyReconcileLogger) WithMachineDeployment(md *clusterv1.MachineDeployment) Logger {
 	topologyName := md.Labels[clusterv1.ClusterTopologyMachineDeploymentLabelName]