Skip to content

Commit 674e4b7

Browse files
fabriziopandinifabriziopandini
committed
Address feedback
1 parent 917cab3 commit 674e4b7

5 files changed

+66
-42
lines changed

‎internal/controllers/machine/machine_controller.go

+39-31
Original file line numberDiff line numberDiff line change
@@ -195,12 +195,12 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (_ ctrl.Re
195195
return ctrl.Result{}, nil
196196
}
197197

198-
// Initialize the patch helper
199198
s := &scope{
200199
cluster: cluster,
201200
machine: m,
202201
}
203202

203+
// Initialize the patch helper
204204
patchHelper, err := patch.NewHelper(m, r.Client)
205205
if err != nil {
206206
return ctrl.Result{}, err
@@ -220,12 +220,6 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (_ ctrl.Re
220220
}
221221
}()
222222

223-
// Always add the cluster label labels.
224-
if m.Labels == nil {
225-
m.Labels = make(map[string]string)
226-
}
227-
m.Labels[clusterv1.ClusterNameLabel] = m.Spec.ClusterName
228-
229223
// Add finalizer first if not set to avoid the race condition between init and delete.
230224
// Note: Finalizers in general can only be added when the deletionTimestamp is not set.
231225
if !controllerutil.ContainsFinalizer(m, clusterv1.MachineFinalizer) && m.ObjectMeta.DeletionTimestamp.IsZero() {
@@ -234,6 +228,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (_ ctrl.Re
234228
}
235229

236230
alwaysReconcile := []machineReconcileFunc{
231+
r.reconcileMachineOwnerAndLabels,
237232
r.reconcileBootstrap,
238233
r.reconcileInfrastructure,
239234
r.reconcileNode,
@@ -258,12 +253,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (_ ctrl.Re
258253
}
259254

260255
// Handle normal reconciliation loop.
261-
reconcileNormal := append(
262-
[]machineReconcileFunc{r.reconcileMachineOwner},
263-
alwaysReconcile...,
264-
)
265-
266-
res, err := doReconcile(ctx, reconcileNormal, s)
256+
res, err := doReconcile(ctx, alwaysReconcile, s)
267257
// Requeue if the reconcile failed because the ClusterCacheTracker was locked for
268258
// the current cluster because of concurrent access.
269259
if errors.Is(err, remote.ErrClusterLocked) {
@@ -355,15 +345,21 @@ type scope struct {
355345
// Machine. It is set after reconcileInfrastructure is called.
356346
infraMachine *unstructured.Unstructured
357347

348+
// infraMachineNotFound is true if getting the infra machine object failed with an IsNotFound err
349+
infraMachineIsNotFound bool
350+
358351
// bootstrapConfig is the BootstrapConfig object that is referenced by the
359352
// Machine. It is set after reconcileBootstrap is called.
360353
bootstrapConfig *unstructured.Unstructured
361354

355+
// bootstrapConfigNotFound is true if getting the BootstrapConfig object failed with an IsNotFound err
356+
bootstrapConfigIsNotFound bool
357+
362358
// node is the Kubernetes node hosted on the machine.
363359
node *corev1.Node
364360
}
365361

366-
func (r *Reconciler) reconcileMachineOwner(_ context.Context, s *scope) (ctrl.Result, error) {
362+
func (r *Reconciler) reconcileMachineOwnerAndLabels(_ context.Context, s *scope) (ctrl.Result, error) {
367363
// If the machine is a stand-alone one, meaning not originated from a MachineDeployment, then set it as directly
368364
// owned by the Cluster (if not already present).
369365
if r.shouldAdopt(s.machine) {
@@ -375,6 +371,12 @@ func (r *Reconciler) reconcileMachineOwner(_ context.Context, s *scope) (ctrl.Re
375371
}))
376372
}
377373

374+
// Always add the cluster label.
375+
if s.machine.Labels == nil {
376+
s.machine.Labels = make(map[string]string)
377+
}
378+
s.machine.Labels[clusterv1.ClusterNameLabel] = s.machine.Spec.ClusterName
379+
378380
return ctrl.Result{}, nil
379381
}
380382

@@ -504,13 +506,15 @@ func (r *Reconciler) reconcileDelete(ctx context.Context, s *scope) (ctrl.Result
504506
return ctrl.Result{}, nil
505507
}
506508

507-
bootstrapDeleted, err := r.reconcileDeleteBootstrap(ctx, s)
508-
if err != nil {
509-
return ctrl.Result{}, err
510-
}
511-
if !bootstrapDeleted {
512-
log.Info("Waiting for bootstrap to be deleted", m.Spec.Bootstrap.ConfigRef.Kind, klog.KRef(m.Spec.Bootstrap.ConfigRef.Namespace, m.Spec.Bootstrap.ConfigRef.Name))
513-
return ctrl.Result{}, nil
509+
if m.Spec.Bootstrap.ConfigRef != nil {
510+
bootstrapDeleted, err := r.reconcileDeleteBootstrap(ctx, s)
511+
if err != nil {
512+
return ctrl.Result{}, err
513+
}
514+
if !bootstrapDeleted {
515+
log.Info("Waiting for bootstrap to be deleted", m.Spec.Bootstrap.ConfigRef.Kind, klog.KRef(m.Spec.Bootstrap.ConfigRef.Namespace, m.Spec.Bootstrap.ConfigRef.Name))
516+
return ctrl.Result{}, nil
517+
}
514518
}
515519

516520
// We only delete the node after the underlying infrastructure is gone.
@@ -869,30 +873,34 @@ func (r *Reconciler) deleteNode(ctx context.Context, cluster *clusterv1.Cluster,
869873
}
870874

871875
func (r *Reconciler) reconcileDeleteBootstrap(ctx context.Context, s *scope) (bool, error) {
872-
if s.bootstrapConfig == nil {
876+
if s.bootstrapConfig == nil && s.bootstrapConfigIsNotFound {
873877
conditions.MarkFalse(s.machine, clusterv1.BootstrapReadyCondition, clusterv1.DeletedReason, clusterv1.ConditionSeverityInfo, "")
874878
return true, nil
875879
}
876880

877-
if err := r.Client.Delete(ctx, s.bootstrapConfig); err != nil && !apierrors.IsNotFound(err) {
878-
return false, errors.Wrapf(err,
879-
"failed to delete %v %q for Machine %q in namespace %q",
880-
s.bootstrapConfig.GroupVersionKind(), s.bootstrapConfig.GetName(), s.machine.Name, s.machine.Namespace)
881+
if s.bootstrapConfig != nil {
882+
if err := r.Client.Delete(ctx, s.bootstrapConfig); err != nil && !apierrors.IsNotFound(err) {
883+
return false, errors.Wrapf(err,
884+
"failed to delete %v %q for Machine %q in namespace %q",
885+
s.bootstrapConfig.GroupVersionKind(), s.bootstrapConfig.GetName(), s.machine.Name, s.machine.Namespace)
886+
}
881887
}
882888

883889
return false, nil
884890
}
885891

886892
func (r *Reconciler) reconcileDeleteInfrastructure(ctx context.Context, s *scope) (bool, error) {
887-
if s.infraMachine == nil {
893+
if s.infraMachine == nil && s.infraMachineIsNotFound {
888894
conditions.MarkFalse(s.machine, clusterv1.InfrastructureReadyCondition, clusterv1.DeletedReason, clusterv1.ConditionSeverityInfo, "")
889895
return true, nil
890896
}
891897

892-
if err := r.Client.Delete(ctx, s.infraMachine); err != nil && !apierrors.IsNotFound(err) {
893-
return false, errors.Wrapf(err,
894-
"failed to delete %v %q for Machine %q in namespace %q",
895-
s.infraMachine.GroupVersionKind(), s.infraMachine.GetName(), s.machine.Name, s.machine.Namespace)
898+
if s.infraMachine != nil {
899+
if err := r.Client.Delete(ctx, s.infraMachine); err != nil && !apierrors.IsNotFound(err) {
900+
return false, errors.Wrapf(err,
901+
"failed to delete %v %q for Machine %q in namespace %q",
902+
s.infraMachine.GroupVersionKind(), s.infraMachine.GetName(), s.machine.Name, s.machine.Namespace)
903+
}
896904
}
897905

898906
return false, nil

‎internal/controllers/machine/machine_controller_phases.go

+16-4
Original file line numberDiff line numberDiff line change
@@ -142,12 +142,14 @@ func (r *Reconciler) reconcileBootstrap(ctx context.Context, s *scope) (ctrl.Res
142142
obj, err := r.reconcileExternal(ctx, cluster, m, m.Spec.Bootstrap.ConfigRef)
143143
if err != nil {
144144
if apierrors.IsNotFound(err) {
145+
s.bootstrapConfigIsNotFound = true
146+
145147
if !s.machine.DeletionTimestamp.IsZero() {
146148
// Tolerate bootstrap object not found when the machine is being deleted.
147149
// TODO: we can also relax this and tolerate the absence of the bootstrap ref way before, e.g. after node ref is set
148150
return ctrl.Result{}, nil
149151
}
150-
log.Info("could not find bootstrap config object, requeuing", m.Spec.Bootstrap.ConfigRef.Kind, klog.KRef(m.Spec.Bootstrap.ConfigRef.Namespace, m.Spec.Bootstrap.ConfigRef.Name))
152+
log.Info("Could not find bootstrap config object, requeuing", m.Spec.Bootstrap.ConfigRef.Kind, klog.KRef(m.Spec.Bootstrap.ConfigRef.Namespace, m.Spec.Bootstrap.ConfigRef.Name))
151153
// TODO: we can make this smarter and requeue only if we are before node ref is set
152154
return ctrl.Result{RequeueAfter: externalReadyWait}, nil
153155
}
@@ -169,9 +171,13 @@ func (r *Reconciler) reconcileBootstrap(ctx context.Context, s *scope) (ctrl.Res
169171
}
170172

171173
// Report a summary of current status of the bootstrap object defined for this machine.
174+
fallBack := conditions.WithFallbackValue(ready, clusterv1.WaitingForDataSecretFallbackReason, clusterv1.ConditionSeverityInfo, "")
175+
if !s.machine.DeletionTimestamp.IsZero() {
176+
fallBack = conditions.WithFallbackValue(ready, clusterv1.DeletingReason, clusterv1.ConditionSeverityInfo, "")
177+
}
172178
conditions.SetMirror(m, clusterv1.BootstrapReadyCondition,
173179
conditions.UnstructuredGetter(s.bootstrapConfig),
174-
conditions.WithFallbackValue(ready, clusterv1.WaitingForDataSecretFallbackReason, clusterv1.ConditionSeverityInfo, ""),
180+
fallBack,
175181
)
176182

177183
// If the bootstrap provider is not ready, return.
@@ -205,6 +211,8 @@ func (r *Reconciler) reconcileInfrastructure(ctx context.Context, s *scope) (ctr
205211
obj, err := r.reconcileExternal(ctx, cluster, m, &m.Spec.InfrastructureRef)
206212
if err != nil {
207213
if apierrors.IsNotFound(err) {
214+
s.infraMachineIsNotFound = true
215+
208216
if !s.machine.DeletionTimestamp.IsZero() {
209217
// Tolerate infra machine not found when the machine is being deleted.
210218
return ctrl.Result{}, nil
@@ -218,7 +226,7 @@ func (r *Reconciler) reconcileInfrastructure(ctx context.Context, s *scope) (ctr
218226
m.Spec.InfrastructureRef.GroupVersionKind(), m.Spec.InfrastructureRef.Name))
219227
return ctrl.Result{}, reconcile.TerminalError(errors.Errorf("could not find %v %q for Machine %q in namespace %q", m.Spec.InfrastructureRef.GroupVersionKind().String(), m.Spec.InfrastructureRef.Name, m.Name, m.Namespace))
220228
}
221-
log.Info("could not find infrastructure machine, requeuing", m.Spec.InfrastructureRef.Kind, klog.KRef(m.Spec.InfrastructureRef.Namespace, m.Spec.InfrastructureRef.Name))
229+
log.Info("Could not find infrastructure machine, requeuing", m.Spec.InfrastructureRef.Kind, klog.KRef(m.Spec.InfrastructureRef.Namespace, m.Spec.InfrastructureRef.Name))
222230
return ctrl.Result{RequeueAfter: externalReadyWait}, nil
223231
}
224232
return ctrl.Result{}, err
@@ -239,9 +247,13 @@ func (r *Reconciler) reconcileInfrastructure(ctx context.Context, s *scope) (ctr
239247
}
240248

241249
// Report a summary of current status of the infrastructure object defined for this machine.
250+
fallBack := conditions.WithFallbackValue(ready, clusterv1.WaitingForInfrastructureFallbackReason, clusterv1.ConditionSeverityInfo, "")
251+
if !s.machine.DeletionTimestamp.IsZero() {
252+
fallBack = conditions.WithFallbackValue(ready, clusterv1.DeletingReason, clusterv1.ConditionSeverityInfo, "")
253+
}
242254
conditions.SetMirror(m, clusterv1.InfrastructureReadyCondition,
243255
conditions.UnstructuredGetter(s.infraMachine),
244-
conditions.WithFallbackValue(ready, clusterv1.WaitingForInfrastructureFallbackReason, clusterv1.ConditionSeverityInfo, ""),
256+
fallBack,
245257
)
246258

247259
// If the infrastructure provider is not ready (and it wasn't ready before), return early.

‎internal/controllers/machine/machine_controller_status.go

+2-2
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
Copyright 2025 The Kubernetes Authors.
2+
Copyright 2024 The Kubernetes Authors.
33
44
Licensed under the Apache License, Version 2.0 (the "License");
55
you may not use this file except in compliance with the License.
@@ -185,7 +185,7 @@ func setInfrastructureReadyCondition(_ context.Context, machine *clusterv1.Machi
185185
Type: clusterv1.MachineInfrastructureReadyV1Beta2Condition,
186186
Status: metav1.ConditionFalse,
187187
Reason: clusterv1.MachineInfrastructureDeletedV1Beta2Reason,
188-
Message: fmt.Sprintf("%s %s has been deleted while the machine still exist", machine.Spec.Bootstrap.ConfigRef.Kind, machine.Name),
188+
Message: fmt.Sprintf("%s %s has been deleted while the machine still exist", machine.Spec.InfrastructureRef.Kind, klog.KRef(machine.Namespace, machine.Spec.InfrastructureRef.Name)),
189189
})
190190
return
191191
}

‎internal/controllers/machine/machine_controller_status_test.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
Copyright 2025 The Kubernetes Authors.
2+
Copyright 2024 The Kubernetes Authors.
33
44
Licensed under the Apache License, Version 2.0 (the "License");
55
you may not use this file except in compliance with the License.

‎internal/controllers/machine/machine_controller_test.go

+8-4
Original file line numberDiff line numberDiff line change
@@ -2782,8 +2782,10 @@ func TestNodeDeletion(t *testing.T) {
27822782
}
27832783

27842784
s := &scope{
2785-
cluster: cluster,
2786-
machine: m,
2785+
cluster: cluster,
2786+
machine: m,
2787+
infraMachineIsNotFound: true,
2788+
bootstrapConfigIsNotFound: true,
27872789
}
27882790
_, err := r.reconcileDelete(context.Background(), s)
27892791

@@ -2905,8 +2907,10 @@ func TestNodeDeletionWithoutNodeRefFallback(t *testing.T) {
29052907
}
29062908

29072909
s := &scope{
2908-
cluster: testCluster.DeepCopy(),
2909-
machine: m,
2910+
cluster: testCluster.DeepCopy(),
2911+
machine: m,
2912+
infraMachineIsNotFound: true,
2913+
bootstrapConfigIsNotFound: true,
29102914
}
29112915
_, err := r.reconcileDelete(context.Background(), s)
29122916

0 commit comments

Comments
 (0)