Address feedback

Author: fabriziopandini

internal/controllers/machinedeployment/mdutil/util.go

@@ -376,7 +376,7 @@ func getMachineSetFraction(ms clusterv1.MachineSet, md clusterv1.MachineDeployme
 return integer.RoundToInt32(newMSsize) - *(ms.Spec.Replicas)
 }
 
-// NotUpToDateResult is the result of calling the UpToDate func for a MachineTemplateSpec.
+// 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
@@ -469,13 +469,15 @@ func FindNewAndOldMachineSets(deployment *clusterv1.MachineDeployment, msList []
 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 newMSCandidates []*clusterv1.MachineSet
 oldMSs = make([]*clusterv1.MachineSet, 0)
 oldMSNotUpToDateResults = make(map[string]NotUpToDateResult)
 var diffs []string
@@ -485,6 +487,12 @@ func FindNewAndOldMachineSets(deployment *clusterv1.MachineDeployment, msList []
 newMSCandidates = append(newMSCandidates, ms)
 } else {
 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, ", ")))
 }
@@ -500,7 +508,6 @@ func FindNewAndOldMachineSets(deployment *clusterv1.MachineDeployment, msList []
 oldMSs = append(oldMSs, ms)
 oldMSNotUpToDateResults[ms.Name] = NotUpToDateResult{
 // No need to set log or condition message for discarded candidates, it is not used anywhere.
-// NOTE: in future we might consider to move not selected newMSCandidates to newMS without performing rollouts, but this is not in scope for the first iteration of in-place.
 EligibleForInPlaceUpdate: false,
 }
 }
@@ -513,7 +520,6 @@ func FindNewAndOldMachineSets(deployment *clusterv1.MachineDeployment, msList []
 oldMSs = append(oldMSs, ms)
 oldMSNotUpToDateResults[ms.Name] = NotUpToDateResult{
 // No need to set log or condition for discarded candidates, it is not used anywhere.
-// NOTE: in future we might consider to move not selected newMSCandidates to newMS without performing rollouts, but this is not in scope for the first iteration of in-place.
 EligibleForInPlaceUpdate: false,
 }
 }
@@ -530,7 +536,6 @@ func FindNewAndOldMachineSets(deployment *clusterv1.MachineDeployment, msList []
 oldMSs = append(oldMSs, ms)
 oldMSNotUpToDateResults[ms.Name] = NotUpToDateResult{
 // No need to set log or condition for discarded candidates, it is not used anywhere.
-// NOTE: in future we might consider to move not selected newMSCandidates to newMS without performing rollouts, but this is not in scope for the first iteration of in-place.
 EligibleForInPlaceUpdate: false,
 }
 }

internal/controllers/machinedeployment/mdutil/util_test.go

@@ -381,7 +381,7 @@ func TestMachineTemplateUpToDate(t *testing.T) {
 }
 }
 
