-
Notifications
You must be signed in to change notification settings - Fork 38.6k
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
Extend resyncPeriods in controllers in production. #15153
Conversation
Labelling this PR as size/L |
GCE e2e test build/test passed for commit 79253030be1034e848bb7f733c67913945dbebe9. |
@@ -54,6 +54,13 @@ var ( | |||
KeyFunc = framework.DeletionHandlingMetaNamespaceKeyFunc | |||
) | |||
|
|||
type ResyncPeriodFunc func() time.Duration |
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.
I can't understand why you're injecting a function, but all the functions you use just return a constant. If you're doing it for the type protection from the compiler, why not just type ResyncPeriod time.Duration
?
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.
No - it's not for type protection.
The reason I'm doing it is that in some controllers we need more than one resyncPeriod and I want to avoid situation of having many periods to be the same to spread the load over time. That' why I'm injecting the function that is generating random values in production code - the constant values are only in unit tests.
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.
It makes sense that you want a different value for every controller, but since each controller only calls the function once (when it makes the informer) it still doesn't make sense to me that you need to inject a function.
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.
It can call the function more than once, because it can have more than one Informer (one informer per resource). I wanted those also to not collide (e.g. daemon controller is using this function for both pods and nodes).
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.
I see, now it makes sense, thank you.
On Wed, Oct 7, 2015 at 1:08 PM, Wojciech Tyczynski <notifications@github.com
wrote:
In pkg/controller/controller_utils.go
#15153 (comment)
:@@ -54,6 +54,13 @@ var (
KeyFunc = framework.DeletionHandlingMetaNamespaceKeyFunc
)+type ResyncPeriodFunc func() time.Duration
It can call the function more than once, because it can have more than one
Informer (one informer per resource). I wanted those also to not collide
(e.g. daemon controller is using this function for both pods and nodes).—
Reply to this email directly or view it on GitHub
https://github.com/kubernetes/kubernetes/pull/15153/files#r41439676.
7925303
to
7123c20
Compare
GCE e2e test build/test passed for commit 7123c201342f5f62ed0c44478a7ecf13e1a5cacd. |
LGTM once tests pass. |
7123c20
to
df79026
Compare
GCE e2e test build/test passed for commit df79026. |
@lavalamp - test didn't pass due to missing flag in "known-flags.txt". I've added it and now tests are passing. Since this was the only change, I'm adding lgtm labale myself. |
Extend resyncPeriods in controllers in production.
…pick-of-#15153-kubernetes#15900-kubernetes#15930-upstream-release-1.1 Auto commit by PR queue bot
…pick-of-#15153-kubernetes#15900-kubernetes#15930-upstream-release-1.1 Auto commit by PR queue bot
This should significantly reduce amount of LIST requests in production cluster (still leaving this branch tested in e2e tests).
cc @lavalamp @kubernetes/goog-csi