✨ Allow non-blocking retrieval of informers

[maxsmythe]

This PR is split off from #2285

It allows users to retrieve an informer before an informer has finished bootstrapping. This is valuable as it allows users to add/remove listeners without waiting for the informer to be ready. Since a listener is a passive observer of events, it does not require the informer to be fully initialized. Not waiting for full initialization can improve bootstrapping times and enable more flexibility with how dynamic watches are handled.

[k8s-ci-robot]

Hi @maxsmythe. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[alvaroaleman]

/ok-to-test

[maxsmythe]

/retest-required

[maxsmythe]

@alvaroaleman comments should be addressed

[maxsmythe]

rebased.

@alvaroaleman @vincepri, let me know what approach you'd like to see for the unit tests, per this thread:

#2371 (comment)

[maxsmythe]

Rebased again. @alvaroaleman @vincepri Any further feedback? AFACT all threads are closed and/or awaiting feedback.

[ritazh]

bump @vincepri do you mind reviewing this again? Thanks in advance!

[sbueringer]

@maxsmythe looks like there is something slightly of with the unit test.

(I'll take a closer look later, I had it working yesterday)

[maxsmythe]

@sbueringer It looks like there are multiple possible structures for the underlying object:

• delegating.defaultCache.Informers (delegating cache wraps normal cache)
• delegating.defaultCache.Informers + delegating.caches[GVK].Informers (delegating + gvk cache)
• delegating.defaultCache.clusterCache.Informers + delegating.defaultCache[NAMESPACE].Informers (multi-namespaced cache)
• the above + delegating.caches[GVK].Informers (multi-NS + gvk cache)

I'm inferring this from

controller-runtime/pkg/cache/cache.go

Lines 267 to 301 in 304027b

	newCacheFunc := newCache(cfg, opts)
	var defaultCache Cache
	if len(opts.DefaultNamespaces) > 0 {
	defaultConfig := optionDefaultsToConfig(&opts)
	defaultCache = newMultiNamespaceCache(newCacheFunc, opts.Scheme, opts.Mapper, opts.DefaultNamespaces, &defaultConfig)
	} else {
	defaultCache = newCacheFunc(optionDefaultsToConfig(&opts), corev1.NamespaceAll)
	}
	if len(opts.ByObject) == 0 {
	return defaultCache, nil
	}
	delegating := &delegatingByGVKCache{
	scheme: opts.Scheme,
	caches: make(map[schema.GroupVersionKind]Cache, len(opts.ByObject)),
	defaultCache: defaultCache,
	}
	for obj, config := range opts.ByObject {
	gvk, err := apiutil.GVKForObject(obj, opts.Scheme)
	if err != nil {
	return nil, fmt.Errorf("failed to get GVK for type %T: %w", obj, err)
	}
	var cache Cache
	if len(config.Namespaces) > 0 {
	cache = newMultiNamespaceCache(newCacheFunc, opts.Scheme, opts.Mapper, config.Namespaces, nil)
	} else {
	cache = newCacheFunc(byObjectToConfig(config), corev1.NamespaceAll)
	}
	delegating.caches[gvk] = cache
	}
	return delegating, nil

We could make newInformer a private member of the cache.Options struct. It would need to be "public" for the internal package, but that's an internal package. That may make things not-settable by users but less brittle to implementation changes (and statically analyzable). WDYT?

[sbueringer]

Ah yup makes sense. I probably didn't test all variants somehow.

We could make newInformer a private member of the cache.Options struct. It would need to be "public" for the internal package, but that's an internal package. That may make things not-settable by users but less brittle to implementation changes (and statically analyzable). WDYT?

+1 Sounds like the best option

(feel free to squash directly)

[sbueringer]

@maxsmythe Do you have time to address the issue? (context: would be nice to get this PR into the upcoming v0.16 release)

[maxsmythe]

Yep! Sorry, was out Friday, aiming to push something in the next ~hour

[sbueringer]

Yep! Sorry, was out Friday, aiming to push something in the next ~hour

No worries, thank you!

[maxsmythe]

For sure! Took a bit longer than an hour... still needed to get around golang's visibility restrictions, and was unable to do that without turning newInformer into a pointer-to-function. Trying to use a function-type variable resulted in a segfault.

[k8s-ci-robot]

@maxsmythe: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-controller-runtime-apidiff	2aa20a6	link	false	/test pull-controller-runtime-apidiff

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

[maxsmythe]

@sbueringer unit tests pass

[sbueringer]

Perfect. Thank you very much!

/lgtm

/assign @alvaroaleman @vincepri

[wyike]

pull-controller-runtime-apidiff job seems still failing

[sbueringer]

pull-controller-runtime-apidiff job seems still failing

This is expected behavior. Not sure if we should flag this PR as a breaking change as we're just adding a vararg so it doesn't break anyone using the changed funcs.

[wyike]

pull-controller-runtime-apidiff job seems still failing

This is expected behavior. Not sure if we should flag this PR as a breaking change as we're just adding a vararg so it doesn't break anyone using the changed funcs.

I see. In such case, apidiff job failure is acceptable to merge a PR.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alvaroaleman, maxsmythe

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [alvaroaleman]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[alvaroaleman]

Thanks for sticking to it and getting this done, @maxsmythe !

[maxsmythe]

Thank you for the reviews!