-func TestFindNewMachineSet(t *testing.T) {
+func TestFindNewAndOldMachineSets(t *testing.T) {
 threeBeforeRolloutAfter := metav1.Now()
 twoBeforeRolloutAfter := metav1.NewTime(threeBeforeRolloutAfter.Add(time.Minute))
 oneBeforeRolloutAfter := metav1.NewTime(twoBeforeRolloutAfter.Add(time.Minute))
@@ -407,6 +407,9 @@ func TestFindNewMachineSet(t *testing.T) {
 oldMS := generateMS(deployment)
 oldMS.Spec.Template.Spec.InfrastructureRef.Name = "old-infra-ref"
 
+oldMSCreatedThreeBeforeRolloutAfter := *oldMS.DeepCopy()
+oldMSCreatedThreeBeforeRolloutAfter.CreationTimestamp = threeBeforeRolloutAfter
+
 msCreatedThreeBeforeRolloutAfter := generateMS(deployment)
 msCreatedThreeBeforeRolloutAfter.CreationTimestamp = threeBeforeRolloutAfter
 
@@ -438,6 +441,21 @@ func TestFindNewMachineSet(t *testing.T) {
 expectedOldMSNotUpToDateResults: nil,
 expectedCreateReason: "no MachineSets exist for the MachineDeployment",
 },
+{
+Name: "Get nil if there are no MachineTemplate that matches the intent of the MachineDeployment",
+deployment: deployment,
+msList: []*clusterv1.MachineSet{&oldMS},
+expectedNewMS: nil,
+expectedOldMSs: []*clusterv1.MachineSet{&oldMS},
+expectedOldMSNotUpToDateResults: map[string]NotUpToDateResult{
+oldMS.Name: {
+LogMessages: []string{"spec.infrastructureRef InfrastructureMachineTemplate old-infra-ref, InfrastructureMachineTemplate new-infra-ref required"},
+ConditionMessages: []string{"InfrastructureMachine is not up-to-date"},
+EligibleForInPlaceUpdate: true,
+},
+},
+expectedCreateReason: fmt.Sprintf(`couldn't find MachineSet matching MachineDeployment spec template: MachineSet %s: diff: spec.infrastructureRef InfrastructureMachineTemplate old-infra-ref, InfrastructureMachineTemplate new-infra-ref required`, oldMS.Name),
+},
 {
 Name: "Get the MachineSet with the MachineTemplate that matches the intent of the MachineDeployment",
 deployment: deployment,
@@ -452,6 +470,14 @@ func TestFindNewMachineSet(t *testing.T) {
 },
 },
 },
+{
+Name: "Get empty old MachineSets",
+deployment: deployment,
+msList: []*clusterv1.MachineSet{&matchingMS},
+expectedNewMS: &matchingMS,
+expectedOldMSs: []*clusterv1.MachineSet{},
+expectedOldMSNotUpToDateResults: map[string]NotUpToDateResult{},
+},
 {
 Name: "Get the MachineSet with the higher replicas if multiple MachineSets match the desired intent on the MachineDeployment",
 deployment: deployment,
@@ -498,6 +524,26 @@ func TestFindNewMachineSet(t *testing.T) {
 },
 expectedCreateReason: fmt.Sprintf(`couldn't find MachineSet matching MachineDeployment spec template: MachineSet %s: diff: spec.infrastructureRef InfrastructureMachineTemplate old-infra-ref, InfrastructureMachineTemplate new-infra-ref required`, oldMS.Name),
 },
+{
+Name: "Get nil if no MachineSet matches the desired intent of the MachineDeployment, reconciliationTime is > rolloutAfter",
+deployment: *deploymentWithRolloutAfter,
+msList: []*clusterv1.MachineSet{&oldMSCreatedThreeBeforeRolloutAfter},
+expectedNewMS: nil,
+expectedOldMSs: []*clusterv1.MachineSet{&oldMSCreatedThreeBeforeRolloutAfter},
+expectedOldMSNotUpToDateResults: map[string]NotUpToDateResult{
+oldMS.Name: {
+ConditionMessages: []string{"InfrastructureMachine is not up-to-date"},
+LogMessages: []string{
+// An additional message must be added to old machine sets when reconciliationTime is > rolloutAfter.
+"spec.infrastructureRef InfrastructureMachineTemplate old-infra-ref, InfrastructureMachineTemplate new-infra-ref required",
+"metadata.creationTimestamp is older than MachineDeployment.spec.rollout.after",
+},
+// EligibleForInPlaceUpdate decision should change for oldMS when reconciliationTime is > rolloutAfter.
+EligibleForInPlaceUpdate: false,
+},
+},
+expectedCreateReason: fmt.Sprintf(`couldn't find MachineSet matching MachineDeployment spec template: MachineSet %s: diff: spec.infrastructureRef InfrastructureMachineTemplate old-infra-ref, InfrastructureMachineTemplate new-infra-ref required, metadata.creationTimestamp is older than MachineDeployment.spec.rollout.after`, oldMS.Name),
+},
 {
 Name: "Get the MachineSet if reconciliationTime < rolloutAfter",
 deployment: *deploymentWithRolloutAfter,
@@ -514,17 +560,27 @@ func TestFindNewMachineSet(t *testing.T) {
 {
 Name: "Get nil if reconciliationTime is > rolloutAfter and no MachineSet is created after rolloutAfter",
 deployment: *deploymentWithRolloutAfter,
-msList: []*clusterv1.MachineSet{&msCreatedTwoBeforeRolloutAfter, &msCreatedThreeBeforeRolloutAfter},
+msList: []*clusterv1.MachineSet{&msCreatedTwoBeforeRolloutAfter, &msCreatedThreeBeforeRolloutAfter, &oldMSCreatedThreeBeforeRolloutAfter},
 reconciliationTime: &oneAfterRolloutAfter,
 expectedNewMS: nil,
-expectedOldMSs: []*clusterv1.MachineSet{&msCreatedThreeBeforeRolloutAfter, &msCreatedTwoBeforeRolloutAfter},
+expectedOldMSs: []*clusterv1.MachineSet{&oldMSCreatedThreeBeforeRolloutAfter, &msCreatedThreeBeforeRolloutAfter, &msCreatedTwoBeforeRolloutAfter},
 expectedOldMSNotUpToDateResults: map[string]NotUpToDateResult{
 msCreatedTwoBeforeRolloutAfter.Name: {
 EligibleForInPlaceUpdate: false,
 },
 msCreatedThreeBeforeRolloutAfter.Name: {
 EligibleForInPlaceUpdate: false,
 },
+oldMS.Name: {
+ConditionMessages: []string{"InfrastructureMachine is not up-to-date"},
+LogMessages: []string{
+"spec.infrastructureRef InfrastructureMachineTemplate old-infra-ref, InfrastructureMachineTemplate new-infra-ref required",
+// An additional message must be added to old machine sets when reconciliationTime is > rolloutAfter.
+"metadata.creationTimestamp is older than MachineDeployment.spec.rollout.after",
+},
+// EligibleForInPlaceUpdate decision should change for oldMS when reconciliationTime is > rolloutAfter.
+EligibleForInPlaceUpdate: false,
+},
 },
 expectedCreateReason: fmt.Sprintf("spec.rollout.after on MachineDeployment set to %s, no MachineSet has been created afterwards", rolloutAfter.Format(time.RFC3339)),
 },