Default to closing watch requests during graceful shutdown #130991
Conversation
The change gives `--shutdown-watch-termination-grace-period` a default value of 67 seconds so that the API server will wait up to that duration for watches to drain before signaling that requests have been drained. Among other things, this prevents the API server from terminating gRPC connections to KMS plugins until after draining has been attempted. Signed-off-by: Monis Khan <[email protected]>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: enj The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
This PR may require API review. If so, when the changes are ready, complete the pre-review checklist and request an API review. Status of requested reviews is tracked in the API Review project. |
|
@enj: The following tests 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. |
|
Changelog suggestion -The `--shutdown-watch-termination-grace-period` flag now defaults to 67 seconds.
+Added a grace period for watch closure during API server graceful shutdown. The `--shutdown-watch-termination-grace-period` option now defaults to 67 seconds. |
| @@ -319,9 +319,6 @@ type Config struct { | |||
| // number of active watch request(s) in flight and during shutdown | |||
| // it will wait, at most, for the specified duration and allow these | |||
| // active watch requests to drain with some rate limiting in effect. | |||
| // The default is zero, which implies the apiserver will not keep | |||
There was a problem hiding this comment.
is the cost of keeping track of pending watch requests meaningful? I want to make sure we're not defaulting configs into something noticeably more expensive for kube-apiserver to run
There was a problem hiding this comment.
If you ask about resource consumption (cpu/mem), it's fairly cheap. We basically only have a rate-limited waiting group:
https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/apimachinery/pkg/util/waitgroup/ratelimited_waitgroup.go
and closing watches is rate-limited when apiserver is in shutdown:
https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/apimachinery/pkg/util/waitgroup/ratelimited_waitgroup.go#L70
Corresponding PR adding this logic: https://github.com/kubernetes/kubernetes/pull/114925/files
So I wouldn't worry about the resource overhead.
What changes though is that apiserver graceful shutdown can now be visibly longer (by this 67s) by default with this change. I think this is fine, but we need to weigh that in.
There was a problem hiding this comment.
this also need to be updated
kubernetes/staging/src/k8s.io/apiserver/pkg/server/genericapiserver.go
Lines 292 to 297 in c90a4b1
| StorageObjectCountTracker: flowcontrolrequest.NewStorageObjectCountTracker(), | ||
| // By default, attempt to drain existing watch connections during graceful shutdown. | ||
| // Use a value close to a minute that is unique enough to jump out in logs. | ||
| ShutdownWatchTerminationGracePeriod: 67 * time.Second, |
There was a problem hiding this comment.
does this only make sense to be >= other tunable timeouts defaulted above, like this one:
RequestTimeout: time.Duration(60) * time.Second,If someone is setting --request-timeout longer or shorter than the default, would we expect this to be adjusted as well? Will it break something if this stays at the new default of 67s and that is much longer or much shorter than an overridden --request-timeout?
There was a problem hiding this comment.
This setting (shutdown watch termination grace period) is affecting only watches.
OTOH, RequestTimeout is not used for watches (it's only used for non-streaming requests). So I don't think we need additional logic to cross-configure those.
There was a problem hiding this comment.
kubernetes/staging/src/k8s.io/apiserver/pkg/server/genericapiserver.go
Lines 292 to 297 in c90a4b1
|
unit test failures are legit, this change on behavior will have an impact on designs that depend on it for upgrades of control planes, @tkashem as he was involved on the implementation |
|
it feels odd changing a default for an specific setup as reported in the bug, or do we envision this can cause more issues? |
If the default being 0 meant "terminate watch connections instantly when shutting down," that would actually probably be better. Instead it means "leave watch connections open, but tell the rest of the server they can shut down" which is a really problematic combination. |
that was the backward compatibility change ... but yes, maybe that was the wrong behavior to maintain |
|
The Kubernetes project currently lacks enough contributors to adequately respond to all PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale |
The change gives
--shutdown-watch-termination-grace-perioda default value of 67 seconds so that the API server will wait up to that duration for watches to drain before signaling that requests have been drained.Among other things, this prevents the API server from terminating gRPC connections to KMS plugins until after draining has been attempted.
xref #130898
/kind bug
/kind api-change