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

shivi28 · 2022-02-23T05:15:00Z

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #2320

Special notes for your reviewer:
Tested scenarios:

If nothing is passed in AZ and subnets, then ASG created in all the available AZs and have once subnet from each AZ
If AZ is passed, then ASG is created in that AZ and in subnets belonged to given AZ
If AZ is passed and subnet IDs are given, then we check if given AZ and subnet IDs belong to same AZ, if yes then ASG is created in given AZ and given subnet IDs
If AZ is passed and subnet IDs are given, then we check if given AZ and subnet IDs belong to same AZ, if not then returns error unable to update ASG. The availability zones of the specified subnets and the Auto Scaling group do not match

Checklist:

squashed commits
includes documentation
adds unit tests
adds or updates e2e tests

Release note:

Use availabilityZones provided in AWSMachinePool when creating ASG

k8s-ci-robot · 2022-02-23T05:15:07Z

@shivi28: This issue is currently awaiting triage.

If CAPA/CAPI contributors determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

k8s-ci-robot · 2022-02-23T05:15:21Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign sedefsavas after the PR has been reviewed.
You can assign the PR to them by writing /assign @sedefsavas in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

shivi28 · 2022-02-25T06:48:15Z

/hold
We can merge this PR, once this PR gets merged. Meanwhile anyone can review it

Co-authored-by: anvddriesch Co-authored-by: Shivani Singhal <[email protected]>

shivi28 · 2022-03-09T07:07:23Z

/unhold

shivi28 · 2022-03-12T13:13:00Z

/test ?

k8s-ci-robot · 2022-03-12T13:13:02Z

@shivi28: The following commands are available to trigger required jobs:

/test pull-cluster-api-provider-aws-build
/test pull-cluster-api-provider-aws-test
/test pull-cluster-api-provider-aws-verify

The following commands are available to trigger optional jobs:

/test pull-cluster-api-provider-aws-apidiff-main
/test pull-cluster-api-provider-aws-e2e
/test pull-cluster-api-provider-aws-e2e-blocking
/test pull-cluster-api-provider-aws-e2e-conformance
/test pull-cluster-api-provider-aws-e2e-conformance-with-ci-artifacts
/test pull-cluster-api-provider-aws-e2e-eks

Use /test all to run the following jobs that were automatically triggered:

pull-cluster-api-provider-aws-apidiff-main
pull-cluster-api-provider-aws-build
pull-cluster-api-provider-aws-test
pull-cluster-api-provider-aws-verify

In response to this:

/test ?

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

shivi28 · 2022-03-12T13:13:22Z

/test pull-cluster-api-provider-aws-e2e-eks
/test pull-cluster-api-provider-aws-e2e

shivi28 · 2022-03-16T07:11:37Z

Looping @richardcase to have a look at this PR as we had a initial discussion on this issue 🙇‍♀️

Ankitasw · 2022-03-17T12:42:01Z

exp/controllers/awsmachinepool_controller.go

+		return existing[i] < existing[j]
+	})
+
+	return reflect.DeepEqual(input, existing)


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

Yes we should change this to use cmp.Equal

Ankitasw · 2022-03-17T12:46:45Z

exp/controllers/awsmachinepool_controller.go

+		return existing[i] < existing[j]
+	})
+
+	return reflect.DeepEqual(input, existing)


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

Yes, good spot @Ankitasw

k8s-ci-robot · 2022-03-29T18:35:37Z

@shivi28: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

richardcase

Do you think it would also be worth ensuring we cover both scenarios in the e2e tests?

richardcase · 2022-04-11T13:34:49Z

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] == "" {


Do we see this situation?

richardcase · 2022-04-11T13:37:38Z

exp/controllers/awsmachinepool_controller.go

+		return existing[i] < existing[j]
+	})
+
+	return reflect.DeepEqual(input, existing)


Yes we should change this to use cmp.Equal

richardcase · 2022-04-11T13:37:53Z

exp/controllers/awsmachinepool_controller.go

+		return existing[i] < existing[j]
+	})
+
+	return reflect.DeepEqual(input, existing)


Yes, good spot @Ankitasw

sedefsavas · 2022-04-12T19:05:39Z

Do you think it would also be worth ensuring we cover both scenarios in the e2e tests?

+1 to add the scenarios in the PR definition to e2e tests.

k8s-triage-robot · 2022-07-24T05:30:50Z

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

Mark this issue or PR as fresh with /remove-lifecycle stale
Mark this issue or PR as rotten with /lifecycle rotten
Close this issue or PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

richardcase · 2022-07-25T16:57:40Z

/milestone clear

k8s-ci-robot · 2022-07-29T12:50:16Z

@shivi28: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-cluster-api-provider-aws-build-release-1-5	`3b7a483`	link	true	`/test pull-cluster-api-provider-aws-build-release-1-5`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

k8s-triage-robot · 2022-08-28T13:23:19Z

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

Mark this issue or PR as fresh with /remove-lifecycle rotten
Close this issue or PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

