split up md/ms reconciliation

config/rbac/role.yaml

@@ -110,13 +110,9 @@ rules:
   - cluster.x-k8s.io
   resources:
   - machinedeployments
-  - machinedeployments/finalizers
   verbs:
-  - delete
   - get
   - list
-  - patch
-  - update
   - watch
 - apiGroups:
   - cluster.x-k8s.io
@@ -182,6 +178,14 @@ rules:
   - get
   - list
   - watch
+- apiGroups:
+  - cluster.x-k8s.io
+  resources:
+  - machinesets
+  verbs:
+  - get
+  - list
+  - watch
 - apiGroups:
   - cluster.x-k8s.io
   resources:

controllers/topology/machineset_controller.go

@@ -0,0 +1,210 @@
+/*
+Copyright 2021 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 topology
+
+import (
+	"context"
+
+	"github.com/pkg/errors"
+	apierrors "k8s.io/apimachinery/pkg/api/errors"
+	"k8s.io/apimachinery/pkg/runtime/schema"
+	"k8s.io/apimachinery/pkg/types"
+	clusterv1 "sigs.k8s.io/cluster-api/api/v1alpha4"
+	"sigs.k8s.io/cluster-api/util"
+	"sigs.k8s.io/cluster-api/util/annotations"
+	"sigs.k8s.io/cluster-api/util/patch"
+	"sigs.k8s.io/cluster-api/util/predicates"
+	ctrl "sigs.k8s.io/controller-runtime"
+	"sigs.k8s.io/controller-runtime/pkg/client"
+	"sigs.k8s.io/controller-runtime/pkg/controller"
+	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
+)
+
+// +kubebuilder:rbac:groups=infrastructure.cluster.x-k8s.io;bootstrap.cluster.x-k8s.io,resources=*,verbs=delete
+// +kubebuilder:rbac:groups=cluster.x-k8s.io,resources=clusters,verbs=get;list;watch
+// +kubebuilder:rbac:groups=cluster.x-k8s.io,resources=machinedeployments,verbs=get;list;watch
+// +kubebuilder:rbac:groups=cluster.x-k8s.io,resources=machinesets;machinesets/finalizers,verbs=get;list;watch;update;patch;delete
+
+// MachineSetReconciler deletes referenced templates during deletion of topology-owned MachineSets.
+// The templates are only deleted, if they are not used in other MachineDeployments or MachineSets which are not in deleting,
+// i.e. the templates would otherwise be orphaned after the MachineSet deletion completes.
+// Note: To achieve this the reconciler sets a finalizer to hook into the MachineSet deletions.
+type MachineSetReconciler struct {
+	Client           client.Client
+	WatchFilterValue string
+}
+
+func (r *MachineSetReconciler) SetupWithManager(ctx context.Context, mgr ctrl.Manager, options controller.Options) error {
+	err := ctrl.NewControllerManagedBy(mgr).
+		For(&clusterv1.MachineSet{}).
+		Named("machineset/topology").
+		WithOptions(options).
+		WithEventFilter(predicates.All(ctrl.LoggerFrom(ctx),
+			predicates.ResourceNotPausedAndHasFilterLabel(ctrl.LoggerFrom(ctx), r.WatchFilterValue),
+			predicates.ResourceIsTopologyOwned(ctrl.LoggerFrom(ctx)),
+		)).
+		Complete(r)
+	if err != nil {
+		return errors.Wrap(err, "failed setting up with a controller manager")
+	}
+
+	return nil
+}
+
+// Reconcile deletes referenced templates during deletion of topology-owned MachineSets.
+// The templates are only deleted, if they are not used in other MachineDeployments or MachineSets which are not in deleting,
+// i.e. the templates would otherwise be orphaned after the MachineSet deletion completes.
+// Additional context:
+// * MachineSet deletion:
+//   * MachineSets are deleted and garbage collected first (without waiting until all Machines are also deleted)
+//   * After that, deletion of Machines is automatically triggered by Kubernetes based on owner references.
+// Note: We assume templates are not reused by different MachineDeployments, which is (only) true for topology-owned
+//       MachineDeployments.
+func (r *MachineSetReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) {
+	log := ctrl.LoggerFrom(ctx)
+
+	// Fetch the MachineSet instance.
+	ms := &clusterv1.MachineSet{}
+	if err := r.Client.Get(ctx, req.NamespacedName, ms); err != nil {
+		if apierrors.IsNotFound(err) {
+			// Object not found, return.
+			return ctrl.Result{}, nil
+		}
+		// Error reading the object - requeue the request.
+		return ctrl.Result{}, errors.Wrapf(err, "failed to get MachineSet/%s", req.NamespacedName.Name)
+	}
+
+	cluster, err := util.GetClusterByName(ctx, r.Client, ms.Namespace, ms.Spec.ClusterName)
+	if err != nil {
+		return ctrl.Result{}, err
+	}
+
+	// Return early if the object or Cluster is paused.
+	if annotations.IsPaused(cluster, ms) {
+		log.Info("Reconciliation is paused for this object")
+		return ctrl.Result{}, nil
+	}
+
+	// Set finalizer, if it's not already set.
+	if !controllerutil.ContainsFinalizer(ms, clusterv1.MachineSetTopologyFinalizer) {
+		patchHelper, err := patch.NewHelper(ms, r.Client)
+		if err != nil {
+			return ctrl.Result{}, errors.Wrapf(err, "failed to create patch helper for %s", KRef{Obj: ms})
+		}
+		controllerutil.AddFinalizer(ms, clusterv1.MachineSetTopologyFinalizer)
+		if err := patchHelper.Patch(ctx, ms); err != nil {
+			return ctrl.Result{}, errors.Wrapf(err, "failed to patch %s", KRef{Obj: ms})
+		}
+		return ctrl.Result{}, nil
+	}
+
+	// Handle deletion reconciliation loop.
+	if !ms.ObjectMeta.DeletionTimestamp.IsZero() {
+		return r.reconcileDelete(ctx, ms)
+	}
+
+	// Nothing to do.
+	return ctrl.Result{}, nil
+}
+
+func (r MachineSetReconciler) reconcileDelete(ctx context.Context, ms *clusterv1.MachineSet) (ctrl.Result, error) {
+	// Calculate the name of the corresponding MachineDeployment.
+	mdName, err := calculateMachineDeploymentName(ms)
+	if err != nil {
+		return ctrl.Result{}, err
+	}
+
+	// Get the corresponding MachineSets.
+	msList, err := getMachineSetsForDeployment(ctx, r.Client, *mdName)
+	if err != nil {
+		return ctrl.Result{}, err
+	}
+
+	// Fetch the MachineDeployment instance, if it still exists.
+	// Note: This can happen because MachineDeployments are deleted before their corresponding MachineSets.
+	md := &clusterv1.MachineDeployment{}
+	if err := r.Client.Get(ctx, *mdName, md); err != nil {
+		if !apierrors.IsNotFound(err) {
+			// Error reading the object - requeue the request.
+			return ctrl.Result{}, errors.Wrapf(err, "failed to get MachineDeployment/%s", mdName.Name)
+		}
+		// If the MachineDeployment doesn't exist anymore, set md to nil, so we can handle that case correctly below.
+		md = nil
+	}
+
+	// Calculate which templates are still in use by MachineDeployments or MachineSets which are not in deleting.
+	templatesInUse, err := calculateTemplatesInUse(md, msList)
+	if err != nil {
+		return ctrl.Result{}, err
+	}
+
+	// Delete unused templates of the MachineSet.
+	if err := r.deleteUnusedMachineSetTemplates(ctx, templatesInUse, ms); err != nil {
+		return ctrl.Result{}, err
+	}
+
+	// Remove the finalizer so the MachineSet can be garbage collected by Kubernetes.
+	patchHelper, err := patch.NewHelper(ms, r.Client)
+	if err != nil {
+		return ctrl.Result{}, errors.Wrapf(err, "failed to create patch helper for %s", KRef{Obj: ms})
+	}
+	controllerutil.RemoveFinalizer(ms, clusterv1.MachineSetTopologyFinalizer)
+	if err := patchHelper.Patch(ctx, ms); err != nil {
+		return ctrl.Result{}, errors.Wrapf(err, "failed to patch %s", KRef{Obj: ms})
+	}
+
+	return ctrl.Result{}, nil
+}
+
+// calculateMachineDeploymentName calculates the MachineDeployment name based on owner references.
+func calculateMachineDeploymentName(ms *clusterv1.MachineSet) (*types.NamespacedName, error) {
+	for _, ref := range ms.OwnerReferences {
+		if ref.Kind != "MachineDeployment" {
+			continue
+		}
+		gv, err := schema.ParseGroupVersion(ref.APIVersion)
+		if err != nil {
+			// This should never happen.
+			return nil, errors.Errorf("could not calculate MachineDeployment name for %s: invalid apiVersion %q: %v",
+				KRef{Obj: ms}, ref.APIVersion, err)
+		}
+		if gv.Group == clusterv1.GroupVersion.Group {
+			return &client.ObjectKey{Namespace: ms.Namespace, Name: ref.Name}, nil
+		}
+	}
+
+	return nil, errors.Errorf("could not calculate MachineDeployment name for %s", KRef{Obj: ms})
+}
+
+// deleteUnusedMachineSetTemplates deletes templates referenced in MachineSets, if:
+// the templates are not used by other MachineDeployments or MachineSets (i.e. they are not in templatesInUse).
+// Note: We don't care about Machines, because the Machine deletion is triggered based
+//       on owner references by Kubernetes *after* we remove the finalizer from the
+//       MachineSet and the MachineSet has been garbage collected by Kubernetes.
+func (r *MachineSetReconciler) deleteUnusedMachineSetTemplates(ctx context.Context, templatesInUse map[string]bool, ms *clusterv1.MachineSet) error {
+	ref := ms.Spec.Template.Spec.Bootstrap.ConfigRef
+	if err := deleteTemplateIfNotUsed(ctx, r.Client, templatesInUse, ref); err != nil {
+		return errors.Wrapf(err, "failed to delete bootstrap template for %s", KRef{Obj: ms})
+	}
+
+	ref = &ms.Spec.Template.Spec.InfrastructureRef
+	if err := deleteTemplateIfNotUsed(ctx, r.Client, templatesInUse, ref); err != nil {
+		return errors.Wrapf(err, "failed to delete infrastructure template for %s", KRef{Obj: ms})
+	}
+
+	return nil
+}

controllers/topology/machineset_controller_test.go

@@ -0,0 +1,98 @@
+/*
+Copyright 2021 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 topology
+
+import (
+	"testing"
+
+	. "github.com/onsi/gomega"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"sigs.k8s.io/cluster-api/internal/testtypes"
+	"sigs.k8s.io/controller-runtime/pkg/client/fake"
+)
+
+func TestDeleteUnusedMachineSetTemplates(t *testing.T) {
+	msBT := testtypes.NewBootstrapTemplateBuilder(metav1.NamespaceDefault, "msBT").Build()
+	msIMT := testtypes.NewInfrastructureMachineTemplateBuilder(metav1.NamespaceDefault, "msIMT").Build()
+	ms := testtypes.NewMachineSetBuilder(metav1.NamespaceDefault, "ms").
+		WithBootstrapTemplate(msBT).
+		WithInfrastructureTemplate(msIMT).
+		Build()
+
+	msWithoutBootstrapTemplateIMT := testtypes.NewInfrastructureMachineTemplateBuilder(metav1.NamespaceDefault, "msWithoutBootstrapTemplateIMT").Build()
+	msWithoutBootstrapTemplate := testtypes.NewMachineSetBuilder(metav1.NamespaceDefault, "msWithoutBootstrapTemplate").
+		WithInfrastructureTemplate(msWithoutBootstrapTemplateIMT).
+		Build()
+
+	t.Run("Should delete templates of a MachineSet", func(t *testing.T) {
+		g := NewWithT(t)
+
+		fakeClient := fake.NewClientBuilder().
+			WithScheme(fakeScheme).
+			WithObjects(ms, msBT, msIMT).
+			Build()
+
+		r := &MachineSetReconciler{
+			Client: fakeClient,
+		}
+		err := r.deleteUnusedMachineSetTemplates(ctx, map[string]bool{}, ms)
+		g.Expect(err).ToNot(HaveOccurred())
+
+		g.Expect(templateExists(fakeClient, msBT)).To(BeFalse())
+		g.Expect(templateExists(fakeClient, msIMT)).To(BeFalse())
+	})
+
+	t.Run("Should not delete templates of a MachineSet when they are still in use", func(t *testing.T) {
+		g := NewWithT(t)
+
+		fakeClient := fake.NewClientBuilder().
+			WithScheme(fakeScheme).
+			WithObjects(ms, msBT, msIMT).
+			Build()
+
+		templatesInUse := map[string]bool{
+			mustTemplateRefID(ms.Spec.Template.Spec.Bootstrap.ConfigRef): true,
+			mustTemplateRefID(&ms.Spec.Template.Spec.InfrastructureRef):  true,
+		}
+
+		r := &MachineSetReconciler{
+			Client: fakeClient,
+		}
+		err := r.deleteUnusedMachineSetTemplates(ctx, templatesInUse, ms)
+		g.Expect(err).ToNot(HaveOccurred())
+
+		g.Expect(templateExists(fakeClient, msBT)).To(BeTrue())
+		g.Expect(templateExists(fakeClient, msIMT)).To(BeTrue())
+	})
+
+	t.Run("Should delete infra template of a MachineSet without a bootstrap template", func(t *testing.T) {
+		g := NewWithT(t)
+
+		fakeClient := fake.NewClientBuilder().
+			WithScheme(fakeScheme).
+			WithObjects(msWithoutBootstrapTemplate, msWithoutBootstrapTemplateIMT).
+			Build()
+
+		r := &MachineSetReconciler{
+			Client: fakeClient,
+		}
+		err := r.deleteUnusedMachineSetTemplates(ctx, map[string]bool{}, msWithoutBootstrapTemplate)
+		g.Expect(err).ToNot(HaveOccurred())
+
+		g.Expect(templateExists(fakeClient, msWithoutBootstrapTemplateIMT)).To(BeFalse())
+	})
+}

controllers/topology/reconcile_state.go

@@ -201,30 +201,28 @@ func (r *ClusterReconciler) updateMachineDeployment(ctx context.Context, cluster
 	log := loggerFrom(ctx).WithMachineDeployment(desiredMD.Object)
 
 	ctx, _ = log.WithObject(desiredMD.InfrastructureMachineTemplate).Into(ctx)
-	_, err := r.reconcileReferencedTemplate(ctx, reconcileReferencedTemplateInput{
+	if _, err := r.reconcileReferencedTemplate(ctx, reconcileReferencedTemplateInput{
 		ref:     &desiredMD.Object.Spec.Template.Spec.InfrastructureRef,
 		current: currentMD.InfrastructureMachineTemplate,
 		desired: desiredMD.InfrastructureMachineTemplate,
 		templateNamer: func() string {
 			return infrastructureMachineTemplateNamePrefix(clusterName, mdTopologyName)
 		},
 		compatibilityChecker: check.ReferencedObjectsAreCompatible,
-	})
-	if err != nil {
+	}); err != nil {
 		return errors.Wrapf(err, "failed to update %s/%s", currentMD.Object.GroupVersionKind(), currentMD.Object.Name)
 	}
 
 	ctx, _ = log.WithObject(desiredMD.BootstrapTemplate).Into(ctx)
-	_, 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,
 		templateNamer: func() string {
 			return bootstrapTemplateNamePrefix(clusterName, mdTopologyName)
 		},
 		compatibilityChecker: check.ObjectsAreInTheSameNamespace,
-	})
-	if err != nil {
+	}); err != nil {
 		return errors.Wrapf(err, "failed to update %s/%s", currentMD.Object.GroupVersionKind(), currentMD.Object.Name)
 	}