Merge pull request kubernetes-sigs#10589 from davidvossel/nodedrain-d…

…uring-delete 🌱 Propagate timeout fields from MachineSet to Machine during Machine deletion
sbueringer · Jun 12, 2024 · 28fbfbb · 28fbfbb
2 parents 9cb5d3c + da2b7c8
commit 28fbfbb
Show file tree

Hide file tree

Showing 2 changed files with 40 additions and 1 deletion.
diff --git a/internal/controllers/machineset/machineset_controller.go b/internal/controllers/machineset/machineset_controller.go
@@ -364,8 +364,24 @@ func (r *Reconciler) syncMachines(ctx context.Context, machineSet *clusterv1.Mac
 	log := ctrl.LoggerFrom(ctx)
 	for i := range machines {
 		m := machines[i]
-		// 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 a machine
 		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))
+			}
+
+			// 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))
+			}
 			continue
 		}
 

diff --git a/internal/controllers/machineset/machineset_controller_test.go b/internal/controllers/machineset/machineset_controller_test.go
@@ -1000,6 +1000,7 @@ func TestMachineSetReconciler_syncMachines(t *testing.T) {
 	replicas := int32(2)
 	version := "v1.25.3"
 	duration10s := &metav1.Duration{Duration: 10 * time.Second}
+	duration11s := &metav1.Duration{Duration: 11 * time.Second}
 	ms := &clusterv1.MachineSet{
 		ObjectMeta: metav1.ObjectMeta{
 			UID:       "abc-123-ms-uid",
@@ -1338,6 +1339,28 @@ func TestMachineSetReconciler_syncMachines(t *testing.T) {
 		g.Expect(updatedDeletingMachine.Spec.NodeDeletionTimeout).Should(Equal(deletingMachine.Spec.NodeDeletionTimeout))
 		g.Expect(updatedDeletingMachine.Spec.NodeVolumeDetachTimeout).Should(Equal(deletingMachine.Spec.NodeVolumeDetachTimeout))
 	}, 5*time.Second).Should(Succeed())
+
+	// Verify in-place mutable fields are updated on the deleting machine
+	ms.Spec.Template.Spec.NodeDrainTimeout = duration11s
+	ms.Spec.Template.Spec.NodeDeletionTimeout = duration11s
+	ms.Spec.Template.Spec.NodeVolumeDetachTimeout = duration11s
+	g.Expect(reconciler.syncMachines(ctx, ms, []*clusterv1.Machine{updatedInPlaceMutatingMachine, deletingMachine})).To(Succeed())
+	updatedDeletingMachine := deletingMachine.DeepCopy()
+
+	g.Expect(env.GetAPIReader().Get(ctx, client.ObjectKeyFromObject(updatedDeletingMachine), updatedDeletingMachine)).To(Succeed())
+	// Verify Node timeout values
+	g.Expect(updatedDeletingMachine.Spec.NodeDrainTimeout).Should(And(
+		Not(BeNil()),
+		HaveValue(Equal(*ms.Spec.Template.Spec.NodeDrainTimeout)),
+	))
+	g.Expect(updatedDeletingMachine.Spec.NodeDeletionTimeout).Should(And(
+		Not(BeNil()),
+		HaveValue(Equal(*ms.Spec.Template.Spec.NodeDeletionTimeout)),
+	))
+	g.Expect(updatedDeletingMachine.Spec.NodeVolumeDetachTimeout).Should(And(
+		Not(BeNil()),
+		HaveValue(Equal(*ms.Spec.Template.Spec.NodeVolumeDetachTimeout)),
+	))
 }
 
 func TestMachineSetReconciler_reconcileUnhealthyMachines(t *testing.T) {