From 3b7a483c5266ebc3bff195a9971d3f3f33b57f38 Mon Sep 17 00:00:00 2001 From: shivi28 Date: Fri, 7 Jan 2022 11:08:12 +0530 Subject: [PATCH] Use availabilityZones provided in AWSMachinePool when creating ASG Co-authored-by: anvddriesch Co-authored-by: Shivani Singhal --- exp/api/v1alpha3/conversion.go | 6 ++++++ exp/api/v1alpha3/zz_generated.conversion.go | 16 ++++++---------- exp/api/v1alpha4/conversion.go | 5 +++++ exp/api/v1alpha4/zz_generated.conversion.go | 16 ++++++---------- exp/api/v1beta1/awsmachinepool_webhook.go | 3 +++ exp/api/v1beta1/types.go | 1 + exp/api/v1beta1/zz_generated.deepcopy.go | 5 +++++ exp/controllers/awsmachinepool_controller.go | 16 ++++++++++++++++ pkg/cloud/scope/machinepool.go | 12 ++++++++++++ .../services/autoscaling/autoscalinggroup.go | 12 +++++++++++- .../autoscaling/autoscalinggroup_test.go | 6 ++++++ 11 files changed, 77 insertions(+), 21 deletions(-) diff --git a/exp/api/v1alpha3/conversion.go b/exp/api/v1alpha3/conversion.go index ed77988ef4..b24c1831d3 100644 --- a/exp/api/v1alpha3/conversion.go +++ b/exp/api/v1alpha3/conversion.go @@ -195,3 +195,9 @@ func Convert_v1beta1_Volume_To_v1alpha3_Volume(in *infrav1.Volume, out *infrav1a func Convert_v1alpha3_Volume_To_v1beta1_Volume(in *infrav1alpha3.Volume, out *infrav1.Volume, s apiconversion.Scope) error { return infrav1alpha3.Convert_v1alpha3_Volume_To_v1beta1_Volume(in, out, s) } + +// Convert_v1beta1_AutoScalingGroup_To_v1alpha3_AutoScalingGroup is an autogenerated conversion function. +func Convert_v1beta1_AutoScalingGroup_To_v1alpha3_AutoScalingGroup(in *infrav1exp.AutoScalingGroup, out *AutoScalingGroup, s apiconversion.Scope) error { + return autoConvert_v1beta1_AutoScalingGroup_To_v1alpha3_AutoScalingGroup(in, out, s) +} + diff --git a/exp/api/v1alpha3/zz_generated.conversion.go b/exp/api/v1alpha3/zz_generated.conversion.go index d03024073c..c0830b71b3 100644 --- a/exp/api/v1alpha3/zz_generated.conversion.go +++ b/exp/api/v1alpha3/zz_generated.conversion.go @@ -161,11 +161,6 @@ func RegisterConversions(s *runtime.Scheme) error { }); err != nil { return err } - if err := s.AddGeneratedConversionFunc((*v1beta1.AutoScalingGroup)(nil), (*AutoScalingGroup)(nil), func(a, b interface{}, scope conversion.Scope) error { - return Convert_v1beta1_AutoScalingGroup_To_v1alpha3_AutoScalingGroup(a.(*v1beta1.AutoScalingGroup), b.(*AutoScalingGroup), scope) - }); err != nil { - return err - } if err := s.AddGeneratedConversionFunc((*BlockDeviceMapping)(nil), (*v1beta1.BlockDeviceMapping)(nil), func(a, b interface{}, scope conversion.Scope) error { return Convert_v1alpha3_BlockDeviceMapping_To_v1beta1_BlockDeviceMapping(a.(*BlockDeviceMapping), b.(*v1beta1.BlockDeviceMapping), scope) }); err != nil { @@ -301,6 +296,11 @@ func RegisterConversions(s *runtime.Scheme) error { }); err != nil { return err } + if err := s.AddConversionFunc((*v1beta1.AutoScalingGroup)(nil), (*AutoScalingGroup)(nil), func(a, b interface{}, scope conversion.Scope) error { + return Convert_v1beta1_AutoScalingGroup_To_v1alpha3_AutoScalingGroup(a.(*v1beta1.AutoScalingGroup), b.(*AutoScalingGroup), scope) + }); err != nil { + return err + } return nil } @@ -881,6 +881,7 @@ func autoConvert_v1beta1_AutoScalingGroup_To_v1alpha3_AutoScalingGroup(in *v1bet out.Subnets = *(*[]string)(unsafe.Pointer(&in.Subnets)) out.DefaultCoolDown = in.DefaultCoolDown out.CapacityRebalance = in.CapacityRebalance + // WARNING: in.AvailabilityZones requires manual conversion: does not exist in peer-type out.MixedInstancesPolicy = (*MixedInstancesPolicy)(unsafe.Pointer(in.MixedInstancesPolicy)) out.Status = ASGStatus(in.Status) if in.Instances != nil { @@ -897,11 +898,6 @@ func autoConvert_v1beta1_AutoScalingGroup_To_v1alpha3_AutoScalingGroup(in *v1bet return nil } -// Convert_v1beta1_AutoScalingGroup_To_v1alpha3_AutoScalingGroup is an autogenerated conversion function. -func Convert_v1beta1_AutoScalingGroup_To_v1alpha3_AutoScalingGroup(in *v1beta1.AutoScalingGroup, out *AutoScalingGroup, s conversion.Scope) error { - return autoConvert_v1beta1_AutoScalingGroup_To_v1alpha3_AutoScalingGroup(in, out, s) -} - func autoConvert_v1alpha3_BlockDeviceMapping_To_v1beta1_BlockDeviceMapping(in *BlockDeviceMapping, out *v1beta1.BlockDeviceMapping, s conversion.Scope) error { out.DeviceName = in.DeviceName if err := Convert_v1alpha3_EBS_To_v1beta1_EBS(&in.Ebs, &out.Ebs, s); err != nil { diff --git a/exp/api/v1alpha4/conversion.go b/exp/api/v1alpha4/conversion.go index baa3cbe8c7..ffbc7f528e 100644 --- a/exp/api/v1alpha4/conversion.go +++ b/exp/api/v1alpha4/conversion.go @@ -146,3 +146,8 @@ func Convert_v1beta1_Instance_To_v1alpha4_Instance(in *infrav1.Instance, out *in func Convert_v1alpha4_Instance_To_v1beta1_Instance(in *infrav1alpha4.Instance, out *infrav1.Instance, s apiconversion.Scope) error { return infrav1alpha4.Convert_v1alpha4_Instance_To_v1beta1_Instance(in, out, s) } + +// Convert_v1beta1_AutoScalingGroup_To_v1alpha4_AutoScalingGroup is a conversion function. +func Convert_v1beta1_AutoScalingGroup_To_v1alpha4_AutoScalingGroup(in *infrav1exp.AutoScalingGroup, out *AutoScalingGroup, s apiconversion.Scope) error { + return autoConvert_v1beta1_AutoScalingGroup_To_v1alpha4_AutoScalingGroup(in, out, s) +} diff --git a/exp/api/v1alpha4/zz_generated.conversion.go b/exp/api/v1alpha4/zz_generated.conversion.go index 48d17d5c73..aa497ac2a9 100644 --- a/exp/api/v1alpha4/zz_generated.conversion.go +++ b/exp/api/v1alpha4/zz_generated.conversion.go @@ -161,11 +161,6 @@ func RegisterConversions(s *runtime.Scheme) error { }); err != nil { return err } - if err := s.AddGeneratedConversionFunc((*v1beta1.AutoScalingGroup)(nil), (*AutoScalingGroup)(nil), func(a, b interface{}, scope conversion.Scope) error { - return Convert_v1beta1_AutoScalingGroup_To_v1alpha4_AutoScalingGroup(a.(*v1beta1.AutoScalingGroup), b.(*AutoScalingGroup), scope) - }); err != nil { - return err - } if err := s.AddGeneratedConversionFunc((*BlockDeviceMapping)(nil), (*v1beta1.BlockDeviceMapping)(nil), func(a, b interface{}, scope conversion.Scope) error { return Convert_v1alpha4_BlockDeviceMapping_To_v1beta1_BlockDeviceMapping(a.(*BlockDeviceMapping), b.(*v1beta1.BlockDeviceMapping), scope) }); err != nil { @@ -306,6 +301,11 @@ func RegisterConversions(s *runtime.Scheme) error { }); err != nil { return err } + if err := s.AddConversionFunc((*v1beta1.AutoScalingGroup)(nil), (*AutoScalingGroup)(nil), func(a, b interface{}, scope conversion.Scope) error { + return Convert_v1beta1_AutoScalingGroup_To_v1alpha4_AutoScalingGroup(a.(*v1beta1.AutoScalingGroup), b.(*AutoScalingGroup), scope) + }); err != nil { + return err + } if err := s.AddConversionFunc((*apiv1beta1.Instance)(nil), (*apiv1alpha4.Instance)(nil), func(a, b interface{}, scope conversion.Scope) error { return Convert_v1beta1_Instance_To_v1alpha4_Instance(a.(*apiv1beta1.Instance), b.(*apiv1alpha4.Instance), scope) }); err != nil { @@ -837,6 +837,7 @@ func autoConvert_v1beta1_AutoScalingGroup_To_v1alpha4_AutoScalingGroup(in *v1bet out.Subnets = *(*[]string)(unsafe.Pointer(&in.Subnets)) out.DefaultCoolDown = in.DefaultCoolDown out.CapacityRebalance = in.CapacityRebalance + // WARNING: in.AvailabilityZones requires manual conversion: does not exist in peer-type out.MixedInstancesPolicy = (*MixedInstancesPolicy)(unsafe.Pointer(in.MixedInstancesPolicy)) out.Status = ASGStatus(in.Status) if in.Instances != nil { @@ -853,11 +854,6 @@ func autoConvert_v1beta1_AutoScalingGroup_To_v1alpha4_AutoScalingGroup(in *v1bet return nil } -// Convert_v1beta1_AutoScalingGroup_To_v1alpha4_AutoScalingGroup is an autogenerated conversion function. -func Convert_v1beta1_AutoScalingGroup_To_v1alpha4_AutoScalingGroup(in *v1beta1.AutoScalingGroup, out *AutoScalingGroup, s conversion.Scope) error { - return autoConvert_v1beta1_AutoScalingGroup_To_v1alpha4_AutoScalingGroup(in, out, s) -} - func autoConvert_v1alpha4_BlockDeviceMapping_To_v1beta1_BlockDeviceMapping(in *BlockDeviceMapping, out *v1beta1.BlockDeviceMapping, s conversion.Scope) error { out.DeviceName = in.DeviceName if err := Convert_v1alpha4_EBS_To_v1beta1_EBS(&in.Ebs, &out.Ebs, s); err != nil { diff --git a/exp/api/v1beta1/awsmachinepool_webhook.go b/exp/api/v1beta1/awsmachinepool_webhook.go index a17c44f9d6..1f53dbffb9 100644 --- a/exp/api/v1beta1/awsmachinepool_webhook.go +++ b/exp/api/v1beta1/awsmachinepool_webhook.go @@ -150,4 +150,7 @@ func (r *AWSMachinePool) Default() { log.Info("DefaultCoolDown is zero, setting 300 seconds as default") r.Spec.DefaultCoolDown.Duration = 300 * time.Second } + if len(r.Spec.AvailabilityZones) == 1 && r.Spec.AvailabilityZones[0] == "" { + r.Spec.AvailabilityZones = nil + } } diff --git a/exp/api/v1beta1/types.go b/exp/api/v1beta1/types.go index 404266d517..649166c894 100644 --- a/exp/api/v1beta1/types.go +++ b/exp/api/v1beta1/types.go @@ -183,6 +183,7 @@ type AutoScalingGroup struct { Subnets []string `json:"subnets,omitempty"` DefaultCoolDown metav1.Duration `json:"defaultCoolDown,omitempty"` CapacityRebalance bool `json:"capacityRebalance,omitempty"` + AvailabilityZones []string `json:"availabilityZones,omitempty"` MixedInstancesPolicy *MixedInstancesPolicy `json:"mixedInstancesPolicy,omitempty"` Status ASGStatus diff --git a/exp/api/v1beta1/zz_generated.deepcopy.go b/exp/api/v1beta1/zz_generated.deepcopy.go index ceadce5dd8..15ebfdd2e7 100644 --- a/exp/api/v1beta1/zz_generated.deepcopy.go +++ b/exp/api/v1beta1/zz_generated.deepcopy.go @@ -500,6 +500,11 @@ func (in *AutoScalingGroup) DeepCopyInto(out *AutoScalingGroup) { copy(*out, *in) } out.DefaultCoolDown = in.DefaultCoolDown + if in.AvailabilityZones != nil { + in, out := &in.AvailabilityZones, &out.AvailabilityZones + *out = make([]string, len(*in)) + copy(*out, *in) + } if in.MixedInstancesPolicy != nil { in, out := &in.MixedInstancesPolicy, &out.MixedInstancesPolicy *out = new(MixedInstancesPolicy) diff --git a/exp/controllers/awsmachinepool_controller.go b/exp/controllers/awsmachinepool_controller.go index 39b3074f8d..7944f30339 100644 --- a/exp/controllers/awsmachinepool_controller.go +++ b/exp/controllers/awsmachinepool_controller.go @@ -20,6 +20,7 @@ import ( "context" "fmt" "reflect" + "sort" "github.com/go-logr/logr" "github.com/pkg/errors" @@ -531,11 +532,26 @@ func asgNeedsUpdates(machinePoolScope *scope.MachinePoolScope, existingASG *expi return true } + if checkAZsNeedsUpdate(machinePoolScope.AvailabilityZones(), existingASG.AvailabilityZones) { + return true + } + // todo subnet diff return false } +func checkAZsNeedsUpdate(input, existing []string) bool { + sort.Slice(input, func(i, j int) bool { + return input[i] < input[j] + }) + sort.Slice(existing, func(i, j int) bool { + return existing[i] < existing[j] + }) + + return reflect.DeepEqual(input, existing) +} + // getOwnerMachinePool returns the MachinePool object owning the current resource. func getOwnerMachinePool(ctx context.Context, c client.Client, obj metav1.ObjectMeta) (*expclusterv1.MachinePool, error) { for _, ref := range obj.OwnerReferences { diff --git a/pkg/cloud/scope/machinepool.go b/pkg/cloud/scope/machinepool.go index c014db1549..a5ad4d6322 100644 --- a/pkg/cloud/scope/machinepool.go +++ b/pkg/cloud/scope/machinepool.go @@ -235,6 +235,18 @@ func (m *MachinePoolScope) SubnetIDs(subnetIDs []string) ([]string, error) { }) } +// AvailabilityZones returns the machine pool availability zones. +// In case availability zones are not given in the AWSMachinePoolSpec, we fall back to the Failure Domains on the MachinePool. +func (m *MachinePoolScope) AvailabilityZones() []string { + if m.AWSMachinePool.Spec.AvailabilityZones != nil { + return m.AWSMachinePool.Spec.AvailabilityZones + } + if m.MachinePool.Spec.FailureDomains != nil { + return m.MachinePool.Spec.FailureDomains + } + return nil +} + // NodeStatus represents the status of a Kubernetes node. type NodeStatus struct { Ready bool diff --git a/pkg/cloud/services/autoscaling/autoscalinggroup.go b/pkg/cloud/services/autoscaling/autoscalinggroup.go index 11a1d7b813..16efcd8cfe 100644 --- a/pkg/cloud/services/autoscaling/autoscalinggroup.go +++ b/pkg/cloud/services/autoscaling/autoscalinggroup.go @@ -44,6 +44,7 @@ func (s *Service) SDKToAutoScalingGroup(v *autoscaling.Group) (*expinfrav1.AutoS MaxSize: int32(aws.Int64Value(v.MaxSize)), MinSize: int32(aws.Int64Value(v.MinSize)), CapacityRebalance: aws.BoolValue(v.CapacityRebalance), + AvailabilityZones: aws.StringValueSlice(v.AvailabilityZones), //TODO: determine what additional values go here and what else should be in the struct } @@ -158,6 +159,7 @@ func (s *Service) CreateASG(scope *scope.MachinePoolScope) (*expinfrav1.AutoScal DefaultCoolDown: scope.AWSMachinePool.Spec.DefaultCoolDown, CapacityRebalance: scope.AWSMachinePool.Spec.CapacityRebalance, MixedInstancesPolicy: scope.AWSMachinePool.Spec.MixedInstancesPolicy, + AvailabilityZones: scope.AvailabilityZones(), } if scope.MachinePool.Spec.Replicas != nil { @@ -223,6 +225,10 @@ func (s *Service) runPool(i *expinfrav1.AutoScalingGroup, launchTemplateID strin input.Tags = BuildTagsFromMap(i.Name, i.Tags) } + if i.AvailabilityZones != nil { + input.AvailabilityZones = aws.StringSlice(i.AvailabilityZones) + } + if _, err := s.ASGClient.CreateAutoScalingGroup(input); err != nil { return errors.Wrap(err, "failed to create autoscaling group") } @@ -294,6 +300,10 @@ func (s *Service) UpdateASG(scope *scope.MachinePoolScope) error { } } + if scope.AvailabilityZones() != nil { + input.AvailabilityZones = aws.StringSlice(scope.AWSMachinePool.Spec.AvailabilityZones) + } + if _, err := s.ASGClient.UpdateAutoScalingGroup(input); err != nil { return errors.Wrapf(err, "failed to update ASG %q", scope.Name()) } @@ -309,7 +319,7 @@ func (s *Service) CanStartASGInstanceRefresh(scope *scope.MachinePoolScope) (boo return false, err } hasUnfinishedRefresh := false - if err == nil && len(refreshes.InstanceRefreshes) != 0 { + if len(refreshes.InstanceRefreshes) != 0 { for i := range refreshes.InstanceRefreshes { if *refreshes.InstanceRefreshes[i].Status == autoscaling.InstanceRefreshStatusInProgress || *refreshes.InstanceRefreshes[i].Status == autoscaling.InstanceRefreshStatusPending || diff --git a/pkg/cloud/services/autoscaling/autoscalinggroup_test.go b/pkg/cloud/services/autoscaling/autoscalinggroup_test.go index 65dc69ab9d..04761ce95b 100644 --- a/pkg/cloud/services/autoscaling/autoscalinggroup_test.go +++ b/pkg/cloud/services/autoscaling/autoscalinggroup_test.go @@ -144,6 +144,7 @@ func TestService_SDKToAutoScalingGroup(t *testing.T) { MaxSize: aws.Int64(1234), MinSize: aws.Int64(1234), CapacityRebalance: aws.Bool(true), + AvailabilityZones: aws.StringSlice([]string{"test-az"}), MixedInstancesPolicy: &autoscaling.MixedInstancesPolicy{ InstancesDistribution: &autoscaling.InstancesDistribution{ OnDemandAllocationStrategy: aws.String("prioritized"), @@ -168,6 +169,7 @@ func TestService_SDKToAutoScalingGroup(t *testing.T) { MaxSize: int32(1234), MinSize: int32(1234), CapacityRebalance: true, + AvailabilityZones: []string{"test-az"}, MixedInstancesPolicy: &expinfrav1.MixedInstancesPolicy{ InstancesDistribution: &expinfrav1.InstancesDistribution{ OnDemandAllocationStrategy: expinfrav1.OnDemandAllocationStrategyPrioritized, @@ -222,6 +224,7 @@ func TestService_SDKToAutoScalingGroup(t *testing.T) { LifecycleState: aws.String("lifecycleState"), }, }, + AvailabilityZones: []*string{aws.String("az1")}, }, want: &expinfrav1.AutoScalingGroup{ ID: "test-id", @@ -253,6 +256,7 @@ func TestService_SDKToAutoScalingGroup(t *testing.T) { State: "lifecycleState", }, }, + AvailabilityZones: []string{"az1"}, }, wantErr: false, }, @@ -265,6 +269,7 @@ func TestService_SDKToAutoScalingGroup(t *testing.T) { MaxSize: aws.Int64(1234), MinSize: aws.Int64(1234), CapacityRebalance: aws.Bool(true), + AvailabilityZones: aws.StringSlice([]string{"test-az"}), MixedInstancesPolicy: nil, }, want: &expinfrav1.AutoScalingGroup{ @@ -274,6 +279,7 @@ func TestService_SDKToAutoScalingGroup(t *testing.T) { MaxSize: int32(1234), MinSize: int32(1234), CapacityRebalance: true, + AvailabilityZones: []string{"test-az"}, MixedInstancesPolicy: nil, }, wantErr: false,