Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] Use availabilityZones provided in AWSMachinePool when creating ASG #3245

Closed
wants to merge 1 commit into from
Closed

[WIP] Use availabilityZones provided in AWSMachinePool when creating ASG #3245

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions exp/api/v1alpha3/conversion.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}

16 changes: 6 additions & 10 deletions exp/api/v1alpha3/zz_generated.conversion.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 5 additions & 0 deletions exp/api/v1alpha4/conversion.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
16 changes: 6 additions & 10 deletions exp/api/v1alpha4/zz_generated.conversion.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 3 additions & 0 deletions exp/api/v1beta1/awsmachinepool_webhook.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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] == "" {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we see this situation?

r.Spec.AvailabilityZones = nil
}
}
1 change: 1 addition & 0 deletions exp/api/v1beta1/types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
5 changes: 5 additions & 0 deletions exp/api/v1beta1/zz_generated.deepcopy.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

16 changes: 16 additions & 0 deletions exp/controllers/awsmachinepool_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import (
"context"
"fmt"
"reflect"
"sort"

"github.com/go-logr/logr"
"github.com/pkg/errors"
Expand Down Expand Up @@ -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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we have a PR in place to replace this with gocmp, it would be good to follow that for new PRs, wdyt?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes we should change this to use cmp.Equal

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be !reflect.DeepEqual(input, existing), coz if both are same we won't need the update?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, good spot @Ankitasw

}

// 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 {
Expand Down
12 changes: 12 additions & 0 deletions pkg/cloud/scope/machinepool.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
12 changes: 11 additions & 1 deletion pkg/cloud/services/autoscaling/autoscalinggroup.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}

Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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")
}
Expand Down Expand Up @@ -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())
}
Expand All @@ -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 ||
Expand Down
6 changes: 6 additions & 0 deletions pkg/cloud/services/autoscaling/autoscalinggroup_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"),
Expand All @@ -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,
Expand Down Expand Up @@ -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",
Expand Down Expand Up @@ -253,6 +256,7 @@ func TestService_SDKToAutoScalingGroup(t *testing.T) {
State: "lifecycleState",
},
},
AvailabilityZones: []string{"az1"},
},
wantErr: false,
},
Expand All @@ -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{
Expand All @@ -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,
Expand Down