Signed-off-by: Mike Brown [email protected]

• One-line PR description: Adding new KEP to support Subsecond Probes.
• Issue link: #3066
• Other comments: wip

Skipping CI for Draft Pull Request. If you want CI signal for your change, please convert it to an actual PR. You can still manually trigger a test run with /test all

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:

• future health check mechanisms that are really fast (maybe they establish a connection and then the subsequent checks are super low latency)
• admission restrictions or other mechanisms to require a minimum timeout (eg, any microsecond duration > 1000μs).

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 periodSeconds, do we want to support sub-second periods? If we do then a similar mechanism to what I proposed above (early failure) would work, but it would need a different name. We can bikeshed that name if needed, and we might want admission controls to enforce a minimum check interval (eg 0.1s - just speculating).

Subsecond Probe Options

Thanks @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 early*Offset as an extra field in the current API. I definitely wouldn't call that negative offset periodMilliseconds as that's surely liable to confuse people (early in the field name should be a massive clue). See https://github.com/kubernetes/enhancements/pull/3067#issuecomment-1039311016 for more details.

I saw my proposal as distinct because it did fully avoid the inability to round-trip with a combined field.

I suggested using early*Offset ...

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 #sig-architecture with questions/comments about this KEP that can be found here, thanks!

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:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue or PR as fresh with /remove-lifecycle stale
• Mark this issue or PR as rotten with /lifecycle rotten
• Close this issue or PR with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

/remove-lifecycle stale

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: mikebrow Once this PR has been reviewed and has the lgtm label, please assign johnbelamaric for approval by writing /assign @johnbelamaric in a comment. For more information see:The Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• keps/prod-readiness/OWNERS
• keps/sig-node/OWNERS

Approvers can indicate their approval by writing /approve in a comment Approvers can cancel approval by writing /approve cancel in a comment

Have we considered the status-updates flow velocity from kubelet to api-server? e.g., https://github.com/kubernetes/kubernetes/blob/master/pkg/kubelet/status/status_manager.go#L152

I understand the requirements. But please keep in mind that tighter loop probes still flow using the same velocity to api server.

Have we considered the status-updates flow velocity from kubelet to api-server? e.g., https://github.com/kubernetes/kubernetes/blob/master/pkg/kubelet/status/status_manager.go#L152

I understand the requirements. But please keep in mind that tighter loop probes still flow using the same velocity to api server.

nod.. benchmarks were using a kubelet web service for probe result timing.. but yes we also need to consider limits set based on the sync loop timer..

PRR looks good now, will wait for SIG approval before I approve

Could we merge this as provisional and record where there needs to be some refinement before we advance the KEP to implementable?

@smarterclayton if ok I'd like to move ahead with this as provisional while we vote and discuss the issue around overriding default or not, to have that decided before making the KEP implementable. If I had to pick a preference on the override default thoughts I liked your idea for allowing "the value to be tuned per kubelet with a reasonably shorter default that doesn't impact CPU dramatically" and to "clearly communicate to users that nodes / health checks may be run less frequently than requested, but don't explicitly document to users a public guarantee."

IOW for period kubelet could tune to somewhere between 1 and N (10sec legacy) for a default period.. and further have a different default based on type exec/grpc/http and still further based on node resource pressure..

I am fine with that, my comments basically were:

• not realizing that we can't safely set default 0 on period because api would be incompatible (good for this to be explicitly called out: we cannot change the range of allowable returned values for period seconds to client)
• once i understood that, looking for the simplest way to communicate that in the api itself - we are adding a field that modifies another field and users need to clearly understand how the two fields interact
• finally, clearly communicating that implementations reserve the right to determine the exact effective period based on the cost of performing the check, and how we would suggest that

I'd like a bit of cleanup in the KEP to make those points clearer, but other than that I don't see an obstacle to attempt this this release in terms of riskiness of feature.

I am fine with moving ahead with this as provisional and we close the remaining gaps.

I am fine with moving ahead with this as provisional and we close the remaining gaps.

agreed.. nits/recommended changes are incorporated will follow up remaining gaps with a new PR on the KEP.

What needs to happen next on this PR?

@sftim , assuming the relevant people sign off, I think it's a question of does this get merged as provisional once the v1.27 window opens and then we a follow-up PR get get it to implementable, or do we make the changes now and once they are in merge as implementable?

I probably can't touch this until next month due to PC commitments, but I don't have a strong preference either way.

cc @mikebrow

just a heads up that httpprobes (and probably tcp probes) using 1 second for 100 pods are able to cause issues with conntrack and sockets

https://github.com/kubernetes/kubernetes/issues/89898

I'm still investigating, but the point is we should have clear testing of what we currently support and how much we are going to support, it somes that right now there is zero testing of this area

The Kubernetes project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

• Mark this PR as fresh with /remove-lifecycle stale
• Close this PR with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

The Kubernetes project currently lacks enough active contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

• Mark this PR as fresh with /remove-lifecycle rotten
• Close this PR with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

@SergeyKanzhelev - is this planned for 1.28 for SIG-Node (I'm asking in the context of making PRR)

/remove-lifecycle rotten

/label api-review

@smarterclayton you have started looking at this PR long time ago. Do you think you can approve this PR this week from API perspective?

Talked with @mrunalp - API review approval seems to be a long poll here and we can take it if can get ack from @smarterclayton or somebody.

last commit added to address PRR comments https://github.com/kubernetes/enhancements/issues/3066#issuecomment-1593271805

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: mikebrow Once this PR has been reviewed and has the lgtm label, please ask for approval from wojtek-t and additionally assign mrunalp for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• keps/prod-readiness/OWNERS
• keps/sig-node/OWNERS

Approvers can indicate their approval by writing /approve in a comment Approvers can cancel approval by writing /approve cancel in a comment

@soltysh thx for the review will update tomorrow

The Kubernetes project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

• Mark this PR as fresh with /remove-lifecycle stale
• Close this PR with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale