finalize finalizer change

kubernetes-sigs · Sep 9, 2021 · 41017ee · 41017ee
1 parent cb87484
commit 41017ee
Show file tree

Hide file tree

Showing 8 changed files with 39 additions and 15 deletions.
diff --git a/config/rbac/role.yaml b/config/rbac/role.yaml
@@ -66,7 +66,6 @@ rules:
   - cluster.x-k8s.io
   resources:
   - clusterclasses
-  - machinedeployments
   verbs:
   - get
   - list
@@ -111,8 +110,23 @@ rules:
   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
@@ -192,7 +206,6 @@ rules:
   - machinesets
   - machinesets/finalizers
   verbs:
-  - delete
   - get
   - list
   - patch

diff --git a/controllers/machinedeployment_sync.go b/controllers/machinedeployment_sync.go
@@ -177,9 +177,11 @@ func (r *MachineDeploymentReconciler) getNewMachineSet(ctx context.Context, d *c
 	}
 
 	if feature.Gates.Enabled(feature.ClusterTopology) {
-		// If the MachineDeployment is owned my a Cluster Topology,
+		// If the MachineDeployment is owned by a Cluster Topology,
 		// add the finalizer to allow the topology controller to
-		// cleanup resources when templates are rotated, or the object is deleted.
+		// 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)
 		}
@@ -191,7 +193,7 @@ func (r *MachineDeploymentReconciler) getNewMachineSet(ctx context.Context, d *c
 
 	// 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)

diff --git a/controllers/topology/cluster_controller.go b/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.

diff --git a/controllers/topology/desired_state.go b/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.
@@ -281,9 +282,6 @@ func computeMachineDeployment(_ context.Context, s *scope.Scope, machineDeployme
 		ObjectMeta: metav1.ObjectMeta{
 			Name:      names.SimpleNameGenerator.GenerateName(fmt.Sprintf("%s-%s-", s.Current.Cluster.Name, machineDeploymentTopology.Name)),
 			Namespace: s.Current.Cluster.Namespace,
-			Finalizers: []string{
-				clusterv1.MachineDeploymentTopologyFinalizer,
-			},
 		},
 		Spec: clusterv1.MachineDeploymentSpec{
 			ClusterName: s.Current.Cluster.Name,
@@ -304,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 {

diff --git a/controllers/topology/desired_state_test.go b/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"))

diff --git a/controllers/topology/internal/mergepatch/mergepatch.go b/controllers/topology/internal/mergepatch/mergepatch.go
@@ -32,7 +32,6 @@ import (
 var allowedPaths = []contract.Path{
 	{"metadata", "labels"},
 	{"metadata", "annotations"},
-	{"metadata", "finalizers"},
 	{"spec"},
 }
 

diff --git a/controllers/topology/machinedeployment_controller.go b/controllers/topology/machinedeployment_controller.go
@@ -35,7 +35,7 @@ import (
 
 // +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=machinedeployments;machinedeployments/finalizers,verbs=get;list;watch;update;patch
 // +kubebuilder:rbac:groups=cluster.x-k8s.io,resources=machinesets,verbs=get;list;watch
 
 // MachineDeploymentReconciler deletes referenced templates during deletion of topology-owned MachineDeployments.
@@ -76,6 +76,8 @@ func (r *MachineDeploymentReconciler) SetupWithManager(ctx context.Context, mgr
 //   * After that, deletion of MachineSets 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.
+// We don't have to set the finalizer, as it's already set during MachineDeployment creation
+// in the cluster topology controller.
 func (r *MachineDeploymentReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) {
 	log := ctrl.LoggerFrom(ctx)
 
@@ -101,9 +103,6 @@ func (r *MachineDeploymentReconciler) Reconcile(ctx context.Context, req ctrl.Re
 		return ctrl.Result{}, nil
 	}
 
-	// We don't have to set the finalizer, as it's already set during MachineDeployment creation
-	// in the cluster topology controller.
-
 	// Handle deletion reconciliation loop.
 	if !md.ObjectMeta.DeletionTimestamp.IsZero() {
 		return r.reconcileDelete(ctx, md)

diff --git a/controllers/topology/machineset_controller.go b/controllers/topology/machineset_controller.go
@@ -38,7 +38,7 @@ import (
 // +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
+// +kubebuilder:rbac:groups=cluster.x-k8s.io,resources=machinesets;machinesets/finalizers,verbs=get;list;watch;update;patch
 
 // 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 state,
@@ -78,6 +78,8 @@ func (r *MachineSetReconciler) SetupWithManager(ctx context.Context, mgr ctrl.Ma
 //   * 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.
+// We don't have to set the finalizer, as it's already set during MachineSet creation
+// in the MachineSet controller.
 func (r *MachineSetReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) {
 	log := ctrl.LoggerFrom(ctx)