Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

🌱 Add a design document to filter cache by selectors #1419

Merged
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
125 changes: 125 additions & 0 deletions designs/use-selectors-at-cache.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,125 @@
Filter cache ListWatch using selectors
===================
## Motivation

Controller-Runtime controllers use a cache to subscribe to events from
Kubernetes objects and to read those objects more efficiently by avoiding
to call out to the API. This cache is backed by Kubernetes informers.

The only way to filter this cache is by namespace and resource type.
In cases where a controller is only interested in a small subset of objects
(for example all pods on a node), this might end up not being efficient enough.

Requests to a client backed by a filtered cache for objects that do not match
the filter will never return anything, so we need to make sure that we properly
warn users to only use this when they are sure they know what they are doing.

This proposal sidesteps the issue of "How to we plug this into the cache-backed
client so that users get feedback when they request something that is
not matching the caches filter" by only implementing the filter logic in the
cache package. This allows advanced users to combine a filtered cache with the
already existing `NewCacheFunc` option in the manager and cluster package,
while simultaneously hiding it from newer users that might not be aware of the
implications and the associated foot-shoot potential.

The only alternative today to get a filtered cache with controller-runtime is
to build it out-of tree. Because such a cache would mostly copy the existing
cache and add just some options, this is not great for consumers.

This proposal is related to the following issue [2]

## Proposal

Add a new selector code at `pkg/cache/internal/selector.go` with common structs
and helpers
```golang
package internal

...

// SelectorsByObject associate a runtime.Object to a field/label selector
type SelectorsByObject map[client.Object]Selector
Copy link
Contributor

@estroz estroz Apr 5, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be restricted to runtime.Object so users don't get the idea that you can filter by object name/namespace via SetName() and SetNamespace().

Suggested change
type SelectorsByObject map[client.Object]Selector
type SelectorsByObject map[runtime.Object]Selector

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@estroz why? It is a map, the values can not access the keys. Making this runtime.Object means we have to deal with stuff like ppl putting List types in there

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alvaroaleman don't we have the same problem with client.Object since it's embedding runtime.Object ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alvaroaleman it may be confusing to users to have all these object fields exposed by the metav1.Object, since they may assume the key can actually apply its own selector values, ex. by SetName(). Instead it would be better to constrain the contract to just GroupVersionKind().

There can be some internal validation step to make sure the object is not a list type.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using GVK is inconvenient. Using the smaller interface (runtime.Object) doesn't prevent ppl from putting in objects that fulfill a lager interface (client.Object). It does however keep them from putting something like List types in there and makes the compiler verify that rather than getting a runtime error. Also, I think the idea that ppl might think the map key has any further relevance for filtering when the value is a Selector struct seems a bit far fetched to me.

We also have plenty of prior art where we use client.Object to only derive the type, e.G. in source.Kind or the clients delete method.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could similarly argue that someone would not try to map a selector to a list type because that's not the type they add to For(), Owns(), or Watches().

However since there is prior art I am fine with client.Object. There can always be another selector map type added later for runtime.Object without breakage if we see bugs filed because of confusion. It's not worth worrying about too much now.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Going back to client.Object as proposed originally.


// SelectorsByGVK associate a GroupVersionResource to a field/label selector
type SelectorsByGVK map[schema.GroupVersionKind]Selector

// Selector specify the label/field selector to fill in ListOptions
type Selector struct {
Label labels.Selector
Field fields.Selector
}

// ApplyToList fill in ListOptions LabelSelector and FieldSelector if needed
func (s Selector) ApplyToList(listOpts *metav1.ListOptions) {
...
}


Add a type alias to `pkg/cache/cache.go` to internal

```golang

type SelectorsByObject internal.SelectorsByObject

```

Extend `cache.Options` as follows:

```golang
type Options struct {
Scheme *runtime.Scheme
Mapper meta.RESTMapper
Resync *time.Duration
Namespace string
SelectorsByObject SelectorsByObject
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice to generalize this to something like

Suggested change
SelectorsByObject SelectorsByObject
Selectors SelectorSet

Where SelectorSet is any set of selectors that implements

type SelectorSet interface {
	client.ListOption
	Select()
}

where Select() is just an identifier method so users don't pass in other client.ListOptions.

Copy link
Contributor Author

@qinqon qinqon Apr 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We apply options per GVK so it does not make sense to implement ListOption at the map level.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, get rid of client.ListOption from that interface. Regardless, I may want to construct a GVK myself instead of using a typed object or Unstructured, so why not also expose SelectorsByGVK?

Copy link
Contributor Author

@qinqon qinqon Apr 8, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As we have previously stated we are going to expose the map key as client.Object, internally will be GVK, in fact at the implementation PR I will remove SelectorsByObject from internal and use it only as a type to pass to the cache Options, at Options parsing it will be converted to the SelectorsByGVK.

}
```

Add new builder function that will return a cache constructor using the passed
cache.Options, users can set SelectorsByObject there to filter out cache, it
will convert SelectorByObject to SelectorsByGVK


```golang
func BuilderWithOptions(options cache.Options) NewCacheFunc {
...
}
```

is passed to informer's ListWatch and add the filtering option:

```golang

# At pkg/cache/internal/informers_map.go

ListFunc: func(opts metav1.ListOptions) (runtime.Object, error) {
ip.selectors[gvk].ApplyToList(&opts)
...
```

Here is a PR with the implementatin at the `pkg/cache` part [3]

## Example

User will override `NewCache` function to make clear that they know exactly the
implications of using a different cache than the default one

```golang
ctrl.Options.NewCache = cache.BuilderWithOptions(cache.Options{
SelectorsByObject: cache.SelectorsByObject{
&corev1.Node{}: {
Field: fields.SelectorFromSet(fields.Set{"metadata.name": "node01"}),
}
&v1beta1.NodeNetworkState{}: {
Field: fields.SelectorFromSet(fields.Set{"metadata.name": "node01"}),
Label: labels.SelectorFromSet(labels.Set{"app": "kubernetes-nmstate})",
}
}
}
)
```


[1] https://github.com/nmstate/kubernetes-nmstate/pull/687
[2] https://github.com/kubernetes-sigs/controller-runtime/issues/244
[3] https://github.com/kubernetes-sigs/controller-runtime/pull/1404