Enable filtered list watches as watches

[nkvoll]

When setting up watches during initialization it's currently not possible to filter by any selectors (which is possible using list watches).

For example it is not possible to only watch pods with specific labels (e.g having the label pod-type: my-controller-type). The current behavior results in very broad caching, which might not be desirable for large Kubernetes deployments.

In some scenarios an operator could contain multiple controllers, and they all share caches, so keying caches on GVK's alone might be problematic if they want to watch the same resource type, but with different filters.

When doing List/Get, how would one decide which of the caches to use? It seems that perhaps this needs to be an explicit choice by the operator developers?

[jeefy]

+1

I've run into a similar issue with a controller I'm writing. I wound up starting a goroutine when the controller gets added to watch/act on certain objects that are created and labeled by another controller outside of mine.

[DirectXMan12]

we'd have to specify this at the manager level, but it should be possible to pass restricting items.

[DirectXMan12]

(since caches are shared)

[DirectXMan12]

we could also let users manually specify auxiliary caches

[DirectXMan12]

if anyone has ideas for a design doc, I'd love to see them.

[DirectXMan12]

/kind feature
/priority important-soon

this has been coming up a lot lately, and seems like a good thing to tackle in the list of related issues.

[DirectXMan12]

i.e. one sketch:

Have a list-options watch predicate, smartly detect the presence of that (in some generic way) and pass it down to GetInformer. If we already have a global informer, just use that. If we don't have a global informer, create a new filtered informer. We delay creation of informers to figure out if one person needs a global informer, and just use that, to allow splitting and combining seamlessly. The question is how to deal with the cached client:

• ❌ cached client always tries for a global informer (seems like a performance footgun)
• ❌ only one filtered informer max (breaks multi-controller managers)
• ❌ cache client refuses to use global informer if filtered informers exist and a global informer does not (breaks multi-controller informers)
• 🆗 keep track of which controller a given client belongs to and which watches that client set up, and try to scope those gets to the informer it belongs to, in the case of filtered informers (complicated, serious internal refactors)
• 🆗 allow users to turn off auto-caching, make them explicitly get filtered caching clients if they want (doable, more work for users). This potentially lets us allow users to specify that they don't want the client to ever cache a particular object (could encourage wrong patterns).

That last one might solve multiple problems in one fell swoop, but I'd need to see a design sketch to make sure that it's not too arcane.

[poidag-zz]

I will follow this to understand how this is properly implemented and any questions from a user or testing perspective please ask and I'll do my best to assist. In the interim I've implemented this for my own use case.

kvp := &keyValuePair{
	key:   "app",
	value: "test",
}

	filter := newFilter(keyValueFilter(kvp))

	err = c.Watch(&source.Kind{Type: &v1.Deployment{}}, &handler.EnqueueRequestForObject{}, filter)
if err != nil {
	return err
}

type keyValuePair struct {
	key   string
	value string
}

var kvp keyValuePair

type filterFuncs struct {
	// Create returns true if the Create event should be processed
	CreateFunc func(event.CreateEvent) bool

	// Delete returns true if the Delete event should be processed
	DeleteFunc func(event.DeleteEvent) bool

	// Update returns true if the Update event should be processed
	UpdateFunc func(event.UpdateEvent) bool

	// Generic returns true if the Generic event should be processed
	GenericFunc func(event.GenericEvent) bool
	Filter      keyValuePair
}

func keyValueFilter(kvp *keyValuePair) func(*filterFuncs) {
	return func(f *filterFuncs) {
		f.Filter.key = kvp.key
		f.Filter.value = kvp.value
	}
}

func newFilter(opts ...func(*filterFuncs)) *filterFuncs {
	f := &filterFuncs{}
	for _, opt := range opts {
		opt(f)
	}
	return f
}


func (p filterFuncs) Create(e event.CreateEvent) bool {
	labels := e.Meta.GetLabels()
	if val, ok := labels[p.Filter.key]; ok {
		if val == p.Filter.value {
			return true
		}
	}
	return false
}

