Skip to content

Improvement/handle namespace deletion on finalizer gracefully #1988

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

Conversation

Donnerbart
Copy link
Contributor

Fixes #1987

@openshift-ci openshift-ci bot requested review from adam-sandor and metacosm July 31, 2023 14:51
@csviri
Copy link
Collaborator

csviri commented Aug 1, 2023

Hi @Donnerbart , thx for the PR, pls can you run mvn clean install on the code and commit the changes, it will format the code.

@csviri
Copy link
Collaborator

csviri commented Aug 1, 2023

What does not adds up to me is, if the update failed, how the resources was deleted. In other words if removal of finalizer failed, how is the resource null, after getting it, should not be deleted. But other than that this look reasonable.

@Donnerbart Donnerbart force-pushed the improvement/handle-namespace-deletion-on-finalizer-gracefully branch from 235907d to 921c1e9 Compare August 1, 2023 10:37
@Donnerbart
Copy link
Contributor Author

Yeah, I tried to reproduce this several times, but couldn't trigger this exception again.

I think it might be a missing permission that's being removed by deleting the namespace, so the operator fails to remove the finalizer and then cannot retrieve the custom resource again.

@csviri
Copy link
Collaborator

csviri commented Aug 1, 2023

Yeah, I tried to reproduce this several times, but couldn't trigger this exception again.

I think it might be a missing permission that's being removed by deleting the namespace, so the operator fails to remove the finalizer and then cannot retrieve the custom resource again.

Makes, sense but in that case we should rather not retry, well not sure... this is quite a corner case.

@csviri
Copy link
Collaborator

csviri commented Aug 1, 2023

Probably would be good do a warning log there, if this happens.

@Donnerbart Donnerbart force-pushed the improvement/handle-namespace-deletion-on-finalizer-gracefully branch from 921c1e9 to c461a84 Compare August 1, 2023 11:07
@Donnerbart
Copy link
Contributor Author

I agree that this is probably a very rare corner case.

I've added a warning for the null case.

Copy link
Collaborator

@csviri csviri left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

@csviri csviri merged commit d4201fe into operator-framework:main Aug 1, 2023
@Donnerbart Donnerbart deleted the improvement/handle-namespace-deletion-on-finalizer-gracefully branch August 1, 2023 14:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

NullPointerException on finalizer removal when namespace is deleted concurrently
2 participants