Remove suspend process flow from create ASG #3864

Skarlso · 2022-11-19T10:17:39Z

/kind bug

What this PR does / why we need it:

The suspend flow in the create ASG process is failing because during the ASG creation, we never return the actual ASG. That's because the CreateAutoscalingGroup flow is not returning an ASG so we reconcile it.

I just removed the suspend during creation, the update flow should still suspend or unsuspend anything that is different.

I'll try adding some more tests to this part of the code.

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 #

Special notes for your reviewer:

Checklist:

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

richardcase · 2022-11-19T13:33:02Z

/test ?

k8s-ci-robot · 2022-11-19T13:33:04Z

@richardcase: 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-clusterclass
/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.

richardcase · 2022-11-19T13:33:38Z

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

Skarlso · 2022-11-19T15:23:21Z

I've been thinking about how to unit test this but it ain't an easy one. I was thinking about adding an e2e test but that takes time and I don't want to keep the upgrade test broken. @richardcase any ideas?

richardcase · 2022-11-19T15:29:48Z

I've toyed with the idea of adding the option to run "extended" e2e tests.

richardcase · 2022-11-19T15:31:17Z

It wouldn't be easy to unit test but it's doable. I can have a bash if you want?

Skarlso · 2022-11-19T15:43:01Z

It's fine I'll be home soonish I'll take a stab at it. 😊 thank you. I'll shout if I get stuck.

Skarlso · 2022-11-19T15:44:16Z

The question is what to unit test. I guess I can test that during an upgrade suspension is called while during create it does not.

Skarlso · 2022-11-19T17:02:06Z

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

Skarlso · 2022-11-19T17:17:46Z

Turns out I already head unit tests, but I was missing a call to the mock to verify that it is called or not.

Skarlso · 2022-11-19T17:41:23Z

The previous eks failure was not because of this. The create ASG was successful:

I1119 15:18:15.351354       1 recorder.go:103] "events: Normal" object={Kind:AWSMachinePool Namespace:cluster-rvmce3 Name:cluster-0qrm8w-mp-0 UID:0d199a75-8c29-490a-8e95-2ebc3e590ae0 APIVersion:infrastructure.cluster.x-k8s.io/v1beta2 ResourceVersion:19141 FieldPath:} reason="FailedDescribeInstances" message="No Auto Scaling Groups with cluster-0qrm8w-mp-0 found"
I1119 15:18:16.645556       1 recorder.go:103] "events: Normal" object={Kind:AWSMachinePool Namespace:cluster-rvmce3 Name:cluster-0qrm8w-mp-0 UID:0d199a75-8c29-490a-8e95-2ebc3e590ae0 APIVersion:infrastructure.cluster.x-k8s.io/v1beta2 ResourceVersion:19141 FieldPath:} reason="SuccessfulCreate" message="Created new ASG: cluster-0qrm8w-mp-0"
I1119 15:18:16.645799       1 recorder.go:103] "events: Normal" object={Kind:AWSMachinePool Namespace:cluster-rvmce3 Name:cluster-0qrm8w-mp-0 UID:0d199a75-8c29-490a-8e95-2ebc3e590ae0 APIVersion:infrastructure.cluster.x-k8s.io/v1beta2 ResourceVersion:19141 FieldPath:} reason="SuccessfulCreate" message="Created new ASG: cluster-0qrm8w-mp-0"
I1119 15:18:16.664759       1 logger.go:80] "identity: Creating an AWS Session" controller="awsmachinepool" controllerGroup="infrastructure.cluster.x-k8s.io" controllerKind="AWSMachinePool" aWSMachinePool="cluster-rvmce3/cluster-0qrm8w-mp-0" namespace="cluster-rvmce3" name="cluster-0qrm8w-mp-0" reconcileID=83771b90-9210-48df-ad1b-ce6c8824f76f machinePool="cluster-rvmce3/cluster-0qrm8w-mp-0" cluster="cluster-rvmce3/cluster-0qrm8w"
I1119 15:18:16.664807       1 logger.go:80] "identity: Getting identity" controller="awsmachinepool" controllerGroup="infrastructure.cluster.x-k8s.io" controllerKind="AWSMachinePool" aWSMachinePool="cluster-rvmce3/cluster-0qrm8w-mp-0" namespace="cluster-rvmce3" name="cluster-0qrm8w-mp-0" reconcileID=83771b90-9210-48df-ad1b-ce6c8824f76f machinePool="cluster-rvmce3/cluster-0qrm8w-mp-0" cluster="cluster-rvmce3/cluster-0qrm8w" identityKey="/e2e-account"
I1119 15:18:16.665977       1 logger.go:68] "Reconciling AWSMachinePool" controller="awsmachinepool" controllerGroup="infrastructure.cluster.x-k8s.io" controllerKind="AWSMachinePool" aWSMachinePool="cluster-rvmce3/cluster-0qrm8w-mp-0" namespace="cluster-rvmce3" name="cluster-0qrm8w-mp-0" reconcileID=83771b90-9210-48df-ad1b-ce6c8824f76f machinePool="cluster-rvmce3/cluster-0qrm8w-mp-0" cluster="cluster-rvmce3/cluster-0qrm8w"
I1119 15:18:16.666914       1 logger.go:68] "checking for existing launch template"

