Skip to content

Commit

Permalink
There is no need to check instance refresh if ASG is not found
Browse files Browse the repository at this point in the history
Co-authored-by: Andreas Sommer <andreas@giantswarm.io>
  • Loading branch information
fiunchinho and AndiDog committed Nov 28, 2023
1 parent b4506f8 commit d44d5f5
Show file tree
Hide file tree
Showing 4 changed files with 24 additions and 23 deletions.
24 changes: 17 additions & 7 deletions exp/controllers/awsmachinepool_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -227,13 +227,30 @@ func (r *AWSMachinePoolReconciler) reconcileNormal(ctx context.Context, machineP
ec2Svc := r.getEC2Service(ec2Scope)
asgsvc := r.getASGService(clusterScope)

// Find existing ASG
asg, err := r.findASG(machinePoolScope, asgsvc)
if err != nil {
conditions.MarkUnknown(machinePoolScope.AWSMachinePool, expinfrav1.ASGReadyCondition, expinfrav1.ASGNotFoundReason, err.Error())
return err
}

canUpdateLaunchTemplate := func() (bool, error) {
// If there is a change: before changing the template, check if there exist an ongoing instance refresh,
// because only 1 instance refresh can be "InProgress". If template is updated when refresh cannot be started,
// that change will not trigger a refresh. Do not start an instance refresh if only userdata changed.
if asg == nil {
// If the ASG hasn't been created yet, there is no need to check if we can start the instance refresh.
// But we want to update the LaunchTemplate because an error in the LaunchTemplate may be blocking the ASG creation.
return true, nil
}
return asgsvc.CanStartASGInstanceRefresh(machinePoolScope)
}
runPostLaunchTemplateUpdateOperation := func() error {
// skip instance refresh if ASG is not created yet
if asg == nil {
machinePoolScope.Debug("ASG does not exist yet, skipping instance refresh")
return nil
}
// skip instance refresh if explicitly disabled
if machinePoolScope.AWSMachinePool.Spec.RefreshPreferences != nil && machinePoolScope.AWSMachinePool.Spec.RefreshPreferences.Disable {
machinePoolScope.Debug("instance refresh disabled, skipping instance refresh")
Expand All @@ -260,13 +277,6 @@ func (r *AWSMachinePoolReconciler) reconcileNormal(ctx context.Context, machineP
// set the LaunchTemplateReady condition
conditions.MarkTrue(machinePoolScope.AWSMachinePool, expinfrav1.LaunchTemplateReadyCondition)

// Find existing ASG
asg, err := r.findASG(machinePoolScope, asgsvc)
if err != nil {
conditions.MarkUnknown(machinePoolScope.AWSMachinePool, expinfrav1.ASGReadyCondition, expinfrav1.ASGNotFoundReason, err.Error())
return err
}

if asg == nil {
// Create new ASG
if err := r.createPool(machinePoolScope, clusterScope); err != nil {
Expand Down
9 changes: 7 additions & 2 deletions exp/controllers/awsmachinepool_controller_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -208,8 +208,6 @@ func TestAWSMachinePoolReconciler(t *testing.T) {
defer teardown(t, g)
getASG(t, g)

ec2Svc.EXPECT().ReconcileLaunchTemplate(gomock.Any(), gomock.Any(), gomock.Any())

_ = reconciler.reconcileNormal(context.Background(), ms, cs, cs)

g.Expect(ms.AWSMachinePool.Finalizers).To(ContainElement(expinfrav1.MachinePoolFinalizer))
Expand Down Expand Up @@ -253,11 +251,18 @@ func TestAWSMachinePoolReconciler(t *testing.T) {
t.Helper()
ms.AWSMachinePool.Spec.ProviderID = id
}
getASG := func(t *testing.T, g *WithT) {
t.Helper()

ec2Svc.EXPECT().GetLaunchTemplate(gomock.Any()).Return(nil, "", nil).AnyTimes()
asgSvc.EXPECT().GetASGByName(gomock.Any()).Return(nil, nil).AnyTimes()
}
t.Run("should look up by provider ID when one exists", func(t *testing.T) {
g := NewWithT(t)
setup(t, g)
defer teardown(t, g)
setProviderID(t, g)
getASG(t, g)

expectedErr := errors.New("no connection available ")
ec2Svc.EXPECT().ReconcileLaunchTemplate(gomock.Any(), gomock.Any(), gomock.Any()).Return(expectedErr)
Expand Down
3 changes: 0 additions & 3 deletions pkg/cloud/services/autoscaling/autoscalinggroup.go
Original file line number Diff line number Diff line change
Expand Up @@ -323,9 +323,6 @@ func (s *Service) CanStartASGInstanceRefresh(scope *scope.MachinePoolScope) (boo
describeInput := &autoscaling.DescribeInstanceRefreshesInput{AutoScalingGroupName: aws.String(scope.Name())}
refreshes, err := s.ASGClient.DescribeInstanceRefreshesWithContext(context.TODO(), describeInput)
if err != nil {
if awserrors.IsNotFound(err) {
return false, nil
}
return false, err
}
hasUnfinishedRefresh := false
Expand Down
11 changes: 0 additions & 11 deletions pkg/cloud/services/autoscaling/autoscalinggroup_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1094,17 +1094,6 @@ func TestServiceCanStartASGInstanceRefresh(t *testing.T) {
Return(nil, awserrors.NewConflict("some error"))
},
},
{
name: "should NOT return error if describe instance failed due to 'not found'",
wantErr: false,
canStart: false,
expect: func(m *mock_autoscalingiface.MockAutoScalingAPIMockRecorder) {
m.DescribeInstanceRefreshesWithContext(context.TODO(), gomock.Eq(&autoscaling.DescribeInstanceRefreshesInput{
AutoScalingGroupName: aws.String("machinePoolName"),
})).
Return(nil, awserrors.NewNotFound("not found"))
},
},
{
name: "should return true if no instance available for refresh",
wantErr: false,
Expand Down

0 comments on commit d44d5f5

Please sign in to comment.