diff --git a/controllers/machineset_controller.go b/controllers/machineset_controller.go index 7655bc777844..d50d8f0a0141 100644 --- a/controllers/machineset_controller.go +++ b/controllers/machineset_controller.go @@ -208,7 +208,7 @@ func (r *MachineSetReconciler) reconcile(ctx context.Context, cluster *clusterv1 filteredMachines := make([]*clusterv1.Machine, 0, len(allMachines.Items)) for idx := range allMachines.Items { machine := &allMachines.Items[idx] - if shouldExcludeMachine(machineSet, machine, log) { + if shouldExcludeMachine(machineSet, machine) { continue } @@ -228,6 +228,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) { log.Info("Deleting unhealthy machine", "machine", machine.GetName()) patch := client.MergeFrom(machine.DeepCopy()) @@ -436,12 +441,12 @@ 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 { +func shouldExcludeMachine(machineSet *clusterv1.MachineSet, machine *clusterv1.Machine) 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. diff --git a/controllers/machineset_controller_test.go b/controllers/machineset_controller_test.go index d7977a797caf..bb39c5789c00 100644 --- a/controllers/machineset_controller_test.go +++ b/controllers/machineset_controller_test.go @@ -577,15 +577,14 @@ func TestShouldExcludeMachine(t *testing.T) { }, }, }, - expected: true, + expected: false, }, } - logger := klogr.New() for _, tc := range testCases { g := NewWithT(t) - got := shouldExcludeMachine(&tc.machineSet, &tc.machine, logger) + got := shouldExcludeMachine(&tc.machineSet, &tc.machine) g.Expect(got).To(Equal(tc.expected)) } diff --git a/controllers/mdutil/util.go b/controllers/mdutil/util.go index c99fadff8781..289354051726 100644 --- a/controllers/mdutil/util.go +++ b/controllers/mdutil/util.go @@ -472,6 +472,22 @@ func GetActualReplicaCountForMachineSets(machineSets []*clusterv1.MachineSet) in return totalActualReplicas } +// TotalMachineSetsReplicaSum 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 TotalMachineSetsReplicaSum(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 +537,7 @@ func NewMSNewReplicas(deployment *clusterv1.MachineDeployment, allMSs []*cluster return 0, err } // Find the total number of machines - currentMachineCount := GetReplicaCountForMachineSets(allMSs) + currentMachineCount := TotalMachineSetsReplicaSum(allMSs) maxTotalMachines := *(deployment.Spec.Replicas) + int32(maxSurge) if currentMachineCount >= maxTotalMachines { // Cannot scale up. @@ -533,24 +549,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) } } diff --git a/controllers/mdutil/util_test.go b/controllers/mdutil/util_test.go index 5f44982a17c9..ebb8aed3603a 100644 --- a/controllers/mdutil/util_test.go +++ b/controllers/mdutil/util_test.go @@ -411,7 +411,7 @@ 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 { @@ -419,18 +419,21 @@ func TestGetReplicaCountForMachineSets(t *testing.T) { 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(TotalMachineSetsReplicaSum(test.Sets)).To(Equal(test.ExpectedTotal)) }) } }