Separate interface for cleanup part of reconciler and Dependent Resources #1035

csviri · 2022-03-15T09:49:36Z

No description provided.

metacosm · 2022-03-15T17:51:51Z

See #922 (comment)

docs/documentation/features.md

...javaoperatorsdk/operator/processing/dependent/kubernetes/CruKubernetesDependentResource.java

...k-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/ControllerConfiguration.java

metacosm · 2022-03-17T13:13:12Z

...amework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/dependent/Cleaner.java

+ */
+@FunctionalInterface
+public interface Cleaner<P extends HasMetadata> {
+  void cleanup(P primary, Context<P> context);


Maybe this should also return a DeleteControl object, but then this doesn't work very well with the method being named cleanup instead of delete…

How that would work with more Dependent Resources returning different Delete Controls?

Good point. Ultimately, though, I think it will make sense for dependent resources to return more information so that the main reconciler can make decisions based on the returned status…

Yes, for now only the state is available, at that time. Although we changed the reconcile() to return ReconcileResult this result is not available. Not sure if I would push this further ATM, but would be make those results also available in the future.

metacosm · 2022-03-17T13:39:11Z

operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/Controller.java

+
+                @Override
+                public DeleteControl execute() {
+                  dependents.cleanup(resource, context);


So the idea is that dependents will get cleaned even if the main reconciler doesn't implement Cleaner? That does seem a little weird since the whole point of implementing Cleaner on Reconciler is to clean dependent resources (since the primary one is already dealt with).

I agree, dependent cleanup should be called only if the reconciler implement Cleaner. Maybe a warning or a controller configuration exception should be raised if dependents and reconciler do not align ?

I think this is kinda a side effect that we don't have this in layers. In case of a special Reconciler this could be done very transparent way. Will create one prototype in the future.

If you take a look from the other point of view, even the reconcile method could be empty when using managed dependent resources, what is already weird. So if we have managed dependent resources where does implement cleaner, the custom resources must have a finalizer otherwise it does not work. And also cleanup needs to be called.

I agree that this is little weird, but the other alternative is to just log a warning, and don't do finalizer and cleanup call, is very error prone in sense that could lead easily to operator implementation that is not correct.

So personally I would go this way, so rather have under the hood something happening that is correct but maybe not intuitive (however nicely documented), and users could then rather check / learn why is this happening. It might cause less headaches then having not cleaned up resources, because Cleaner is not implement on reconciler.

But I don't have actually that strong opinion, just lean to this. Let me know what do you think about this reasoning, we can change in case.

...avaoperatorsdk/operator/processing/dependent/kubernetes/CrudKubernetesDependentResource.java

...java/io/javaoperatorsdk/operator/processing/event/source/controller/ResourceEventFilter.java

…operator-sdk into separate-cleanup

scrocquesel · 2022-03-17T18:08:47Z

operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/Controller.java

@@ -58,40 +52,42 @@ public Controller(Reconciler<P> reconciler,

  @Override
  public DeleteControl cleanup(P resource, Context<P> context) {
-    dependents.cleanup(resource, context);


I noticed recently it was excluded but IMHO having a metric for the total time should exists.
If needed, we can add a per dependent scoped metric.

scrocquesel · 2022-03-17T18:10:26Z

operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/Controller.java

+
+                @Override
+                public DeleteControl execute() {
+                  dependents.cleanup(resource, context);


I agree, dependent cleanup should be called only if the reconciler implement Cleaner. Maybe a warning or a controller configuration exception should be raised if dependents and reconciler do not align ?

...ore/src/main/java/io/javaoperatorsdk/operator/processing/event/ReconciliationDispatcher.java

...samples/common/src/main/java/io/javaoperatorsdk/operator/sample/CustomServiceReconciler.java

scrocquesel · 2022-03-17T18:37:38Z

...amework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/dependent/Cleaner.java

+ * @param <P> primary resource type
+ */
+@FunctionalInterface
+public interface Cleaner<P extends HasMetadata> {


+1 it will also help disambiguate import resolution due to the same class name.

…operator-sdk into separate-cleanup

…tor/processing/dependent/kubernetes/CruKubernetesDependentResource.java Co-authored-by: Chris Laprun <[email protected]>

…tor/processing/event/source/controller/ResourceEventFilter.java Co-authored-by: Chris Laprun <[email protected]>

…tor/processing/event/ReconciliationDispatcher.java Co-authored-by: Sébastien CROCQUESEL <[email protected]>

…operator-sdk into separate-cleanup

metacosm · 2022-03-18T09:05:56Z

More generally speaking, I would have preferred we moved the classes in a different PR so that it's easier to follow and merge other waiting PRs. That said, I've done this before as well so 🤷🏼

sonarqubecloud · 2022-03-18T17:50:17Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
64 Code Smells

40.0% Coverage
0.3% Duplication

metacosm

LGTM

csviri marked this pull request as ready for review March 15, 2022 13:38

csviri self-assigned this Mar 15, 2022

csviri linked an issue Mar 15, 2022 that may be closed by this pull request

Extract Cleanup Interface from Reconciler #922

Closed

csviri changed the title ~~(WIP) Separate interface for cleanup part of reconciler~~ Separate interface for cleanup part of reconciler Mar 15, 2022

csviri requested a review from metacosm March 15, 2022 13:39

csviri added 9 commits March 16, 2022 10:34

feat: separate cleaner interface from reconciler

c01b7d7

fix: builds without tests

9dcfe42

wip

21cc194

fix: unit tests

ff1aa95

fix: integration tests

14ec0d7

fix: integration tests

94b68da

fix: format

6c33fa5

fix: added integration test

8416246

docs: update docs

396b5a1

csviri force-pushed the separate-cleanup branch from ee0c9ed to 396b5a1 Compare March 16, 2022 09:37

csviri added 4 commits March 16, 2022 10:49

fix: format

4831df7

fix: rename

7360e66

feat: separate cleaner interface for DependetResource

aa1a2b6

refactor: dependent resources package structure

6f5dfb8

csviri changed the title ~~Separate interface for cleanup part of reconciler~~ Separate interface for cleanup part of reconciler and Dependent Resources Mar 16, 2022

csviri and others added 7 commits March 16, 2022 16:36

fix: use finalizer if dependent resources are cleaners

b12f728

fix: mysql using the current approach without finalizer

bb1b135

fix: reconciler cleaner IT

49ef6c7

fix: managed dependent resource IT

e41bf14

fix: additional unit tests

1443d1d

refactor: simplify

cf5c716

refactor: avoid looping over dependents twice to check for Cleaner

2563bae

metacosm reviewed Mar 17, 2022

View reviewed changes

fix: class rename

3a68a09

Merge branch 'separate-cleanup' of github.com:java-operator-sdk/java-…

da8424b

…operator-sdk into separate-cleanup

scrocquesel reviewed Mar 17, 2022

View reviewed changes

csviri and others added 6 commits March 18, 2022 08:34

Merge branch 'separate-cleanup' of github.com:java-operator-sdk/java-…

ae75dcf

…operator-sdk into separate-cleanup

Update operator-framework-core/src/main/java/io/javaoperatorsdk/opera…

6935c2e

…tor/processing/dependent/kubernetes/CruKubernetesDependentResource.java Co-authored-by: Chris Laprun <[email protected]>

Update operator-framework-core/src/main/java/io/javaoperatorsdk/opera…

4616838

…tor/processing/event/source/controller/ResourceEventFilter.java Co-authored-by: Chris Laprun <[email protected]>

Update operator-framework-core/src/main/java/io/javaoperatorsdk/opera…

09c20f9

…tor/processing/event/ReconciliationDispatcher.java Co-authored-by: Sébastien CROCQUESEL <[email protected]>

fix: fixes from CR

a024bc3

Merge branch 'separate-cleanup' of github.com:java-operator-sdk/java-…

e3dd811

…operator-sdk into separate-cleanup

csviri added the dependent-resources-epic label Mar 18, 2022

Merge branch 'next' into separate-cleanup

6024cd0

csviri requested a review from metacosm March 18, 2022 15:35

metacosm added 2 commits March 18, 2022 18:39

refactor: minor optimizations

de52b3c

refactor: remove unneeded classes

606480e

metacosm approved these changes Mar 18, 2022

View reviewed changes

metacosm merged commit 5a9bbe2 into next Mar 18, 2022

metacosm deleted the separate-cleanup branch March 18, 2022 19:32

metacosm mentioned this pull request Mar 18, 2022

Re-organize DependentResource implementations #1049

Closed

Separate interface for cleanup part of reconciler and Dependent Resources #1035

Separate interface for cleanup part of reconciler and Dependent Resources #1035

Uh oh!

Conversation

csviri commented Mar 15, 2022

Uh oh!

metacosm commented Mar 15, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

csviri Mar 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

metacosm commented Mar 18, 2022

Uh oh!

sonarqubecloud bot commented Mar 18, 2022

Uh oh!

metacosm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

csviri Mar 18, 2022 •

edited

Loading