Merge pull request #12464 from sbueringer/pr-remove-explicit-defaulting

k8s-ci-robot · web-flow · commit 07f2337b4c2b · 2025-07-09T03:13:33.000-07:00
🌱 Remove explicit defaulting of MS deletePolicy, MHC maxUnhealthy, KCPTemplate rolloutStrategy
diff --git a/controlplane/kubeadm/internal/webhooks/kubeadmcontrolplanetemplate.go b/controlplane/kubeadm/internal/webhooks/kubeadmcontrolplanetemplate.go
@@ -59,8 +59,6 @@ func (webhook *KubeadmControlPlaneTemplate) Default(_ context.Context, obj runti
 
 	k.Spec.Template.Spec.KubeadmConfigSpec.Default()
 
-	k.Spec.Template.Spec.RolloutStrategy = defaultRolloutStrategy(k.Spec.Template.Spec.RolloutStrategy)
-
 	return nil
 }
 
diff --git a/controlplane/kubeadm/internal/webhooks/kubeadmcontrolplanetemplate_test.go b/controlplane/kubeadm/internal/webhooks/kubeadmcontrolplanetemplate_test.go
@@ -58,8 +58,6 @@ func TestKubeadmControlPlaneTemplateDefault(t *testing.T) {
 	g.Expect(webhook.Default(ctx, kcpTemplate)).To(Succeed())
 
 	g.Expect(kcpTemplate.Spec.Template.Spec.KubeadmConfigSpec.Format).To(Equal(bootstrapv1.CloudConfig))
-	g.Expect(kcpTemplate.Spec.Template.Spec.RolloutStrategy.Type).To(Equal(controlplanev1.RollingUpdateStrategyType))
-	g.Expect(kcpTemplate.Spec.Template.Spec.RolloutStrategy.RollingUpdate.MaxSurge.IntVal).To(Equal(int32(1)))
 }
 
 func TestKubeadmControlPlaneTemplateValidationFeatureGateEnabled(t *testing.T) {
diff --git a/exp/topology/desiredstate/desired_state_test.go b/exp/topology/desiredstate/desired_state_test.go
@@ -3339,7 +3339,6 @@ func TestMergeMap(t *testing.T) {
 }
 
 func Test_computeMachineHealthCheck(t *testing.T) {
-	maxUnhealthyValue := intstr.FromString("100%")
 	mhcSpec := &clusterv1.MachineHealthCheckClass{
 		UnhealthyNodeConditions: []clusterv1.UnhealthyNodeCondition{
 			{
@@ -3382,8 +3381,6 @@ func Test_computeMachineHealthCheck(t *testing.T) {
 			Selector: metav1.LabelSelector{MatchLabels: map[string]string{
 				"foo": "bar",
 			}},
-			// MaxUnhealthy is added by defaulting values using MachineHealthCheck.Default()
-			MaxUnhealthy: &maxUnhealthyValue,
 			UnhealthyNodeConditions: []clusterv1.UnhealthyNodeCondition{
 				{
 					Type:           corev1.NodeReady,
diff --git a/internal/controllers/machinehealthcheck/machinehealthcheck_controller.go b/internal/controllers/machinehealthcheck/machinehealthcheck_controller.go
@@ -71,6 +71,10 @@ const (
 	totalTargetKeyLog      = "totalTarget"
 )
 
+var (
+	defaultMaxUnhealthy = intstr.FromString("100%")
+)
+
 // +kubebuilder:rbac:groups=core,resources=events,verbs=create;patch
 // +kubebuilder:rbac:groups=core,resources=secrets,verbs=get;list;watch
 // +kubebuilder:rbac:groups=cluster.x-k8s.io,resources=machines;machines/status,verbs=get;list;watch;delete
@@ -251,16 +255,17 @@ func (r *Reconciler) reconcile(ctx context.Context, logger logr.Logger, cluster
 		var message string
 
 		if m.Spec.UnhealthyRange == "" {
+			maxUnhealthyValue := ptr.To(ptr.Deref(m.Spec.MaxUnhealthy, defaultMaxUnhealthy)).String()
 			logger.V(3).Info(
 				"Short-circuiting remediation",
 				totalTargetKeyLog, totalTargets,
-				maxUnhealthyKeyLog, m.Spec.MaxUnhealthy,
+				maxUnhealthyKeyLog, maxUnhealthyValue,
 				unhealthyTargetsKeyLog, len(unhealthy),
 			)
 			message = fmt.Sprintf("Remediation is not allowed, the number of not started or unhealthy machines exceeds maxUnhealthy (total: %v, unhealthy: %v, maxUnhealthy: %v)",
 				totalTargets,
 				len(unhealthy),
-				m.Spec.MaxUnhealthy)
+				maxUnhealthyValue)
 		} else {
 			logger.V(3).Info(
 				"Short-circuiting remediation",
@@ -328,7 +333,7 @@ func (r *Reconciler) reconcile(ctx context.Context, logger logr.Logger, cluster
 		logger.V(3).Info(
 			"Remediations are allowed",
 			totalTargetKeyLog, totalTargets,
-			maxUnhealthyKeyLog, m.Spec.MaxUnhealthy,
+			maxUnhealthyKeyLog, ptr.To(ptr.Deref(m.Spec.MaxUnhealthy, defaultMaxUnhealthy)).String(),
 			unhealthyTargetsKeyLog, len(unhealthy),
 		)
 	} else {
@@ -706,10 +711,7 @@ func getUnhealthyRange(mhc *clusterv1.MachineHealthCheck) (int, int, error) {
 }
 
 func getMaxUnhealthy(mhc *clusterv1.MachineHealthCheck) (int, error) {
-	if mhc.Spec.MaxUnhealthy == nil {
-		return 0, errors.New("spec.maxUnhealthy must be set")
-	}
-	maxUnhealthy, err := intstr.GetScaledValueFromIntOrPercent(mhc.Spec.MaxUnhealthy, int(ptr.Deref[int32](mhc.Status.ExpectedMachines, 0)), false)
+	maxUnhealthy, err := intstr.GetScaledValueFromIntOrPercent(ptr.To(ptr.Deref(mhc.Spec.MaxUnhealthy, defaultMaxUnhealthy)), int(ptr.Deref[int32](mhc.Status.ExpectedMachines, 0)), false)
 	if err != nil {
 		return 0, err
 	}
diff --git a/internal/controllers/machinehealthcheck/machinehealthcheck_controller_test.go b/internal/controllers/machinehealthcheck/machinehealthcheck_controller_test.go
@@ -2279,7 +2279,7 @@ func TestIsAllowedRemediation(t *testing.T) {
 			maxUnhealthy:     nil,
 			expectedMachines: int32(3),
 			currentHealthy:   int32(0),
-			allowed:          false,
+			allowed:          true,
 		},
 		{
 			name:             "when maxUnhealthy is not an int or percentage",
@@ -2365,9 +2365,8 @@ func TestGetMaxUnhealthy(t *testing.T) {
 		{
 			name:                 "when maxUnhealthy is nil",
 			maxUnhealthy:         nil,
-			expectedMaxUnhealthy: 0,
+			expectedMaxUnhealthy: 7,
 			actualMachineCount:   7,
-			expectedErr:          errors.New("spec.maxUnhealthy must be set"),
 		},
 		{
 			name:                 "when maxUnhealthy is not an int or percentage",
diff --git a/internal/controllers/machineset/machineset_controller.go b/internal/controllers/machineset/machineset_controller.go
@@ -17,6 +17,7 @@ limitations under the License.
 package machineset
 
 import (
+	"cmp"
 	"context"
 	"fmt"
 	"math"
@@ -825,7 +826,7 @@ func (r *Reconciler) syncReplicas(ctx context.Context, s *scope) (ctrl.Result, e
 		}
 		return ctrl.Result{}, r.waitForMachineCreation(ctx, machineList)
 	case diff > 0:
-		log.Info(fmt.Sprintf("MachineSet is scaling down to %d replicas by deleting %d machines", *(ms.Spec.Replicas), diff), "replicas", *(ms.Spec.Replicas), "machineCount", len(machines), "deletePolicy", ms.Spec.DeletePolicy)
+		log.Info(fmt.Sprintf("MachineSet is scaling down to %d replicas by deleting %d machines", *(ms.Spec.Replicas), diff), "replicas", *(ms.Spec.Replicas), "machineCount", len(machines), "deletePolicy", cmp.Or(ms.Spec.DeletePolicy, clusterv1.RandomMachineSetDeletePolicy))
 
 		deletePriorityFunc, err := getDeletePriorityFunc(ms)
 		if err != nil {
diff --git a/internal/webhooks/machinehealthcheck.go b/internal/webhooks/machinehealthcheck.go
@@ -77,11 +77,6 @@ func (webhook *MachineHealthCheck) Default(_ context.Context, obj runtime.Object
 	}
 	m.Labels[clusterv1.ClusterNameLabel] = m.Spec.ClusterName
 
-	if m.Spec.MaxUnhealthy == nil {
-		defaultMaxUnhealthy := intstr.FromString("100%")
-		m.Spec.MaxUnhealthy = &defaultMaxUnhealthy
-	}
-
 	if m.Spec.NodeStartupTimeoutSeconds == nil {
 		m.Spec.NodeStartupTimeoutSeconds = &clusterv1.DefaultNodeStartupTimeoutSeconds
 	}
diff --git a/internal/webhooks/machinehealthcheck_test.go b/internal/webhooks/machinehealthcheck_test.go
@@ -53,7 +53,6 @@ func TestMachineHealthCheckDefault(t *testing.T) {
 	g.Expect(webhook.Default(ctx, mhc)).To(Succeed())
 
 	g.Expect(mhc.Labels[clusterv1.ClusterNameLabel]).To(Equal(mhc.Spec.ClusterName))
-	g.Expect(mhc.Spec.MaxUnhealthy.String()).To(Equal("100%"))
 	g.Expect(mhc.Spec.NodeStartupTimeoutSeconds).ToNot(BeNil())
 	g.Expect(*mhc.Spec.NodeStartupTimeoutSeconds).To(Equal(int32(10 * 60)))
 }
diff --git a/internal/webhooks/machineset.go b/internal/webhooks/machineset.go
@@ -103,10 +103,6 @@ func (webhook *MachineSet) Default(ctx context.Context, obj runtime.Object) erro
 	}
 	m.Spec.Replicas = ptr.To[int32](replicas)
 
-	if m.Spec.DeletePolicy == "" {
-		m.Spec.DeletePolicy = clusterv1.RandomMachineSetDeletePolicy
-	}
-
 	if m.Spec.Selector.MatchLabels == nil {
 		m.Spec.Selector.MatchLabels = make(map[string]string)
 	}
diff --git a/internal/webhooks/machineset_test.go b/internal/webhooks/machineset_test.go
@@ -60,7 +60,6 @@ func TestMachineSetDefault(t *testing.T) {
 	g.Expect(webhook.Default(reqCtx, ms)).To(Succeed())
 
 	g.Expect(ms.Labels[clusterv1.ClusterNameLabel]).To(Equal(ms.Spec.ClusterName))
-	g.Expect(ms.Spec.DeletePolicy).To(Equal(clusterv1.RandomMachineSetDeletePolicy))
 	g.Expect(ms.Spec.Selector.MatchLabels).To(HaveKeyWithValue(clusterv1.MachineSetNameLabel, "test-ms"))
 	g.Expect(ms.Spec.Template.Labels).To(HaveKeyWithValue(clusterv1.MachineSetNameLabel, "test-ms"))
 	g.Expect(ms.Spec.Template.Spec.Version).To(Equal("v1.19.10"))

Original file line number	Diff line number	Diff line change
`@@ -59,8 +59,6 @@ func (webhook *KubeadmControlPlaneTemplate) Default(_ context.Context, obj runti`
`59`	`59`
`60`	`60`	`k.Spec.Template.Spec.KubeadmConfigSpec.Default()`
`61`	`61`
`62`		`- k.Spec.Template.Spec.RolloutStrategy = defaultRolloutStrategy(k.Spec.Template.Spec.RolloutStrategy)`
`63`		`-`
`64`	`62`	`return nil`
`65`	`63`	`}`
`66`	`64`
Original file line number	Diff line number	Diff line change
`@@ -58,8 +58,6 @@ func TestKubeadmControlPlaneTemplateDefault(t *testing.T) {`
`58`	`58`	`g.Expect(webhook.Default(ctx, kcpTemplate)).To(Succeed())`
`59`	`59`
`60`	`60`	`g.Expect(kcpTemplate.Spec.Template.Spec.KubeadmConfigSpec.Format).To(Equal(bootstrapv1.CloudConfig))`
`61`		`- g.Expect(kcpTemplate.Spec.Template.Spec.RolloutStrategy.Type).To(Equal(controlplanev1.RollingUpdateStrategyType))`
`62`		`- g.Expect(kcpTemplate.Spec.Template.Spec.RolloutStrategy.RollingUpdate.MaxSurge.IntVal).To(Equal(int32(1)))`
`63`	`61`	`}`
`64`	`62`
`65`	`63`	`func TestKubeadmControlPlaneTemplateValidationFeatureGateEnabled(t *testing.T) {`
Original file line number	Diff line number	Diff line change
`@@ -3339,7 +3339,6 @@ func TestMergeMap(t *testing.T) {`
`3339`	`3339`	`}`
`3340`	`3340`
`3341`	`3341`	`func Test_computeMachineHealthCheck(t *testing.T) {`
`3342`		`- maxUnhealthyValue := intstr.FromString("100%")`
`3343`	`3342`	`mhcSpec := &clusterv1.MachineHealthCheckClass{`
`3344`	`3343`	`UnhealthyNodeConditions: []clusterv1.UnhealthyNodeCondition{`
`3345`	`3344`	`{`
`@@ -3382,8 +3381,6 @@ func Test_computeMachineHealthCheck(t *testing.T) {`
`3382`	`3381`	`Selector: metav1.LabelSelector{MatchLabels: map[string]string{`
`3383`	`3382`	`"foo": "bar",`
`3384`	`3383`	`}},`
`3385`		`- // MaxUnhealthy is added by defaulting values using MachineHealthCheck.Default()`
`3386`		`- MaxUnhealthy: &maxUnhealthyValue,`
`3387`	`3384`	`UnhealthyNodeConditions: []clusterv1.UnhealthyNodeCondition{`
`3388`	`3385`	`{`
`3389`	`3386`	`Type: corev1.NodeReady,`
Original file line number	Diff line number	Diff line change
`@@ -2279,7 +2279,7 @@ func TestIsAllowedRemediation(t *testing.T) {`
`2279`	`2279`	`maxUnhealthy: nil,`
`2280`	`2280`	`expectedMachines: int32(3),`
`2281`	`2281`	`currentHealthy: int32(0),`
`2282`		`- allowed: false,`
	`2282`	`+ allowed: true,`
`2283`	`2283`	`},`
`2284`	`2284`	`{`
`2285`	`2285`	`name: "when maxUnhealthy is not an int or percentage",`
`@@ -2365,9 +2365,8 @@ func TestGetMaxUnhealthy(t *testing.T) {`
`2365`	`2365`	`{`
`2366`	`2366`	`name: "when maxUnhealthy is nil",`
`2367`	`2367`	`maxUnhealthy: nil,`
`2368`		`- expectedMaxUnhealthy: 0,`
	`2368`	`+ expectedMaxUnhealthy: 7,`
`2369`	`2369`	`actualMachineCount: 7,`
`2370`		`- expectedErr: errors.New("spec.maxUnhealthy must be set"),`
`2371`	`2370`	`},`
`2372`	`2371`	`{`
`2373`	`2372`	`name: "when maxUnhealthy is not an int or percentage",`
Original file line number	Diff line number	Diff line change
`@@ -77,11 +77,6 @@ func (webhook *MachineHealthCheck) Default(_ context.Context, obj runtime.Object`
`77`	`77`	`}`
`78`	`78`	`m.Labels[clusterv1.ClusterNameLabel] = m.Spec.ClusterName`
`79`	`79`
`80`		`- if m.Spec.MaxUnhealthy == nil {`
`81`		`- defaultMaxUnhealthy := intstr.FromString("100%")`
`82`		`- m.Spec.MaxUnhealthy = &defaultMaxUnhealthy`
`83`		`- }`
`84`		`-`
`85`	`80`	`if m.Spec.NodeStartupTimeoutSeconds == nil {`
`86`	`81`	`m.Spec.NodeStartupTimeoutSeconds = &clusterv1.DefaultNodeStartupTimeoutSeconds`
`87`	`82`	`}`
Original file line number	Diff line number	Diff line change
`@@ -53,7 +53,6 @@ func TestMachineHealthCheckDefault(t *testing.T) {`
`53`	`53`	`g.Expect(webhook.Default(ctx, mhc)).To(Succeed())`
`54`	`54`
`55`	`55`	`g.Expect(mhc.Labels[clusterv1.ClusterNameLabel]).To(Equal(mhc.Spec.ClusterName))`
`56`		`- g.Expect(mhc.Spec.MaxUnhealthy.String()).To(Equal("100%"))`
`57`	`56`	`g.Expect(mhc.Spec.NodeStartupTimeoutSeconds).ToNot(BeNil())`
`58`	`57`	`g.Expect(mhc.Spec.NodeStartupTimeoutSeconds).To(Equal(int32(10 60)))`
`59`	`58`	`}`
Original file line number	Diff line number	Diff line change
`@@ -103,10 +103,6 @@ func (webhook *MachineSet) Default(ctx context.Context, obj runtime.Object) erro`
`103`	`103`	`}`
`104`	`104`	`m.Spec.Replicas = ptr.To[int32](replicas)`
`105`	`105`
`106`		`- if m.Spec.DeletePolicy == "" {`
`107`		`- m.Spec.DeletePolicy = clusterv1.RandomMachineSetDeletePolicy`
`108`		`- }`
`109`		`-`
`110`	`106`	`if m.Spec.Selector.MatchLabels == nil {`
`111`	`107`	`m.Spec.Selector.MatchLabels = make(map[string]string)`
`112`	`108`	`}`