Skip to content

🌱 Refactor MachineTemplateUpToDate #12811

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Oct 2, 2025
Merged

🌱 Refactor MachineTemplateUpToDate #12811

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5c81b7d
Refactor MachineTemplateUpToDate
fabriziopandini Oct 1, 2025
3c535ac
Address feedback
fabriziopandini Oct 1, 2025
aa1e139
Rebase on Main
fabriziopandini Oct 1, 2025
5021d0b
More feedback
fabriziopandini Oct 2, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions internal/controllers/machinedeployment/machinedeployment_rollout_sequence_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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")
}
Expand Down Expand Up @@ -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
}
}
Expand All @@ -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)
}
}
Expand All @@ -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) {
Expand All @@ -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) {
Expand Down
27 changes: 6 additions & 21 deletions internal/controllers/machinedeployment/machinedeployment_sync.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down
136 changes: 80 additions & 56 deletions internal/controllers/machinedeployment/mdutil/util.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand All @@ -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() && ms.CreationTimestamp.Sub(deployment.Spec.Rollout.After.Time) < 0 {
notUpToDateResult.EligibleForInPlaceUpdate = false
notUpToDateResult.LogMessages = append(notUpToDateResult.LogMessages, "metadata.creationTimestamp is older than MachineDeployment.spec.rollout.after")
// 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.
Expand Down
Loading