Introduce `ConfiguredController` concept, decouple Metrics code from business logic #517

metacosm · 2021-08-26T10:29:42Z

The idea is to expand on the ControllerRef class to associate a controller with its configuration and propagate this entity instead of passing the controller, the configuration and very often the kube client to each object that needs that information.
See commit message on 1c47ecf for more details.

I've also taken the opportunity to clean up things from the Metrics class so that it's better decoupled from the business logic.

Fixes #496.

operator-framework-core/src/main/java/io/javaoperatorsdk/operator/Operator.java

csviri · 2021-08-29T20:18:11Z

...ramework-core/src/main/java/io/javaoperatorsdk/operator/processing/ConfiguredController.java

+import io.javaoperatorsdk.operator.processing.event.DefaultEventSourceManager;
+import io.javaoperatorsdk.operator.processing.event.EventSourceManager;
+
+public class ConfiguredController<R extends CustomResource<?, ?>> implements ResourceController<R>,


I don't think this should implement ResourceController, couldn't we just get the controller when it's needed instead of delegating and implementing this interface. I would rather see this class which just couples these objects. Implementing this interface does not feels right, it gives this class a mixed responsibility.

In other words this principle loosly applies (in regard of interfaces):
https://en.wikipedia.org/wiki/Composition_over_inheritance

We're using composition here and to me, it makes sense to have that class implement ResourceController. We're not inheriting anything, just stating that this class behaves for all intents and purposes like a ResourceController. What are the disadvantages of having the class extend the interface? Implementing the interface allows to pass the object around anywhere a ResourceController is used currently which simplifies the current code, imo. The whole idea of this class is that it is a ResourceController that also manages its configuration (instead of asking a ConfigurationService for it).

This is probably more a philosphical question, so also a point of view how we are looking at this class. So if we take a look on it as a "proxy" class then yes you are right there is no harm to implement it. If we take a look on this as a wrapper that just collects some related classes then it is a smell.
So it's all about how we express notions in OOP. But actually you are probably right that we can look on this as a "proxy" class.

csviri · 2021-08-29T20:25:22Z

operator-framework-core/src/main/java/io/javaoperatorsdk/operator/Operator.java

-    this.controllers = new ArrayList<>();
-    this.started = false;
-    this.metrics = metrics;
+    DefaultEventHandler.setEventMonitor(new EventMonitor() {


I think this would be better to have these metrics per controller. (Something to think about in the future)

Might make sense, indeed, though we can side-step the issue by adding the controller's name to the counter tags so that we can only have one handler and still be able to filter per controller. Having a different monitor per controller would only make sense if we wanted to track different metrics per controller (which we indeed might).

csviri · 2021-08-29T20:35:44Z

...re/src/main/java/io/javaoperatorsdk/operator/processing/event/DefaultEventSourceManager.java

-    registerEventSource(
-        CUSTOM_RESOURCE_EVENT_SOURCE_NAME, new CustomResourceEventSource<>(client, configuration));
+  public DefaultEventSourceManager(ConfiguredController<R> controller) {
+    this(new DefaultEventHandler<>(controller), true);


This is just a remark for the future, maybe to discuss separately. It's little bit messy how we wire up and/or instantiate these object DefaultEventHandler here and the DefaultEventSourceManager in the Configured controller. Those should be probable more explicit somewhere, making the code more readable, since these two classes are quite fundamental. Now it's quite hard to trace back, what is instantiated where.

Yes, this still needs to be cleaned up but this PR is already doing too much as it stands so that work should be done in subsequent PRs.

csviri · 2021-08-29T20:38:23Z

Made some comments, but in general I like the idea of this ConfiguredController, so we break down the large chunk of code from Operator class to separate parts. These classes in it are feels cohesive.

lburgazzoli · 2021-08-31T05:55:59Z

operator-framework-core/src/main/java/io/javaoperatorsdk/operator/Operator.java

@@ -62,63 +64,61 @@ public ConfigurationService getConfigurationService() {
   */
  @SuppressWarnings("unchecked")
  public void start() {
-    synchronized (lock) {


don't we need to keep this lock ? if sop is invoked on a separate thread it may not perform any cleanup but controller may be running

Do you have examples / use cases where close might be invoked asynchronously?

🤔 Actually, thinking about it some more, if a shutdown hook is installed, then, indeed, close will be called from a different thread so we do need to re-instate synchronization.

But that would be still just 1 thread. Is there still a situation where can be a race condition? By definition this should not be called by multiple threads.

it depends who invokes the shutdown process, i.e. in spring & quarkus this is done by the underlying runtime ad we don't know if this happen in the same thread or not.

If the operator app is stopped during the start process then it's possible that there would two different threads accessing the started and controllers fields at the same time.

That is true.

lburgazzoli · 2021-08-31T06:25:33Z

...re/src/main/java/io/javaoperatorsdk/operator/api/config/AbstractControllerConfiguration.java

-public abstract class AbstractControllerConfiguration<R extends CustomResource>
+import io.fabric8.kubernetes.client.CustomResource;
+
+public abstract class AbstractControllerConfiguration<R extends CustomResource<?, ?>>


wonder if this need to be an abstract class or just have some more params to set the crd class name and the configuration service

Maybe… the reason why it's abstract is that it was needed like that at some point for the Quarkus extension. Maybe it doesn't need to be abstract anymore. Let me check.

It appears it's not needed anymore. I have local changes to this PR to account for changes made for #499 that I intend to merge first. I will make the changes to this PR after the other one is merged.

🤔 Cleaning this up properly, though, would require a breaking API change because, obviously, the class shouldn't be still named AbstractControllerConfiguration if it's not abstract anymore. Then again #499 is also breaking the API so… 🤷🏼

I fixed the backwards compatibility issue but name has to stay as-is for now.

metacosm · 2021-08-31T08:15:37Z

On a more general note, the integration tests seem to be particularly flaky with this PR so there may something more to it than just flakiness. 😢

Introduced ConfiguredController to gather in one entity ResourceController, its associated configuration and Kubernetes client so that they can be passed around more easily in an always-associated manner (as, right now, configuration can be disassociated from the controller). Another aspect is that it allows us to simplify the management of the lifecycle as ConfiguredControllers can be started/stopped and the Operator doesn't need to bother about the details.

A global EventMonitor is set when the Operator is created based on the Metrics instance provided by the ConfigurationService.

This is meant to avoid diamond inheritance issues where we could have conflicting version of the method if a class implements several interfaces with a close method.

Introduce ControllerExecution interface to encapsulate a controller call that can be timed. Move logic of call to ConfiguredController where it's more logical so that Metrics class doesn't have any business logic anymore.

Note that this shouldn't happen during normal execution but can happen during tests.

Obviously, it should be renamed to something else since it's not abstract anymore but doing so would break backwards compatibility so best left for some later time.

metacosm · 2021-08-31T13:06:48Z

@csviri can we merge this?

csviri · 2021-08-31T14:00:18Z

@csviri can we merge this?

Yes, LGTM

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

It seems like using replaceStatus is causing issues with integration tests. patchStatus would work but this will be discussed in a subsequent issue / PR.

metacosm self-assigned this Aug 26, 2021

metacosm requested review from csviri and adam-sandor August 26, 2021 10:29

metacosm mentioned this pull request Aug 26, 2021

Operator does not receive any events after startup #514

Closed

snowdrop-bot mentioned this pull request Aug 26, 2021

Introduce ConfiguredController concept snowdrop-zen/java-operator-sdk#39

Closed

metacosm force-pushed the configured-controller branch from 56fcc0a to ccfd6c7 Compare August 26, 2021 20:53

attilameszaros-form3 reviewed Aug 27, 2021

View reviewed changes

operator-framework-core/src/main/java/io/javaoperatorsdk/operator/Operator.java Outdated Show resolved Hide resolved

metacosm changed the title ~~Introduce ConfiguredController concept~~ Introduce ConfiguredController concept, decouple Metrics code from business logic Aug 27, 2021

metacosm marked this pull request as ready for review August 27, 2021 12:06

csviri reviewed Aug 29, 2021

View reviewed changes

operator-framework-core/src/main/java/io/javaoperatorsdk/operator/Operator.java Outdated Show resolved Hide resolved

csviri reviewed Aug 29, 2021

View reviewed changes

operator-framework-core/src/main/java/io/javaoperatorsdk/operator/Operator.java Outdated Show resolved Hide resolved

csviri reviewed Aug 29, 2021

View reviewed changes

lburgazzoli reviewed Aug 31, 2021

View reviewed changes

metacosm added 12 commits August 31, 2021 11:35

fix: use resources method instead of deprecated customResources

7feb861

feat: add direct access to custom resource client

d105dd6

refactor: propagate use of ConfiguredController, generify some

b954a95

fix: avoid crash if for some reason the build time is unavailable

aa6528f

feat: use ConfiguredController

992a4fa

refactor: use non-deprecated replaceStatus method

d4e88e7

refactor: more generification

f63ada0

chore: clean up unused code

d3331fe

chore: use resources method instead of deprecated customResources

4fadeaa

feat: add EventMonitor to decouple metrics from DefaultEventHandler

bcb3dc5

A global EventMonitor is set when the Operator is created based on the Metrics instance provided by the ConfigurationService.

fix: use proper close method signature

b391bf7

This is meant to avoid diamond inheritance issues where we could have conflicting version of the method if a class implements several interfaces with a close method.

metacosm added 10 commits August 31, 2021 11:35

refactor: use more appropriate method names

4d0514b

refactor: simplify test-specific constructor

d2b3c79

refactor: use ReentrantLock instead of locking on Object

d811a17

feat: improve error logging when cluster is not online

a1fcd5d

refactor: generify timing of controller execution

f988d41

Introduce ControllerExecution interface to encapsulate a controller call that can be timed. Move logic of call to ConfiguredController where it's more logical so that Metrics class doesn't have any business logic anymore.

chore: remove synchronization altogether

be95a3a

fix: avoid NPE in ConfiguredController.close if manager is not set

8e26e84

Note that this shouldn't happen during normal execution but can happen during tests.

fix: improper TestConfiguration implementation

843f44b

feat: make AbstractControllerConfiguration directly usable

e34bac9

Obviously, it should be renamed to something else since it's not abstract anymore but doing so would break backwards compatibility so best left for some later time.

fix: add ControllerManager to handle controllers and status atomically

d3920bc

metacosm force-pushed the configured-controller branch from bf2af63 to d3920bc Compare August 31, 2021 09:56

csviri approved these changes Aug 31, 2021

View reviewed changes

csviri reviewed Aug 31, 2021

View reviewed changes

...tor-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/EventDispatcher.java Outdated Show resolved Hide resolved

metacosm force-pushed the configured-controller branch from 096c41b to 8b3e22c Compare August 31, 2021 15:42

fix: revert to use updateStatus for now

fcef377

It seems like using replaceStatus is causing issues with integration tests. patchStatus would work but this will be discussed in a subsequent issue / PR.

metacosm force-pushed the configured-controller branch from 8b3e22c to fcef377 Compare August 31, 2021 15:43

metacosm merged commit aff192e into master Aug 31, 2021

metacosm deleted the configured-controller branch August 31, 2021 16:04

metacosm mentioned this pull request Aug 31, 2021

Continue improvements on event handling #428

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce `ConfiguredController` concept, decouple Metrics code from business logic #517

Introduce `ConfiguredController` concept, decouple Metrics code from business logic #517

metacosm commented Aug 26, 2021 •

edited

Loading

csviri Aug 29, 2021 •

edited

Loading

metacosm Aug 30, 2021

csviri Aug 30, 2021 •

edited

Loading

csviri Aug 29, 2021

metacosm Aug 30, 2021

csviri Aug 29, 2021

metacosm Aug 30, 2021

csviri commented Aug 29, 2021

lburgazzoli Aug 31, 2021

metacosm Aug 31, 2021

metacosm Aug 31, 2021

attilameszaros-form3 Aug 31, 2021

lburgazzoli Aug 31, 2021

metacosm Aug 31, 2021

csviri Aug 31, 2021

lburgazzoli Aug 31, 2021

metacosm Aug 31, 2021

metacosm Aug 31, 2021

metacosm Aug 31, 2021 •

edited

Loading

metacosm Aug 31, 2021

metacosm commented Aug 31, 2021

metacosm commented Aug 31, 2021

csviri commented Aug 31, 2021

Introduce ConfiguredController concept, decouple Metrics code from business logic #517

Introduce ConfiguredController concept, decouple Metrics code from business logic #517

Conversation

metacosm commented Aug 26, 2021 • edited Loading

csviri Aug 29, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

csviri Aug 30, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

csviri commented Aug 29, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

metacosm Aug 31, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

metacosm commented Aug 31, 2021

metacosm commented Aug 31, 2021

csviri commented Aug 31, 2021

Introduce `ConfiguredController` concept, decouple Metrics code from business logic #517

Introduce `ConfiguredController` concept, decouple Metrics code from business logic #517

metacosm commented Aug 26, 2021 •

edited

Loading

csviri Aug 29, 2021 •

edited

Loading

csviri Aug 30, 2021 •

edited

Loading

metacosm Aug 31, 2021 •

edited

Loading