diff --git a/pkg/controller/machinepool/machinepool_controller.go b/pkg/controller/machinepool/machinepool_controller.go index d590a2da9dc..ee6bb28facb 100644 --- a/pkg/controller/machinepool/machinepool_controller.go +++ b/pkg/controller/machinepool/machinepool_controller.go @@ -816,7 +816,7 @@ func (r *ReconcileMachinePool) syncMachineAutoscalers( for i, ms := range machineSets { minReplicas, maxReplicas := getMinMaxReplicasForMachineSet(pool, machineSets, i) found := false - for _, rMA := range remoteMachineAutoscalers.Items { + for j, rMA := range remoteMachineAutoscalers.Items { if ms.Name == rMA.Name { found = true objectModified := false @@ -826,7 +826,7 @@ func (r *ReconcileMachinePool) syncMachineAutoscalers( maLog.WithField("desired", minReplicas). WithField("observed", rMA.Spec.MinReplicas). Info("min replicas out of sync") - rMA.Spec.MinReplicas = minReplicas + remoteMachineAutoscalers.Items[j].Spec.MinReplicas = minReplicas objectModified = true } @@ -834,17 +834,21 @@ func (r *ReconcileMachinePool) syncMachineAutoscalers( maLog.WithField("desired", maxReplicas). WithField("observed", rMA.Spec.MaxReplicas). Info("max replicas out of sync") - rMA.Spec.MaxReplicas = maxReplicas + remoteMachineAutoscalers.Items[j].Spec.MaxReplicas = maxReplicas objectModified = true } - if objectModified { - machineAutoscalersToUpdate = append(machineAutoscalersToUpdate, &rMA) + // A MachineAutoscaler can't have MaxReplicas==0. In that case, update will + // bounce. We must delete the MachineAutoscaler instead, which we trigger + // below based on having set MaxReplicas to zero above. + if objectModified && maxReplicas > 0 { + machineAutoscalersToUpdate = append(machineAutoscalersToUpdate, &remoteMachineAutoscalers.Items[j]) } break } } + // Don't attempt to create a MachineAutoscaler with MaxReplicas==0. if !found && maxReplicas > 0 { ma := &autoscalingv1beta1.MachineAutoscaler{ ObjectMeta: metav1.ObjectMeta{ @@ -879,7 +883,10 @@ func (r *ReconcileMachinePool) syncMachineAutoscalers( if pool.DeletionTimestamp == nil && pool.Spec.Autoscaling != nil { for _, ms := range machineSets { if rMA.Name == ms.Name { - delete = false + // A MachineAutoscaler can't have MaxReplicas==0. Delete it if that's the case. + if rMA.Spec.MaxReplicas > 0 { + delete = false + } break } } diff --git a/pkg/controller/machinepool/machinepool_controller_test.go b/pkg/controller/machinepool/machinepool_controller_test.go index 4f10222691c..ad316f11314 100644 --- a/pkg/controller/machinepool/machinepool_controller_test.go +++ b/pkg/controller/machinepool/machinepool_controller_test.go @@ -1163,6 +1163,38 @@ func TestRemoteMachineSetReconcile(t *testing.T) { *testClusterAutoscaler("1"), }, }, + { + name: "Delete machine autoscalers whose maxReplicas would be zero", + clusterDeployment: testClusterDeployment(), + machinePool: testAutoscalingMachinePool(1, 2), + remoteExisting: []runtime.Object{ + testMachine("master1", "master"), + testMachineSetWithAZ("foo-12345-worker-us-east-1a", "worker", true, 1, 0, "us-east-1a"), + testMachineSetWithAZ("foo-12345-worker-us-east-1b", "worker", true, 1, 0, "us-east-1b"), + testMachineSetWithAZ("foo-12345-worker-us-east-1c", "worker", true, 1, 0, "us-east-1c"), + testClusterAutoscaler("1"), + testMachineAutoscaler("foo-12345-worker-us-east-1a", "1", 1, 1), + testMachineAutoscaler("foo-12345-worker-us-east-1b", "1", 2, 2), + testMachineAutoscaler("foo-12345-worker-us-east-1c", "1", 1, 1), + }, + generatedMachineSets: []*machineapi.MachineSet{ + testMachineSetWithAZ("foo-12345-worker-us-east-1a", "worker", false, 1, 0, "us-east-1a"), + testMachineSetWithAZ("foo-12345-worker-us-east-1b", "worker", false, 1, 0, "us-east-1b"), + testMachineSetWithAZ("foo-12345-worker-us-east-1c", "worker", false, 1, 0, "us-east-1c"), + }, + expectedRemoteMachineSets: []*machineapi.MachineSet{ + testMachineSetWithAZ("foo-12345-worker-us-east-1a", "worker", true, 1, 0, "us-east-1a"), + testMachineSetWithAZ("foo-12345-worker-us-east-1b", "worker", true, 1, 0, "us-east-1b"), + testMachineSetWithAZ("foo-12345-worker-us-east-1c", "worker", true, 0, 1, "us-east-1c"), + }, + expectedRemoteMachineAutoscalers: []autoscalingv1beta1.MachineAutoscaler{ + *testMachineAutoscaler("foo-12345-worker-us-east-1a", "1", 1, 1), + *testMachineAutoscaler("foo-12345-worker-us-east-1b", "2", 0, 1), + }, + expectedRemoteClusterAutoscalers: []autoscalingv1.ClusterAutoscaler{ + *testClusterAutoscaler("1"), + }, + }, { name: "Create machine autoscalers where maxReplicas < #AZs and minReplicas==0", clusterDeployment: testClusterDeployment(), @@ -1288,8 +1320,8 @@ func TestRemoteMachineSetReconcile(t *testing.T) { for _, rMS := range rMSL.Items { if eMS.Name == rMS.Name { found = true - assert.Equal(t, *eMS.Spec.Replicas, *rMS.Spec.Replicas, "Replicas") - assert.Equal(t, eMS.Generation, rMS.Generation, "Generation") + assert.Equal(t, *eMS.Spec.Replicas, *rMS.Spec.Replicas, "Replicas for %s", rMS.Name) + assert.Equal(t, eMS.Generation, rMS.Generation, "Generation for %s", rMS.Name) if !reflect.DeepEqual(eMS.ObjectMeta.Labels, rMS.ObjectMeta.Labels) { t.Errorf("machineset %v has unexpected labels:\nexpected: %v\nactual: %v", eMS.Name, eMS.Labels, rMS.Labels) }