[DirectXMan12]

That's just like a predicate, though -- it doesn't actually get you most of the benefits of the filtering that people are asking for here, since it doesn't ask for server-side filtering.

[poidag-zz]

I know... As I said above the code block

I will follow this to understand how this is properly implemented and any questions from a user or testing perspective please ask and I'll do my best to assist. In the interim I've implemented this for my own use case.

[DirectXMan12]

Ack, just wanted to make sure we were on the same page :-)

[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

[fejta-bot]

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

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 rotten

[DirectXMan12]

/remove-lifecycle stale

[fejta-bot]

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

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

[k8s-ci-robot]

@fejta-bot: Closing this issue.

In response to this:

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

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

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]

/reopen
/remove-lifecycle rotten

[k8s-ci-robot]

@alvaroaleman: Reopened this issue.

In response to this:

/reopen
/remove-lifecycle rotten

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.

[shawn-hurley]

bumping this feature request:

allow users to turn off auto-caching, make them explicitly get filtered caching clients if they want (doable, more work for users). This potentially lets us allow users to specify that they don't want the client to ever cache a particular object (could encourage wrong patterns).

I think this is a viable option, but I wonder if we could have a more straightforward pattern like a user specifies a watch with a label. If they do, then we use the label-filtered cache for that specific GVK. If another watch is created without that label-filter, then we will error out and not establish the watch. The most significant caveat that I see is the client will only use the label filtered cache. We would need to make that clear in the documentation, or is there some other mechanism to alert the user to this.

[pagarwal-tibco]

Any updates on this?

[qinqon]

I have try a PR to specify a fieldselector at manager build time #1404.

[timebertt]

New PR: #1435

[invidian]

#1435 is now merged, so is this resolved?

[taylormgeorge91]

It is being worked into a release currently @invidian

Looking at the tags there is a v0.9.0-beta available if you want to test.

[invidian]

Looking at the tags there is a v0.9.0-beta available if you want to test.

Thanks, will do!

[estroz]

#1435 is now merged, so is this resolved?

@invidian yes I believe this is resolved.

/close

[k8s-ci-robot]

@estroz: Closing this issue.

In response to this:

#1435 is now merged, so is this resolved?

@invidian yes I believe this is resolved.

/close

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.

[roee88]

It seems like the current solution doesn't address the use case mentioned in
#244 (comment).

It should be possible to create a namespace scoped client in ListFunc and WatchFunc if there is a field selector for metadata.namespace (vs the current logic of just looking at the global namespace field). Is that reasonable?

[alvaroaleman]

@roee88 what was merged allows to set a FieldSelector so yes, this is possible

[roee88]

To clarify, I suspect that without affecting the parameters to NamespaceIfScoped, the selector opts have no effect on the required rbac (cluster role bindings vs role bindings). For example here:

controller-runtime/pkg/cache/internal/informers_map.go

Lines 282 to 288 in 64b1c72

	WatchFunc: func(opts metav1.ListOptions) (watch.Interface, error) {
	ip.selectors[gvk].ApplyToList(&opts)
	// Watch needs to be set to true separately
	opts.Watch = true
	isNamespaceScoped := ip.namespace != "" && mapping.Scope.Name() != meta.RESTScopeNameRoot
	return client.Get().NamespaceIfScoped(ip.namespace, isNamespaceScoped).Resource(mapping.Resource.Resource).VersionedParams(&opts, ip.paramCodec).Watch(ctx)
	},

My question is whether changing the code here makes sense. Specifically for the case where ip.namespace is empty, the value from a metadata.namespace field selector should be used (if exists).

cc @shlomitk1

[alvaroaleman]

My question is whether changing the code here makes sense. Specifically for the case where ip.namespace is empty, the value from a metadata.namespace field selector should be used (if exists).

That sounds reasonable