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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: EnsureFinalizers #1828

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

acpana
Copy link
Collaborator

@acpana acpana commented May 16, 2024

WIP

Do we want to have the lifecycle handler update the api server with the finalizers?

Signed-off-by: Alex Pana <8968914+acpana@users.noreply.github.com>
Signed-off-by: Alex Pana <8968914+acpana@users.noreply.github.com>
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from acpana. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found 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

@maqiuyujoyce
Copy link
Collaborator

Copying my thoughts over here for posterity:

I think whether we want to have the lifecycle handler update the api server with the finalizers is more of a design question: Should we add the finalizer to the resource asap during the reconciliation, or are we okay to wait?

To prevent the resource from being deleted unexpectedly, we should add the finalizer at the beginning of the reconciliation (so something similar to #1179). And for compatibility purpose, we should probably do it anyways.

We can also choose to not updating the apiserver until the reconciliation is completed if we have solid reasons why that's better - This is the current behavior in the direct controller though I'm actually a bit concerned about it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants