Skip to content
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

Multiple controllers handling same descendant kind #169

Closed
adriffaud opened this issue Nov 9, 2022 · 7 comments
Closed

Multiple controllers handling same descendant kind #169

adriffaud opened this issue Nov 9, 2022 · 7 comments

Comments

@adriffaud
Copy link
Contributor

Environment

  • Bonny version: 1.0.0-rc.1
  • Elixir & Erlang/OTP versions (elixir --version): Elixir 1.14.1 / Erlang 25.1.2
  • Operating system: Ubuntu 22.04 on WSL 2

Current behavior

I have an operator with 3 different controllers handling other operators custom resources, config maps, deployments, service accounts, etc.
While trying to migrate to 1.0.0-rc.1 I encounter an issue with the descendants handling.

It seems to me that with the current implementation it is not possible to have multiple controllers called for a same descendant kind as we can only pass one controller along a query, and if we add two similar queries for different controllers, we get this error:

** (EXIT) bad child specification, more than one child specification has the id:
%K8s.Operation{method: :get, verb: :list_all_namespaces, api_version: "v1", name: "ConfigMap",
  data: nil, path_params: [],
  query_params: [labelSelector: %K8s.Selector{match_labels: %{"app.kubernetes.io/managed-by" => "my-operator"},
  match_expressions: []}]}.

Expected behavior

I would expect to be able to have multiple controllers called on an event descendants of the same kind.

In my operator implementation, if the event resource is not one of the CRDs of the operator, I rely on the resource ownerReference to know which controller to call based on the kind. This allows to only have one watcher per kind while dispatching events to the right controllers.

@mruoss
Copy link
Collaborator

mruoss commented Nov 9, 2022

Oooh right. Nice catch. I was confused at first as I am using the term "descendant" for the descending object, i.e. the ones created by a controller. If I get you correctly, you have multiple handlers/controllers for the same query. I think it is clear.

@mruoss
Copy link
Collaborator

mruoss commented Nov 9, 2022

Ooh no now I think I understand. You ARE talking about descendants. So... what stops you from implementing a dispatching controller? This way you wont need to watch the same query multiple times.

@adriffaud
Copy link
Contributor Author

I am indeed talking about objects created by controllers 🙂
And I guess I was too focused on a controller in a kubernetes sense to think of a dispatching controller at the top 😅
I will try that, thanks!

@mruoss
Copy link
Collaborator

mruoss commented Nov 10, 2022

Yeah so with Bonny 1.0 controllers are just Pluggable steps. Which makes the whole thing... well... pluggable. I am thinking about adding something like a Bonny.Pluggable.DispatchToOwner step. But ain't there yet with my thinking process.

Instead of using ownerReferences (which technically is a list anyway) I am thinking about adding a label to descendants like example.com/owning-controller: Elixir.MyOperator.Controller.MyController

@mruoss
Copy link
Collaborator

mruoss commented Nov 11, 2022

@adriffaud On master I have added the Bonny.Pluggable.AddManagedByLabelToDescendants to simplify adding that label to all descendants. You can just plug in into your operator and all registered descendants will have that label.

I'm not proceeding with Bonny.Pluggable.DispatchToOwner as I don't see a simple step handling it all. Instead it would be a "add this step and do that in your controller" etc. So you'll have to implement your dispatching mechanism for now. I see however a neat solution where you would add a "your-operator.com/owning-controller` annotation to all descendants and then use that info to dispatch. But obviously you can also use the ownerReferences field.

@mruoss
Copy link
Collaborator

mruoss commented Nov 12, 2022

@adriffaud can we close this?

@adriffaud
Copy link
Contributor Author

Yes sorry! Thanks for the additions!

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

No branches or pull requests

2 participants