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

Support adding new watches dynamically at runtime #540

Closed
ncdc opened this issue Jul 25, 2019 · 9 comments · Fixed by #542
Closed

Support adding new watches dynamically at runtime #540

ncdc opened this issue Jul 25, 2019 · 9 comments · Fixed by #542

Comments

@ncdc
Copy link
Contributor

ncdc commented Jul 25, 2019

This feature was added before (and requested in #246), but the updated Manager & Builder patterns don't support this any more because they take care of creating a Controller instead of having user-written code do it, and the Controller isn't accessible to your reconciler.

There was a discussion about this in Slack (https://kubernetes.slack.com/archives/CAR30FCJZ/p1561759175084100). I'm thinking we could modify the inject package like so (names TBD - I know they don't follow the current pattern 100% but we need to scope down the Controller interface to avoid import cycles):

type Watcher interface {
  Watch(src source.Source, eventhandler handler.EventHandler, predicates ...predicate.Predicate) error
}

type WatcherInjector interface {
  InjectWatcher(watcher Watcher)
}

Do you think something like this would work @DirectXMan12?

@ncdc
Copy link
Contributor Author

ncdc commented Jul 25, 2019

cc @vincepri

@DirectXMan12
Copy link
Contributor

We're trending towards not adding more injectors when they're not strictly needed by internals -- most cases are more obviously solved by structuring your application so that you can pass them to your reconcilers directly (like we do in KB v2).

Thus, I'd prefer to see a solution to this that makes it possible to get a handle to the underlying controller with the builder somehow. If we can't figure out how to do that cleanly, an injector is fine, but I'd prefer the more direct approach.

@ncdc
Copy link
Contributor Author

ncdc commented Jul 29, 2019

Ok, I'll do some brainstorming to see what I can come up with.

@ncdc
Copy link
Contributor Author

ncdc commented Jul 29, 2019

@DirectXMan12 what if we modify builder.Complete() to return the controller it just created? Then the user can do whatever they want with it (including passing it to their reconciler).

@ncdc
Copy link
Contributor Author

ncdc commented Jul 29, 2019

Or expose a GetController() method on Builder

@vincepri
Copy link
Member

+1 On having either a method to get the controller or Complete() to return the created controller and an error. Today kubebuilder invokes ctrl.NewControllerManagedBy(...) which usually, I'd think it's going to return a new Controller object that I can use.

@DirectXMan12
Copy link
Contributor

I'd prefer not Complete (just to avoid breakage of a commonly used function, if we can), but I'm on board with a similar method. Since Build has been deprecated for a couple releases now, we could repurpose that (although it could lead to breakage) or come up with a new method.

@ncdc
Copy link
Contributor Author

ncdc commented Jul 29, 2019

I don't think we'd want 2 different ways to complete/build - 1 that returns a controller, and 1 that doesn't - would we?

@ncdc
Copy link
Contributor Author

ncdc commented Jul 30, 2019

Summarizing our discussion from Slack yesterday (https://kubernetes.slack.com/archives/CAR30FCJZ/p1564433195123400), we're going to un-deprecate Build and have it return the Controller instead of the Manager. We're also going to remove the logic in the builder that creates a manager for you if you don't pass one in.

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

Successfully merging a pull request may close this issue.

3 participants