Skip to content

Commit 7919371

Browse files
k8s-ci-robotk8s-ci-robot
authored
Merge pull request #3936 from wyike/asgsubnet
Update ASG if subnet changes
2 parents 7564dcd + cc7e95e commit 7919371

File tree

5 files changed

+91
-4
lines changed

5 files changed

+91
-4
lines changed

‎exp/controllers/awsmachinepool_controller.go

Lines changed: 13 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@ import (
2121
"fmt"
2222

2323
"github.com/google/go-cmp/cmp"
24+
"github.com/google/go-cmp/cmp/cmpopts"
2425
"github.com/pkg/errors"
2526
corev1 "k8s.io/api/core/v1"
2627
apierrors "k8s.io/apimachinery/pkg/api/errors"
@@ -393,7 +394,18 @@ func (r *AWSMachinePoolReconciler) reconcileDelete(machinePoolScope *scope.Machi
393394

394395
func (r *AWSMachinePoolReconciler) updatePool(machinePoolScope *scope.MachinePoolScope, clusterScope cloud.ClusterScoper, existingASG *expinfrav1.AutoScalingGroup) error {
395396
asgSvc := r.getASGService(clusterScope)
396-
if asgNeedsUpdates(machinePoolScope, existingASG) {
397+
398+
subnetIDs, err := asgSvc.SubnetIDs(machinePoolScope)
399+
if err != nil {
400+
return errors.Wrapf(err, "fail to get subnets for ASG")
401+
}
402+
machinePoolScope.Debug("determining if subnets change in machinePoolScope",
403+
"subnets of machinePoolScope", subnetIDs,
404+
"subnets of existing asg", existingASG.Subnets)
405+
less := func(a, b string) bool { return a < b }
406+
subnetChanges := cmp.Diff(subnetIDs, existingASG.Subnets, cmpopts.SortSlices(less)) != ""
407+
408+
if asgNeedsUpdates(machinePoolScope, existingASG) || subnetChanges {
397409
machinePoolScope.Info("updating AutoScalingGroup")
398410

399411
if err := asgSvc.UpdateASG(machinePoolScope); err != nil {
@@ -509,8 +521,6 @@ func asgNeedsUpdates(machinePoolScope *scope.MachinePoolScope, existingASG *expi
509521
return true
510522
}
511523

512-
// todo subnet diff
513-
514524
return false
515525
}
516526

‎exp/controllers/awsmachinepool_controller_test.go

Lines changed: 57 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -301,6 +301,7 @@ func TestAWSMachinePoolReconciler(t *testing.T) {
301301
asgSvc.EXPECT().GetASGByName(gomock.Any()).Return(&expinfrav1.AutoScalingGroup{
302302
Name: "name",
303303
}, nil)
304+
asgSvc.EXPECT().SubnetIDs(gomock.Any()).Return([]string{}, nil).Times(1)
304305
asgSvc.EXPECT().UpdateASG(gomock.Any()).Return(nil).AnyTimes()
305306
asgSvc.EXPECT().SuspendProcesses("name", gomock.InAnyOrder([]string{
306307
"ScheduledActions",
@@ -341,6 +342,7 @@ func TestAWSMachinePoolReconciler(t *testing.T) {
341342
Name: "name",
342343
CurrentlySuspendProcesses: []string{"Launch", "process3"},
343344
}, nil)
345+
asgSvc.EXPECT().SubnetIDs(gomock.Any()).Return([]string{}, nil).Times(1)
344346
asgSvc.EXPECT().UpdateASG(gomock.Any()).Return(nil).AnyTimes()
345347
asgSvc.EXPECT().SuspendProcesses("name", []string{"Terminate"}).Return(nil).AnyTimes().Times(1)
346348
asgSvc.EXPECT().ResumeProcesses("name", []string{"process3"}).Return(nil).AnyTimes().Times(1)
@@ -360,6 +362,7 @@ func TestAWSMachinePoolReconciler(t *testing.T) {
360362
DesiredCapacity: pointer.Int32(1),
361363
}
362364
asgSvc.EXPECT().GetASGByName(gomock.Any()).Return(&asg, nil).AnyTimes()
365+
asgSvc.EXPECT().SubnetIDs(gomock.Any()).Return([]string{}, nil).Times(1)
363366
asgSvc.EXPECT().UpdateASG(gomock.Any()).Return(nil).AnyTimes()
364367
ec2Svc.EXPECT().GetLaunchTemplate(gomock.Any()).Return(nil, "", nil).AnyTimes()
365368
ec2Svc.EXPECT().DiscoverLaunchTemplateAMI(gomock.Any()).Return(nil, nil).AnyTimes()
@@ -377,6 +380,60 @@ func TestAWSMachinePoolReconciler(t *testing.T) {
377380
_, _ = reconciler.reconcileNormal(context.Background(), ms, cs, cs)
378381
g.Expect(*ms.MachinePool.Spec.Replicas).To(Equal(int32(1)))
379382
})
383+
t.Run("No need to update Asg because asgNeedsUpdates is false and no subnets change", func(t *testing.T) {
384+
g := NewWithT(t)
385+
setup(t, g)
386+
defer teardown(t, g)
387+
388+
asg := expinfrav1.AutoScalingGroup{
389+
MinSize: int32(0),
390+
MaxSize: int32(1),
391+
Subnets: []string{"subnet1", "subnet2"}}
392+
ec2Svc.EXPECT().ReconcileLaunchTemplate(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil)
393+
ec2Svc.EXPECT().ReconcileTags(gomock.Any(), gomock.Any()).Return(nil)
394+
asgSvc.EXPECT().GetASGByName(gomock.Any()).Return(&asg, nil).AnyTimes()
395+
asgSvc.EXPECT().SubnetIDs(gomock.Any()).Return([]string{"subnet2", "subnet1"}, nil).Times(1)
396+
asgSvc.EXPECT().UpdateASG(gomock.Any()).Return(nil).Times(0)
397+
398+
_, err := reconciler.reconcileNormal(context.Background(), ms, cs, cs)
399+
g.Expect(err).To(Succeed())
400+
})
401+
t.Run("update Asg due to subnet changes", func(t *testing.T) {
402+
g := NewWithT(t)
403+
setup(t, g)
404+
defer teardown(t, g)
405+
406+
asg := expinfrav1.AutoScalingGroup{
407+
MinSize: int32(0),
408+
MaxSize: int32(1),
409+
Subnets: []string{"subnet1", "subnet2"}}
410+
ec2Svc.EXPECT().ReconcileLaunchTemplate(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil)
411+
ec2Svc.EXPECT().ReconcileTags(gomock.Any(), gomock.Any()).Return(nil)
412+
asgSvc.EXPECT().GetASGByName(gomock.Any()).Return(&asg, nil).AnyTimes()
413+
asgSvc.EXPECT().SubnetIDs(gomock.Any()).Return([]string{"subnet1"}, nil).Times(1)
414+
asgSvc.EXPECT().UpdateASG(gomock.Any()).Return(nil).Times(1)
415+
416+
_, err := reconciler.reconcileNormal(context.Background(), ms, cs, cs)
417+
g.Expect(err).To(Succeed())
418+
})
419+
t.Run("update Asg due to asgNeedsUpdates returns true", func(t *testing.T) {
420+
g := NewWithT(t)
421+
setup(t, g)
422+
defer teardown(t, g)
423+
424+
asg := expinfrav1.AutoScalingGroup{
425+
MinSize: int32(0),
426+
MaxSize: int32(2),
427+
Subnets: []string{}}
428+
ec2Svc.EXPECT().ReconcileLaunchTemplate(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil)
429+
ec2Svc.EXPECT().ReconcileTags(gomock.Any(), gomock.Any()).Return(nil)
430+
asgSvc.EXPECT().GetASGByName(gomock.Any()).Return(&asg, nil).AnyTimes()
431+
asgSvc.EXPECT().SubnetIDs(gomock.Any()).Return([]string{}, nil).Times(1)
432+
asgSvc.EXPECT().UpdateASG(gomock.Any()).Return(nil).Times(1)
433+
434+
_, err := reconciler.reconcileNormal(context.Background(), ms, cs, cs)
435+
g.Expect(err).To(Succeed())
436+
})
380437
})
381438

382439
t.Run("Deleting an AWSMachinePool", func(t *testing.T) {

‎pkg/cloud/services/autoscaling/autoscalinggroup.go

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,10 @@ func (s *Service) SDKToAutoScalingGroup(v *autoscaling.Group) (*expinfrav1.AutoS
4747
//TODO: determine what additional values go here and what else should be in the struct
4848
}
4949

50+
if v.VPCZoneIdentifier != nil {
51+
i.Subnets = strings.Split(*v.VPCZoneIdentifier, ",")
52+
}
53+
5054
if v.MixedInstancesPolicy != nil {
5155
i.MixedInstancesPolicy = &expinfrav1.MixedInstancesPolicy{
5256
InstancesDistribution: &expinfrav1.InstancesDistribution{
@@ -286,7 +290,7 @@ func (s *Service) UpdateASG(scope *scope.MachinePoolScope) error {
286290
AutoScalingGroupName: aws.String(scope.Name()), //TODO: define dynamically - borrow logic from ec2
287291
MaxSize: aws.Int64(int64(scope.AWSMachinePool.Spec.MaxSize)),
288292
MinSize: aws.Int64(int64(scope.AWSMachinePool.Spec.MinSize)),
289-
VPCZoneIdentifier: aws.String(strings.Join(subnetIDs, ", ")),
293+
VPCZoneIdentifier: aws.String(strings.Join(subnetIDs, ",")),
290294
CapacityRebalance: aws.Bool(scope.AWSMachinePool.Spec.CapacityRebalance),
291295
}
292296

‎pkg/cloud/services/interfaces.go

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,7 @@ type ASGInterface interface {
4444
DeleteASGAndWait(id string) error
4545
SuspendProcesses(name string, processes []string) error
4646
ResumeProcesses(name string, processes []string) error
47+
SubnetIDs(scope *scope.MachinePoolScope) ([]string, error)
4748
}
4849

4950
// EC2Interface encapsulates the methods exposed to the machine

‎pkg/cloud/services/mock_services/autoscaling_interface_mock.go

Lines changed: 15 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

0 commit comments

Comments
 (0)