Merge pull request kubernetes-sigs#11128 from sbueringer/pr-kcp-propa…

…gate-timeouts 🌱 KCP: propagate timeouts to Machines with deletionTimestamp
sbueringer · Sep 4, 2024 · b6a3e68 · b6a3e68
2 parents ad08926 + f0d197a
commit b6a3e68
Show file tree

Hide file tree

Showing 3 changed files with 41 additions and 12 deletions.
diff --git a/controlplane/kubeadm/internal/controllers/controller.go b/controlplane/kubeadm/internal/controllers/controller.go
@@ -610,8 +610,29 @@ func (r *KubeadmControlPlaneReconciler) syncMachines(ctx context.Context, contro
 	patchHelpers := map[string]*patch.Helper{}
 	for machineName := range controlPlane.Machines {
 		m := controlPlane.Machines[machineName]
-		// If the machine is already being deleted, we don't need to update it.
+		// If the Machine is already being deleted, we only need to sync
+		// the subset of fields that impact tearing down the Machine.
 		if !m.DeletionTimestamp.IsZero() {
+			patchHelper, err := patch.NewHelper(m, r.Client)
+			if err != nil {
+				return err
+			}
+
+			// Set all other in-place mutable fields that impact the ability to tear down existing machines.
+			m.Spec.NodeDrainTimeout = controlPlane.KCP.Spec.MachineTemplate.NodeDrainTimeout
+			m.Spec.NodeDeletionTimeout = controlPlane.KCP.Spec.MachineTemplate.NodeDeletionTimeout
+			m.Spec.NodeVolumeDetachTimeout = controlPlane.KCP.Spec.MachineTemplate.NodeVolumeDetachTimeout
+
+			if err := patchHelper.Patch(ctx, m); err != nil {
+				return err
+			}
+
+			controlPlane.Machines[machineName] = m
+			patchHelper, err = patch.NewHelper(m, r.Client)
+			if err != nil {
+				return err
+			}
+			patchHelpers[machineName] = patchHelper
 			continue
 		}
 

diff --git a/controlplane/kubeadm/internal/controllers/controller_test.go b/controlplane/kubeadm/internal/controllers/controller_test.go
@@ -1554,6 +1554,9 @@ func TestKubeadmControlPlaneReconciler_syncMachines(t *testing.T) {
 			Bootstrap: clusterv1.Bootstrap{
 				DataSecretName: ptr.To("machine-bootstrap-secret"),
 			},
+			NodeDrainTimeout:        duration5s,
+			NodeVolumeDetachTimeout: duration5s,
+			NodeDeletionTimeout:     duration5s,
 		},
 	}
 	g.Expect(env.Create(ctx, deletingMachine, client.FieldOwner(classicManager))).To(Succeed())
@@ -1636,16 +1639,16 @@ func TestKubeadmControlPlaneReconciler_syncMachines(t *testing.T) {
 		KCP:     kcp,
 		Cluster: testCluster,
 		Machines: collections.Machines{
-			inPlaceMutatingMachine.Name: inPlaceMutatingMachine,
-			deletingMachine.Name:        deletingMachine,
-			nilInfraMachineMachine.Name: nilInfraMachineMachine,
+			inPlaceMutatingMachine.Name: inPlaceMutatingMachine.DeepCopy(),
+			deletingMachine.Name:        deletingMachine.DeepCopy(),
+			nilInfraMachineMachine.Name: nilInfraMachineMachine.DeepCopy(),
 		},
 		KubeadmConfigs: map[string]*bootstrapv1.KubeadmConfig{
-			inPlaceMutatingMachine.Name: existingKubeadmConfig,
+			inPlaceMutatingMachine.Name: existingKubeadmConfig.DeepCopy(),
 			deletingMachine.Name:        nil,
 		},
 		InfraResources: map[string]*unstructured.Unstructured{
-			inPlaceMutatingMachine.Name: existingInfraMachine,
+			inPlaceMutatingMachine.Name: existingInfraMachine.DeepCopy(),
 			deletingMachine.Name:        nil,
 		},
 	}
@@ -1806,7 +1809,14 @@ func TestKubeadmControlPlaneReconciler_syncMachines(t *testing.T) {
 	// Verify the machine labels and annotations are unchanged.
 	g.Expect(updatedDeletingMachine.Labels).Should(Equal(deletingMachine.Labels))
 	g.Expect(updatedDeletingMachine.Annotations).Should(Equal(deletingMachine.Annotations))
-	// Verify the machine spec is unchanged.
+	// Verify Node timeout values
+	g.Expect(updatedDeletingMachine.Spec.NodeDrainTimeout).Should(Equal(kcp.Spec.MachineTemplate.NodeDrainTimeout))
+	g.Expect(updatedDeletingMachine.Spec.NodeDeletionTimeout).Should(Equal(kcp.Spec.MachineTemplate.NodeDeletionTimeout))
+	g.Expect(updatedDeletingMachine.Spec.NodeVolumeDetachTimeout).Should(Equal(kcp.Spec.MachineTemplate.NodeVolumeDetachTimeout))
+	// Verify the machine spec is otherwise unchanged.
+	deletingMachine.Spec.NodeDrainTimeout = kcp.Spec.MachineTemplate.NodeDrainTimeout
+	deletingMachine.Spec.NodeDeletionTimeout = kcp.Spec.MachineTemplate.NodeDeletionTimeout
+	deletingMachine.Spec.NodeVolumeDetachTimeout = kcp.Spec.MachineTemplate.NodeVolumeDetachTimeout
 	g.Expect(updatedDeletingMachine.Spec).Should(BeComparableTo(deletingMachine.Spec))
 }
 

diff --git a/internal/controllers/machineset/machineset_controller.go b/internal/controllers/machineset/machineset_controller.go
@@ -375,18 +375,16 @@ func (r *Reconciler) syncMachines(ctx context.Context, machineSet *clusterv1.Mac
 		if !m.DeletionTimestamp.IsZero() {
 			patchHelper, err := patch.NewHelper(m, r.Client)
 			if err != nil {
-				return errors.Wrapf(err, "failed to generate patch for Machine %q", klog.KObj(m))
+				return err
 			}
 
 			// Set all other in-place mutable fields that impact the ability to tear down existing machines.
 			m.Spec.NodeDrainTimeout = machineSet.Spec.Template.Spec.NodeDrainTimeout
 			m.Spec.NodeDeletionTimeout = machineSet.Spec.Template.Spec.NodeDeletionTimeout
 			m.Spec.NodeVolumeDetachTimeout = machineSet.Spec.Template.Spec.NodeVolumeDetachTimeout
 
-			err = patchHelper.Patch(ctx, m)
-			if err != nil {
-				log.Error(err, "Failed to update Machine", "Machine", klog.KObj(m))
-				return errors.Wrapf(err, "failed to update Machine %q", klog.KObj(m))
+			if err := patchHelper.Patch(ctx, m); err != nil {
+				return err
 			}
 			continue
 		}