diff --git a/internal/controllers/machinedeployment/machinedeployment_rollout_rollingupdate.go b/internal/controllers/machinedeployment/machinedeployment_rollout_rollingupdate.go index 7b692963d67c..87868e6ad406 100644 --- a/internal/controllers/machinedeployment/machinedeployment_rollout_rollingupdate.go +++ b/internal/controllers/machinedeployment/machinedeployment_rollout_rollingupdate.go @@ -247,12 +247,18 @@ func (p *rolloutPlanner) reconcileOldMachineSetsRollingUpdate(ctx context.Contex } } + // On top of considering maxUnavailable, before scaling down it is also important to consider how unavailability is distributed + // across MachineSets, because if there are unavailable machines on the new MachineSet it is necessary to slow down or stop rollout + // to prevent to transition all the Machines to a broken state. + // In order to do so, compute the number of unavailable machines on the new MachineSet and use it to reduce totalScaleDownCount. + newMSUnavailableMachineCount := max(ptr.Deref(p.newMS.Spec.Replicas, 0)-ptr.Deref(p.newMS.Status.AvailableReplicas, 0), 0) + // Compute the total number of replicas that can be scaled down. // Exit immediately if there is no room for scaling down. // NOTE: this is a quick preliminary check to verify if there is room for scaling down any of the oldMSs; further down the code // will make additional checks to ensure scale down actually happens without breaching MaxUnavailable, and // if necessary, it will reduce the extent of the scale down accordingly. - totalScaleDownCount := max(totalSpecReplicas-totalPendingScaleDown-minAvailable, 0) + totalScaleDownCount := max(totalSpecReplicas-totalPendingScaleDown-minAvailable-newMSUnavailableMachineCount, 0) if totalScaleDownCount <= 0 { return nil } diff --git a/internal/controllers/machinedeployment/machinedeployment_rollout_rollingupdate_test.go b/internal/controllers/machinedeployment/machinedeployment_rollout_rollingupdate_test.go index 39c9dc7890cd..c75122cc6ce3 100644 --- a/internal/controllers/machinedeployment/machinedeployment_rollout_rollingupdate_test.go +++ b/internal/controllers/machinedeployment/machinedeployment_rollout_rollingupdate_test.go @@ -463,6 +463,34 @@ func Test_reconcileOldMachineSetsRollingUpdate(t *testing.T) { }, expectedNotes: map[string][]string{}, }, + { + name: "do not scale down if there are unavailable replicas on the new MachineSet (maxSurge 1, maxUnavailable 0), all machines unavailable", + scaleIntent: map[string]int32{}, + md: createMD("v2", 3, withRollingUpdateStrategy(1, 0)), + newMS: createMS("ms2", "v2", 1, withStatusReplicas(1), withStatusAvailableReplicas(0)), // one machine created on the new NewMS due to maxSurge, but it is not reaching the available state, + oldMSs: []*clusterv1.MachineSet{ + createMS("ms1", "v1", 3, withStatusReplicas(3), withStatusAvailableReplicas(0)), + }, + expectScaleIntent: map[string]int32{ + // no new scale down intent for oldMSs (ms1): unavailability on new MS + }, + expectedNotes: map[string][]string{}, + skipMaxUnavailabilityCheck: true, // The test case is simulating all machines not unavailable, so this check will fail + }, + { + name: "do not scale down if there are unavailable replicas on the new MachineSet (maxSurge 0, maxUnavailable 1), all machines unavailable", + scaleIntent: map[string]int32{}, + md: createMD("v2", 3, withRollingUpdateStrategy(0, 1)), + newMS: createMS("ms2", "v2", 1, withStatusReplicas(1), withStatusAvailableReplicas(0)), // one machine created on the new NewMS after scale down on the old MS, but it is not reaching the available state, + oldMSs: []*clusterv1.MachineSet{ + createMS("ms1", "v1", 2, withStatusReplicas(2), withStatusAvailableReplicas(0)), // OldMS scaled down due to maxUnavailable 1 + }, + expectScaleIntent: map[string]int32{ + // no new scale down intent for oldMSs (ms1): unavailability on new MS + }, + expectedNotes: map[string][]string{}, + skipMaxUnavailabilityCheck: true, // The test case is simulating all machines not unavailable, so this check will fail + }, { name: "do not scale down if there are more replicas than minAvailable replicas, but scale down from current reconcile already takes the availability buffer (newMS is scaling down)", scaleIntent: map[string]int32{