include machines in deleting status when calculate machineset replicas

controllers/machineset_controller.go

@@ -230,6 +230,11 @@ func (r *MachineSetReconciler) reconcile(ctx context.Context, cluster *clusterv1
 
 	var errs []error
 	for _, machine := range filteredMachines {
+		// filteredMachines contains machines in deleting status to calculate correct status
+		// skip remidiation for those in deleting status
+		if !machine.DeletionTimestamp.IsZero() {
+			continue
+		}
 		if conditions.IsFalse(machine, clusterv1.MachineOwnerRemediatedCondition) {
 			logger.Info("Deleting unhealthy machine", "machine", machine.GetName())
 			patch := client.MergeFrom(machine.DeepCopy())
@@ -440,10 +445,10 @@ func (r *MachineSetReconciler) getNewMachine(machineSet *clusterv1.MachineSet) *
 // shouldExcludeMachine returns true if the machine should be filtered out, false otherwise.
 func shouldExcludeMachine(machineSet *clusterv1.MachineSet, machine *clusterv1.Machine, logger logr.Logger) bool {
 	if metav1.GetControllerOf(machine) != nil && !metav1.IsControlledBy(machine, machineSet) {
-		logger.V(4).Info("Machine is not controlled by machineset", "machine", machine.Name)
 		return true
 	}
-	return !machine.ObjectMeta.DeletionTimestamp.IsZero()
+
+	return false
 }
 
 // adoptOrphan sets the MachineSet as a controller OwnerReference to the Machine.

controllers/machineset_controller_test.go

@@ -602,7 +602,7 @@ func TestShouldExcludeMachine(t *testing.T) {
 					},
 				},
 			},
-			expected: true,
+			expected: false,
 		},
 	}
 

controllers/mdutil/util.go

@@ -472,6 +472,21 @@ func GetActualReplicaCountForMachineSets(machineSets []*clusterv1.MachineSet) in
 	return totalActualReplicas
 }
 
+// GetSumOfMaxOfSpecAndStatusReplicaCountForMachineSets returns sum of max(ms.Spec.Replicas, ms.Status.Replicas) across all the machine sets.
+// This is used to guarantee that the total number of machines will not exceed md.Spec.Replicas + maxSurge
+// Use max(spec.Replicas,status.Replicas) to cover the cases that:
+// 1. Scale up, where spec.Replicas increased but no machine created yet, so spec.Replicas > status.Replicas
+// 2. Scale down, where spec.Replicas decreased but machine not deleted yet, so spec.Replicas < status.Replicas
+func GetSumOfMaxOfSpecAndStatusReplicaCountForMachineSets(machineSets []*clusterv1.MachineSet) int32 {
+	totalReplicas := int32(0)
+	for _, ms := range machineSets {
+		if ms != nil {
+			totalReplicas += integer.Int32Max(*(ms.Spec.Replicas), ms.Status.Replicas)
+		}
+	}
+	return totalReplicas
+}
+
 // GetReadyReplicaCountForMachineSets returns the number of ready machines corresponding to the given machine sets.
 func GetReadyReplicaCountForMachineSets(machineSets []*clusterv1.MachineSet) int32 {
 	totalReadyReplicas := int32(0)
@@ -521,7 +536,7 @@ func NewMSNewReplicas(deployment *clusterv1.MachineDeployment, allMSs []*cluster
 			return 0, err
 		}
 		// Find the total number of machines
-		currentMachineCount := GetReplicaCountForMachineSets(allMSs)
+		currentMachineCount := GetSumOfMaxOfSpecAndStatusReplicaCountForMachineSets(allMSs)
 		maxTotalMachines := *(deployment.Spec.Replicas) + int32(maxSurge)
 		if currentMachineCount >= maxTotalMachines {
 			// Cannot scale up.
@@ -533,24 +548,7 @@ func NewMSNewReplicas(deployment *clusterv1.MachineDeployment, allMSs []*cluster
 		scaleUpCount = integer.Int32Min(scaleUpCount, *(deployment.Spec.Replicas)-*(newMS.Spec.Replicas))
 		return *(newMS.Spec.Replicas) + scaleUpCount, nil
 	default:
-		// Check if we can scale up.
-		maxSurge, err := intstrutil.GetValueFromIntOrPercent(deployment.Spec.Strategy.RollingUpdate.MaxSurge, int(*(deployment.Spec.Replicas)), true)
-		if err != nil {
-			return 0, err
-		}
-		// Find the total number of machines
-		currentMachineCount := GetReplicaCountForMachineSets(allMSs)
-		maxTotalMachines := *(deployment.Spec.Replicas) + int32(maxSurge)
-		if currentMachineCount >= maxTotalMachines {
-			// Cannot scale up.
-			return *(newMS.Spec.Replicas), nil
-		}
-		// Scale up.
-		scaleUpCount := maxTotalMachines - currentMachineCount
-		// Do not exceed the number of desired replicas.
-		scaleUpCount = integer.Int32Min(scaleUpCount, *(deployment.Spec.Replicas)-*(newMS.Spec.Replicas))
-		return *(newMS.Spec.Replicas) + scaleUpCount, nil
-		// -- return 0, errors.Errorf("deployment type %v isn't supported", deployment.Spec.Strategy.Type)
+		return 0, fmt.Errorf("deployment type %v isn't supported", deployment.Spec.Strategy.Type)
 	}
 }
 

controllers/mdutil/util_test.go

@@ -411,26 +411,29 @@ func TestGetReplicaCountForMachineSets(t *testing.T) {
 	*(ms1.Spec.Replicas) = 1
 	ms1.Status.Replicas = 2
 	ms2 := generateMS(generateDeployment("bar"))
-	*(ms2.Spec.Replicas) = 2
+	*(ms2.Spec.Replicas) = 5
 	ms2.Status.Replicas = 3
 
 	tests := []struct {
 		Name           string
 		Sets           []*clusterv1.MachineSet
 		ExpectedCount  int32
 		ExpectedActual int32
+		ExpectedTotal  int32
 	}{
 		{
 			Name:           "1:2 Replicas",
 			Sets:           []*clusterv1.MachineSet{&ms1},
 			ExpectedCount:  1,
 			ExpectedActual: 2,
+			ExpectedTotal:  2,
 		},
 		{
-			Name:           "3:5 Replicas",
+			Name:           "6:5 Replicas",
 			Sets:           []*clusterv1.MachineSet{&ms1, &ms2},
-			ExpectedCount:  3,
+			ExpectedCount:  6,
 			ExpectedActual: 5,
+			ExpectedTotal:  7,
 		},
 	}
 
@@ -440,6 +443,7 @@ func TestGetReplicaCountForMachineSets(t *testing.T) {
 
 			g.Expect(GetReplicaCountForMachineSets(test.Sets)).To(Equal(test.ExpectedCount))
 			g.Expect(GetActualReplicaCountForMachineSets(test.Sets)).To(Equal(test.ExpectedActual))
+			g.Expect(GetSumOfMaxOfSpecAndStatusReplicaCountForMachineSets(test.Sets)).To(Equal(test.ExpectedTotal))
 		})
 	}
 }