controller/engine: use local cache for client read requests #554
Conversation
I haven't had a chance to look closely at this yet, but I wanted to note that there was a reason for this. It's been a while, but I'm pretty sure it had something to do with not being able to stop an individual informer cleanly, at least at the time this code was added. |
sttts
force-pushed
the
sttts-engine-cached-client
branch
5 times, most recently
from
341baf8 to
9746110
Compare
I get that. It's still the case in controller-runtime (not so in client-go). My point in this PR is that there is no need to start a new cache for everything else than the GVK you want to dynamically watch. I.e. this PR gives you composition for caches. |
sttts
force-pushed
the
sttts-engine-cached-client
branch
4 times, most recently
from
5329645 to
55772bd
Compare
| // whose lifecycle is coupled to the controller. The controller is created with | ||
| // the supplied options, and configured with the supplied watches. It is not | ||
| // started yet. | ||
| func (e *Engine) Create(name string, o controller.Options, w ...Watch) (NamedController, error) { |
There was a problem hiding this comment.
Do we need to return an interface here? Could we export nameController and return that type?
It's fine if it helps in some practical way, but I prefer to avoid it if it doesn't. Especially in libraries I find it frustrating as a user when I want to click through in my IDE or Godoc to explore the behaviour of the returned type, only to find it's returning an interface. I then need to go dig into the implementation to find out what type it's actually returning and how it works. (I experience this frequently when using controller-runtime, which returns a lot of interfaces that are satisfied by a single, private type.)
There was a problem hiding this comment.
I guess struct is fine. Let me try.
There was a problem hiding this comment.
Reverted back to interface. The reason is that a struct cannot be mocked.
Signed-off-by: Dr. Stefan Schimanski <stefan.schimanski@upbound.io>
Signed-off-by: Dr. Stefan Schimanski <stefan.schimanski@upbound.io>
sttts
force-pushed
the
sttts-engine-cached-client
branch
from
55772bd to
d77466e
Compare
|
@negz addressed all comments. |
Signed-off-by: Dr. Stefan Schimanski <stefan.schimanski@upbound.io>
Description of your changes
Two changes:
The controller engine starts a new cache to run controllers dynamically against changing GVRs/GVKs. The cache holds informers and the informers back clients. In today's design, every new cache will lead to new informers even though the parent manager might already have an informer. This PR changes that by sharing the parent cache for all GVRs/GVKs, and only route the dynamic GVR to the new one.
This is an optimization in general, but it is especially interesting and necessary in feat[compositions]: realtime compositor – part 2: changes to MRs crossplane#4637 to share MR informers.
This PRs introduces a cache wrapper that can route by GVK. A similar thing exists in controller-runtime, but it is not exposed.
Indexes can only added to an informer before it is started. To allow custom indexes,
Startis split into creation and the actual start of the controller (and its backing cache).I have:
make reviewable testto ensure this PR is ready for review.How has this code been tested
Through crossplane/crossplane#4637 and through unit tests.