-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
🌱 add MachineFinalizer during machine computation #8463
Conversation
Hi @cahillsf. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
/ok-to-test |
/retest-required |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking into this more, it looks like the finalizer is only owned by the capi-machineset
SSA manager. Because we dont re-add the finalizer in the machine controller, if the finalizer already exists, it looks like that finalizer is not co-owned.
Also, since we dont use SSA when dropping the finalizers (we re-write the full list) I think that allows it to drop the finalizer without any ownership problems.
4adef16
to
cba1d28
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/area machineset
@ykakarap @sbueringer when you have some time, can you make another pass to this PR? This seems a nice improvement, and I think we can start getting this merged while deferring the refactoring of the test suggested in #8463 (comment) to a follow-up PR.... |
Is in my backlog somewhere |
I am good with doing the test refactor in a follow-up PR. The rest of the PR is good to go. /lgtm |
LGTM label has been added. Git tree hash: b4abb738e08ded7fcefe286c4bec9364afc3ab2a
|
@cahillsf are you planning to revive this? |
@enxebre sure thing, had a |
Sorry for the slow response. Just have a neverending backlog and then lost a bit track of this PR. With the suggested fix, tests should work and otherwise the PR looks good to me. Not sure about #8172 (comment) |
thanks @sbueringer! all tests are now passing when you have a chance to take another look |
Thx, change is fine. Please run |
Sorry |
We decided to move forward with this change. More context here: #8172 (comment) @cahillsf sorry again for the delay in getting this reviewed. /lgtm |
LGTM label has been added. Git tree hash: 5669cbbeab1a84ffea670ce4b7ed59db59689fde
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: sbueringer 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 |
What this PR does / why we need it:
This PR adds the
MachineFinalizer
during calculation of theMachine
object so the finalizer is set on object creation.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 #8172