Delete MachineAutoscalers that would get MaxReplicas==0

MachineAutoscalers are not allowed to have MaxReplicas==0.

PR #2215 / bce2d47 partially fixed the scenario where a MachinePool's
autoscaling maxReplicas is less than the number of AZs by causing such
MAs not to be *created*. However, when reducing the MachinePool's
maxReplicas to below the number of AZs, we were still trying to update
*existing* MAs to have MaxReplicas==0. This commit adjusts the logic to
delete them instead.

HIVE-2415

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,25 +826,29 @@ 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
 					}
 
 					if rMA.Spec.MaxReplicas != maxReplicas {
 						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
 				}
 			}

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)
 							}