Define a supported way of setting a cache on source.Kind

[alvaroaleman]

In order to be able to write a controller that has informers in multiple clusters, one has to inject a cache into source.Kind. Options to do this include:

• Just call kind.InjectCache, it has a nilcheck that will prevent the injectCache call from the controller to overwrite the cache. However this behavior doesn't have tests and may go away in the future
• Write a wrapper around kind.Source that implements InjectCache with the nilcheck
• Write a wrapper around kind.Source that does not implement inject.Cache

We should agree on one option, declare it the supported way to build multicluster controllers and probably document it.

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

[loewenstein]

We are interested to use controller-runtime in a scenario involving multiple api-servers/clusters (see #745) and this issue looks like a first step towards multi-cluster support.

Is there anything we could do to drive this forward?

[detiber]

/remove-lifecycle stale

[DirectXMan12]

Is there anything we could do to drive this forward?

Mainly just figure out which option looks the best here with a little proposal (nothing super long, just like a paragraph in this issue) and then implement it ;-)

[alvaroaleman]

Mainly just figure out which option looks the best here with a little proposal (nothing super long, just like a paragraph in this issue) and then implement it ;-)

I am in favor of just documenting and testing the current approach, the two other options are IMHO inferior:

Write a wrapper around kind.Source that implements InjectCache with the nilcheck

This would extend the API needlessly.

Write a wrapper around kind.Source that does not implement inject.Cache

This would extend the API needlessly and be super confusing in our code, because the new KindInOtherCluster would need a public method to set the cache that must not be an implementation of InjectCache.

Godoc change for the first option could look something like this:

@@ -55,7 +55,9 @@ type Source interface {
        Start(handler.EventHandler, workqueue.RateLimitingInterface, ...predicate.Predicate) error
 }
 
-// Kind is used to provide a source of events originating inside the cluster from Watches (e.g. Pod Create)
+// Kind is used to provide a source of events originating inside the cluster from Watches (e.g. Pod Create).
+// It can be used to watch objects in a different cluster by calling its InjectCache method with the cache
+// from that other cluster.
 type Kind struct {
        // Type is the type of object to watch.  e.g. &v1.Pod{}
        Type runtime.Object

@DirectXMan12 There is no proposal process for c-r documented (that I can find). Is the outline in this comment enough or do you want a PR that adds it to the repo?

[c0d1ngm0nk3y]

Write a wrapper around kind.Source that does not implement inject.Cache

This would extend the API needlessly and be super confusing in our code, because the new KindInOtherCluster would need a public method to set the cache that must not be an implementation of InjectCache.

@alvaroaleman How about a more explicit solution than "just" documenting the behaviour. How about a wrapper similar to f7d3056 (just a quick first draft). I think it is not possible to use this wrapper in a wrong way and we don't rely on the current undocumented behaviour of InjectCache.

func CreateSourceWithFixedCache(object runtime.Object, cache cache.Cache) Source {
	return &sourceWithFixedCache{kind: Kind{Type: object, cache: cache}}
}

type sourceWithFixedCache struct {
	kind Kind
}

@DirectXMan12 What do you think of this?

[alvaroaleman]

@alvaroaleman How about a more explicit solution than "just" documenting the behaviour. How about a wrapper similar to f7d3056 (just a quick first draft).

Sounds good to me as well. I am mostly concerned about the question if this will be intuitive, because in the local cluster case the source is called Kind, in the other cluster case its called SourceWithFixedCache. Maybe KindWithFixedCache would be better?

I think it is not possible to use this wrapper in a wrong way and we don't rely on the current undocumented behaviour of InjectCache.

Well, the missing documentation can be added :)

I do not have a strong opinion on either of the two, but having anything that is officially supported would be great

[DirectXMan12]

The more explicit type I think seems slightly better to me

[detiber]

+1 for more explicit, The implicit method is harder to rationalize about without understanding the implementation.