[WIP] Configure watchcache history window based on request timeout #129020
Conversation
|
Please note that we're already in Test Freeze for the Fast forwards are scheduled to happen every 6 hours, whereas the most recent run was: Thu Nov 28 10:39:15 UTC 2024. |
|
This issue is currently awaiting triage. If a SIG or subproject determines this is a relevant issue, they will accept it by applying the The 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-sigs/prow repository. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: wojtek-t The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
| @@ -180,6 +180,10 @@ func BuildGenericConfig( | |||
| sets.NewString("attach", "exec", "proxy", "log", "portforward"), | |||
| ) | |||
|
|
|||
| if genericConfig.RequestTimeout > 0 { | |||
There was a problem hiding this comment.
@sttts - would you mind taking a look at the second commit - in particular this line and suggest if there is a better place for achieving this?
There was a problem hiding this comment.
There is prior work using this pattern below. So adding another one here is not the end of the world.
But in general, mutating s here in the config constructor has smell (unexpected side-effect).
The cleaner solution, and question is whether there are other ripples, would be to move this kind of cross-options logic into the option completion func, i.e. here for controlplane/apiserver that would be
.There was a problem hiding this comment.
And of course referencing ServerRunOptions.RequestTimeout. The config value (which is the same? hopefully) is not accessible there yet.
There was a problem hiding this comment.
The pattern is always:
NewOptions, tweak values, AddFlags(), evaluate flags, Complete().
NewConfig(options), tweak values, Complete().
NewServer(config), tweak values, Complete(), PrepareRun(), Run().
The Complete() func is where you add defaulting, or cross options logic. Every constructor is side-effect-free.
|
@wojtek-t: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. 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-sigs/prow repository. I understand the commands that are listed here. |
For clusters with a lot of resources, listing all objects of a given type may take more than 1 minute. In such case, to make "list+watch" pattern usable, we need to ensure that we can establish a watch from more than 1 minute ago.
Previously watchcache history window was hardcoded to 75 seconds, which in some cases may not be enough. This PR configures it as maximum from previous 75s and a function of RequestTimeout.
/sig api-machinery
/priority important-longterm
/kind cleanup