Address comments

Author: fabriziopandini

internal/controllers/machinedeployment/machinedeployment_controller.go

@@ -362,21 +362,9 @@ func (r *Reconciler) createOrUpdateMachineSetsAndSyncMachineDeploymentRevision(c
 		// 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_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)
 			}