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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

馃悰 Persist AWSCluster/AWSMachine finalizers immediately to prevent orphaned AWS resources #1455

Merged
merged 1 commit into from
Jan 17, 2020
Merged

馃悰 Persist AWSCluster/AWSMachine finalizers immediately to prevent orphaned AWS resources #1455

merged 1 commit into from
Jan 17, 2020

Conversation

aaroniscode
Copy link
Contributor

What this PR does / why we need it:
If Cluster/AWSCluster resources are created and deleted relatively quickly (seconds up to a minute or so) the AWS resources in the VPC are orphaned as the AWSCluster is deleted without destroying the resources.

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 #1454

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Dec 28, 2019
@k8s-ci-robot k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Dec 28, 2019
vincepri
vincepri reviewed Dec 28, 2019
View reviewed changes
pkg/cloud/scope/cluster.go Outdated Show resolved Hide resolved
controllers/awscluster_controller.go Outdated Show resolved Hide resolved
@aaroniscode
Copy link
Contributor Author

I believe all review comments are resolved.

@k8s-ci-robot k8s-ci-robot requested a review from ncdc January 6, 2020 16:37
@ncdc
Copy link
Contributor

ncdc commented Jan 14, 2020

Should we do the same thing for AWSMachine?

@aaroniscode
Copy link
Contributor Author

aaroniscode commented Jan 14, 2020 •
edited

Should we do the same thing for AWSMachine?

AWSMachine does not have the same issue. Right after the finalizer is added, it checks for the Bootstrap data, which doesn't yet exist as it needs to wait for the bootstrap controller and it returns and persists the finalizer. So the finalizer is persisted before the EC2 instance is created.

...

But just got to thinking ... if the capa controller was down for some reason when the AWSMachine was created ... is it possible the bootstrap data could be created before the first AWSMachine reconciliation ran? If so ... then it's possible a delete could come in during EC2 creation and delete the AWSMachine before the EC2 creation is finished and the finalizer is added.

@ncdc
Copy link
Contributor

ncdc commented Jan 14, 2020

Better to be safe, I think

@aaroniscode
Copy link
Contributor Author

Better to be safe, I think

ok, thanks @ncdc ... I'll add it to AWSMachine as well. Assuming we want it in this PR. I'll add it and force-push. Just let me know if you want it in a separate PR

@ncdc
Copy link
Contributor

ncdc commented Jan 14, 2020

This one's good, thanks.

@randomvariable
Copy link
Member

/held
Awaiting one more addition.

@aaroniscode remove hold when ready.

@ncdc
Copy link
Contributor

ncdc commented Jan 15, 2020

/hold
for realz 馃槢

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 15, 2020
@aaroniscode aaroniscode changed the title 馃悰 Prevent orphaned AWS resources if AWSCluster is deleted shortly after creation 馃悰 Persist AWSCluster/AWSMachine finalizers immediately to prevent orphaned AWS resources Jan 16, 2020
@aaroniscode
Copy link
Contributor Author

aaroniscode commented Jan 16, 2020 •
edited

updated the PR to include AWSMachine, so both now immediately persist the finalizer after adding

/unhold

edit ... somehow missed the edits in the last force push. it's there now.

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 16, 2020
@aaroniscode
Copy link
Contributor Author

not sure why this test failed ... i don't see an obvious reason in the code going to try to re-run

/retest

@k8s-ci-robot 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 Jan 16, 2020
@aaroniscode
Copy link
Contributor Author

not sure why this test failed ... i don't see an obvious reason in the code going to try to re-run

so it turned out that having the AWSMachine controller call the patchHelper after adding the finalizer caused about half of the controller tests to fail. This was because the fake client couldn't find the AWSMachine to patch it. So I added a fake AWSMachine to the fake client and the tests are working again.

@aaroniscode
Copy link
Contributor Author

ready for review again

@ncdc
Copy link
Contributor

ncdc commented Jan 17, 2020

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aaroniscode, ncdc

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:

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 17, 2020
@aaroniscode
update the cluster scope after adding the finalizer
c48dd60 
this prevents a delete occuring before the finalizer has been added
and orphaning AWS resources
vincepri
vincepri approved these changes Jan 17, 2020
View reviewed changes
Copy link
Member

@vincepri vincepri left a comment

Choose a reason for hiding this comment

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

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 17, 2020
@ncdc
Copy link
Contributor

ncdc commented Jan 17, 2020

/lgtm

@k8s-ci-robot k8s-ci-robot merged commit 7734c5c into kubernetes-sigs:master Jan 17, 2020
@aaroniscode aaroniscode deleted the update_clusterscope branch January 18, 2020 03:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ncdc ncdc ncdc left review comments

@vincepri vincepri vincepri approved these changes

@detiber detiber Awaiting requested review from detiber

@justinsb justinsb Awaiting requested review from justinsb

Assignees

@ncdc ncdc

@vincepri vincepri

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Creating and deleting Cluster/AWSCluster quickly results orphaned AWS resources
5 participants
@aaroniscode @ncdc @randomvariable @k8s-ci-robot @vincepri