k8s-triage-robot · 2022-09-27T14:00:39Z

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Reopen this PR with /reopen
Mark this PR as fresh with /remove-lifecycle rotten
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

k8s-ci-robot · 2022-09-27T14:00:43Z

@k8s-triage-robot: Closed this PR.

In response to this:

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied

After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied

After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Reopen this PR with /reopen

Mark this PR as fresh with /remove-lifecycle rotten

Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

k8s-ci-robot added needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 23, 2022

k8s-ci-robot requested review from pydctw and sedefsavas February 23, 2022 05:15

shivi28 force-pushed the aws_2320 branch from 9cb9911 to b777bed Compare February 23, 2022 05:41

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 23, 2022

shivi28 force-pushed the aws_2320 branch 2 times, most recently from 99d8c8b to ee40a77 Compare February 23, 2022 06:40

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 23, 2022

shivi28 mentioned this pull request Feb 23, 2022

bug: Use availabilityZones provided in AWSMachinePool when creating ASG #2588

Closed

4 tasks

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Feb 23, 2022

shivi28 changed the title ~~[WIP] Use availabilityZones provided in AWSMachinePool when creating ASG~~ Use availabilityZones provided in AWSMachinePool when creating ASG Feb 25, 2022

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 25, 2022

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 25, 2022

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 8, 2022

Use availabilityZones provided in AWSMachinePool when creating ASG

3b7a483

Co-authored-by: anvddriesch Co-authored-by: Shivani Singhal <[email protected]>

shivi28 force-pushed the aws_2320 branch from ee40a77 to 3b7a483 Compare March 9, 2022 07:07

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 9, 2022

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 9, 2022

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 9, 2022

sedefsavas added this to the v1.4.0 milestone Mar 16, 2022

Ankitasw reviewed Mar 17, 2022

View reviewed changes

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 29, 2022

richardcase reviewed Apr 11, 2022

View reviewed changes

shivi28 changed the title ~~Use availabilityZones provided in AWSMachinePool when creating ASG~~ [WIP] Use availabilityZones provided in AWSMachinePool when creating ASG Apr 25, 2022

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 25, 2022

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jul 24, 2022

k8s-ci-robot removed this from the v1.5.0 milestone Jul 25, 2022

k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Aug 28, 2022

k8s-ci-robot closed this Sep 27, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

shivi28 commented Feb 23, 2022 •

edited

Loading

k8s-ci-robot commented Feb 23, 2022

k8s-ci-robot commented Feb 23, 2022

shivi28 commented Feb 25, 2022

shivi28 commented Mar 9, 2022

shivi28 commented Mar 12, 2022

k8s-ci-robot commented Mar 12, 2022

shivi28 commented Mar 12, 2022

shivi28 commented Mar 16, 2022

Ankitasw Mar 17, 2022

richardcase Apr 11, 2022

Ankitasw Mar 17, 2022

richardcase Apr 11, 2022

k8s-ci-robot commented Mar 29, 2022

richardcase left a comment

richardcase Apr 11, 2022

richardcase Apr 11, 2022

richardcase Apr 11, 2022

sedefsavas commented Apr 12, 2022

k8s-triage-robot commented Jul 24, 2022

richardcase commented Jul 25, 2022

k8s-ci-robot commented Jul 29, 2022

k8s-triage-robot commented Aug 28, 2022

k8s-triage-robot commented Sep 27, 2022

k8s-ci-robot commented Sep 27, 2022

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

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

Conversation

shivi28 commented Feb 23, 2022 • edited Loading

k8s-ci-robot commented Feb 23, 2022

k8s-ci-robot commented Feb 23, 2022

shivi28 commented Feb 25, 2022

shivi28 commented Mar 9, 2022

shivi28 commented Mar 12, 2022

k8s-ci-robot commented Mar 12, 2022

shivi28 commented Mar 12, 2022

shivi28 commented Mar 16, 2022

Ankitasw Mar 17, 2022

Choose a reason for hiding this comment

richardcase Apr 11, 2022

Choose a reason for hiding this comment

Ankitasw Mar 17, 2022

Choose a reason for hiding this comment

richardcase Apr 11, 2022

Choose a reason for hiding this comment

k8s-ci-robot commented Mar 29, 2022

richardcase left a comment

Choose a reason for hiding this comment

richardcase Apr 11, 2022

Choose a reason for hiding this comment

richardcase Apr 11, 2022

Choose a reason for hiding this comment

richardcase Apr 11, 2022

Choose a reason for hiding this comment

sedefsavas commented Apr 12, 2022

k8s-triage-robot commented Jul 24, 2022

richardcase commented Jul 25, 2022

k8s-ci-robot commented Jul 29, 2022

k8s-triage-robot commented Aug 28, 2022

k8s-triage-robot commented Sep 27, 2022

k8s-ci-robot commented Sep 27, 2022

shivi28 commented Feb 23, 2022 •

edited

Loading