-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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 | ||||||
|
||||||
// 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 | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It would be nice to generalize this to something like
Suggested change
Where type SelectorSet interface {
client.ListOption
Select()
} where There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure, get rid of There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||||||
} | ||||||
``` | ||||||
|
||||||
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 |
There was a problem hiding this comment.
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 viaSetName()
andSetNamespace()
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
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 thereThere was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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. bySetName()
. Instead it would be better to constrain the contract to justGroupVersionKind()
.There can be some internal validation step to make sure the object is not a list type.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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()
, orWatches()
.However since there is prior art I am fine with
client.Object
. There can always be another selector map type added later forruntime.Object
without breakage if we see bugs filed because of confusion. It's not worth worrying about too much now.There was a problem hiding this comment.
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.