Skarlso · 2022-11-19T17:50:17Z

Hmm, interesting. The test failed again.

Skarlso · 2022-11-19T17:51:16Z

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

Skarlso · 2022-11-19T17:52:21Z

Kind of suspicious. The Suspend PR had both these tests running:

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

And passing.

Skarlso · 2022-11-19T19:45:39Z

I'm running eks test here as well #3804

Just to see if something isn't messed up.

Skarlso · 2022-11-20T14:24:51Z

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

Skarlso · 2022-11-21T08:15:04Z

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

edit: Wohoo, this passed.

Skarlso · 2022-11-21T15:09:59Z

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

Once more into the fray. Once more we go.

I want to see if AWS got their things together. :)

Skarlso · 2022-11-23T09:28:28Z

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

Skarlso · 2022-11-25T15:34:19Z

@richardcase @Ankitasw This is now ready. I don't think there is anything else to fix in here. Should I add a suspend e2e during this pr?

Ankitasw · 2022-11-30T12:40:43Z

It seems fine to push to v2.0.2 as it doesn't break any functionality and hence take care of E2E tests in a separate issue so:

/lgtm

Ankitasw · 2022-11-30T12:47:42Z

/approve

k8s-ci-robot · 2022-11-30T12:48:03Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Ankitasw

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [Ankitasw]

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

Ankitasw · 2022-11-30T12:56:01Z

/cherry-pick release-2.0

k8s-infra-cherrypick-robot · 2022-11-30T12:56:02Z

@Ankitasw: once the present PR merges, I will cherry-pick it on top of release-2.0 in a new PR and assign it to you.

In response to this:

/cherry-pick release-2.0

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-infra-cherrypick-robot · 2022-11-30T13:01:33Z

@Ankitasw: new pull request created: #3892

In response to this:

/cherry-pick release-2.0

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.

Remove suspend process flow from create ASG

b0e3658

k8s-ci-robot requested review from dthorsen and pydctw November 19, 2022 10:17

Added some logs and fixed the unit test

37b8794

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Nov 19, 2022

Skarlso changed the title ~~[WIP] Remove suspend process flow from create ASG ( fix upgrade test )~~ Remove suspend process flow from create ASG Nov 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 Nov 25, 2022

k8s-ci-robot assigned Ankitasw Nov 30, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 30, 2022

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 30, 2022

k8s-ci-robot merged commit aec9433 into kubernetes-sigs:main Nov 30, 2022

k8s-infra-cherrypick-robot mentioned this pull request Nov 30, 2022

[release-2.0] Remove suspend process flow from create ASG #3892

Merged

Remove suspend process flow from create ASG #3864

Remove suspend process flow from create ASG #3864

Uh oh!

Conversation

Skarlso commented Nov 19, 2022

Uh oh!

richardcase commented Nov 19, 2022

Uh oh!

k8s-ci-robot commented Nov 19, 2022

Uh oh!

richardcase commented Nov 19, 2022

Uh oh!

Skarlso commented Nov 19, 2022

Uh oh!

richardcase commented Nov 19, 2022

Uh oh!

richardcase commented Nov 19, 2022

Uh oh!

Skarlso commented Nov 19, 2022

Uh oh!

Skarlso commented Nov 19, 2022

Uh oh!

Skarlso commented Nov 19, 2022

Uh oh!

Skarlso commented Nov 19, 2022

Uh oh!

Skarlso commented Nov 19, 2022

Uh oh!

Skarlso commented Nov 19, 2022

Uh oh!

Skarlso commented Nov 19, 2022

Uh oh!

Skarlso commented Nov 19, 2022

Uh oh!

Skarlso commented Nov 19, 2022

Uh oh!

Skarlso commented Nov 20, 2022

Uh oh!

Skarlso commented Nov 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Skarlso commented Nov 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Skarlso commented Nov 23, 2022

Uh oh!

Skarlso commented Nov 25, 2022

Uh oh!

Ankitasw commented Nov 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Ankitasw commented Nov 30, 2022

Uh oh!

k8s-ci-robot commented Nov 30, 2022

Uh oh!

Ankitasw commented Nov 30, 2022

Uh oh!

k8s-infra-cherrypick-robot commented Nov 30, 2022

Uh oh!

k8s-infra-cherrypick-robot commented Nov 30, 2022

Uh oh!

Uh oh!

Skarlso commented Nov 21, 2022 •

edited

Loading

Skarlso commented Nov 21, 2022 •

edited

Loading

Ankitasw commented Nov 30, 2022 •

edited

Loading