Merge pull request #8415 from PiotrLewandowski323/8394

🌱 Drop the first return value from FindOldMachineSets

Author: k8s-ci-robot

internal/controllers/machinedeployment/machinedeployment_sync.go

@@ -82,7 +82,7 @@ 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 bool) (*clusterv1.MachineSet, []*clusterv1.MachineSet, error) {
 	reconciliationTime := metav1.Now()
-	_, allOldMSs := mdutil.FindOldMachineSets(md, msList, &reconciliationTime)
+	allOldMSs := mdutil.FindOldMachineSets(md, msList, &reconciliationTime)
 
 	// Get new machine set with the updated revision number
 	newMS, err := r.getNewMachineSet(ctx, md, msList, allOldMSs, createIfNotExisted, &reconciliationTime)

internal/controllers/machinedeployment/mdutil/util.go

@@ -439,11 +439,8 @@ func shouldRolloutAfter(ms *clusterv1.MachineSet, reconciliationTime *metav1.Tim
 }
 
 // FindOldMachineSets returns the old machine sets targeted by the given Deployment, within the given slice of MSes.
-// Returns two list of machine sets
-//   - the first contains all old machine sets with non-zero replicas
-//   - the second contains all old machine sets
-func FindOldMachineSets(deployment *clusterv1.MachineDeployment, msList []*clusterv1.MachineSet, reconciliationTime *metav1.Time) ([]*clusterv1.MachineSet, []*clusterv1.MachineSet) {
-	var requiredMSs []*clusterv1.MachineSet
+// 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 {
 	allMSs := make([]*clusterv1.MachineSet, 0, len(msList))
 	newMS := FindNewMachineSet(deployment, msList, reconciliationTime)
 	for _, ms := range msList {
@@ -452,11 +449,8 @@ func FindOldMachineSets(deployment *clusterv1.MachineDeployment, msList []*clust
 			continue
 		}
 		allMSs = append(allMSs, ms)
-		if *(ms.Spec.Replicas) != 0 {
-			requiredMSs = append(requiredMSs, ms)
-		}
 	}
-	return requiredMSs, allMSs
+	return allMSs
 }
 
 // GetReplicaCountForMachineSets returns the sum of Replicas of the given machine sets.

internal/controllers/machinedeployment/mdutil/util_test.go

@@ -431,69 +431,59 @@ func TestFindOldMachineSets(t *testing.T) {
 		msList             []*clusterv1.MachineSet
 		reconciliationTime *metav1.Time
 		expected           []*clusterv1.MachineSet
-		expectedRequire    []*clusterv1.MachineSet
 	}{
 		{
-			Name:            "Get old MachineSets",
-			deployment:      deployment,
-			msList:          []*clusterv1.MachineSet{&newMS, &oldMS},
-			expected:        []*clusterv1.MachineSet{&oldMS},
-			expectedRequire: nil,
+			Name:       "Get old MachineSets",
+			deployment: deployment,
+			msList:     []*clusterv1.MachineSet{&newMS, &oldMS},
+			expected:   []*clusterv1.MachineSet{&oldMS},
 		},
 		{
-			Name:            "Get old MachineSets with no new MachineSet",
-			deployment:      deployment,
-			msList:          []*clusterv1.MachineSet{&oldMS},
-			expected:        []*clusterv1.MachineSet{&oldMS},
-			expectedRequire: nil,
+			Name:       "Get old MachineSets with no new MachineSet",
+			deployment: deployment,
+			msList:     []*clusterv1.MachineSet{&oldMS},
+			expected:   []*clusterv1.MachineSet{&oldMS},
 		},
 		{
-			Name:            "Get old MachineSets with two new MachineSets, only the MachineSet with higher replicas is seen as new MachineSet",
-			deployment:      deployment,
-			msList:          []*clusterv1.MachineSet{&oldMS, &newMS, &newMSHigherReplicas},
-			expected:        []*clusterv1.MachineSet{&oldMS, &newMS},
-			expectedRequire: []*clusterv1.MachineSet{&newMS},
+			Name:       "Get old MachineSets with two new MachineSets, only the MachineSet with higher replicas is seen as new MachineSet",
+			deployment: deployment,
+			msList:     []*clusterv1.MachineSet{&oldMS, &newMS, &newMSHigherReplicas},
+			expected:   []*clusterv1.MachineSet{&oldMS, &newMS},
 		},
 		{
-			Name:            "Get old MachineSets with two new MachineSets, when replicas are matching only the MachineSet with lower name is seen as new MachineSet",
-			deployment:      deployment,
-			msList:          []*clusterv1.MachineSet{&oldMS, &newMS, &newMSHigherName},
-			expected:        []*clusterv1.MachineSet{&oldMS, &newMSHigherName},
-			expectedRequire: []*clusterv1.MachineSet{&newMSHigherName},
+			Name:       "Get old MachineSets with two new MachineSets, when replicas are matching only the MachineSet with lower name is seen as new MachineSet",
+			deployment: deployment,
+			msList:     []*clusterv1.MachineSet{&oldMS, &newMS, &newMSHigherName},
+			expected:   []*clusterv1.MachineSet{&oldMS, &newMSHigherName},
 		},
 		{
-			Name:            "Get empty old MachineSets",
-			deployment:      deployment,
-			msList:          []*clusterv1.MachineSet{&newMS},
-			expected:        []*clusterv1.MachineSet{},
-			expectedRequire: nil,
+			Name:       "Get empty old MachineSets",
+			deployment: deployment,
+			msList:     []*clusterv1.MachineSet{&newMS},
+			expected:   []*clusterv1.MachineSet{},
 		},
 		{
 			Name:               "Get old MachineSets with new MachineSets, MachineSets created before rolloutAfter are considered new when reconciliationTime < rolloutAfter",
 			deployment:         deployment,
 			msList:             []*clusterv1.MachineSet{&msCreatedTwoBeforeRolloutAfter},
 			reconciliationTime: &oneBeforeRolloutAfter,
 			expected:           nil,
-			expectedRequire:    nil,
 		},
 		{
 			Name:               "Get old MachineSets with new MachineSets, MachineSets created after rolloutAfter are considered new when reconciliationTime > rolloutAfter",
 			deployment:         deployment,
 			msList:             []*clusterv1.MachineSet{&msCreatedTwoBeforeRolloutAfter, &msCreatedAfterRolloutAfter},
 			reconciliationTime: &twoAfterRolloutAfter,
 			expected:           []*clusterv1.MachineSet{&msCreatedTwoBeforeRolloutAfter},
-			expectedRequire:    nil,
 		},
 	}
 
 	for _, test := range tests {
 		t.Run(test.Name, func(t *testing.T) {
 			g := NewWithT(t)
 
-			requireMS, allMS := FindOldMachineSets(&test.deployment, test.msList, test.reconciliationTime)
+			allMS := FindOldMachineSets(&test.deployment, test.msList, test.reconciliationTime)
 			g.Expect(allMS).To(ConsistOf(test.expected))
-			// MSs are getting filtered correctly by ms.spec.replicas
-			g.Expect(requireMS).To(ConsistOf(test.expectedRequire))
 		})
 	}
 }