KEP-4818: Allow zero value for Sleep Action of PreStop Hook

[sreeram-venkitesh]

• One-line PR description: Added the kep.yaml files and the initial readme file for KEP 4818.

• Issue link: Allow zero value for Sleep Action of PreStop Hook #4818

• Other comments: Discussion doc

[kannon92]

/cc

[kannon92]

You will need a PRR file also.

[SergeyKanzhelev]

can you please expand on whether kubelet accepts 0 today and it is only API server validaiton or kubelet also validates the value?

[sreeram-venkitesh]

Currently, kubelet accepts 0. No validation is done at the kubelet level. The feature gate for PodLifecycleSleepAction is used inside runSleepHandler function and it returns nil if the feature is disabled. We can do a similar check at the kubelet level but it doesn't really make sense since a sleep of zero seconds would be similar to returning nil right? Since the logic in kubelet already supports zero value (time.After(0)), I feel that we don't need the feature gate in kubelet. Also the feature gate check for PodLifecycleSleepAction would be removed in v1.32 since its planning to graduate to stable. Please let me know what you think!

Relevant function in kubelet's lifecyle/handlers.go

func (hr *handlerRunner) runSleepHandler(ctx context.Context, seconds int64) error {
	if !utilfeature.DefaultFeatureGate.Enabled(features.PodLifecycleSleepAction) {
		return nil
	}
	c := time.After(time.Duration(seconds) * time.Second)
	select {
	case <-ctx.Done():
		// unexpected termination
		return fmt.Errorf("container terminated before sleep hook finished")
	case <-c:
		return nil
	}
}

[sreeram-venkitesh]

Addressed all the above comments, please let me know if there are any more tests that I can add!

[SergeyKanzhelev]

lgtm, with couple of items to address.

[kannon92]

/cc

[sreeram-venkitesh]

@SergeyKanzhelev @kannon92 Thank you so much! I've resolved the comments and added you two as approver and reviewer. I still need to add the PRR approver, but I first pinged wojtek-t (who is the PRR approver for KEP 3960) in Slack to confirm with him first that I can add him as the PRR approver for this KEP too. I will update this field once he confirms.

[SergeyKanzhelev]

/lgtm
/approve

You still need PRR approver to be listed in 4818.yaml

[sreeram-venkitesh]

@SergeyKanzhelev I've added wojtek as the PRR approver. Please approve the PR again since there are new changes. Thanks a lot!

[SergeyKanzhelev]

/lgtm

[SergeyKanzhelev]

/assign @wojtek-t

[kannon92]

@SergeyKanzhelev I don't think your owner file worked here.

@mrunalp @dchen1107 can you help here?

[SergeyKanzhelev]

@SergeyKanzhelev I don't think your owner file worked here.

@mrunalp @dchen1107 can you help here?

Yes, it will start working once merged. Also we need @wojtek-t 's PRR review

[wojtek-t]

One minor comment - other than that LGTM from PRR perspective.

[sreeram-venkitesh]

@wojtek-t: I've fixed the section on downgrading, I missed that earlier! Thanks for catching it. For #4855 (comment), I've added a line mentioning this test case under the list of unit tests planned to be implemented in alpha.

@pacoxu: I've updated the validation error message to mention the feature gate. Will update this in kubernetes/kubernetes#127094.

Please take a look! Thanks folks!

[SergeyKanzhelev]

/lgtm

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: mrunalp, SergeyKanzhelev, sreeram-venkitesh
Once this PR has been reviewed and has the lgtm label, please ask for approval from wojtek-t. 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 [mrunalp]

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

[pacoxu]

/lgtm