Skip to content

Commit 663b129

Browse files
cnmcavoycnmcavoy
committed
Improve AWSMachinePool diff to default to existing values when comparing launch templates
1 parent 562a585 commit 663b129

File tree

2 files changed

+138
-29
lines changed

2 files changed

+138
-29
lines changed

‎exp/controllers/awsmachinepool_controller.go

Lines changed: 35 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -404,9 +404,16 @@ func (r *AWSMachinePoolReconciler) updatePool(machinePoolScope *scope.MachinePoo
404404
"subnets of machinePoolScope", subnetIDs,
405405
"subnets of existing asg", existingASG.Subnets)
406406
less := func(a, b string) bool { return a < b }
407-
subnetChanges := cmp.Diff(subnetIDs, existingASG.Subnets, cmpopts.SortSlices(less)) != ""
407+
subnetDiff := cmp.Diff(subnetIDs, existingASG.Subnets, cmpopts.SortSlices(less))
408+
if subnetDiff != "" {
409+
machinePoolScope.Debug("asg subnet diff detected", "diff", subnetDiff)
410+
}
408411

409-
if asgNeedsUpdates(machinePoolScope, existingASG) || subnetChanges {
412+
asgDiff := diffASG(machinePoolScope, existingASG)
413+
if asgDiff != "" {
414+
machinePoolScope.Debug("asg diff detected", "diff", subnetDiff)
415+
}
416+
if asgDiff != "" || subnetDiff != "" {
410417
machinePoolScope.Info("updating AutoScalingGroup")
411418

412419
if err := asgSvc.UpdateASG(machinePoolScope); err != nil {
@@ -493,36 +500,37 @@ func (r *AWSMachinePoolReconciler) findASG(machinePoolScope *scope.MachinePoolSc
493500
return asg, nil
494501
}
495502

496-
// asgNeedsUpdates compares incoming AWSMachinePool and compares against existing ASG.
497-
func asgNeedsUpdates(machinePoolScope *scope.MachinePoolScope, existingASG *expinfrav1.AutoScalingGroup) bool {
503+
// diffASG compares incoming AWSMachinePool and compares against existing ASG.
504+
func diffASG(machinePoolScope *scope.MachinePoolScope, existingASG *expinfrav1.AutoScalingGroup) string {
505+
detectedMachinePoolSpec := machinePoolScope.MachinePool.Spec.DeepCopy()
506+
498507
if !scope.ReplicasExternallyManaged(machinePoolScope.MachinePool) {
499-
if machinePoolScope.MachinePool.Spec.Replicas != nil {
500-
if existingASG.DesiredCapacity == nil || *machinePoolScope.MachinePool.Spec.Replicas != *existingASG.DesiredCapacity {
501-
return true
502-
}
503-
} else if existingASG.DesiredCapacity != nil {
504-
return true
508+
detectedMachinePoolSpec.Replicas = existingASG.DesiredCapacity
509+
}
510+
if diff := cmp.Diff(machinePoolScope.MachinePool.Spec, *detectedMachinePoolSpec); diff != "" {
511+
return diff
512+
}
513+
514+
detectedAWSMachinePoolSpec := machinePoolScope.AWSMachinePool.Spec.DeepCopy()
515+
detectedAWSMachinePoolSpec.MaxSize = existingASG.MaxSize
516+
detectedAWSMachinePoolSpec.MinSize = existingASG.MinSize
517+
detectedAWSMachinePoolSpec.CapacityRebalance = existingASG.CapacityRebalance
518+
{
519+
mixedInstancesPolicy := machinePoolScope.AWSMachinePool.Spec.MixedInstancesPolicy
520+
// InstancesDistribution is optional, and the default values come from AWS, so
521+
// they are not set by the AWSMachinePool defaulting webhook. If InstancesDistribution is
522+
// not set, we use the AWS values for the purpose of comparison.
523+
if mixedInstancesPolicy != nil && mixedInstancesPolicy.InstancesDistribution == nil {
524+
mixedInstancesPolicy = machinePoolScope.AWSMachinePool.Spec.MixedInstancesPolicy.DeepCopy()
525+
mixedInstancesPolicy.InstancesDistribution = existingASG.MixedInstancesPolicy.InstancesDistribution
505526
}
506-
}
507-
508-
if machinePoolScope.AWSMachinePool.Spec.MaxSize != existingASG.MaxSize {
509-
return true
510-
}
511-
512-
if machinePoolScope.AWSMachinePool.Spec.MinSize != existingASG.MinSize {
513-
return true
514-
}
515527

516-
if machinePoolScope.AWSMachinePool.Spec.CapacityRebalance != existingASG.CapacityRebalance {
517-
return true
518-
}
519-
520-
if !cmp.Equal(machinePoolScope.AWSMachinePool.Spec.MixedInstancesPolicy, existingASG.MixedInstancesPolicy) {
521-
machinePoolScope.Info("got a mixed diff here", "incoming", machinePoolScope.AWSMachinePool.Spec.MixedInstancesPolicy, "existing", existingASG.MixedInstancesPolicy)
522-
return true
528+
if !cmp.Equal(mixedInstancesPolicy, existingASG.MixedInstancesPolicy) {
529+
detectedAWSMachinePoolSpec.MixedInstancesPolicy = existingASG.MixedInstancesPolicy
530+
}
523531
}
524532

525-
return false
533+
return cmp.Diff(machinePoolScope.AWSMachinePool.Spec, *detectedAWSMachinePoolSpec)
526534
}
527535

528536
// getOwnerMachinePool returns the MachinePool object owning the current resource.

‎exp/controllers/awsmachinepool_controller_test.go

Lines changed: 103 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@ import (
2323
"fmt"
2424
"testing"
2525

26+
"github.com/aws/aws-sdk-go/aws"
2627
"github.com/go-logr/logr"
2728
"github.com/golang/mock/gomock"
2829
. "github.com/onsi/gomega"
@@ -531,7 +532,7 @@ func setupCluster(clusterName string) (*scope.ClusterScope, error) {
531532
})
532533
}
533534

534-
func TestASGNeedsUpdates(t *testing.T) {
535+
func TestDiffASG(t *testing.T) {
535536
type args struct {
536537
machinePoolScope *scope.MachinePoolScope
537538
existingASG *expinfrav1.AutoScalingGroup
@@ -695,6 +696,106 @@ func TestASGNeedsUpdates(t *testing.T) {
695696
},
696697
want: true,
697698
},
699+
{
700+
name: "MixedInstancesPolicy.InstancesDistribution != asg.MixedInstancesPolicy.InstancesDistribution",
701+
args: args{
702+
machinePoolScope: &scope.MachinePoolScope{
703+
MachinePool: &expclusterv1.MachinePool{
704+
Spec: expclusterv1.MachinePoolSpec{
705+
Replicas: pointer.Int32(1),
706+
},
707+
},
708+
AWSMachinePool: &expinfrav1.AWSMachinePool{
709+
Spec: expinfrav1.AWSMachinePoolSpec{
710+
MaxSize: 2,
711+
MinSize: 0,
712+
CapacityRebalance: true,
713+
MixedInstancesPolicy: &expinfrav1.MixedInstancesPolicy{
714+
InstancesDistribution: &expinfrav1.InstancesDistribution{
715+
OnDemandAllocationStrategy: expinfrav1.OnDemandAllocationStrategyPrioritized,
716+
SpotAllocationStrategy: expinfrav1.SpotAllocationStrategyCapacityOptimized,
717+
OnDemandBaseCapacity: aws.Int64(0),
718+
OnDemandPercentageAboveBaseCapacity: aws.Int64(100),
719+
},
720+
Overrides: []expinfrav1.Overrides{
721+
{
722+
InstanceType: "m6a.32xlarge",
723+
},
724+
},
725+
},
726+
},
727+
},
728+
Logger: *logger.NewLogger(logr.Discard()),
729+
},
730+
existingASG: &expinfrav1.AutoScalingGroup{
731+
DesiredCapacity: pointer.Int32(1),
732+
MaxSize: 2,
733+
MinSize: 0,
734+
CapacityRebalance: true,
735+
MixedInstancesPolicy: &expinfrav1.MixedInstancesPolicy{
736+
InstancesDistribution: &expinfrav1.InstancesDistribution{
737+
OnDemandAllocationStrategy: expinfrav1.OnDemandAllocationStrategyPrioritized,
738+
SpotAllocationStrategy: expinfrav1.SpotAllocationStrategyLowestPrice,
739+
OnDemandBaseCapacity: aws.Int64(0),
740+
OnDemandPercentageAboveBaseCapacity: aws.Int64(100),
741+
},
742+
Overrides: []expinfrav1.Overrides{
743+
{
744+
InstanceType: "m6a.32xlarge",
745+
},
746+
},
747+
},
748+
},
749+
},
750+
want: true,
751+
},
752+
{
753+
name: "MixedInstancesPolicy.InstancesDistribution unset",
754+
args: args{
755+
machinePoolScope: &scope.MachinePoolScope{
756+
MachinePool: &expclusterv1.MachinePool{
757+
Spec: expclusterv1.MachinePoolSpec{
758+
Replicas: pointer.Int32(1),
759+
},
760+
},
761+
AWSMachinePool: &expinfrav1.AWSMachinePool{
762+
Spec: expinfrav1.AWSMachinePoolSpec{
763+
MaxSize: 2,
764+
MinSize: 0,
765+
CapacityRebalance: true,
766+
MixedInstancesPolicy: &expinfrav1.MixedInstancesPolicy{
767+
Overrides: []expinfrav1.Overrides{
768+
{
769+
InstanceType: "m6a.32xlarge",
770+
},
771+
},
772+
},
773+
},
774+
},
775+
Logger: *logger.NewLogger(logr.Discard()),
776+
},
777+
existingASG: &expinfrav1.AutoScalingGroup{
778+
DesiredCapacity: pointer.Int32(1),
779+
MaxSize: 2,
780+
MinSize: 0,
781+
CapacityRebalance: true,
782+
MixedInstancesPolicy: &expinfrav1.MixedInstancesPolicy{
783+
InstancesDistribution: &expinfrav1.InstancesDistribution{
784+
OnDemandAllocationStrategy: expinfrav1.OnDemandAllocationStrategyPrioritized,
785+
SpotAllocationStrategy: expinfrav1.SpotAllocationStrategyLowestPrice,
786+
OnDemandBaseCapacity: aws.Int64(0),
787+
OnDemandPercentageAboveBaseCapacity: aws.Int64(100),
788+
},
789+
Overrides: []expinfrav1.Overrides{
790+
{
791+
InstanceType: "m6a.32xlarge",
792+
},
793+
},
794+
},
795+
},
796+
},
797+
want: false,
798+
},
698799
{
699800
name: "SuspendProcesses != asg.SuspendProcesses",
700801
args: args{
@@ -821,7 +922,7 @@ func TestASGNeedsUpdates(t *testing.T) {
821922
for _, tt := range tests {
822923
t.Run(tt.name, func(t *testing.T) {
823924
g := NewWithT(t)
824-
g.Expect(asgNeedsUpdates(tt.args.machinePoolScope, tt.args.existingASG)).To(Equal(tt.want))
925+
g.Expect(diffASG(tt.args.machinePoolScope, tt.args.existingASG) != "").To(Equal(tt.want))
825926
})
826927
}
827928
}

0 commit comments

Comments
 (0)