Why DelegatingClient is needed?

[M00nF1sh]

From the code,
the Manager.GetClient returns an DelegatingClient which read from cache for structured object, but read directly for unstructured object.

This forbids us to use the client before controller loops(since cache needs to be synced). (Our use case is load configurations from configMap before controller loops).

From my perspective, users should use manager.GetCache if they need caching behavior(and we can do the delegation there to read directly for unstructured object).
For manager.GetClient, it's better to let it always read directly without cache.

[DirectXMan12]

We're trying to make it as easy as possible to use the "correct" Kubernetes pattern. So you don't have to manage two separate objects -- you just get the correct behavior off the bat. This does mean that for things like loading configuration from specific objects, you do need to manually instantiate the client.

You do raise a valid point here, and it's difficult to balance the "can we make it really easy so the user just doesn't have to care" and "is this magic going to trip people up". Did this trip you up or cause significant confusion? I'm happy to have a conversation on this feature, and/or to see if we can make the pitfalls of the default client clearer. I definitely don't want using controller-runtime to be confusing or painful :-)

[DirectXMan12]

/kind design

[M00nF1sh]

the major confusing part came from manager exposed both getClient & getCache(both of them provided reader interface), which make me think naturally that one provides uncached behavior and one provides cached behavior.
If we can merge them together, then it's less confusing.

Also, i think loading configuration from configMaps before controller loops seems a pretty general use case. Maybe we can make waitForCacheSync happen inside manager.New(or another explicit method call) instead of manager.Start? ( i think it's not ideal for users to manually init an k8s client just to fulfill this use case)

[DirectXMan12]

@droot thoughts here?

Yeah, I definitely see how it's a common use case. We can prob make the naming a bit clearer, and introducing an explicit GetRemoteOnlyClient helper (terrible name, just a placeholder) might make this clearer too.

[DirectXMan12]

The problem with starting the caches in manager.New is that it makes it impossible to safely add indexes (at least, how we do it now, IIRC), which can be super useful in clearing up bits of code. It's a bit of an advanced use case, but nice to have.

[M00nF1sh]

another tricky solution i can think of is make the manager.GetClient read directly at first, and change to delegated cache implementation once cache is synced 😸 . (maybe via a flag in the delegating client). Then all use case are satisfied and transparent to users.

[DirectXMan12]

So, the other "nice" thing that the reader does it that it auto-adds new watches if you request a new object. This might be too magic, but so far it seems to have worked out well. Your suggestion, while not necessarily bad in the absence of this logic, cause the behavior of the client to become inconsistent when paired with the auto-add.

[M00nF1sh]

Just checked the auto-add implementation. I don't think the behavior of client will be inconsistent.

Since auto-add behavior is not necessary for Read/List operations from user's perspective.(It's an implementation detail needed when read/list is performed by cache).

And when they need watch behavior, they have to use cache.GetInformers, which setup auto-add correctly. 😸

[shawn-hurley]

This might be a really bad idea but what if before starting the controllers, we use a direct client, and then during start change the client to a cache client.

Allows for the setup and then does the right things for the controllers?

[M00nF1sh]

happy to contribute if we are going for the solution of switch read client 😸 .

But the problem of both manager.GetClient and manager.GetCache exposed reader still exists. I'm a big fan of the python zen There should be one-- and preferably only one --obvious way to do it 😄

[shawn-hurley]

I am going to take the other side of the argument here. I think that exposing the cache helps advanced users, and making the client “smart” and have a default that is to use the cache on reads makes sense 90% of the time. This reduces the cognitive overhead for new users and helps them get the right outcome.

What about something like for the status writer. A client.NoCache().Get(...)?

[droot]

the major confusing part came from manager exposed both getClient & getCache(both of them provided reader interface), which make me think naturally that one provides uncached behavior and one provides cached behavior.

This is good feedback. Now I read it, it is confusing.

For our next major revision, we can probably try to do the following:

• Remove GetCache API from manager
• Introduce method on Client interface NoCache (as @shawn-hurley suggested above)

[shawn-hurley]

@droot Where would I get these methods: https://github.com/kubernetes-sigs/controller-runtime/blob/master/pkg/cache/cache.go#L51-L55 then if we remove the GetCache() interface?

Maybe a GetInformer() method is added and an Informer interface is exposed?