Skip to content

Extract Cleanup Interface from Reconciler #922

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

Closed
csviri opened this issue Feb 10, 2022 · 6 comments · Fixed by #1035
Closed

Extract Cleanup Interface from Reconciler #922

csviri opened this issue Feb 10, 2022 · 6 comments · Fixed by #1035
Assignees
Labels
kind/feature Categorizes issue or PR as related to a new feature.

Comments

@csviri
Copy link
Collaborator

csviri commented Feb 10, 2022

The nice part would be that, this would server also as a switch for the finalizer. So the if the cleanup interface is implemented the finalizers are used otherwise not.
However then we should probably extract the name customization to a new annotation too. Like @Finalizer(name = "my-custom-name") in order to customize the finalizer name.

Original idea from @scrocquesel .

@csviri csviri added the kind/feature Categorizes issue or PR as related to a new feature. label Feb 10, 2022
@csviri csviri self-assigned this Mar 14, 2022
@csviri
Copy link
Collaborator Author

csviri commented Mar 15, 2022

Not sure if we should have a separate finalizer annotation, probably would be good enough to have the finalizerName stay on ControllerConfiguration.

@csviri
Copy link
Collaborator Author

csviri commented Mar 15, 2022

A question is what should happen in case of managed dependent resources. If the Cleaner interface not implemented by the Reconciler. Still the cleanup should be called for the managed dependent resources?

cc @metacosm

@metacosm
Copy link
Collaborator

Not sure if we should have a separate finalizer annotation, probably would be good enough to have the finalizerName stay on ControllerConfiguration.

Maybe the finalizer name should be part of the Cleaner interface, with a default implementation matching what we currently have? This way, everything related would be part of the same interface. I'm not sure that this needs to be part of the annotation anymore since, if you want the behavior, you have to implement the Cleaner interface and you might as well provide the finalizer name there if you don't like the default one?

@metacosm
Copy link
Collaborator

That's a good question… but I'm not sure what the answer should be :)

@csviri
Copy link
Collaborator Author

csviri commented Mar 16, 2022

Not sure if we should have a separate finalizer annotation, probably would be good enough to have the finalizerName stay on ControllerConfiguration.

Maybe the finalizer name should be part of the Cleaner interface, with a default implementation matching what we currently have? This way, everything related would be part of the same interface. I'm not sure that this needs to be part of the annotation anymore since, if you want the behavior, you have to implement the Cleaner interface and you might as well provide the finalizer name there if you don't like the default one?

Yeah, not sure either, I see there three options:

  1. Have a separate annotation to configure cleaner. Like @CleanerConfiguration, what would have an attribute finalizerName. Or just simply having an annotation @Finalizer(name=""). The downside of this is that this will be a separate annotation to handle. The config service would still manage this property?
  2. Have it as now on on @ControllerConfiguration. The is very easy to implement (basically it is), just could be confusing that one can configure it when there will be no annotation added. (However this is also true for 1.)
  3. Have it as a default method on the Cleaner interface. This would work, but would be nicer I think with annotations, since we already do it for other features.

So honestly I would just stay with variant 2. - document it, and change it in following versions based on feedback, especially if it would be confusing.

@csviri
Copy link
Collaborator Author

csviri commented Mar 16, 2022

Regarding the managed dependent resources. What I would do that maybe separate there the cleanup method to new interface too. And change the behavior based on that. That means:
If the reconciler does not implement the Cleaner interface, but there is a dependent resource which does. We will still add the finalizer and call the cleanup on that dependent resource.

@csviri csviri closed this as completed Mar 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature Categorizes issue or PR as related to a new feature.
Projects
None yet
2 participants