Merge pull request #3864 from Skarlso/fix_suspend_process

Remove suspend process flow from create ASG

Author: k8s-ci-robot

exp/controllers/awsmachinepool_controller.go

@@ -267,7 +267,7 @@ func (r *AWSMachinePoolReconciler) reconcileNormal(ctx context.Context, machineP
 
 	if asg == nil {
 		// Create new ASG
-		if _, err := r.createPool(machinePoolScope, clusterScope); err != nil {
+		if err := r.createPool(machinePoolScope, clusterScope); err != nil {
 			conditions.MarkFalse(machinePoolScope.AWSMachinePool, expinfrav1.ASGReadyCondition, expinfrav1.ASGProvisionFailedReason, clusterv1.ConditionSeverityError, err.Error())
 			return ctrl.Result{}, err
 		}
@@ -404,6 +404,7 @@ func (r *AWSMachinePoolReconciler) updatePool(machinePoolScope *scope.MachinePoo
 
 	suspendedProcessesSlice := machinePoolScope.AWSMachinePool.Spec.SuspendProcesses.ConvertSetValuesToStringSlice()
 	if !cmp.Equal(existingASG.CurrentlySuspendProcesses, suspendedProcessesSlice) {
+		clusterScope.Info("reconciling processes", "suspend-processes", suspendedProcessesSlice)
 		var (
 			toBeSuspended []string
 			toBeResumed   []string
@@ -431,7 +432,7 @@ func (r *AWSMachinePoolReconciler) updatePool(machinePoolScope *scope.MachinePoo
 			delete(currentlySuspended, k)
 		}
 
-		// Convert them back into lists so
+		// Convert them back into lists to pass them to resume/suspend.
 		for k := range desiredSuspended {
 			toBeSuspended = append(toBeSuspended, k)
 		}
@@ -441,11 +442,13 @@ func (r *AWSMachinePoolReconciler) updatePool(machinePoolScope *scope.MachinePoo
 		}
 
 		if len(toBeSuspended) > 0 {
+			clusterScope.Info("suspending processes", "processes", toBeSuspended)
 			if err := asgSvc.SuspendProcesses(existingASG.Name, toBeSuspended); err != nil {
 				return errors.Wrapf(err, "failed to suspend processes while trying update pool")
 			}
 		}
 		if len(toBeResumed) > 0 {
+			clusterScope.Info("resuming processes", "processes", toBeResumed)
 			if err := asgSvc.ResumeProcesses(existingASG.Name, toBeResumed); err != nil {
 				return errors.Wrapf(err, "failed to resume processes while trying update pool")
 			}
@@ -454,21 +457,17 @@ func (r *AWSMachinePoolReconciler) updatePool(machinePoolScope *scope.MachinePoo
 	return nil
 }
 
-func (r *AWSMachinePoolReconciler) createPool(machinePoolScope *scope.MachinePoolScope, clusterScope cloud.ClusterScoper) (*expinfrav1.AutoScalingGroup, error) {
+func (r *AWSMachinePoolReconciler) createPool(machinePoolScope *scope.MachinePoolScope, clusterScope cloud.ClusterScoper) error {
 	clusterScope.Info("Initializing ASG client")
 
 	asgsvc := r.getASGService(clusterScope)
 
 	machinePoolScope.Info("Creating Autoscaling Group")
-	asg, err := asgsvc.CreateASG(machinePoolScope)
-	if err != nil {
-		return nil, errors.Wrapf(err, "failed to create AWSMachinePool")
-	}
-	suspendedProcessesSlice := machinePoolScope.AWSMachinePool.Spec.SuspendProcesses.ConvertSetValuesToStringSlice()
-	if err := asgsvc.SuspendProcesses(asg.Name, suspendedProcessesSlice); err != nil {
-		return nil, errors.Wrapf(err, "failed to suspend processes while trying to create Pool")
+	if _, err := asgsvc.CreateASG(machinePoolScope); err != nil {
+		return errors.Wrapf(err, "failed to create AWSMachinePool")
 	}
-	return asg, nil
+
+	return nil
 }
 
 func (r *AWSMachinePoolReconciler) findASG(machinePoolScope *scope.MachinePoolScope, asgsvc services.ASGInterface) (*expinfrav1.AutoScalingGroup, error) {

exp/controllers/awsmachinepool_controller_test.go

@@ -266,7 +266,7 @@ func TestAWSMachinePoolReconciler(t *testing.T) {
 					},
 				}
 			}
-			t.Run("it should suspend these processes", func(t *testing.T) {
+			t.Run("it should not call suspend as we don't have an ASG yet", func(t *testing.T) {
 				g := NewWithT(t)
 				setup(t, g)
 				defer teardown(t, g)
@@ -277,7 +277,7 @@ func TestAWSMachinePoolReconciler(t *testing.T) {
 				asgSvc.EXPECT().CreateASG(gomock.Any()).Return(&expinfrav1.AutoScalingGroup{
 					Name: "name",
 				}, nil)
-				asgSvc.EXPECT().SuspendProcesses("name", []string{"Launch", "Terminate"}).Return(nil).AnyTimes()
+				asgSvc.EXPECT().SuspendProcesses("name", []string{"Launch", "Terminate"}).Return(nil).AnyTimes().Times(0)
 
 				_, err := reconciler.reconcileNormal(context.Background(), ms, cs, cs)
 				g.Expect(err).To(Succeed())
@@ -290,56 +290,29 @@ func TestAWSMachinePoolReconciler(t *testing.T) {
 					All: true,
 				}
 			}
-			t.Run("it should result in all processes being included except the value all", func(t *testing.T) {
+			t.Run("processes should be suspended during an update call", func(t *testing.T) {
 				g := NewWithT(t)
 				setup(t, g)
 				defer teardown(t, g)
 				setSuspendedProcesses(t, g)
-
+				ms.AWSMachinePool.Spec.SuspendProcesses.All = true
 				ec2Svc.EXPECT().ReconcileLaunchTemplate(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil)
-				asgSvc.EXPECT().GetASGByName(gomock.Any()).Return(nil, nil)
-				asgSvc.EXPECT().CreateASG(gomock.Any()).Return(&expinfrav1.AutoScalingGroup{
+				ec2Svc.EXPECT().ReconcileTags(gomock.Any(), gomock.Any()).Return(nil)
+				asgSvc.EXPECT().GetASGByName(gomock.Any()).Return(&expinfrav1.AutoScalingGroup{
 					Name: "name",
 				}, nil)
-				asgSvc.EXPECT().SuspendProcesses("name", []string{
+				asgSvc.EXPECT().UpdateASG(gomock.Any()).Return(nil).AnyTimes()
+				asgSvc.EXPECT().SuspendProcesses("name", gomock.InAnyOrder([]string{
+					"ScheduledActions",
 					"Launch",
 					"Terminate",
 					"AddToLoadBalancer",
 					"AlarmNotification",
 					"AZRebalance",
-					"HealthCheck",
 					"InstanceRefresh",
-					"ReplaceUnhealthy",
-					"ScheduledActions",
-				}).Return(nil).AnyTimes()
-
-				_, err := reconciler.reconcileNormal(context.Background(), ms, cs, cs)
-				g.Expect(err).To(Succeed())
-			})
-			t.Run("and one or more processes are disabled, it should not list those", func(t *testing.T) {
-				g := NewWithT(t)
-				setup(t, g)
-				defer teardown(t, g)
-				setSuspendedProcesses(t, g)
-				ms.AWSMachinePool.Spec.SuspendProcesses.Processes = &expinfrav1.Processes{
-					Launch:      pointer.Bool(false),
-					AZRebalance: pointer.Bool(true), // this should still be included but not twice...
-				}
-				ec2Svc.EXPECT().ReconcileLaunchTemplate(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil)
-				asgSvc.EXPECT().GetASGByName(gomock.Any()).Return(nil, nil)
-				asgSvc.EXPECT().CreateASG(gomock.Any()).Return(&expinfrav1.AutoScalingGroup{
-					Name: "name",
-				}, nil)
-				asgSvc.EXPECT().SuspendProcesses("name", []string{
-					"Terminate",
-					"AddToLoadBalancer",
-					"AlarmNotification",
-					"AZRebalance",
 					"HealthCheck",
-					"InstanceRefresh",
 					"ReplaceUnhealthy",
-					"ScheduledActions",
-				}).Return(nil).AnyTimes()
+				})).Return(nil).AnyTimes().Times(1)
 
 				_, err := reconciler.reconcileNormal(context.Background(), ms, cs, cs)
 				g.Expect(err).To(Succeed())
@@ -369,8 +342,8 @@ func TestAWSMachinePoolReconciler(t *testing.T) {
 					CurrentlySuspendProcesses: []string{"Launch", "process3"},
 				}, nil)
 				asgSvc.EXPECT().UpdateASG(gomock.Any()).Return(nil).AnyTimes()
-				asgSvc.EXPECT().SuspendProcesses("name", []string{"Terminate"}).Return(nil).AnyTimes()
-				asgSvc.EXPECT().ResumeProcesses("name", []string{"process3"}).Return(nil).AnyTimes()
+				asgSvc.EXPECT().SuspendProcesses("name", []string{"Terminate"}).Return(nil).AnyTimes().Times(1)
+				asgSvc.EXPECT().ResumeProcesses("name", []string{"process3"}).Return(nil).AnyTimes().Times(1)
 
 				_, err := reconciler.reconcileNormal(context.Background(), ms, cs, cs)
 				g.Expect(err).To(Succeed())