KEP-3066: subsecond probes #3067
Conversation
|
Skipping CI for Draft Pull Request. |
k8s-ci-robot
added
do-not-merge/work-in-progress
k8s-ci-robot
added
kind/kep
| Allow for 0 second Probe, with setting millisecond. | ||
|
|
||
| Cons: | ||
| Loosens validation. |
There was a problem hiding this comment.
This sounds similar to tv_sec and tv_nsec from POSIX time structs. If so, we could mention that analogy.
| <!-- | ||
| What other approaches did you consider, and why did you rule them out? These do | ||
| not need to be as detailed as the proposal, but should include enough | ||
| information to express the idea and why it was not acceptable. | ||
| --> |
There was a problem hiding this comment.
We should mention microseconds and nanoseconds too (as these are plausible alternatives to milliseconds).
There was a problem hiding this comment.
Allowing microseconds might be useful if we support future healthcheck mechanisms.
| ### v2 api for probe. | ||
|
|
||
| I think this means a v2 API for Container therefore Pod. | ||
| This seems too invasive. | ||
|
|
||
| All Seconds fields become resource.Quantity instead of int32. This supports subdivision in a single field. |
There was a problem hiding this comment.
The bigger problem is how to support round-tripping to the (still GA) v1 API. Even if we did a v2 API, we'd need a way to represent the subsecond timeout in the v1 API, right?
(I'm personally all for adopting a v2 Pod and v2 PodTemplate API, but lots of clients would expect to still create v1 Deployments, etc).
|
|
||
| V2 API for existing objects. | ||
| Converting fields from int32 to resource.Quantity. | ||
| Subsecond resolution less than one millisecond. |
There was a problem hiding this comment.
I'd omit this non-goal; it sounds more like a design choice than a goal we're not pursuing.
| If the Milliseconds variant of a field is set, | ||
| *use it in preference* to the existing Seconds field, | ||
| and **completely ignore** the value of the existing field. | ||
| This behavior makes it opt-in on setting a non-zero Milliseconds field. |
There was a problem hiding this comment.
If there is an existing admission control mechanism that enforces constraints on timeoutSeconds, does this (draft) change break any API contract that the admission control mechanism is entitled to assume?
There was a problem hiding this comment.
@ehashman recommended in the sig node call that we forgo the alternate fields idea and move to adding a new flag to indicate seconds should be processed as milliseconds.. etc.
great point on we will need to check for admission control impacts..
|
Overall, I think my concerns about these API changes are around changes we might want to make a few years in the future. If we specify probe timeouts in microseconds, that allows for two things:
If we ever have to support second-granularity, millisecond-granularity and microsecond-granularity probes, I hope the API for that won't look as ugly as I fear it might. The approach I'd still promote is to have an extra field to specify “early failure”: an integer quantity of milliseconds (or microseconds - we should leave room for that). That early failure offset is something that legacy clients can ignore without a big problem and would fully support round-tripping to a future Pod v2 API that unifies these fields. Using a negative offset makes the distinction between (1s - 995ms) more obvious to a legacy client that's unaware of the new fields. A positive offset (0s + 5ms, or maybe 0s + 5000μs) could get interpreted quite differently. If we do that, we should absolutely disallow a negative offset ≥ 1.0s seconds. Otherwise we have a challenge around unambiguous round-tripping (eg from a future Pod v2 API back to Pod v1). |
|
For |
| New Field: int32 PeriodOffsetMilliseconds | ||
|
|
||
| If I want to set to 0.5 seconds, 500 milliseconds, | ||
| PeriodSeconds <= 10 & PeriodOffsetmilliseconds <= -9500 |
There was a problem hiding this comment.
This is an example of a pair of values that could be ambiguous for round-tripping, forcing future APIs to store and track both pieces of data.
By contrast, if we had:
....
periodSeconds: 1
earlyRecheckOffset:
- unit: Millisecond
value: 500
...and the v1 Pod API doesn't allow ambiguous values, then a future v2 Pod API can store a single field value behind the scenes
(eg: periodMicroseconds = 500000u64)
There was a problem hiding this comment.
Another option, still clamped to less than 1s:
...
periodSeconds: 0
extraRecheckOffset:
- unit: Millisecond
value: 500
...
|
|
||
| All Seconds fields become resource.Quantity instead of int32. This supports subdivision in a single field. | ||
|
|
||
| ### OffsetMilliseconds |
There was a problem hiding this comment.
@mikebrow can remove this section from the alternatives since it's the main proposal now
There was a problem hiding this comment.
nod let's focus in on the offset milliseconds option since and fix the naming above..
| Allow for 0 second Probe, with setting millisecond. Similar to what is done with `tv_sec` and `tv_nsec` from POSIX time structs. | ||
|
|
||
| Cons: | ||
| Loosens validation. |
There was a problem hiding this comment.
So this proposal would just add a new field for each existing *Seconds field, like PeriodSeconds and PeriodMilliseconds and then the actual time is the sum? This seems like a way more intuitive and understandable API to me. It's also simpler to enter - just one new field instead of a new struct. And follows the existing *Seconds pattern. I am not seeing the downside? Validation can be done on the total duration as well as the individual fields, can't it?
There was a problem hiding this comment.
Clients that don't recognise the new field would be liable to trigger much too early, right? How do we avoid that?
Also, how do we avoid setting a precedent that this riskier approach is the one to take.
There was a problem hiding this comment.
Yeah, that's true. The lack of versioning at this granularity is a problem. Of course the "offset" has a similar problem (inaccurate interval from old clients). Here's another (maybe bad) idea:
on write:
- if *Milliseconds is set, then set *Seconds = ceil(*Milliseconds/1000)
- otherwise, set *Milliseconds = *Seconds * 1000
Then, on read, both will be set, and old clients will have a slightly longer interval.
|
|
||
| Pros: | ||
| Cons: | ||
|
|
There was a problem hiding this comment.
Another alternative is to use "Duration", like PeriodDuration: 1.5s or PeriodDuration: 1500ms or PeriodDuration: 1s500ms. But AFAIK we don't do that anywhere else, it would be a new type.
There was a problem hiding this comment.
The existing API guidelines say
Duration fields must be represented as integer fields with units being part of the field name
so if for some of those options, if we adopt them I imagine that we also need to change the API guidelines.
| ```yaml | ||
| ... | ||
| periodSeconds: 1 | ||
| periodSecondsOffset: |
There was a problem hiding this comment.
I find this very unintuitive and verbose as well.
Subsecond Probe OptionsThanks @sftim and @johnbelamaric for your reviews! Trying to summarize the options in one place (since it's a bit messy in the KEP at the moment), here's four options that have been discussed for configuring a probe to run on a subsecond interval: (edit: given that this discussion is still ongoing, probably better to use hackmd rather than having to continue to edit a comment in a PR...) |
|
(I've got the above in a hackmd ... if it's easier to edit there vs. doing this in issue comments, let me know and I'll add you both to it) |
|
@psschwei I suggested using I saw my proposal as distinct because it did fully avoid the inability to round-trip with a combined field. |
You're right, let me add it to the hackmd now. Sorry for the oversight; that's why I thought it would be good to put all the options in a single place... |
|
This KEP was read and discussed at this month's session of the KEP reading club, commenting here for discoverability purposes. There is a slack thread on |
mikebrow
force-pushed
the
subsecond-probes
branch
from
9b71650
to
513660c
Compare
|
The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs. This bot triages issues and PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale |
k8s-ci-robot
added
the
lifecycle/stale
|
/remove-lifecycle stale |
|
@k8s-triage-robot: Closed this PR. In response to this:
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/test-infra repository. |
|
/reopen |
|
@mikebrow: Reopened this PR. In response to this:
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/test-infra repository. |
|
@mikebrow: 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/test-infra repository. I understand the commands that are listed here. |
k8s-ci-robot
added
cncf-cla: no
|
/remove-lifecycle rotten |
k8s-ci-robot
added
cncf-cla: yes
|
@mikebrow Please sign the CLA and fix failing CI tests, thanks. |
|
/cc |
k8s-ci-robot
added
cncf-cla: no
|
Is this KEP still alive? |
|
Hi. --- a/keps/sig-node/3066-subsecond-probes/README.md
+++ b/keps/sig-node/3066-subsecond-probes/README.md
@@ -48,10 +48,10 @@ Probe timeouts are limited to seconds and that does NOT work well for clients lo
- [Implementation History](#implementation-history)
- [Drawbacks](#drawbacks)
- [Alternatives](#alternatives)
- - [<code>early*Offset</code>](#)
- - [<code>*Duration</code>](#-1)
- - [A new struct, <code>ProbeOffset</code>](#a-new-struct-)
- - [Setting the time units in a different field, <code>ReadSecondsAs</code>](#setting-the-time-units-in-a-different-field-)
+ - [<code>early*Offset</code>](#earlyoffset)
+ - [<code>*Duration</code>](#duration)
+ - [A new struct, <code>ProbeOffset</code>](#a-new-struct-probeoffset)
+ - [Setting the time units in a different field, <code>ReadSecondsAs</code>](#setting-the-time-units-in-a-different-field-readsecondsas)
- [v2 api for probe.](#v2-api-for-probe)
- [OffsetMilliseconds](#offsetmilliseconds)
- [Reconcile seconds field to nearest whole second.](#reconcile-seconds-field-to-nearest-whole-second) |
was paused for 1.31 will bring it back to life for 1.32! |
There was a problem hiding this comment.
I have a feeling this may end up in the same place that fast-restart are approaching -- some node-level "budget" for probes, with an HTB to provide some "fairness", such that the probe's period is a request which will be satisfied when possible, but may be delayed when the node is under contention. E.g. if every pod asks for sub-second probes, we could end up doing hundreds of QPS of traffic just on probing.
|
|
||
| There are a few corner cases around default (effective period) values: | ||
|
|
||
| `periodSeconds: 0` and `periodMilliseconds: 500` would be 10.5s / 10500ms (as 0 => 10 for `periodSeconds` via [defaults.go](https://github.com/kubernetes/kubernetes/blob/3b13e9445a3bf86c94781c898f224e6690399178/pkg/apis/core/v1/defaults.go#L213-L215)) |
There was a problem hiding this comment.
I think we should fail in that case, no? Sorry, you can't use MS unless you also specify S. Any existing pod will still be OK, right?
There was a problem hiding this comment.
nod will update.. should fail, if one wants mills offset must say from any int except 0, because...
|
|
||
| `periodSeconds: 2` and `periodMilliseconds: -500` would be 1.5s / 1500ms. | ||
|
|
||
| `periodSeconds: 1` and `periodMilliseconds: -500` would be 0.5s / 500ms. (*** To reduce un-necessary resource usage, because periodMilliseconds is used to reduce the period to less than a second the offset is only used until success is reached then no longer becoming 1second after success.***) |
There was a problem hiding this comment.
Can we draw out the state-machine for probes and consider how this changes that, specifically in the context of startup, readiness, and liveness actually being different?
In the past we have added fields to probes that don't apply to all modes and it's gross.
|
I think a budget makes sense... we had proposed a type of throttling as a mitigation, but should be able to change that to a node-level budget |
|
I think it makes sense to have both a sane lower-bound (which can depend on probe type) and an overall budget. I think that if we can describe a "budget" semantic that balances requirements, we can use that same pattern in other places. I think requirements include:
@lauralorenz we had a very similar discussion wrt restarts. |
|
Hi @psschwei @mikebrow and any other authors -- yes as @thockin mentions, I'm trending towards needing a budget and something that can admit a restart request that is determined in budget for CrashLoopBackOff (#4603) longer term. The restart I'm trying to budget for would be queuing against kubelet directly as the operation is done inline of the kubelet runtime's SyncPod loop. Yours is a little different in that the restart of interest is queueing into to the probe worker's runtime, or perhaps earlier as to whether to allocate a probe worker in the first place (?). Either way it's still taking up resources from the kubelet component so I think we would use somewhat the same kubelet stats to hand out budget and should work together to define it! Let's hang |
Signed-off-by: Mike Brown brownwm@us.ibm.com