diff --git a/controllers/machineset_controller.go b/controllers/machineset_controller.go index 5c605ba9eb67..207a4986c897 100644 --- a/controllers/machineset_controller.go +++ b/controllers/machineset_controller.go @@ -230,7 +230,7 @@ func (r *MachineSetReconciler) reconcile(ctx context.Context, cluster *clusterv1 var errs []error for _, machine := range filteredMachines { - if conditions.IsFalse(machine, clusterv1.MachineOwnerRemediatedCondition) { + if machine.DeletionTimestamp.IsZero() && conditions.IsFalse(machine, clusterv1.MachineOwnerRemediatedCondition) { logger.Info("Deleting unhealthy machine", "machine", machine.GetName()) patch := client.MergeFrom(machine.DeepCopy()) if err := r.Client.Delete(ctx, machine); err != nil { @@ -443,7 +443,8 @@ func shouldExcludeMachine(machineSet *clusterv1.MachineSet, machine *clusterv1.M 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..ba625937ea19 100644 --- a/controllers/mdutil/util.go +++ b/controllers/mdutil/util.go @@ -472,6 +472,21 @@ func GetActualReplicaCountForMachineSets(machineSets []*clusterv1.MachineSet) in return totalActualReplicas } +// GetTotalReplicaCountForMachineSets 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 GetTotalReplicaCountForMachineSets(machineSets []*clusterv1.MachineSet) int32 { + totalReplicas := int32(0) + for _, ms := range machineSets { + if ms != nil { + totalReplicas += max(*(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 := GetTotalReplicaCountForMachineSets(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) } } @@ -698,3 +696,10 @@ func ComputeHash(template *clusterv1.MachineTemplateSpec) uint32 { DeepHashObject(machineTemplateSpecHasher, *template) return machineTemplateSpecHasher.Sum32() } + +func max(a, b int32) int32 { + if a > b { + return a + } + return b +} diff --git a/controllers/mdutil/util_test.go b/controllers/mdutil/util_test.go index ffc765117a25..a976da3ac693 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(GetTotalReplicaCountForMachineSets(test.Sets)).To(Equal(test.ExpectedTotal)) }) } }