-
Notifications
You must be signed in to change notification settings - Fork 1.3k
📖 Add a design for cache options configuration #2261
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
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,226 @@ | ||||||||||||||||||||||||||
| Cache Options | ||||||||||||||||||||||||||
| =================== | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| This document describes how we imagine the cache options to look in | ||||||||||||||||||||||||||
| the future. | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| ## Goals | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| * Align everyone on what settings on the cache we want to support and | ||||||||||||||||||||||||||
| their configuration surface | ||||||||||||||||||||||||||
| * Ensure that we support both complicated cache setups and provide an | ||||||||||||||||||||||||||
| intuitive configuration UX | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| ## Non-Goals | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| * Describe the design and implementation of the cache itself. | ||||||||||||||||||||||||||
| The assumption is that the most granular level we will end up with is | ||||||||||||||||||||||||||
| "per-object multiple namespaces with distinct selectors" and that this | ||||||||||||||||||||||||||
| can be implemented using a "meta cache" that delegates per object and by | ||||||||||||||||||||||||||
| extending the current multi-namespace cache | ||||||||||||||||||||||||||
| * Outline any kind of timeline for when these settings will be implemented. | ||||||||||||||||||||||||||
| Implementation will happen gradually over time whenever someone steps up | ||||||||||||||||||||||||||
| to do the actual work | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| ## Proposal | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| ``` | ||||||||||||||||||||||||||
| const ( | ||||||||||||||||||||||||||
| AllNamespaces = corev1.NamespaceAll | ||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||
| type Config struct { | ||||||||||||||||||||||||||
| // LabelSelector specifies a label selector. A nil value allows to | ||||||||||||||||||||||||||
| // default this. | ||||||||||||||||||||||||||
| LabelSelector labels.Selector | ||||||||||||||||||||||||||
| // FieldSelector specifics a field selector. A nil value allows to | ||||||||||||||||||||||||||
| // default this. | ||||||||||||||||||||||||||
| FieldSelector fields.Selector | ||||||||||||||||||||||||||
| // Transform specifies a transform func. A nil value allows to default | ||||||||||||||||||||||||||
| // this. | ||||||||||||||||||||||||||
| Transform toolscache.TransformFunc | ||||||||||||||||||||||||||
| // UnsafeDisableDeepCopy specifies if List and Get requests against the | ||||||||||||||||||||||||||
| // cache should not DeepCopy. A nil value allows to default this. | ||||||||||||||||||||||||||
| UnsafeDisableDeepCopy *bool | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
| type ByObject struct { | ||||||||||||||||||||||||||
| // Namespaces maps a namespace name to cache setting. If set, only the | ||||||||||||||||||||||||||
| // namespaces in this map will be cached. | ||||||||||||||||||||||||||
| // | ||||||||||||||||||||||||||
| // Settings in the map value that are unset because either the value as a | ||||||||||||||||||||||||||
| // whole is nil or because the specific setting is nil will be defaulted. | ||||||||||||||||||||||||||
| // Use an empty value for the specific setting to prevent that. | ||||||||||||||||||||||||||
| // | ||||||||||||||||||||||||||
| // It is possible to have specific Config for just some namespaces | ||||||||||||||||||||||||||
| // but cache all namespaces by using the AllNamespaces const as the map key. | ||||||||||||||||||||||||||
| // This wil then include all namespaces that do not have a more specific | ||||||||||||||||||||||||||
| // setting. | ||||||||||||||||||||||||||
| // | ||||||||||||||||||||||||||
| // A nil map allows to default this to the cache's DefaultNamespaces setting. | ||||||||||||||||||||||||||
| // An empty map prevents this. | ||||||||||||||||||||||||||
| // | ||||||||||||||||||||||||||
| // This must be unset for cluster-scoped objects. | ||||||||||||||||||||||||||
| Namespaces map[string]*Config | ||||||||||||||||||||||||||
|
Member
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. How does this map to RBAC? Do I only need list, get, watch permissions in the namespaces specified in this map, or do I need those permissions for all namespaces? Does this option limit what is requested/ watched from the APIServer or does it just limit what is being cached?
Member
Author
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. What is cached is what is being watched, so both. 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.
At the moment the calls to the kube apiserver to LIST resources are different (this might be in client-go, not sure) depending on whether the whole cache is scoped to a namespace or whether the namespace selector is applied as a field selector. We have a use case where we want to watch Secrets in namespace X and Configmaps in all namespaces. We create two caches for this because although we could use the existing cache options to apply namespace filter to Secrets, only namespaced cache allows us to scope RBAC down so that users only need to give our controller permissions to list Secrets in that one namespace. For us, it would be quite useful if we were able to use this
Member
Author
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. Yes, it will be. Something like your use-case is what we want to support. Your config would look something like: The above config would watch secrets in one namespace and everything else in all namespaces. That also applies to rbac. 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. Thank you- for us this would be very valuable 👍🏼 |
||||||||||||||||||||||||||
| // Config will be used for cluster-scoped objects and to default | ||||||||||||||||||||||||||
| // Config in the Namespaces field. | ||||||||||||||||||||||||||
| // | ||||||||||||||||||||||||||
| // It gets defaulted from the cache'sDefaultLabelSelector, DefaultFieldSelector, | ||||||||||||||||||||||||||
| // DefaultUnsafeDisableDeepCopy and DefaultTransform. | ||||||||||||||||||||||||||
| Config *Config | ||||||||||||||||||||||||||
|
Comment on lines
+71
to
+76
Member
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.
Suggested change
Maybe? Could be a bit more readable, 🤔
Member
Author
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. I think that might be confusing because with an override, ppl expect the value in there to be the final setting but:
|
||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
| type Options struct { | ||||||||||||||||||||||||||
FillZpp marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||||||||||
| // ByObject specifies per-object cache settings. If unset for a given | ||||||||||||||||||||||||||
| // object, this will fall through to Default* settings. | ||||||||||||||||||||||||||
| ByObject map[client.Object]*ByObject | ||||||||||||||||||||||||||
|
Member
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.
Suggested change
Maybe?
Member
Author
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. Not a super strong opinion here, the reason I chose the pointer type is in some contexts an empty thing has a meaning (for example an empty labelSelector is a match-everything selector) so I thought by not requireding ppl to do a |
||||||||||||||||||||||||||
| // DefaultNamespaces maps namespace names to cache settings. If set, it | ||||||||||||||||||||||||||
| // will be used for all objects that have a nil Namespaces setting. | ||||||||||||||||||||||||||
| // | ||||||||||||||||||||||||||
| // It is possible to have a specific Config for just some namespaces | ||||||||||||||||||||||||||
| // but cache all namespaces by using the `AllNamespaces` const as the map | ||||||||||||||||||||||||||
| // key. This wil then include all namespaces that do not have a more | ||||||||||||||||||||||||||
| // specific setting. | ||||||||||||||||||||||||||
| // | ||||||||||||||||||||||||||
| // The options in the Config that are nil will be defaulted from | ||||||||||||||||||||||||||
| // the respective Default* settings. | ||||||||||||||||||||||||||
| DefaultNamespaces map[string]*Config | ||||||||||||||||||||||||||
| // DefaultLabelSelector is the label selector that will be used as | ||||||||||||||||||||||||||
| // the default field label selector for everything that doesn't | ||||||||||||||||||||||||||
| // have one configured. | ||||||||||||||||||||||||||
| DefaultLabelSelector labels.Selector | ||||||||||||||||||||||||||
| // DefaultFieldSelector is the field selector that will be used as | ||||||||||||||||||||||||||
| // the default field selector for everything that doesn't have | ||||||||||||||||||||||||||
| // one configured. | ||||||||||||||||||||||||||
| DefaultFieldSelector fields.Selector | ||||||||||||||||||||||||||
| // DefaultUnsafeDisableDeepCopy is the default for UnsafeDisableDeepCopy | ||||||||||||||||||||||||||
| // for everything that doesn't specify this. | ||||||||||||||||||||||||||
| DefaultUnsafeDisableDeepCopy *bool | ||||||||||||||||||||||||||
| // DefaultTransform will be used as transform for all object types | ||||||||||||||||||||||||||
| // unless they have a more specific transform set in ByObject. | ||||||||||||||||||||||||||
| DefaultTransform toolscache.TransformFunc | ||||||||||||||||||||||||||
sbueringer marked this conversation as resolved.
Show resolved
Hide resolved
Comment on lines
+84
to
+112
Member
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. Would it be better or worse to drop the
Member
Author
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. I like it because it clarifies that this is the last layer of defaulting |
||||||||||||||||||||||||||
| // HTTPClient is the http client to use for the REST client | ||||||||||||||||||||||||||
| HTTPClient *http.Client | ||||||||||||||||||||||||||
| // Scheme is the scheme to use for mapping objects to GroupVersionKinds | ||||||||||||||||||||||||||
| Scheme *runtime.Scheme | ||||||||||||||||||||||||||
| // Mapper is the RESTMapper to use for mapping GroupVersionKinds to Resources | ||||||||||||||||||||||||||
| Mapper meta.RESTMapper | ||||||||||||||||||||||||||
| // SyncPeriod determines the minimum frequency at which watched resources are | ||||||||||||||||||||||||||
| // reconciled. A lower period will correct entropy more quickly, but reduce | ||||||||||||||||||||||||||
| // responsiveness to change if there are many watched resources. Change this | ||||||||||||||||||||||||||
| // value only if you know what you are doing. Defaults to 10 hours if unset. | ||||||||||||||||||||||||||
| // there will a 10 percent jitter between the SyncPeriod of all controllers | ||||||||||||||||||||||||||
| // so that all controllers will not send list requests simultaneously. | ||||||||||||||||||||||||||
| // | ||||||||||||||||||||||||||
| // This applies to all controllers. | ||||||||||||||||||||||||||
| // | ||||||||||||||||||||||||||
| // A period sync happens for two reasons: | ||||||||||||||||||||||||||
| // 1. To insure against a bug in the controller that causes an object to not | ||||||||||||||||||||||||||
| // be requeued, when it otherwise should be requeued. | ||||||||||||||||||||||||||
| // 2. To insure against an unknown bug in controller-runtime, or its dependencies, | ||||||||||||||||||||||||||
| // that causes an object to not be requeued, when it otherwise should be | ||||||||||||||||||||||||||
| // requeued, or to be removed from the queue, when it otherwise should not | ||||||||||||||||||||||||||
| // be removed. | ||||||||||||||||||||||||||
| // | ||||||||||||||||||||||||||
| // If you want | ||||||||||||||||||||||||||
| // 1. to insure against missed watch events, or | ||||||||||||||||||||||||||
| // 2. to poll services that cannot be watched, | ||||||||||||||||||||||||||
| // then we recommend that, instead of changing the default period, the | ||||||||||||||||||||||||||
| // controller requeue, with a constant duration `t`, whenever the controller | ||||||||||||||||||||||||||
| // is "done" with an object, and would otherwise not requeue it, i.e., we | ||||||||||||||||||||||||||
| // recommend the `Reconcile` function return `reconcile.Result{RequeueAfter: t}`, | ||||||||||||||||||||||||||
| // instead of `reconcile.Result{}`. | ||||||||||||||||||||||||||
| SyncPeriod *time.Duration | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
| ``` | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| ## Example usages | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| ### Cache ConfigMaps in the `public` and `kube-system` namespaces and Secrets in the `operator` Namespace | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| ``` | ||||||||||||||||||||||||||
| cache.Options{ | ||||||||||||||||||||||||||
| ByObject: map[client.Object]*cache.ByObject{ | ||||||||||||||||||||||||||
| &corev1.ConfigMap{}: { | ||||||||||||||||||||||||||
| Namespaces: map[string]*cache.Config{ | ||||||||||||||||||||||||||
| "public": {}, | ||||||||||||||||||||||||||
| "kube-system": {}, | ||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||
| &corev1.Secret{}: {Namespaces: map[string]*Config{ | ||||||||||||||||||||||||||
| "operator": {}, | ||||||||||||||||||||||||||
| }}, | ||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
| ``` | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| ### Cache ConfigMaps in all namespaces without selector, but have a selector for the `operator` Namespace | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| ``` | ||||||||||||||||||||||||||
| cache.Options{ | ||||||||||||||||||||||||||
| ByObject: map[client.Object]*cache.ByObject{ | ||||||||||||||||||||||||||
| &corev1.ConfigMap{}: { | ||||||||||||||||||||||||||
| Namespaces: map[string]*cache.Config{ | ||||||||||||||||||||||||||
| cache.AllNamespaces: nil, // No selector for all namespaces... | ||||||||||||||||||||||||||
| "operator": {LabelSelector: labelSelector}, // except for the operator namespace | ||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
| ``` | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| ### Only cache the `operator` namespace for namespaced objects and all namespaces for Deployments | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| ``` | ||||||||||||||||||||||||||
| cache.Options{ | ||||||||||||||||||||||||||
| ByObject: map[client.Object]*cache.ByObject{ | ||||||||||||||||||||||||||
| &appsv1.Deployment: {Namespaces: map[string]*cache.Config{ | ||||||||||||||||||||||||||
| cache.AllNamespaces: nil, | ||||||||||||||||||||||||||
| }}, | ||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||
| DefaultNamespaces: map[string]*cache.Config{ | ||||||||||||||||||||||||||
| "operator": nil, | ||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
| ``` | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| ### Use a LabelSelector for everything except Nodes | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| ``` | ||||||||||||||||||||||||||
| cache.Options{ | ||||||||||||||||||||||||||
| ByObject: map[client.Object]*cache.ByObject{ | ||||||||||||||||||||||||||
| &corev1.Node: {LabelSelector: labels.Everything()}, | ||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||
| DefaultLabelSelector: myLabelSelector, | ||||||||||||||||||||||||||
vincepri marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
| ``` | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| ### Only cache namespaced objects in the `foo` and `bar` namespace | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| ``` | ||||||||||||||||||||||||||
| cache.Options{ | ||||||||||||||||||||||||||
| DefaultNamespaces: map[string]*cache.Config{ | ||||||||||||||||||||||||||
| "foo": nil, | ||||||||||||||||||||||||||
| "bar": nil, | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
| ``` | ||||||||||||||||||||||||||
Uh oh!
There was an error while loading. Please reload this page.