Merge pull request #8184 from sbueringer/pr-md-fix-nil-pointer

🐛 MD controller: fix nil pointer when OnDelete policy is used
kubernetes-sigs · Feb 27, 2023 · 5ce87b6 · 5ce87b6
2 parents 4fd3696 + e9a265f
commit 5ce87b6
Show file tree

Hide file tree

Showing 2 changed files with 51 additions and 1 deletion.
diff --git a/internal/controllers/machinedeployment/machinedeployment_sync.go b/internal/controllers/machinedeployment/machinedeployment_sync.go
@@ -336,7 +336,11 @@ func (r *Reconciler) computeDesiredMachineSet(deployment *clusterv1.MachineDeplo
 
 	// Set all other in-place mutable fields.
 	desiredMS.Spec.MinReadySeconds = pointer.Int32Deref(deployment.Spec.MinReadySeconds, 0)
-	desiredMS.Spec.DeletePolicy = pointer.StringDeref(deployment.Spec.Strategy.RollingUpdate.DeletePolicy, "")
+	if deployment.Spec.Strategy != nil && deployment.Spec.Strategy.RollingUpdate != nil {
+		desiredMS.Spec.DeletePolicy = pointer.StringDeref(deployment.Spec.Strategy.RollingUpdate.DeletePolicy, "")
+	} else {
+		desiredMS.Spec.DeletePolicy = ""
+	}
 	desiredMS.Spec.Template.Spec.NodeDrainTimeout = deployment.Spec.Template.Spec.NodeDrainTimeout
 	desiredMS.Spec.Template.Spec.NodeDeletionTimeout = deployment.Spec.Template.Spec.NodeDeletionTimeout
 	desiredMS.Spec.Template.Spec.NodeVolumeDetachTimeout = deployment.Spec.Template.Spec.NodeVolumeDetachTimeout

diff --git a/internal/controllers/machinedeployment/machinedeployment_sync_test.go b/internal/controllers/machinedeployment/machinedeployment_sync_test.go
@@ -671,6 +671,52 @@ func TestComputeDesiredMachineSet(t *testing.T) {
 		g.Expect(err).To(BeNil())
 		assertMachineSet(g, actualMS, expectedMS)
 	})
+
+	t.Run("should compute the updated MachineSet when no old MachineSets exists (", func(t *testing.T) {
+		// Set rollout strategy to "OnDelete".
+		deployment := deployment.DeepCopy()
+		deployment.Spec.Strategy = &clusterv1.MachineDeploymentStrategy{
+			Type:          clusterv1.OnDeleteMachineDeploymentStrategyType,
+			RollingUpdate: nil,
+		}
+
+		uniqueID := apirand.String(5)
+		existingMS := skeletonMSBasedOnMD.DeepCopy()
+		// computeDesiredMachineSet should retain the UID, name and the "machine-template-hash" label value
+		// of the existing machine.
+		// Other fields like labels, annotations, node timeout, etc are expected to change.
+		existingMSUID := types.UID("abc-123-uid")
+		existingMS.UID = existingMSUID
+		existingMS.Name = deployment.Name + "-" + uniqueID
+		existingMS.Labels = map[string]string{
+			clusterv1.MachineDeploymentUniqueLabel: uniqueID,
+			"ms-label-1":                           "ms-value-1",
+		}
+		existingMS.Annotations = nil
+		existingMS.Spec.Template.Labels = map[string]string{
+			clusterv1.MachineDeploymentUniqueLabel: uniqueID,
+			"ms-label-2":                           "ms-value-2",
+		}
+		existingMS.Spec.Template.Annotations = nil
+		existingMS.Spec.Template.Spec.NodeDrainTimeout = duration5s
+		existingMS.Spec.Template.Spec.NodeDeletionTimeout = duration5s
+		existingMS.Spec.Template.Spec.NodeVolumeDetachTimeout = duration5s
+		existingMS.Spec.DeletePolicy = string(clusterv1.NewestMachineSetDeletePolicy)
+		existingMS.Spec.MinReadySeconds = 0
+
+		expectedMS := skeletonMSBasedOnMD.DeepCopy()
+		expectedMS.UID = existingMSUID
+		expectedMS.Name = deployment.Name + "-" + uniqueID
+		expectedMS.Labels[clusterv1.MachineDeploymentUniqueLabel] = uniqueID
+		expectedMS.Spec.Template.Labels[clusterv1.MachineDeploymentUniqueLabel] = uniqueID
+		// DeletePolicy should be empty with rollout strategy "OnDelete".
+		expectedMS.Spec.DeletePolicy = ""
+
+		g := NewWithT(t)
+		actualMS, err := (&Reconciler{}).computeDesiredMachineSet(deployment, existingMS, nil, log)
+		g.Expect(err).To(BeNil())
+		assertMachineSet(g, actualMS, expectedMS)
+	})
 }
 
 func assertMachineSet(g *WithT, actualMS *clusterv1.MachineSet, expectedMS *clusterv1.MachineSet) {