Skip to content

Commit 7e2e9c4

Browse files
k8s-ci-robotk8s-ci-robot
authored
Merge pull request #11286 from sbueringer/pr-move-finalizer-logic
🌱 Handle finalizers early in Reconciles
2 parents 74d34e3 + 7183826 commit 7e2e9c4

File tree

16 files changed

+266
-122
lines changed

16 files changed

+266
-122
lines changed

‎controlplane/kubeadm/internal/controllers/controller.go

+6-16
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,7 @@ import (
5151
"sigs.k8s.io/cluster-api/util/annotations"
5252
"sigs.k8s.io/cluster-api/util/collections"
5353
"sigs.k8s.io/cluster-api/util/conditions"
54+
"sigs.k8s.io/cluster-api/util/finalizers"
5455
"sigs.k8s.io/cluster-api/util/patch"
5556
"sigs.k8s.io/cluster-api/util/predicates"
5657
"sigs.k8s.io/cluster-api/util/secret"
@@ -149,6 +150,11 @@ func (r *KubeadmControlPlaneReconciler) Reconcile(ctx context.Context, req ctrl.
149150
return ctrl.Result{}, err
150151
}
151152

153+
// Add finalizer first if not set to avoid the race condition between init and delete.
154+
if finalizerAdded, err := finalizers.EnsureFinalizer(ctx, r.Client, kcp, controlplanev1.KubeadmControlPlaneFinalizer); err != nil || finalizerAdded {
155+
return ctrl.Result{}, err
156+
}
157+
152158
// Fetch the Cluster.
153159
cluster, err := util.GetOwnerCluster(ctx, r.Client, kcp.ObjectMeta)
154160
if err != nil {
@@ -176,22 +182,6 @@ func (r *KubeadmControlPlaneReconciler) Reconcile(ctx context.Context, req ctrl.
176182
return ctrl.Result{Requeue: true}, nil
177183
}
178184

179-
// Add finalizer first if not set to avoid the race condition between init and delete.
180-
// Note: Finalizers in general can only be added when the deletionTimestamp is not set.
181-
if kcp.ObjectMeta.DeletionTimestamp.IsZero() && !controllerutil.ContainsFinalizer(kcp, controlplanev1.KubeadmControlPlaneFinalizer) {
182-
controllerutil.AddFinalizer(kcp, controlplanev1.KubeadmControlPlaneFinalizer)
183-
184-
// patch and return right away instead of reusing the main defer,
185-
// because the main defer may take too much time to get cluster status
186-
// Patch ObservedGeneration only if the reconciliation completed successfully
187-
patchOpts := []patch.Option{patch.WithStatusObservedGeneration{}}
188-
if err := patchHelper.Patch(ctx, kcp, patchOpts...); err != nil {
189-
return ctrl.Result{}, errors.Wrapf(err, "failed to add finalizer")
190-
}
191-
192-
return ctrl.Result{}, nil
193-
}
194-
195185
// Initialize the control plane scope; this includes also checking for orphan machines and
196186
// adopt them if necessary.
197187
controlPlane, adoptableMachineFound, err := r.initControlPlaneScope(ctx, cluster, kcp)

‎controlplane/kubeadm/internal/controllers/controller_test.go

+3
Original file line numberDiff line numberDiff line change
@@ -317,6 +317,9 @@ func TestReconcileNoCluster(t *testing.T) {
317317
Name: "foo",
318318
},
319319
},
320+
Finalizers: []string{
321+
controlplanev1.KubeadmControlPlaneFinalizer,
322+
},
320323
},
321324
Spec: controlplanev1.KubeadmControlPlaneSpec{
322325
Version: "v1.16.6",

‎exp/addons/internal/controllers/clusterresourceset_controller.go

+6-7
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,7 @@ import (
4646
resourcepredicates "sigs.k8s.io/cluster-api/exp/addons/internal/controllers/predicates"
4747
"sigs.k8s.io/cluster-api/util"
4848
"sigs.k8s.io/cluster-api/util/conditions"
49+
"sigs.k8s.io/cluster-api/util/finalizers"
4950
"sigs.k8s.io/cluster-api/util/patch"
5051
"sigs.k8s.io/cluster-api/util/predicates"
5152
)
@@ -122,6 +123,11 @@ func (r *ClusterResourceSetReconciler) Reconcile(ctx context.Context, req ctrl.R
122123
return ctrl.Result{}, err
123124
}
124125

126+
// Add finalizer first if not set to avoid the race condition between init and delete.
127+
if finalizerAdded, err := finalizers.EnsureFinalizer(ctx, r.Client, clusterResourceSet, addonsv1.ClusterResourceSetFinalizer); err != nil || finalizerAdded {
128+
return ctrl.Result{}, err
129+
}
130+
125131
// Initialize the patch helper.
126132
patchHelper, err := patch.NewHelper(clusterResourceSet, r.Client)
127133
if err != nil {
@@ -152,13 +158,6 @@ func (r *ClusterResourceSetReconciler) Reconcile(ctx context.Context, req ctrl.R
152158
return ctrl.Result{}, r.reconcileDelete(ctx, clusters, clusterResourceSet)
153159
}
154160

155-
// Add finalizer first if not set to avoid the race condition between init and delete.
156-
// Note: Finalizers in general can only be added when the deletionTimestamp is not set.
157-
if !controllerutil.ContainsFinalizer(clusterResourceSet, addonsv1.ClusterResourceSetFinalizer) {
158-
controllerutil.AddFinalizer(clusterResourceSet, addonsv1.ClusterResourceSetFinalizer)
159-
return ctrl.Result{}, nil
160-
}
161-
162161
errs := []error{}
163162
errClusterLockedOccurred := false
164163
for _, cluster := range clusters {

‎exp/internal/controllers/machinepool_controller.go

+7-8
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,7 @@ import (
4646
"sigs.k8s.io/cluster-api/util"
4747
"sigs.k8s.io/cluster-api/util/annotations"
4848
"sigs.k8s.io/cluster-api/util/conditions"
49+
"sigs.k8s.io/cluster-api/util/finalizers"
4950
"sigs.k8s.io/cluster-api/util/patch"
5051
"sigs.k8s.io/cluster-api/util/predicates"
5152
)
@@ -151,9 +152,14 @@ func (r *MachinePoolReconciler) Reconcile(ctx context.Context, req ctrl.Request)
151152
return ctrl.Result{}, err
152153
}
153154

154-
log = log.WithValues("Cluster", klog.KRef(mp.ObjectMeta.Namespace, mp.Spec.ClusterName))
155+
log = log.WithValues("Cluster", klog.KRef(mp.Namespace, mp.Spec.ClusterName))
155156
ctx = ctrl.LoggerInto(ctx, log)
156157

158+
// Add finalizer first if not set to avoid the race condition between init and delete.
159+
if finalizerAdded, err := finalizers.EnsureFinalizer(ctx, r.Client, mp, expv1.MachinePoolFinalizer); err != nil || finalizerAdded {
160+
return ctrl.Result{}, err
161+
}
162+
157163
cluster, err := util.GetClusterByName(ctx, r.Client, mp.ObjectMeta.Namespace, mp.Spec.ClusterName)
158164
if err != nil {
159165
log.Error(err, "Failed to get Cluster for MachinePool.", "MachinePool", klog.KObj(mp), "Cluster", klog.KRef(mp.ObjectMeta.Namespace, mp.Spec.ClusterName))
@@ -223,13 +229,6 @@ func (r *MachinePoolReconciler) Reconcile(ctx context.Context, req ctrl.Request)
223229
return ctrl.Result{}, err
224230
}
225231

226-
// Add finalizer first if not set to avoid the race condition between init and delete.
227-
// Note: Finalizers in general can only be added when the deletionTimestamp is not set.
228-
if !controllerutil.ContainsFinalizer(mp, expv1.MachinePoolFinalizer) {
229-
controllerutil.AddFinalizer(mp, expv1.MachinePoolFinalizer)
230-
return ctrl.Result{}, nil
231-
}
232-
233232
// Handle normal reconciliation loop.
234233
scope := &scope{
235234
cluster: cluster,

‎internal/controllers/cluster/cluster_controller.go

+6-7
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,7 @@ import (
4848
"sigs.k8s.io/cluster-api/util/annotations"
4949
"sigs.k8s.io/cluster-api/util/collections"
5050
"sigs.k8s.io/cluster-api/util/conditions"
51+
"sigs.k8s.io/cluster-api/util/finalizers"
5152
"sigs.k8s.io/cluster-api/util/patch"
5253
"sigs.k8s.io/cluster-api/util/predicates"
5354
)
@@ -120,6 +121,11 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (_ ctrl.Re
120121
return ctrl.Result{}, err
121122
}
122123

124+
// Add finalizer first if not set to avoid the race condition between init and delete.
125+
if finalizerAdded, err := finalizers.EnsureFinalizer(ctx, r.Client, cluster, clusterv1.ClusterFinalizer); err != nil || finalizerAdded {
126+
return ctrl.Result{}, err
127+
}
128+
123129
// Return early if the object or Cluster is paused.
124130
if annotations.IsPaused(cluster, cluster) {
125131
log.Info("Reconciliation is paused for this object")
@@ -152,13 +158,6 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (_ ctrl.Re
152158
return r.reconcileDelete(ctx, cluster)
153159
}
154160

155-
// Add finalizer first if not set to avoid the race condition between init and delete.
156-
// Note: Finalizers in general can only be added when the deletionTimestamp is not set.
157-
if !controllerutil.ContainsFinalizer(cluster, clusterv1.ClusterFinalizer) {
158-
controllerutil.AddFinalizer(cluster, clusterv1.ClusterFinalizer)
159-
return ctrl.Result{}, nil
160-
}
161-
162161
// Handle normal reconciliation loop.
163162
return r.reconcile(ctx, cluster)
164163
}

‎internal/controllers/machine/machine_controller.go

+9-10
Original file line numberDiff line numberDiff line change
@@ -52,6 +52,7 @@ import (
5252
"sigs.k8s.io/cluster-api/util/annotations"
5353
"sigs.k8s.io/cluster-api/util/collections"
5454
"sigs.k8s.io/cluster-api/util/conditions"
55+
"sigs.k8s.io/cluster-api/util/finalizers"
5556
clog "sigs.k8s.io/cluster-api/util/log"
5657
"sigs.k8s.io/cluster-api/util/patch"
5758
"sigs.k8s.io/cluster-api/util/predicates"
@@ -173,16 +174,21 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (_ ctrl.Re
173174
return ctrl.Result{}, err
174175
}
175176

177+
log := ctrl.LoggerFrom(ctx).WithValues("Cluster", klog.KRef(m.Namespace, m.Spec.ClusterName))
178+
ctx = ctrl.LoggerInto(ctx, log)
179+
180+
// Add finalizer first if not set to avoid the race condition between init and delete.
181+
if finalizerAdded, err := finalizers.EnsureFinalizer(ctx, r.Client, m, clusterv1.MachineFinalizer); err != nil || finalizerAdded {
182+
return ctrl.Result{}, err
183+
}
184+
176185
// AddOwners adds the owners of Machine as k/v pairs to the logger.
177186
// Specifically, it will add KubeadmControlPlane, MachineSet and MachineDeployment.
178187
ctx, log, err := clog.AddOwners(ctx, r.Client, m)
179188
if err != nil {
180189
return ctrl.Result{}, err
181190
}
182191

183-
log = log.WithValues("Cluster", klog.KRef(m.ObjectMeta.Namespace, m.Spec.ClusterName))
184-
ctx = ctrl.LoggerInto(ctx, log)
185-
186192
cluster, err := util.GetClusterByName(ctx, r.Client, m.ObjectMeta.Namespace, m.Spec.ClusterName)
187193
if err != nil {
188194
return ctrl.Result{}, errors.Wrapf(err, "failed to get cluster %q for machine %q in namespace %q",
@@ -220,13 +226,6 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (_ ctrl.Re
220226
}
221227
}()
222228

223-
// Add finalizer first if not set to avoid the race condition between init and delete.
224-
// Note: Finalizers in general can only be added when the deletionTimestamp is not set.
225-
if !controllerutil.ContainsFinalizer(m, clusterv1.MachineFinalizer) && m.ObjectMeta.DeletionTimestamp.IsZero() {
226-
controllerutil.AddFinalizer(m, clusterv1.MachineFinalizer)
227-
return ctrl.Result{}, nil
228-
}
229-
230229
alwaysReconcile := []machineReconcileFunc{
231230
r.reconcileMachineOwnerAndLabels,
232231
r.reconcileBootstrap,

‎internal/controllers/machinedeployment/machinedeployment_controller.go

+6-7
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,7 @@ import (
4343
"sigs.k8s.io/cluster-api/util/annotations"
4444
"sigs.k8s.io/cluster-api/util/conditions"
4545
utilconversion "sigs.k8s.io/cluster-api/util/conversion"
46+
"sigs.k8s.io/cluster-api/util/finalizers"
4647
clog "sigs.k8s.io/cluster-api/util/log"
4748
"sigs.k8s.io/cluster-api/util/patch"
4849
"sigs.k8s.io/cluster-api/util/predicates"
@@ -131,6 +132,11 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (_ ctrl.Re
131132
log = log.WithValues("Cluster", klog.KRef(deployment.Namespace, deployment.Spec.ClusterName))
132133
ctx = ctrl.LoggerInto(ctx, log)
133134

135+
// Add finalizer first if not set to avoid the race condition between init and delete.
136+
if finalizerAdded, err := finalizers.EnsureFinalizer(ctx, r.Client, deployment, clusterv1.MachineDeploymentFinalizer); err != nil || finalizerAdded {
137+
return ctrl.Result{}, err
138+
}
139+
134140
cluster, err := util.GetClusterByName(ctx, r.Client, deployment.Namespace, deployment.Spec.ClusterName)
135141
if err != nil {
136142
return ctrl.Result{}, err
@@ -165,13 +171,6 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (_ ctrl.Re
165171
return ctrl.Result{}, r.reconcileDelete(ctx, deployment)
166172
}
167173

168-
// Add finalizer first if not set to avoid the race condition between init and delete.
169-
// Note: Finalizers in general can only be added when the deletionTimestamp is not set.
170-
if !controllerutil.ContainsFinalizer(deployment, clusterv1.MachineDeploymentFinalizer) {
171-
controllerutil.AddFinalizer(deployment, clusterv1.MachineDeploymentFinalizer)
172-
return ctrl.Result{}, nil
173-
}
174-
175174
err = r.reconcile(ctx, cluster, deployment)
176175
if err != nil {
177176
r.recorder.Eventf(deployment, corev1.EventTypeWarning, "ReconcileError", "%v", err)

‎internal/controllers/machineset/machineset_controller.go

+9-10
Original file line numberDiff line numberDiff line change
@@ -56,6 +56,7 @@ import (
5656
"sigs.k8s.io/cluster-api/util/collections"
5757
"sigs.k8s.io/cluster-api/util/conditions"
5858
utilconversion "sigs.k8s.io/cluster-api/util/conversion"
59+
"sigs.k8s.io/cluster-api/util/finalizers"
5960
"sigs.k8s.io/cluster-api/util/labels/format"
6061
clog "sigs.k8s.io/cluster-api/util/log"
6162
"sigs.k8s.io/cluster-api/util/patch"
@@ -149,16 +150,21 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (_ ctrl.Re
149150
return ctrl.Result{}, err
150151
}
151152

153+
log := ctrl.LoggerFrom(ctx).WithValues("Cluster", klog.KRef(machineSet.Namespace, machineSet.Spec.ClusterName))
154+
ctx = ctrl.LoggerInto(ctx, log)
155+
156+
// Add finalizer first if not set to avoid the race condition between init and delete.
157+
if finalizerAdded, err := finalizers.EnsureFinalizer(ctx, r.Client, machineSet, clusterv1.MachineSetFinalizer); err != nil || finalizerAdded {
158+
return ctrl.Result{}, err
159+
}
160+
152161
// AddOwners adds the owners of MachineSet as k/v pairs to the logger.
153162
// Specifically, it will add MachineDeployment.
154163
ctx, log, err := clog.AddOwners(ctx, r.Client, machineSet)
155164
if err != nil {
156165
return ctrl.Result{}, err
157166
}
158167

159-
log = log.WithValues("Cluster", klog.KRef(machineSet.ObjectMeta.Namespace, machineSet.Spec.ClusterName))
160-
ctx = ctrl.LoggerInto(ctx, log)
161-
162168
cluster, err := util.GetClusterByName(ctx, r.Client, machineSet.ObjectMeta.Namespace, machineSet.Spec.ClusterName)
163169
if err != nil {
164170
return ctrl.Result{}, err
@@ -188,13 +194,6 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (_ ctrl.Re
188194
return ctrl.Result{}, r.reconcileDelete(ctx, machineSet)
189195
}
190196

191-
// Add finalizer first if not set to avoid the race condition between init and delete.
192-
// Note: Finalizers in general can only be added when the deletionTimestamp is not set.
193-
if !controllerutil.ContainsFinalizer(machineSet, clusterv1.MachineSetFinalizer) {
194-
controllerutil.AddFinalizer(machineSet, clusterv1.MachineSetFinalizer)
195-
return ctrl.Result{}, nil
196-
}
197-
198197
result, err := r.reconcile(ctx, cluster, machineSet)
199198
if err != nil {
200199
// Requeue if the reconcile failed because the ClusterCacheTracker was locked for

‎internal/controllers/topology/machinedeployment/machinedeployment_controller.go

+12-13
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@ import (
3535
"sigs.k8s.io/cluster-api/internal/controllers/topology/machineset"
3636
"sigs.k8s.io/cluster-api/util"
3737
"sigs.k8s.io/cluster-api/util/annotations"
38+
"sigs.k8s.io/cluster-api/util/finalizers"
3839
"sigs.k8s.io/cluster-api/util/labels"
3940
"sigs.k8s.io/cluster-api/util/patch"
4041
"sigs.k8s.io/cluster-api/util/predicates"
@@ -124,6 +125,17 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (_ ctrl.Re
124125
log = log.WithValues("Cluster", klog.KRef(md.Namespace, md.Spec.ClusterName))
125126
ctx = ctrl.LoggerInto(ctx, log)
126127

128+
// Return early if the MachineDeployment is not topology owned.
129+
if !labels.IsTopologyOwned(md) {
130+
log.Info(fmt.Sprintf("Reconciliation is skipped because the MachineDeployment does not have the %q label", clusterv1.ClusterTopologyOwnedLabel))
131+
return ctrl.Result{}, nil
132+
}
133+
134+
// Add finalizer first if not set to avoid the race condition between init and delete.
135+
if finalizerAdded, err := finalizers.EnsureFinalizer(ctx, r.Client, md, clusterv1.MachineDeploymentTopologyFinalizer); err != nil || finalizerAdded {
136+
return ctrl.Result{}, err
137+
}
138+
127139
cluster, err := util.GetClusterByName(ctx, r.Client, md.Namespace, md.Spec.ClusterName)
128140
if err != nil {
129141
return ctrl.Result{}, err
@@ -135,12 +147,6 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (_ ctrl.Re
135147
return ctrl.Result{}, nil
136148
}
137149

138-
// Return early if the MachineDeployment is not topology owned.
139-
if !labels.IsTopologyOwned(md) {
140-
log.Info(fmt.Sprintf("Reconciliation is skipped because the MachineDeployment does not have the %q label", clusterv1.ClusterTopologyOwnedLabel))
141-
return ctrl.Result{}, nil
142-
}
143-
144150
// Create a patch helper to add or remove the finalizer from the MachineDeployment.
145151
patchHelper, err := patch.NewHelper(md, r.Client)
146152
if err != nil {
@@ -157,13 +163,6 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (_ ctrl.Re
157163
return ctrl.Result{}, r.reconcileDelete(ctx, md)
158164
}
159165

160-
// Add finalizer first if not set to avoid the race condition between init and delete.
161-
// Note: Finalizers in general can only be added when the deletionTimestamp is not set.
162-
if !controllerutil.ContainsFinalizer(md, clusterv1.MachineDeploymentTopologyFinalizer) {
163-
controllerutil.AddFinalizer(md, clusterv1.MachineDeploymentTopologyFinalizer)
164-
return ctrl.Result{}, nil
165-
}
166-
167166
return ctrl.Result{}, nil
168167
}
169168

0 commit comments

Comments
 (0)