Controller framework #5270
Controller framework #5270
Conversation
|
@lavalamp about to board a plane but i've fetched your code and will take a look. |
|
@lavalamp made a pass at reviewing this- so far, so good. It's coming together nicely. |
|
I focused on the controller framework commit and thought it looked like a nice improvement over what we have today. I am in the midst of writing a controller for namespace termination, so any ETA on when you think you want to mark this Ready? I assume the move to migrate existing controllers will be in subsequent PRs. |
Yeah, sorry, I got preempted by #5336. I hope to get that working and this working tomorrow... |
|
No rush. I have something working that is consistent with others. I just liked this current controller :-) Sent from my iPhone
|
|
I split off #5473 with changes that should be good to go, and updated this PR-- mostly the only new thing is a fake source so that I'll be able to test this thing. (should be very useful for anyone testing a controller, actually) |
|
Also relevant: #5548. |
|
@lavalamp Given that the other PR merged, is this PR still relevant? |
|
Yes, this has more stuff which is WIP. Hope to update this by EOD. |
|
OK, this is mostly done and ready for a look. The Example() gets us to > 80% coverage but I will look at testing a few more cases. |
|
@derekwaynecarr would you like to take a look? |
|
|
||
| // Let's implement a simple controller that just deletes | ||
| // everything that comes in. | ||
| Process: func(obj interface{}) error { |
There was a problem hiding this comment.
So if I want to use this in kube2sky, for example, do I literally just rip off most of this function? That still feels like a lot of boilerplate - easy to get wrong.
Can it be reduced to "call one function, passing a set of function pointers (or an interface), and have my functions called on add/delete/update" ?
There was a problem hiding this comment.
Yeah, this is a good point -- I can make this even easier for the common case.
|
looking, will also look from comments from @ironcladlou , @pmorie |
| // The queue for your objects; either a cache.FIFO or | ||
| // a cache.DeltaFIFO. Your Process() function should accept | ||
| // the output of this Oueue's Pop() method. | ||
| cache.Queue |
There was a problem hiding this comment.
Is there ever a need for a controller to act on two objects instead of just one?
I thought I had use for two until @ironcladlou talked me into making a custom cache.Store and keep the controller processing just one object.
There was a problem hiding this comment.
I had a case for more than one, but it was to build a in-memory cache, and does not need to be solved by this framework.
There was a problem hiding this comment.
Yeah, I think most controllers have one object as a source. Multi-source controllers are out of scope for this framework.
There was a problem hiding this comment.
Endpoint controller.
a) currently identified as a big problem
b) does not watch at all
c) really needs a local cache of all pods and all services
|
@lavalamp I am fine with the PR in current form if you want to merge and refine in a follow-on. |
|
Yeah, let's get this in-- I want to use it to fix #6524. I will address Tim's comment in a followup, and add an error handler in another followup. |
|
SGTM, merging |
DeltaFIFO has tests and is basically ready to go, I can send it in its own PR if others agree. The controller framework is not finished yet.
Addresses #4877