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.

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

馃尡 Add ownerRefs to BootstrapConfig/InfraMachinePool in classy Clusters #9389

Merged
merged 1 commit into from
Sep 15, 2023

Conversation

sbueringer
Copy link
Member

Signed-off-by: Stefan B眉ringer buringerst@vmware.com

What this PR does / why we need it:
Thought about this for a bit. I think it makes sense to do the same as in the MD case.

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

Signed-off-by: Stefan B眉ringer buringerst@vmware.com
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/needs-area PR is missing an area label size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Sep 11, 2023
@sbueringer
Copy link
Member Author

/assign @killianmuldoon

@sbueringer
Copy link
Member Author

/test pull-cluster-api-e2e-full-main

Copy link
Contributor

@killianmuldoon killianmuldoon left a comment

Choose a reason for hiding this comment

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

Definitely makes sense to me.

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 11, 2023
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 7e422934c38d9ee08deffa5c3534edbcbaf65136

Copy link
Contributor

@killianmuldoon killianmuldoon left a comment

Choose a reason for hiding this comment

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

/area clusterclass

@k8s-ci-robot k8s-ci-robot added area/clusterclass Issues or PRs related to clusterclass and removed do-not-merge/needs-area PR is missing an area label labels Sep 11, 2023
@sbueringer
Copy link
Member Author

/assign @chrischdi

@killianmuldoon Would it be fine to merge after Christian reviewed it as well? Or do you think we should get Fabrizio's review?

@killianmuldoon
Copy link
Contributor

Or do you think we should get Fabrizio's review?

I'm not fully read into the context here - was there some decision made about this in some of the other MachinePool ClusterClass PRs?

Copy link
Member

@chrischdi chrischdi left a comment

Choose a reason for hiding this comment

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

/lgtm

@killianmuldoon
Copy link
Contributor

One nice addition - I see it's already a follow-up on #5991 would be to add ownerRef validation for MachinePools in e2e.

@sbueringer
Copy link
Member Author

sbueringer commented Sep 11, 2023

I'm not fully read into the context here - was there some decision made about this in some of the other MachinePool ClusterClass PRs?

I intentionally didn't include it in the first PR because I wanted to think about it some more. Apart from that nobody has any MP-specific context :)

I'm basically reversing the temporary decision to diverge from MachineDeployments.

I think this could basically work the same way for MPs as for MDs. We add the Cluster as an ownerRef which means worst case the objects are getting cleaned up when the Cluster gets cleaned up.

Usually they are explicitly deleted by the MP controller during reconcileDelete. It's not perfect, but feels as good/bad as what we have for MDs

Regular cleanup for templates referenced in MDs is a lot more complicated, but I think the fallback cleanup case is here the same for MP/MD.

@fabriziopandini
Copy link
Member

/lgtm

@killianmuldoon
Copy link
Contributor

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: killianmuldoon

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 Sep 15, 2023
@k8s-ci-robot k8s-ci-robot merged commit a151cf9 into kubernetes-sigs:main Sep 15, 2023
25 checks passed
@k8s-ci-robot k8s-ci-robot added this to the v1.6 milestone Sep 15, 2023
@sbueringer sbueringer deleted the pr-mp-add-ownerref branch September 15, 2023 13:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/clusterclass Issues or PRs related to clusterclass 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/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants