Question & Opinion) ResourceController's init(EventSourceManager) method feels a bit vague.

[heesuk-ahn]

HI, all
I was wondering how to register eventSource in Controller, so I looked at the code.

https://github.com/java-operator-sdk/java-operator-sdk/blob/2e8f219437e53b9ca58f146a24397dbf38f0bfb8/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/ResourceController.java#L55

Looking at this method, it looks like I can create and use my own EventSourceManager.

However, in reality, when operator.start() starts, DefaultEventSourceManager is unconditionally injected in configuredController.
(https://github.com/java-operator-sdk/java-operator-sdk/blob/2e8f219437e53b9ca58f146a24397dbf38f0bfb8/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/ConfiguredController.java#L178-L179)

And actually, the user should directly register the EventSource that the user wants to register with the EventSourceManager inside the init method.
like this

  @Override
  public void init(EventSourceManager eventSourceManager) {
    ...
    eventSourceManager.registerEventSource(eventSource);
  }

Personally, I think this might be a bit ambiguous.
It feels like the user can directly inject and use CustomEventSourceManager .

If that's not what you intended, Rather, I think the interface below would be a little clearer.

public interface ResourceController<R extends CustomResource> {
 ...
  default List<EventSource> registerEventSources() {
    return List.of();
  }
}

And by calling this method in the framework, it will be able to directly register it with the event source manager.

Since EventSourceManager is an important component in the framework, it seems to be able to prevent it from being directly exposed to users.

This is just my personal opinion.
Thank you for reading this question :)

[metacosm]

Hi @heesuk-ahn, I agree with you that the intent of the init method is a little unclear. While it indeed is mostly used to register event sources, it also provides a way for your controller to hold onto the EventSourceManager so that it could be queried. To be fair, I'm not sure how common of a use case that might be. Your idea of inverting the intent i.e. to let the controller provide the list of event sources to register instead of passing the manager for the controller to do the registration itself is interesting and something I think we should consider doing.

[csviri]

@heesuk-ahn thx for the feedback!!
I agree too. Linked a PR, which is just a minor improvement on the current approach. The thing is that historically this was a pre-hook to manage possible other initializations before any reconciliation happening. As @metacosm mentioned already, mostly only makes sense for registering event sources AND getting access for CustomResourceEventSource.

To have access to this class is essential, to get access to the caches related to the custom resource, if we could separate/segragate that maybe access some abstracted access in the context for a cache interface? Then this might make sense.

default` List<EventSource> registerEventSources() {
    return List.of();
}

[lburgazzoli]

Looking at the architecture diagram of Kubebuilder https://book.kubebuilder.io/architecture.html, there is a distinction between the Controller and the Reconciler, in the java operator sdk we are conflating the two concept in a single ResourceController interface however I wonder if we should reason about having a clear split between the two concept which IMHO, could make it easier to split the responsibility and to keep a clean interface so:

• Controller : where events source and ilter are registered, and where advanced set-up may happen
• Reconciler : business logic

Creating a controller should not be required so a minimal operator could still be implemented by providing the implementation of a Reconciler. If any advanced set-up is required, then a Controller can be provided i.e:

var operator = new Operator();
operator.register(new MyReconciler(), new MyController())

@Reconciler(controller = MyController.class) 
class MyReconciler {
}

[csviri]

@lburgazzoli I have quite a mixed feelings following this approach. But will dig deeper in the go impl since not that familiar with this part.

In addition to that this segregation does are not really present in the domain language itself. I agree that it might be nice to have a separate place where we can more complex configuration. Although don't think that the current way is particularly ugly.

Also denoted on that architecture diagram, this is quite fundamentally different approach, in our case its that controller seems to be more closer to DefaulEventHandler where the complex execution logic happening.

But excellent point that we should start a discussion around that.

[metacosm]

If we were to go down that line, I think I'd rather put the init method on a separate interface that Controllers could implement if needed to add more advanced behavior.
That said, I think that some of these issues will go away once we start working more closely on dependent resources.

[lburgazzoli]

Also denoted on that architecture diagram, this is quite fundamentally different approach, in our case its that controller seems to be more closer to DefaulEventHandler where the complex execution logic happening.

Maybe we should just rename DefaulEventHandler to Controller :)

[lburgazzoli]

If we were to go down that line, I think I'd rather put the init method on a separate interface that Controllers could implement if needed to add more advanced behavior.

That would be ok.

My main point here is that the boundaries of the ResourceController as today are a little bit blurred and sometimes confusing as example, you can configure some aspects of the controller using annotations or the configuration object and something else with the init method so I think it would be nice if everything could be configured programmatically in one place (I admit I have not yet had a look at the recent changes)

[csviri]

Will update the PR with a segregated interface.

[csviri]

Updated the PR, pls suggest better naming in case :)

[metacosm]

@csviri this is fixed, right?

[heesuk-ahn]

@csviri

Thanks for isolating the init method :)

I think decoupling the init method from the ResourceController is a good first step. :) !

in my opinion,

it will be clear that ResourceController is only responsible for business logic implemented by users as Reconciler.

In fact, if you look at the methods that ResourceController has, there are methods for business processing such as delete and createOrUpdate.

Perhaps a name like ResourceReconciler would be more appropriate than ResourceController. 🤔

The name Controller in operator feels a bit higher-level concept.

[csviri]

An other way would be to provide the event sources when we register the controller. So either a supplier or a list of event sources can be passed. Not sure if this is more elegant. We still need to provide access to the caches of CustomResourceEventSource inside the controller in some way then.

[csviri]

Btw I'm ok to rename DefaultEventHandler to Controller and the current ResourceController to Reconciler. Just to get closer to controller-runtime with the terminology. The ConfiguredController can stay as it is imho.
@metacosm @lburgazzoli what do you think?

[csviri]

We can then rename the methods too accroding to this:
#374

[lburgazzoli]

I'm +1 on this nut don't hold any strong opinion.

[metacosm]

We could indeed rename ResourceController to Reconciler to be more in line with controller-runtime but I think that renaming DefaultEventHandler to Controller would be confusing because the EventHandler doesn't really match well with what the a golang Controller is doing.

[heesuk-ahn]

@metacosm

I agree with your opinion. In my opinion, Controller is a higher level concept than EventHandler.

[csviri]

Well, if you take a look on the implementation it is not far either:
https://github.com/kubernetes-sigs/controller-runtime/blob/master/pkg/internal/controller/controller.go

• Basically manages the queues - in our case it's basically the executor.
• It calls the Reconciler, what we do too, just adding an additional layer by the dispatcher to implement some higher level logic.
• Reconcile handler (https://github.com/kubernetes-sigs/controller-runtime/blob/c73b143dc50358328b9153329b3b570b1a0e5623/pkg/internal/controller/controller.go#L287) also does the same / similar logic that we do after reconcile with re-scheduling (and exception handling)

It's different that in our case the DefaultEventHandler does not owns/manages the watch. I think this makes sense actually to separate. And it's not accessed directly by user in our case, just would created when reconciler is registered - what is not that big deal either.

So actually this looks quite close to me. It definitely meant to be a higher level concept in controller runtime, but at the end it has quite similar responsibilities as DefaultEventHandler.

On the other hand it's an internal class anyways in our case, renaming would be more to our understanding also not for our users. The Operator class has quite clear api IMHO.

[metacosm]

Ultimately, it doesn't matter, indeed since DefaultEventHandler is an internal class. Renaming ResourceController makes sense, the rest, not so much.

[csviri]

Started to working on this, two related questions:

1. Should we rename the createOrUpdateResource to reconcile, and deleteResource to cleanup ? (see linked issues). For now renamed the to plural ( to createOrUpdateResources), because in the current way it feels like we are creating the custom resource itslef.

2. We have also a @Controller annotation, which actually contains more controller related configurations, not just the reconciler. Just feels weird putting this annotation to reconciler class. Would it be an idea to rename it to @ControllerConfiguration?

[lburgazzoli]

Ultimately, it doesn't matter, indeed since DefaultEventHandler is an internal class. Renaming ResourceController makes sense, the rest, not so much.

hijacking a little the discussion but here we could think to rename it to DefaultEventHandler to EventHandler as it does not seems to me we support pluggable EventHandler (I think there are other cases where we have an interface whit a single and not user customizable interface).

[csviri]

hijacking a little the discussion but here we could think to rename it to DefaultEventHandler to EventHandler as it does not seems to me we support pluggable EventHandler (I think there are other cases where we have an interface whit a single and not user customizable interface).

Will do it, just we have an EventHandler interface, and here the purpose with that is not to have a replaceable implementation but to provide an interface to the users to work with. This is used in the EventSources, so if somebody propagates the event it does it agains the EventHandler interface not to the class, which has other public methods.

So we have to find a good naming to DefaultEventHandler, maybe EventProcessor ?

[csviri]

For now renamed DefaultEventHandler to EventProcessor in the PR. Naming is hard :) Propose anything better pls!

[csviri]

Back to the original topic. We have to bear in mind that the EventSources needs to accessible from the (now) Reconciler, since the caching states of the related reasources are happening in the EventSources.

Given this constraints it's hard to separate these two completely (The event sources and the reconcilier), not even if we have a completely separate caching layer under the hood of the kubernetes client, since there can be non K8S dependent resources.

So either we:

1. stick with the approach we have now - having the initialization interface, so on class level there can be a reference on the related event sources and read during the reconciliation. Or:
2. put the EventSourceManager (or simpler version of it, just to read event sources or their caches) into the Context of reconcilier methods? Which is more ugly IMHO. On the other hand this opens paths, to declaratively define event sources with annotations for example.

[csviri]

Created a separate issues where described the current changes, please take a look, we can continue the discussion there:
#655

will close this issues for now.