From 41017eeb35852fb7f9280ba5189e62eadbcb410a Mon Sep 17 00:00:00 2001 From: Stefan Bueringer Date: Thu, 9 Sep 2021 18:47:18 +0200 Subject: [PATCH] finalize finalizer change --- config/rbac/role.yaml | 17 +++++++++++++++-- controllers/machinedeployment_sync.go | 8 +++++--- controllers/topology/cluster_controller.go | 3 ++- controllers/topology/desired_state.go | 11 ++++++++--- controllers/topology/desired_state_test.go | 3 +++ .../topology/internal/mergepatch/mergepatch.go | 1 - .../topology/machinedeployment_controller.go | 7 +++---- controllers/topology/machineset_controller.go | 4 +++- 8 files changed, 39 insertions(+), 15 deletions(-) diff --git a/config/rbac/role.yaml b/config/rbac/role.yaml index 317ec83d446b..29a39004c3f3 100644 --- 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 index 709f2047c3a8..c3264ff7a00f 100644 --- 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 index 3c747218e93b..57f6b4bb5007 100644 --- 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 index 0505b0ae224d..84dc83fb7b66 100644 --- 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 index d1e0c93e2ba3..ea53a0405f9d 100644 --- 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 index b900a9190f11..b71d1ad33561 100644 --- 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 index 5d9c3de73d9c..6b65c7f5c26d 100644 --- 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 index f861a50608b8..f289f1cfd7a6 100644 --- 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)