Address comments

Author: fabriziopandini

internal/controllers/machinedeployment/machinedeployment_controller.go

@@ -355,28 +355,16 @@ func (r *Reconciler) createOrUpdateMachineSetsAndSyncMachineDeploymentRevision(c
 
 	// Apply changes to MachineSets.
 	for _, ms := range allMSs {
-		log = log.WithValues("MachineSet", klog.KObj(ms))
-		ctx = ctrl.LoggerInto(ctx, log)
+		log := log.WithValues("MachineSet", klog.KObj(ms))
+		ctx := ctrl.LoggerInto(ctx, log)
 
 		// Retrieve the diff for the MachineSet.
 		// Note: no need to check for diff doesn't exist, all the diff have been computed right above.
 		diff := machineSetsDiff[ms.Name]
 
-		// Add to the log kv pairs providing context and details about changes in this MachineSet (reason, diff),
-		// and also add the overview of all the MachineSets computed above.
-		// Note: Those values should not be added to the context to prevent propagation to other func.
-		statusToLogKeyAndValues := []any{
-			"reason", diff.Reason,
-			"diff", diff.OtherChanges,
-			"machinesets", machineSetsSummary,
-		}
-		if len(p.acknowledgedMachineNames) > 0 {
-			statusToLogKeyAndValues = append(statusToLogKeyAndValues, "acknowledgedMachines", sortAndJoin(p.acknowledgedMachineNames))
-		}
-		if len(p.updatingMachineNames) > 0 {
-			statusToLogKeyAndValues = append(statusToLogKeyAndValues, "updatingMachines", sortAndJoin(p.updatingMachineNames))
-		}
-		log := log.WithValues(statusToLogKeyAndValues...)
+		// Add to the log kv pairs providing the overview of all the MachineSets computed above.
+		// Note: This value should not be added to the context to prevent propagation to other func.
+		log = log.WithValues("machineSets", machineSetsSummary)
 
 		if diff.OriginalMS == nil {
 			// Create the MachineSet.
@@ -388,7 +376,7 @@ func (r *Reconciler) createOrUpdateMachineSetsAndSyncMachineDeploymentRevision(c
 				log.Info(fmt.Sprintf("MachineSets need rollout: %s", strings.Join(machineSetNames(p.oldMSs), ", ")), "reason", p.createReason)
 			}
 			log.Info(fmt.Sprintf("MachineSet %s created, it is now the current MachineSet", ms.Name))
-			if ptr.Deref(ms.Spec.Replicas, 0) > 0 {
+			if diff.DesiredReplicas > 0 {
 				log.Info(fmt.Sprintf("Scaled up current MachineSet %s from 0 to %d replicas (+%[2]d)", ms.Name, diff.DesiredReplicas))
 			}
 			r.recorder.Eventf(p.md, corev1.EventTypeNormal, "SuccessfulCreate", "Created MachineSet %s with %d replicas", klog.KObj(ms), diff.DesiredReplicas)
@@ -403,6 +391,20 @@ func (r *Reconciler) createOrUpdateMachineSetsAndSyncMachineDeploymentRevision(c
 			continue
 		}
 
+		// Add to the log kv pairs providing context and details about changes in this MachineSet (reason, diff)
+		// Note: Those values should not be added to the context to prevent propagation to other func.
+		statusToLogKeyAndValues := []any{
+			"reason", diff.Reason,
+			"diff", diff.OtherChanges,
+		}
+		if len(p.acknowledgedMachineNames) > 0 {
+			statusToLogKeyAndValues = append(statusToLogKeyAndValues, "acknowledgedMachines", sortAndJoin(p.acknowledgedMachineNames))
+		}
+		if len(p.updatingMachineNames) > 0 {
+			statusToLogKeyAndValues = append(statusToLogKeyAndValues, "updatingMachines", sortAndJoin(p.updatingMachineNames))
+		}
+		log = log.WithValues(statusToLogKeyAndValues...)
+
 		err := ssa.Patch(ctx, r.Client, machineDeploymentManagerName, ms, ssa.WithCachingProxy{Cache: r.ssaCache, Original: diff.OriginalMS})
 		if err != nil {
 			// Note: If we are Applying a MachineSet with UID set and the MachineSet does not exist anymore, the

internal/controllers/machinedeployment/machinedeployment_rollout_ondelete.go

@@ -111,7 +111,7 @@ func (p *rolloutPlanner) reconcileOldMachineSetsOnDelete(ctx context.Context) {
 		scaleDownCount := max(ptr.Deref(oldMS.Spec.Replicas, 0)-ptr.Deref(oldMS.Status.Replicas, 0), 0)
 		if scaleDownCount > 0 {
 			newScaleIntent := max(ptr.Deref(oldMS.Spec.Replicas, 0)-scaleDownCount, 0)
-			p.addNote(oldMS, "scale down unavailable or not existing Machines")
+			p.addNote(oldMS, "scale down to align to existing Machines")
 			log.V(5).Info(fmt.Sprintf("Setting scale down intent for MachineSet %s to %d replicas (-%d)", oldMS.Name, newScaleIntent, scaleDownCount), "MachineSet", klog.KObj(oldMS))
 			p.scaleIntents[oldMS.Name] = newScaleIntent
 

internal/controllers/machinedeployment/machinedeployment_rollout_ondelete_test.go

@@ -153,8 +153,8 @@ func TestReconcileOldMachineSetsOnDelete(t *testing.T) {
 				"ms2": 0, // scale down by one, one machines has been deleted
 			},
 			expectedNotes: map[string][]string{
-				"ms1": {"scale down unavailable or not existing Machines"},
-				"ms2": {"scale down unavailable or not existing Machines"},
+				"ms1": {"scale down to align to existing Machines"},
+				"ms2": {"scale down to align to existing Machines"},
 			},
 		},
 		{
@@ -330,7 +330,7 @@ func TestReconcileOldMachineSetsOnDelete(t *testing.T) {
 				"ms2": 1, // scale down by one, 1 exceeding machine removed from ms1
 			},
 			expectedNotes: map[string][]string{
-				"ms1": {"scale down unavailable or not existing Machines"},
+				"ms1": {"scale down to align to existing Machines"},
 				"ms2": {"scale down to align MachineSet spec.replicas to MachineDeployment spec.replicas"},
 			},
 		},

internal/controllers/machinedeployment/machinedeployment_rollout_planner.go

@@ -126,6 +126,9 @@ func (p *rolloutPlanner) init(ctx context.Context, md *clusterv1.MachineDeployme
 			return err
 		}
 		p.newMS = desiredNewMS
+		// Note: when an oldMS becomes again the current one, computeDesiredNewMS changes its revision number;
+		// the new revision number is also surfaced in p.revision, and thus we are using this information
+		// to determine when to add this note.
 		if oldRevision != p.revision {
 			p.addNote(p.newMS, "this is now the current MachineSet")
 		}
@@ -379,9 +382,6 @@ func (p *rolloutPlanner) getMachineSetDiff(ms *clusterv1.MachineSet) machineSetD
 	// Compute the desired spec.replicas
 	// Note: Used to determine if this is a scale up or a scale down operation.
 	diff.DesiredReplicas = ptr.Deref(ms.Spec.Replicas, 0)
-	if v, ok := p.scaleIntents[ms.Name]; ok {
-		diff.DesiredReplicas = v
-	}
 
 	// Compute a message that summarize changes to Machine set spec replicas.
 	// Note. Also other replicas counters not changed by the rollout planner are included, because they are used in the computation.

internal/controllers/machinedeployment/machinedeployment_rollout_planner_test.go

@@ -1002,7 +1002,7 @@ func (r rolloutScope) rolloutPlannerResultSummary(p *rolloutPlanner) string {
 		if d.OriginalMS == nil {
 			o += fmt.Sprintf("    - MachineSet %s created, it is now the current MachineSet\n", ms.Name)
 			if d.DesiredReplicas > 0 {
-				o += fmt.Sprintf("    - Scaled up current MachineSet %s from 0 to %d replicas (+%[2]d)\n", ms.Name, ptr.Deref(ms.Spec.Replicas, 0))
+				o += fmt.Sprintf("    - Scaled up current MachineSet %s from 0 to %d replicas (+%[2]d)\n", ms.Name, d.DesiredReplicas)
 				o += fmt.Sprintf("      - Reason: %s\n", d.Reason)
 			}
 			continue

internal/controllers/machinedeployment/machinedeployment_rollout_rollingupdate.go

@@ -372,8 +372,12 @@ func (p *rolloutPlanner) scaleDownOldMSs(ctx context.Context, totalScaleDownCoun
 
 		if scaleDown > 0 {
 			newScaleIntent := max(replicas-scaleDown, 0)
+			// Check if totalAvailableReplicas has been changed above and use it as a signal to determine if scale down
+			// was to align to existing Machines (replicas count > number of machines) or if scale down was performed
+			// removing unavailable replicas when no available replicas exist on the MachineSet.
+			// Note. In both cases overall availability is not impacted.
 			if oldTotalAvailableReplicas == totalAvailableReplicas {
-				p.addNote(oldMS, "scale down unavailable or not existing Machines")
+				p.addNote(oldMS, "scale down to align to existing Machines or scale down by removing unavailable replicas (and no available replicas exist on the MachineSet)")
 			} else {
 				p.addNote(oldMS, "%d available replicas > %d minimum available replicas", oldTotalAvailableReplicas, minAvailable)
 			}

internal/controllers/machinedeployment/machinedeployment_rollout_rollingupdate_test.go

@@ -559,8 +559,8 @@ func Test_reconcileOldMachineSetsRollingUpdate(t *testing.T) {
 				// no need to further scale down.
 			},
 			expectedNotes: map[string][]string{
-				"ms2": {"scale down unavailable or not existing Machines"},
-				"ms3": {"scale down unavailable or not existing Machines"},
+				"ms2": {"scale down to align to existing Machines or scale down by removing unavailable replicas (and no available replicas exist on the MachineSet)"},
+				"ms3": {"scale down to align to existing Machines or scale down by removing unavailable replicas (and no available replicas exist on the MachineSet)"},
 			},
 		},
 		{
@@ -582,8 +582,8 @@ func Test_reconcileOldMachineSetsRollingUpdate(t *testing.T) {
 			},
 			expectedNotes: map[string][]string{
 				"ms1": {"4 available replicas > 3 minimum available replicas"},
-				"ms2": {"scale down unavailable or not existing Machines"},
-				"ms3": {"scale down unavailable or not existing Machines"},
+				"ms2": {"scale down to align to existing Machines or scale down by removing unavailable replicas (and no available replicas exist on the MachineSet)"},
+				"ms3": {"scale down to align to existing Machines or scale down by removing unavailable replicas (and no available replicas exist on the MachineSet)"},
 			},
 		},
 		{
@@ -604,9 +604,9 @@ func Test_reconcileOldMachineSetsRollingUpdate(t *testing.T) {
 				"ms1": 2, // 3 available replicas from ms1 + 1 available replica from ms4 = 4 available replicas > minAvailability, scale down to 2 replicas, also drop the replica without machine (-2)
 			},
 			expectedNotes: map[string][]string{
-				"ms1": {"scale down unavailable or not existing Machines", "4 available replicas > 3 minimum available replicas"},
-				"ms2": {"scale down unavailable or not existing Machines"},
-				"ms3": {"scale down unavailable or not existing Machines"},
+				"ms1": {"scale down to align to existing Machines or scale down by removing unavailable replicas (and no available replicas exist on the MachineSet)", "4 available replicas > 3 minimum available replicas"},
+				"ms2": {"scale down to align to existing Machines or scale down by removing unavailable replicas (and no available replicas exist on the MachineSet)"},
+				"ms3": {"scale down to align to existing Machines or scale down by removing unavailable replicas (and no available replicas exist on the MachineSet)"},
 			},
 		},
 		{
@@ -626,7 +626,7 @@ func Test_reconcileOldMachineSetsRollingUpdate(t *testing.T) {
 				// does not make sense to continue scale down as there is no guarantee that MS3 would remove the unavailable replica
 			},
 			expectedNotes: map[string][]string{
-				"ms2": {"scale down unavailable or not existing Machines"},
+				"ms2": {"scale down to align to existing Machines or scale down by removing unavailable replicas (and no available replicas exist on the MachineSet)"},
 			},
 		},
 		{
@@ -647,9 +647,9 @@ func Test_reconcileOldMachineSetsRollingUpdate(t *testing.T) {
 				// does not make sense to continue scale down.
 			},
 			expectedNotes: map[string][]string{
-				"ms1": {"scale down unavailable or not existing Machines"},
-				"ms2": {"scale down unavailable or not existing Machines"},
-				"ms3": {"scale down unavailable or not existing Machines"},
+				"ms1": {"scale down to align to existing Machines or scale down by removing unavailable replicas (and no available replicas exist on the MachineSet)"},
+				"ms2": {"scale down to align to existing Machines or scale down by removing unavailable replicas (and no available replicas exist on the MachineSet)"},
+				"ms3": {"scale down to align to existing Machines or scale down by removing unavailable replicas (and no available replicas exist on the MachineSet)"},
 			},
 		},
 		{

internal/controllers/machinedeployment/testdata/ondelete/12 replicas, maxuserunavailable 1, scale down to 6, random(0).test.log.golden

@@ -92,7 +92,7 @@
     md: md: 5/6 replicas, 5 available, 0 upToDate
     ms1: from 2/3 to 2/2 replicas, 2 available, 0 upToDate (m10,m11)
       - Scaled down old MachineSet ms1 from 3 to 2 replicas (-1)
-        - Reason: scale down unavailable or not existing Machines
+        - Reason: scale down to align to existing Machines
         - Diff: replicas 2
     ms2: 3/3 replicas, 3 available, 3 upToDate (m13,m14,m15)
 [M controller] Iteration 8, Reconcile m10
@@ -162,7 +162,7 @@
     md: md: 5/6 replicas, 5 available, 0 upToDate
     ms1: from 1/2 to 1/1 replicas, 1 available, 0 upToDate (m10)
       - Scaled down old MachineSet ms1 from 2 to 1 replicas (-1)
-        - Reason: scale down unavailable or not existing Machines
+        - Reason: scale down to align to existing Machines
         - Diff: replicas 1
     ms2: 4/4 replicas, 4 available, 4 upToDate (m13,m14,m15,m16)
 [M controller] Iteration 14, Reconcile m10
@@ -209,7 +209,7 @@
     md: md: 5/6 replicas, 5 available, 0 upToDate
     ms1: from 0/1 to 0/0 replicas, 0 available, 0 upToDate ()
       - Scaled down old MachineSet ms1 from 1 to 0 replicas (-1)
-        - Reason: scale down unavailable or not existing Machines
+        - Reason: scale down to align to existing Machines
         - Diff: replicas 0
     ms2: 5/5 replicas, 5 available, 5 upToDate (m13,m14,m15,m16,m17)
 [M controller] Iteration 17, Reconcile m17

internal/controllers/machinedeployment/testdata/ondelete/12 replicas, maxuserunavailable 1, scale down to 6.test.log.golden

@@ -43,7 +43,7 @@
     md: md: 5/6 replicas, 5 available, 0 upToDate
     ms1: from 2/3 to 2/2 replicas, 2 available, 0 upToDate (m11,m12)
       - Scaled down old MachineSet ms1 from 3 to 2 replicas (-1)
-        - Reason: scale down unavailable or not existing Machines
+        - Reason: scale down to align to existing Machines
         - Diff: replicas 2
     ms2: 3/3 replicas, 3 available, 3 upToDate (m13,m14,m15)
 [MS controller] Iteration 3, Reconcile ms1: 2/2 replicas, 2 available, 0 upToDate (m11,m12)
@@ -88,7 +88,7 @@
     md: md: 5/6 replicas, 5 available, 0 upToDate
     ms1: from 1/2 to 1/1 replicas, 1 available, 0 upToDate (m12)
       - Scaled down old MachineSet ms1 from 2 to 1 replicas (-1)
-        - Reason: scale down unavailable or not existing Machines
+        - Reason: scale down to align to existing Machines
         - Diff: replicas 1
     ms2: 4/4 replicas, 4 available, 4 upToDate (m13,m14,m15,m16)
 [MS controller] Iteration 6, Reconcile ms1: 1/1 replicas, 1 available, 0 upToDate (m12)
@@ -133,7 +133,7 @@
     md: md: 5/6 replicas, 5 available, 0 upToDate
     ms1: from 0/1 to 0/0 replicas, 0 available, 0 upToDate ()
       - Scaled down old MachineSet ms1 from 1 to 0 replicas (-1)
-        - Reason: scale down unavailable or not existing Machines
+        - Reason: scale down to align to existing Machines
         - Diff: replicas 0
     ms2: 5/5 replicas, 5 available, 5 upToDate (m13,m14,m15,m16,m17)
 [MS controller] Iteration 9, Reconcile ms1: 0/0 replicas, 0 available, 0 upToDate ()

internal/controllers/machinedeployment/testdata/ondelete/3 replicas, maxuserunavailable 1, random(0).test.log.golden

@@ -40,7 +40,7 @@
     md: md: 2/3 replicas, 2 available, 0 upToDate
     ms1: from 2/3 to 2/2 replicas, 2 available, 0 upToDate (m1,m3)
       - Scaled down old MachineSet ms1 from 3 to 2 replicas (-1)
-        - Reason: scale down unavailable or not existing Machines
+        - Reason: scale down to align to existing Machines
         - Diff: replicas 2
     ms2: 0/0 replicas, 0 available, 0 upToDate ()
 [MS controller] Iteration 5, Reconcile ms1: 2/2 replicas, 2 available, 0 upToDate (m1,m3)
@@ -109,7 +109,7 @@
     md: md: 2/3 replicas, 2 available, 0 upToDate
     ms1: from 1/2 to 1/1 replicas, 1 available, 0 upToDate (m1)
       - Scaled down old MachineSet ms1 from 2 to 1 replicas (-1)
-        - Reason: scale down unavailable or not existing Machines
+        - Reason: scale down to align to existing Machines
         - Diff: replicas 1
     ms2: 1/1 replicas, 1 available, 1 upToDate (m4)
 [MD controller] Iteration 10, Reconcile md
@@ -154,7 +154,7 @@
     md: md: 2/3 replicas, 2 available, 0 upToDate
     ms1: from 0/1 to 0/0 replicas, 0 available, 0 upToDate ()
       - Scaled down old MachineSet ms1 from 1 to 0 replicas (-1)
-        - Reason: scale down unavailable or not existing Machines
+        - Reason: scale down to align to existing Machines
         - Diff: replicas 0
     ms2: 2/2 replicas, 2 available, 2 upToDate (m4,m5)
 [MS controller] Iteration 14, Reconcile ms2: 2/2 replicas, 2 available, 2 upToDate (m4,m5)