From 276cd4c95f392e80dcf72dc4c7906c829fb4c08b Mon Sep 17 00:00:00 2001 From: jzhoucliqr Date: Thu, 20 Aug 2020 10:43:30 -0700 Subject: [PATCH] include machines in deleting status when calculate machineset replicas --- controllers/machineset_controller.go | 9 ++++-- controllers/machineset_controller_test.go | 2 +- controllers/mdutil/util.go | 36 +++++++++++------------ controllers/mdutil/util_test.go | 10 +++++-- 4 files changed, 32 insertions(+), 25 deletions(-) diff --git a/controllers/machineset_controller.go b/controllers/machineset_controller.go index 5c605ba9eb67..5eecf490cbfc 100644 --- a/controllers/machineset_controller.go +++ b/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. diff --git a/controllers/machineset_controller_test.go b/controllers/machineset_controller_test.go index a68c5b6d1a32..947054ba7e06 100644 --- a/controllers/machineset_controller_test.go +++ b/controllers/machineset_controller_test.go @@ -602,7 +602,7 @@ func TestShouldExcludeMachine(t *testing.T) { }, }, }, - expected: true, + expected: false, }, } diff --git a/controllers/mdutil/util.go b/controllers/mdutil/util.go index 7fb5b94acac1..25ead0b33b40 100644 --- a/controllers/mdutil/util.go +++ b/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) } } diff --git a/controllers/mdutil/util_test.go b/controllers/mdutil/util_test.go index ffc765117a25..ff0f57e9233c 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(GetSumOfMaxOfSpecAndStatusReplicaCountForMachineSets(test.Sets)).To(Equal(test.ExpectedTotal)) }) } }