KEP-1972: kubelet exec probe timeouts

[andrewsykim]

Signed-off-by: Andrew Sy Kim kim.andrewsy@gmail.com

Kubelet today does not respect exec probe timeouts. This is an unexpected behavior
since the timeout value is supported in the Container Probe API. Because exec probe timeouts
were never respected by kubelet, a new feature gate ExecProbeTimeouts will be introduced
so users have an easy way to revert back if the newly introduced probe timeout results
in unexpected behavior.

KEP issue: #1972

[andrewsykim]

/assign @SergeyKanzhelev

For initial review

[SergeyKanzhelev]

small nit: the feature gate is for ops. For developers addressing the issue would be an increase of a timeout. So "users" and the action these users do in this context can be clarified.

Perhaps:

Suggested change

	the timeout value is supported in the Container Probe API. Because exec probe timeouts
	were never respected by kubelet, a new feature gate `ExecProbeTimeouts` will be introduced
	so users have an easy way to revert back if the newly introduced probe timeout results
	in unexpected behavior.
	the timeout value is supported in the Container Probe API. Because exec probe timeouts
	were never respected by kubelet, a new feature gate `ExecProbeTimeouts` will be introduced.
	With this feature nodes can be configured to preserve the current behavior while the proper
	timeouts are configured for the probes.

[andrewsykim]

Updated

[SergeyKanzhelev]

perhaps "treat exec probe timeouts as probe failures in kubelet"

[andrewsykim]

done

[SergeyKanzhelev]

/lgtm

[dims]

@andrewsykim are we missing some discussion on TODO(s) for different runtimes / dockershim?

[andrewsykim]

@dims I don't think so, I think the two TODOs that came out from this discussion were:

1. we should have CRI errors to handle things like timeout, this is going to be addressed in KEP: Custom errors in the CRI #1654
2. deprecating dockershim -- should also be addressed in a separate issue / KEP.

Both of those I imagine will be addressed in separate KEPs, this is one is to only fix exec probe timeouts

[andrewsykim]

/assign @dchen1107 @derekwaynecarr

[dims]

@andrewsykim sounds good. thanks!

[SergeyKanzhelev]

Another suggestion: should we increase scope of the KEP and also respect timeout in preStop exec hook?

kubernetes/kubernetes#92432

If so, the name of the flag may need to change to RespectExecTimeout to signify it's not only about probes. @andrewsykim WDYT?

[andrewsykim]

Another suggestion: should we increase scope of the KEP and also respect timeout in preStop exec hook?

I agree that we should fix this, but I think grace termination period and probe timeouts are separate concerns here. I know some of the efforts around side car containers were also trying to address this so I wouldn't want to overlap there too much. I might be missing context on this one though.

[SergeyKanzhelev]

Another suggestion: should we increase scope of the KEP and also respect timeout in preStop exec hook?

I agree that we should fix this, but I think grace termination period and probe timeouts are separate concerns here. I know some of the efforts around side car containers were also trying to address this so I wouldn't want to overlap there too much. I might be missing context on this one though.

thinking more about it - I'd say even adding logic to respect the gracePeriodOverride would likely not be a breaking change so no flag is needed. So this KEP should be gtg

[aojea]

wow :/

[derekwaynecarr]

thanks for the write-up. agree with approach.

/approve
/lgtm

[dchen1107]

/lgtm
/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andrewsykim, dchen1107, derekwaynecarr

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:

• keps/sig-node/OWNERS [dchen1107,derekwaynecarr]

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