🌱 Refactor MachineTemplateUpToDate (#12811)

* Refactor MachineTemplateUpToDate

* Address feedback

* Rebase on Main

* More feedback

Author: fabriziopandini

internal/controllers/machinedeployment/machinedeployment_rollout_sequence_test.go

@@ -549,7 +549,7 @@ func computeDesiredRolloutScope(current *rolloutScope, desiredMachineNames []str
 		oldMS.Status.AvailableReplicas = ptr.To(int32(0))
 		desired.machineSets = append(desired.machineSets, oldMS)
 
-		if upToDate, _, _ := mdutil.MachineTemplateUpToDate(&oldMS.Spec.Template, &desired.machineDeployment.Spec.Template); upToDate {
+		if upToDate, _ := mdutil.MachineTemplateUpToDate(&oldMS.Spec.Template, &desired.machineDeployment.Spec.Template); upToDate {
 			if newMS != nil {
 				panic("there should be only one MachineSet with MachineTemplateUpToDate")
 			}
@@ -635,7 +635,7 @@ func msLog(ms *clusterv1.MachineSet, machines []*clusterv1.Machine) string {
 
 func (r rolloutScope) newMS() *clusterv1.MachineSet {
 	for _, ms := range r.machineSets {
-		if upToDate, _, _ := mdutil.MachineTemplateUpToDate(&r.machineDeployment.Spec.Template, &ms.Spec.Template); upToDate {
+		if upToDate, _ := mdutil.MachineTemplateUpToDate(&r.machineDeployment.Spec.Template, &ms.Spec.Template); upToDate {
 			return ms
 		}
 	}
@@ -645,7 +645,7 @@ func (r rolloutScope) newMS() *clusterv1.MachineSet {
 func (r rolloutScope) oldMSs() []*clusterv1.MachineSet {
 	var oldMSs []*clusterv1.MachineSet
 	for _, ms := range r.machineSets {
-		if upToDate, _, _ := mdutil.MachineTemplateUpToDate(&r.machineDeployment.Spec.Template, &ms.Spec.Template); !upToDate {
+		if upToDate, _ := mdutil.MachineTemplateUpToDate(&r.machineDeployment.Spec.Template, &ms.Spec.Template); !upToDate {
 			oldMSs = append(oldMSs, ms)
 		}
 	}
@@ -665,7 +665,7 @@ func (r *rolloutScope) Equal(s *rolloutScope) bool {
 }
 
 func machineDeploymentIsEqual(a, b *clusterv1.MachineDeployment) bool {
-	if upToDate, _, _ := mdutil.MachineTemplateUpToDate(&a.Spec.Template, &b.Spec.Template); !upToDate ||
+	if upToDate, _ := mdutil.MachineTemplateUpToDate(&a.Spec.Template, &b.Spec.Template); !upToDate ||
 		ptr.Deref(a.Spec.Replicas, 0) != ptr.Deref(b.Spec.Replicas, 0) ||
 		ptr.Deref(a.Status.Replicas, 0) != ptr.Deref(b.Status.Replicas, 0) ||
 		ptr.Deref(a.Status.AvailableReplicas, 0) != ptr.Deref(b.Status.AvailableReplicas, 0) {
@@ -690,7 +690,7 @@ func machineSetsAreEqual(a, b []*clusterv1.MachineSet) bool {
 		if !ok {
 			return false
 		}
-		if upToDate, _, _ := mdutil.MachineTemplateUpToDate(&desiredMS.Spec.Template, &currentMS.Spec.Template); !upToDate ||
+		if upToDate, _ := mdutil.MachineTemplateUpToDate(&desiredMS.Spec.Template, &currentMS.Spec.Template); !upToDate ||
 			ptr.Deref(desiredMS.Spec.Replicas, 0) != ptr.Deref(currentMS.Spec.Replicas, 0) ||
 			ptr.Deref(desiredMS.Status.Replicas, 0) != ptr.Deref(currentMS.Status.Replicas, 0) ||
 			ptr.Deref(desiredMS.Status.AvailableReplicas, 0) != ptr.Deref(currentMS.Status.AvailableReplicas, 0) {

internal/controllers/machinedeployment/machinedeployment_sync.go

@@ -78,40 +78,25 @@ func (r *Reconciler) sync(ctx context.Context, md *clusterv1.MachineDeployment,
 // This may lead to stale reads of machine sets, thus incorrect deployment status.
 func (r *Reconciler) getAllMachineSetsAndSyncRevision(ctx context.Context, md *clusterv1.MachineDeployment, msList []*clusterv1.MachineSet, createIfNotExisted, templateExists bool) (*clusterv1.MachineSet, []*clusterv1.MachineSet, error) {
 	reconciliationTime := metav1.Now()
-	allOldMSs, err := mdutil.FindOldMachineSets(md, msList, &reconciliationTime)
-	if err != nil {
-		return nil, nil, err
-	}
+	newMS, oldMSs, _, createReason := mdutil.FindNewAndOldMachineSets(md, msList, &reconciliationTime)
 
 	// Get new machine set with the updated revision number
-	newMS, err := r.getNewMachineSet(ctx, md, msList, allOldMSs, createIfNotExisted, templateExists, &reconciliationTime)
+	newMS, err := r.getNewMachineSet(ctx, md, newMS, oldMSs, createIfNotExisted, templateExists, createReason)
 	if err != nil {
 		return nil, nil, err
 	}
 
-	return newMS, allOldMSs, nil
+	return newMS, oldMSs, nil
 }
 
 // Returns a MachineSet that matches the intent of the given MachineDeployment.
 // If there does not exist such a MachineSet and createIfNotExisted is true, create a new MachineSet.
 // If there is already such a MachineSet, update it to propagate in-place mutable fields from the MachineDeployment.
-func (r *Reconciler) getNewMachineSet(ctx context.Context, md *clusterv1.MachineDeployment, msList, oldMSs []*clusterv1.MachineSet, createIfNotExists, templateExists bool, reconciliationTime *metav1.Time) (*clusterv1.MachineSet, error) {
-	// Try to find a MachineSet which matches the MachineDeployments intent, while ignore diffs between
-	// the in-place mutable fields.
-	// If we find a matching MachineSet we just update it to propagate any changes to the in-place mutable
-	// fields and thus we do not trigger an unnecessary rollout (i.e. create a new MachineSet).
-	// If we don't find a matching MachineSet, we need a rollout and thus create a new MachineSet.
-	// Note: The in-place mutable fields can be just updated inline, because they do not affect the actual machines
-	// themselves (i.e. the infrastructure and the software running on the Machines not the Machine object).
-	matchingMS, createReason, err := mdutil.FindNewMachineSet(md, msList, reconciliationTime)
-	if err != nil {
-		return nil, err
-	}
-
+func (r *Reconciler) getNewMachineSet(ctx context.Context, md *clusterv1.MachineDeployment, newMS *clusterv1.MachineSet, oldMSs []*clusterv1.MachineSet, createIfNotExists, templateExists bool, createReason string) (*clusterv1.MachineSet, error) {
 	// If there is a MachineSet that matches the intent of the MachineDeployment, update the MachineSet
 	// to propagate all in-place mutable fields from MachineDeployment to the MachineSet.
-	if matchingMS != nil {
-		updatedMS, err := r.updateMachineSet(ctx, md, matchingMS, oldMSs)
+	if newMS != nil {
+		updatedMS, err := r.updateMachineSet(ctx, md, newMS, oldMSs)
 		if err != nil {
 			return nil, err
 		}

internal/controllers/machinedeployment/mdutil/util.go

@@ -376,16 +376,27 @@ func getMachineSetFraction(ms clusterv1.MachineSet, md clusterv1.MachineDeployme
 	return integer.RoundToInt32(newMSsize) - *(ms.Spec.Replicas)
 }
 
+// NotUpToDateResult is the result of calling the MachineTemplateUpToDate func for a MachineTemplateSpec.
+type NotUpToDateResult struct {
+	LogMessages              []string // consider if to make this private.
+	ConditionMessages        []string
+	EligibleForInPlaceUpdate bool
+}
+
 // MachineTemplateUpToDate returns true if the current MachineTemplateSpec is up-to-date with a corresponding desired MachineTemplateSpec.
 // Note: The comparison does not consider any in-place propagated fields, as well as the version from external references.
-func MachineTemplateUpToDate(current, desired *clusterv1.MachineTemplateSpec) (upToDate bool, logMessages, conditionMessages []string) {
+func MachineTemplateUpToDate(current, desired *clusterv1.MachineTemplateSpec) (bool, *NotUpToDateResult) {
+	res := &NotUpToDateResult{
+		EligibleForInPlaceUpdate: true,
+	}
+
 	currentCopy := MachineTemplateDeepCopyRolloutFields(current)
 	desiredCopy := MachineTemplateDeepCopyRolloutFields(desired)
 
 	if currentCopy.Spec.Version != desiredCopy.Spec.Version {
-		logMessages = append(logMessages, fmt.Sprintf("spec.version %s, %s required", currentCopy.Spec.Version, desiredCopy.Spec.Version))
+		res.LogMessages = append(res.LogMessages, fmt.Sprintf("spec.version %s, %s required", currentCopy.Spec.Version, desiredCopy.Spec.Version))
 		// Note: the code computing the message for MachineDeployment's RolloutOut condition is making assumptions on the format/content of this message.
-		conditionMessages = append(conditionMessages, fmt.Sprintf("Version %s, %s required", currentCopy.Spec.Version, desiredCopy.Spec.Version))
+		res.ConditionMessages = append(res.ConditionMessages, fmt.Sprintf("Version %s, %s required", currentCopy.Spec.Version, desiredCopy.Spec.Version))
 	}
 
 	// Note: we return a message based on desired.bootstrap.ConfigRef != nil, but we always compare the entire bootstrap
@@ -394,33 +405,33 @@ func MachineTemplateUpToDate(current, desired *clusterv1.MachineTemplateSpec) (u
 	// common operation so it is acceptable to handle it in this way).
 	if currentCopy.Spec.Bootstrap.ConfigRef.IsDefined() {
 		if !reflect.DeepEqual(currentCopy.Spec.Bootstrap, desiredCopy.Spec.Bootstrap) {
-			logMessages = append(logMessages, fmt.Sprintf("spec.bootstrap.configRef %s %s, %s %s required", currentCopy.Spec.Bootstrap.ConfigRef.Kind, currentCopy.Spec.Bootstrap.ConfigRef.Name, desiredCopy.Spec.Bootstrap.ConfigRef.Kind, desiredCopy.Spec.Bootstrap.ConfigRef.Name))
+			res.LogMessages = append(res.LogMessages, fmt.Sprintf("spec.bootstrap.configRef %s %s, %s %s required", currentCopy.Spec.Bootstrap.ConfigRef.Kind, currentCopy.Spec.Bootstrap.ConfigRef.Name, desiredCopy.Spec.Bootstrap.ConfigRef.Kind, desiredCopy.Spec.Bootstrap.ConfigRef.Name))
 			// Note: dropping "Template" suffix because conditions message will surface on machine.
-			conditionMessages = append(conditionMessages, fmt.Sprintf("%s is not up-to-date", strings.TrimSuffix(currentCopy.Spec.Bootstrap.ConfigRef.Kind, clusterv1.TemplateSuffix)))
+			res.ConditionMessages = append(res.ConditionMessages, fmt.Sprintf("%s is not up-to-date", strings.TrimSuffix(currentCopy.Spec.Bootstrap.ConfigRef.Kind, clusterv1.TemplateSuffix)))
 		}
 	} else {
 		if !reflect.DeepEqual(currentCopy.Spec.Bootstrap, desiredCopy.Spec.Bootstrap) {
-			logMessages = append(logMessages, fmt.Sprintf("spec.bootstrap.dataSecretName %s, %s required", ptr.Deref(currentCopy.Spec.Bootstrap.DataSecretName, "nil"), ptr.Deref(desiredCopy.Spec.Bootstrap.DataSecretName, "nil")))
-			conditionMessages = append(conditionMessages, fmt.Sprintf("spec.bootstrap.dataSecretName %s, %s required", ptr.Deref(currentCopy.Spec.Bootstrap.DataSecretName, "nil"), ptr.Deref(desiredCopy.Spec.Bootstrap.DataSecretName, "nil")))
+			res.LogMessages = append(res.LogMessages, fmt.Sprintf("spec.bootstrap.dataSecretName %s, %s required", ptr.Deref(currentCopy.Spec.Bootstrap.DataSecretName, "nil"), ptr.Deref(desiredCopy.Spec.Bootstrap.DataSecretName, "nil")))
+			res.ConditionMessages = append(res.ConditionMessages, fmt.Sprintf("spec.bootstrap.dataSecretName %s, %s required", ptr.Deref(currentCopy.Spec.Bootstrap.DataSecretName, "nil"), ptr.Deref(desiredCopy.Spec.Bootstrap.DataSecretName, "nil")))
 		}
 	}
 
 	if !reflect.DeepEqual(currentCopy.Spec.InfrastructureRef, desiredCopy.Spec.InfrastructureRef) {
-		logMessages = append(logMessages, fmt.Sprintf("spec.infrastructureRef %s %s, %s %s required", currentCopy.Spec.InfrastructureRef.Kind, currentCopy.Spec.InfrastructureRef.Name, desiredCopy.Spec.InfrastructureRef.Kind, desiredCopy.Spec.InfrastructureRef.Name))
+		res.LogMessages = append(res.LogMessages, fmt.Sprintf("spec.infrastructureRef %s %s, %s %s required", currentCopy.Spec.InfrastructureRef.Kind, currentCopy.Spec.InfrastructureRef.Name, desiredCopy.Spec.InfrastructureRef.Kind, desiredCopy.Spec.InfrastructureRef.Name))
 		// Note: dropping "Template" suffix because conditions message will surface on machine.
-		conditionMessages = append(conditionMessages, fmt.Sprintf("%s is not up-to-date", strings.TrimSuffix(currentCopy.Spec.InfrastructureRef.Kind, clusterv1.TemplateSuffix)))
+		res.ConditionMessages = append(res.ConditionMessages, fmt.Sprintf("%s is not up-to-date", strings.TrimSuffix(currentCopy.Spec.InfrastructureRef.Kind, clusterv1.TemplateSuffix)))
 	}
 
 	if currentCopy.Spec.FailureDomain != desiredCopy.Spec.FailureDomain {
-		logMessages = append(logMessages, fmt.Sprintf("spec.failureDomain %s, %s required", currentCopy.Spec.FailureDomain, desiredCopy.Spec.FailureDomain))
-		conditionMessages = append(conditionMessages, fmt.Sprintf("Failure domain %s, %s required", currentCopy.Spec.FailureDomain, desiredCopy.Spec.FailureDomain))
+		res.LogMessages = append(res.LogMessages, fmt.Sprintf("spec.failureDomain %s, %s required", currentCopy.Spec.FailureDomain, desiredCopy.Spec.FailureDomain))
+		res.ConditionMessages = append(res.ConditionMessages, fmt.Sprintf("Failure domain %s, %s required", currentCopy.Spec.FailureDomain, desiredCopy.Spec.FailureDomain))
 	}
 
-	if len(logMessages) > 0 || len(conditionMessages) > 0 {
-		return false, logMessages, conditionMessages
+	if len(res.LogMessages) > 0 || len(res.ConditionMessages) > 0 {
+		return false, res
 	}
 
-	return true, nil, nil
+	return true, nil
 }
 
 // MachineTemplateDeepCopyRolloutFields copies a MachineTemplateSpec
@@ -446,81 +457,94 @@ func MachineTemplateDeepCopyRolloutFields(template *clusterv1.MachineTemplateSpe
 	return templateCopy
 }
 
-// FindNewMachineSet returns the new MS this given deployment targets (the one with the same machine template, ignoring
-// in-place mutable fields).
+// FindNewAndOldMachineSets returns the newMS for a MachineDeployment (the one with the same machine template, ignoring
+// in-place mutable fields) as well as return oldMSs.
 // Note: If the reconciliation time is after the deployment's `rolloutAfter` time, a MS has to be newer than
 // `rolloutAfter` to be considered as matching the deployment's intent.
 // NOTE: If we find a matching MachineSet which only differs in in-place mutable fields we can use it to
 // fulfill the intent of the MachineDeployment by just updating the MachineSet to propagate in-place mutable fields.
 // Thus we don't have to create a new MachineSet and we can avoid an unnecessary rollout.
-// NOTE: Even after we changed MachineTemplateUpToDate to ignore fields that are propagated in-place we can guarantee that if there exists a "new machineset"
-// using the old logic then a new machineset will definitely exist using the new logic. The new logic is looser. Therefore, we will
-// not face a case where there exists a machine set matching the old logic but there does not exist a machineset matching the new logic.
-// In fact previously not matching MS can now start matching the target. Since there could be multiple matches, lets choose the
-// MS with the most replicas so that there is minimum machine churn.
-func FindNewMachineSet(deployment *clusterv1.MachineDeployment, msList []*clusterv1.MachineSet, reconciliationTime *metav1.Time) (*clusterv1.MachineSet, string, error) {
+func FindNewAndOldMachineSets(deployment *clusterv1.MachineDeployment, msList []*clusterv1.MachineSet, reconciliationTime *metav1.Time) (newMS *clusterv1.MachineSet, oldMSs []*clusterv1.MachineSet, oldMSNotUpToDateResults map[string]NotUpToDateResult, createReason string) {
 	if len(msList) == 0 {
-		return nil, "no MachineSets exist for the MachineDeployment", nil
+		return nil, nil, nil, "no MachineSets exist for the MachineDeployment"
 	}
 
-	// In rare cases, such as after cluster upgrades, Deployment may end up with
-	// having more than one new MachineSets that have the same template,
-	// see https://github.com/kubernetes/kubernetes/issues/40415
-	// We deterministically choose the oldest new MachineSet with matching template hash.
+	// It could happen that there is more than one newMS candidate when reconciliationTime is > rolloutAfter; considering this, the
+	// current implementation treats candidates that will be discarded as old machine sets not eligible for in-place updates.
+	// NOTE: We could also have more than one MS candidate in the very unlikely case where
+	// the diff logic MachineTemplateUpToDate is changed by dropping one of the existing criteria (version, failureDomain, infra/BootstrapRef).
+	// NOTE: When dealing with more than one newMS candidate, deterministically choose the MS with the most replicas
+	// so that there is minimum machine churn.
+	var newMSCandidates []*clusterv1.MachineSet
 	sort.Sort(MachineSetsByDecreasingReplicas(msList))
 
-	var matchingMachineSets []*clusterv1.MachineSet
+	oldMSs = make([]*clusterv1.MachineSet, 0)
+	oldMSNotUpToDateResults = make(map[string]NotUpToDateResult)
 	var diffs []string
 	for _, ms := range msList {
-		upToDate, logMessages, _ := MachineTemplateUpToDate(&ms.Spec.Template, &deployment.Spec.Template)
+		upToDate, notUpToDateResult := MachineTemplateUpToDate(&ms.Spec.Template, &deployment.Spec.Template)
 		if upToDate {
-			matchingMachineSets = append(matchingMachineSets, ms)
+			newMSCandidates = append(newMSCandidates, ms)
 		} else {
-			diffs = append(diffs, fmt.Sprintf("MachineSet %s: diff: %s", ms.Name, strings.Join(logMessages, ", ")))
+			oldMSs = append(oldMSs, ms)
+			// Override the EligibleForInPlaceUpdate decision if rollout after is expired.
+			if !deployment.Spec.Rollout.After.IsZero() && deployment.Spec.Rollout.After.Before(reconciliationTime) && !ms.CreationTimestamp.After(deployment.Spec.Rollout.After.Time) {
+				notUpToDateResult.EligibleForInPlaceUpdate = false
+				notUpToDateResult.LogMessages = append(notUpToDateResult.LogMessages, "MachineDeployment spec.rolloutAfter expired")
+				// No need to set an additional condition message, it is not used anywhere.
+			}
+			oldMSNotUpToDateResults[ms.Name] = *notUpToDateResult
+			diffs = append(diffs, fmt.Sprintf("MachineSet %s: diff: %s", ms.Name, strings.Join(notUpToDateResult.LogMessages, ", ")))
 		}
 	}
 
-	if len(matchingMachineSets) == 0 {
-		return nil, fmt.Sprintf("couldn't find MachineSet matching MachineDeployment spec template: %s", strings.Join(diffs, "; ")), nil
+	if len(newMSCandidates) == 0 {
+		return nil, oldMSs, oldMSNotUpToDateResults, fmt.Sprintf("couldn't find MachineSet matching MachineDeployment spec template: %s", strings.Join(diffs, "; "))
 	}
 
 	// If RolloutAfter is not set, pick the first matching MachineSet.
 	if deployment.Spec.Rollout.After.IsZero() {
-		return matchingMachineSets[0], "", nil
+		for _, ms := range newMSCandidates[1:] {
+			oldMSs = append(oldMSs, ms)
+			oldMSNotUpToDateResults[ms.Name] = NotUpToDateResult{
+				// No need to set log or condition message for discarded candidates, it is not used anywhere.
+				EligibleForInPlaceUpdate: false,
+			}
+		}
+		return newMSCandidates[0], oldMSs, oldMSNotUpToDateResults, ""
 	}
 
 	// If reconciliation time is before RolloutAfter, pick the first matching MachineSet.
 	if reconciliationTime.Before(&deployment.Spec.Rollout.After) {
-		return matchingMachineSets[0], "", nil
+		for _, ms := range newMSCandidates[1:] {
+			oldMSs = append(oldMSs, ms)
+			oldMSNotUpToDateResults[ms.Name] = NotUpToDateResult{
+				// No need to set log or condition for discarded candidates, it is not used anywhere.
+				EligibleForInPlaceUpdate: false,
+			}
+		}
+		return newMSCandidates[0], oldMSs, oldMSNotUpToDateResults, ""
 	}
 
 	// Pick the first matching MachineSet that has been created at RolloutAfter or later.
-	for _, ms := range matchingMachineSets {
-		if ms.CreationTimestamp.Sub(deployment.Spec.Rollout.After.Time) >= 0 {
-			return ms, "", nil
+	for _, ms := range newMSCandidates {
+		if newMS == nil && ms.CreationTimestamp.Sub(deployment.Spec.Rollout.After.Time) >= 0 {
+			newMS = ms
+			continue
+		}
+
+		oldMSs = append(oldMSs, ms)
+		oldMSNotUpToDateResults[ms.Name] = NotUpToDateResult{
+			// No need to set log or condition for discarded candidates, it is not used anywhere.
+			EligibleForInPlaceUpdate: false,
 		}
 	}
 
 	// If no matching MachineSet was created after RolloutAfter, trigger creation of a new MachineSet.
-	return nil, fmt.Sprintf("spec.rollout.after on MachineDeployment set to %s, no MachineSet has been created afterwards", deployment.Spec.Rollout.After.Format(time.RFC3339)), nil
-}
-
-// FindOldMachineSets returns the old machine sets targeted by the given Deployment, within the given slice of MSes.
-// Returns a list of machine sets which contains all old machine sets.
-func FindOldMachineSets(deployment *clusterv1.MachineDeployment, msList []*clusterv1.MachineSet, reconciliationTime *metav1.Time) ([]*clusterv1.MachineSet, error) {
-	allMSs := make([]*clusterv1.MachineSet, 0, len(msList))
-	newMS, _, err := FindNewMachineSet(deployment, msList, reconciliationTime)
-	if err != nil {
-		return nil, err
-	}
-	for _, ms := range msList {
-		// Filter out new machine set
-		if newMS != nil && ms.UID == newMS.UID {
-			continue
-		}
-		allMSs = append(allMSs, ms)
+	if newMS == nil {
+		return nil, oldMSs, oldMSNotUpToDateResults, fmt.Sprintf("spec.rollout.after on MachineDeployment set to %s, no MachineSet has been created afterwards", deployment.Spec.Rollout.After.Format(time.RFC3339))
 	}
-	return allMSs, nil
+	return newMS, oldMSs, oldMSNotUpToDateResults, ""
 }
 
 // GetReplicaCountForMachineSets returns the sum of Replicas of the given machine sets.