Introducing Sleep Action for PreStop Hook

[AxeZhan]

Enhancement Description

• One-line enhancement description (can be used as a release note): Add Sleep Action for PreStop Hook
• Kubernetes Enhancement Proposal: https://github.com/kubernetes/enhancements/blob/master/keps/sig-node/3960-pod-lifecycle-sleep-action/
• Discussion Link:
  • related issue
  • google doc
• Primary contact (assignee): @AxeZhan @charles-chenzz
• Responsible SIGs: sig-node
• Enhancement target (which target equals to which milestone):
  • Alpha release target (x.y): 1.29
  • Beta release target (x.y): 1.30
  • Stable release target (x.y): 1.32
• Alpha
  • KEP (k/enhancements) update PR(s): KEP-3960: Introducing Sleep Action for PreStop Hook #3961
  • Code (k/k) update PR(s): Introducing Sleep Action for PreStop Hook kubernetes#119026
  • Docs (k/website) update PR(s):Add information about PodLifecycleSleepAction (KEP3960) website#43428
• Beta
  • KEP (k/enhancements) update PR(s): KEP-3960: promote to beta in 1.30 #4367
  • Code (k/k) update PR(s): [KEP 3960]: graduate PodLifecycleSleepAction to beta kubernetes#122456
  • Docs (k/website) update(s): graduate PodLifecycleSleepAction to beta (KEP 3960) website#45129
• Stable
  • KEP (k/enhancements) update PR(s):
  • Code (k/k) update PR(s):
  • Docs (k/website) update(s):

Please keep this description up to date. This will help the Enhancement Team to track the evolution of the enhancement efficiently.

[AxeZhan]

/sig node

[AxeZhan]

/cc @thockin

[SergeyKanzhelev]

/milestone v1.28

adding to the milestone for tracking

[Atharva-Shinde]

Hello @AxeZhan 👋, Enhancements team here.

Just checking in as we approach Enhancements freeze on Thursday, 16th June 2023.

This enhancement is targeting for stage alpha for 1.28 (correct me, if otherwise)

Here's where this enhancement currently stands:

• KEP readme using the latest template has been merged into the k/enhancements repo.
• KEP status is marked as implementable for latest-milestone: 1.28
• KEP readme has a updated detailed test plan section filled out
• KEP readme has up to date graduation criteria
• KEP has a production readiness review that has been completed and merged into k/enhancements.

For this KEP, we would need to update the following:

• Add approver/s to prod-readiness yaml file
• Change the status of the KEP to implementable
• Get approval from the PRR authors

The status of this enhancement is marked as at risk. Please keep the issue description up-to-date with appropriate stages as well. Thank you!

[Atharva-Shinde]

/stage alpha

[Atharva-Shinde]

Hey @AxeZhan,
We would just need to update the status inside kep.yaml to implementable instead of provisional as mentioned in my comment above. Please make the change within 24 hours else you would have to file an exception request.
Thanks :)

[SergeyKanzhelev]

this should be updated now. Since there were no material change, can we get it tracked for 1.28?

[Atharva-Shinde]

With all the KEP requirements in place and merged into k/enhancements, the status of this enhancement is now marked as tracked. Please keep the issue description up-to-date with appropriate stages as well. Thank you :)

[AdminTurnedDevOps]

Hey @AxeZhan

1.28 Docs Shadow here.

Does this enhancement work planned for 1.28 require any new docs or modification to existing docs?

If so, please follows the steps here to open a PR against dev-1.28 branch in the k/website repo. This PR can be just a placeholder at this time and must be created before Thursday 20th July 2023.

Also, take a look at Documenting for a release to get yourself familiarize with the docs requirement for the release.

Thank you!

[charles-chenzz]

hi @AdminTurnedDevOps , we currently working on the implement in k/k repos, when we finish it, we will start to add docs. Thanks.

[AxeZhan]

Hey @AdminTurnedDevOps
Thank you for your remind!

Does this enhancement work planned for 1.28 require any new docs or modification to existing docs?

This definitely needs some new docs :)
I've created a draft pr for place holder, and have updated the link in the Description above, we'll get back to finish the detail after the code changes are done.
We are not very familiar with updating the documentation, so we may need some assistance from your team when the time comes. Thank you in advance!

[AxeZhan]

Hi all, I've created kubernetes/kubernetes#119026 for some discussion(because I'm not certain of some implementations/e2e tests), I'd be super glad if some of you can have a look and give me a help😊。

[Atharva-Shinde]

Hey again @AxeZhan 👋

Just checking in as we approach Code freeze at 01:00 UTC Friday, 19th July 2023 .

Here’s the enhancement’s state for the upcoming code freeze:

• All the PRs that are related to your enhancement are linked in the above issue description (for tracking purposes). This includes code, tests, and documentation related PR/s.
• All code related PR/s are merged or are in merge-ready state ( i.e they have approved and lgtm labels applied) by the code freeze deadline. This includes any tests related PR/s too.

For this enhancement, it looks like the following code related PR/s are open and they need to be merged or should be in merge-ready state before the code freeze commences :

• Introducing Sleep Action for PreStop Hook kubernetes#119026

This is the code related PR/s that I found on this KEP issue

• Introducing Sleep Action for PreStop Hook kubernetes#119026

Please keep the issue description up-to-date with all the PR/s that are associated with this KEP and let me know if there are other PR/s in k/k we should be tracking for this KEP.

As always, we are here to help if any questions come up. Thanks!

[Atharva-Shinde]

Hello @AxeZhan 👋, 1.28 Enhancements Lead here.

Unfortunately, the implementation (code related) PR associated with this enhancement is not in the merge-ready state by code-freeze and hence this enhancement is now removed from the v1.28 milestone.

If you still wish to progress this enhancement in v1.28, please file an exception request. Thanks!

/milestone clear

[pacoxu]

/remove-stage alpha
/stage ga
The stage label is for target status.

[pacoxu]

/stage stable

[AlexanderYastrebov]

Hello.

In our usecase we add preStop sleep hook via admission webhook by default if preStop hook is not specified by the cluster user.
To opt-out from this we ask users to add a no-op preStop hook:

lifecycle:
  preStop:
    exec:
      command:
        - "true"

To remove dependency on true binary we tried:

lifecycle:
  preStop:
    sleep: 0

and discovered it does not pass validation:

Invalid value: 0: must be greater than 0 and less than terminationGracePeriodSeconds (30)

Is it possible to allow zero seconds value to enable no-op preStop hook?
Is there any other method maybe to define no-op preStop hook that does not require executing the binary?

[aojea]

can't you use an annotation?

[AlexanderYastrebov]

can't you use an annotation?

This is possible but my inquiry is more about allowing zero value.

E.g. go time.Sleep permits zero sleep duration:

A negative or zero duration causes Sleep to return immediately.

[thockin]

We could, I guess. It's not semantically wrong. We would have to add a feature gate to allow 0, do 1 alpha release, then move it to beta or GA. Needs a short KEP to track the gate.

…

On Thu, Jul 4, 2024, 9:02 AM Alexander Yastrebov ***@***.***> wrote: can't you use an annotation? This is possible but my inquiry is more about allowing zero value. E.g. go time.Sleep <https://pkg.go.dev/time#Sleep> permits zero sleep duration: A negative or zero duration causes Sleep to return immediately. — Reply to this email directly, view it on GitHub <#3960 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABKWAVF6EEXL4TMYHJ2PUKTZKVWZJAVCNFSM6AAAAAAXHWZB7WVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDEMBZGI4DGOBWGM> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[aojea]

We could, I guess. It's not semantically wrong, I guess.

Prestop is already a pointer

https://github.com/kubernetes/kubernetes/blob/c87c06d7ffb932395f963bc78f03f9d2f0e57698/pkg/apis/core/types.go#L2598-L2614

agree is semantically correct, but the motivation is the one that I'm a bit confused

In our usecase we add preStop sleep hook via admission webhook by default if preStop hook is not specified by the cluster user.
To opt-out from this we ask users to add a no-op preStop hook:

the logic implemented seems to be "by default I will add a hook, if you want to opt-out , just add one that is a noop"
It seems simpler/easier to indicate "use annotation i.do.not.want.you.to.default.my.prestop/hook if you want to opt-out of the default hook"

[thockin]

If the logic was:

if lifecycle.preStop is not set {
    set preStop to sleep(10)
}

Then any user can set preStop, whether to sleep(0) or sleep(3) or exec or whatever. I agree it's a little odd to use this as a trigger, but it seems consistent to support it. It' s sort of morally equivalent to a lifecycle.preStop.doNothing, but less ugly :)

[aojea]

who wants to create the KEP 😄 ???

This change will need to be feature gated, it is a small KEP, but it will require some work

[AlexanderYastrebov]

Does it have to be a KEP and a feature gate at all?

It seems that zero value was already a part of the original KEP, e.g. here I read:

• Test that the validation returns an error when given an invalid duration value (e.g., a negative value).
• Test that the runSleepHandler function returns immediately when given a duration of zero.

[thockin]

It seems that zero value was already a part of the original KEP

oops! It looks like the implementation demands >0 when it should have been >=0.

Does it have to be a KEP and a feature gate at all?

Unfortunately yes. In particular, we need to handle this case:

• Suppose this change goes into 1.31
• Admin rolls out 1.31
• User uses this feature and sets it to 0
• Admin rolls back to 1.30
• The pods/deployments/etc which have this value as 0 will fail to validate

Demanding 1 release with an alpha gate means you CAN roll back and it will be tolerated.

[sreeram-venkitesh]

@aojea @thockin I'd be interested in driving this if @AlexanderYastrebov isn't picking it up already!

[AlexanderYastrebov]

We went ahead with a sleep 1 as the next best thing for now:

lifecycle:
  preStop:
    sleep:
      seconds: 1 # not possible to set to 0: https://github.com/kubernetes/enhancements/issues/3960#issuecomment-2208556397

@aojea @thockin I'd be interested in driving this if @AlexanderYastrebov isn't picking it up already!

I did not pick it up but curious to learn the amount of effort required to fix such a minor bug in KEP implementation.

[szuecs]

@sreeram-venkitesh can you just add @AlexanderYastrebov to whatever you propose as changes to fix it (KEP and PR)?

As far as I understand @AlexanderYastrebov could learn this way and @AlexanderYastrebov can review the change proposals. Please correct me if I am wrong. :)

[sreeram-venkitesh]

@szuecs Looks like we'd need to roll this out as a different KEP as mentioned by @thockin in #3960 (comment) so that we can roll back the change if needed.

[szuecs]

@sreeram-venkitesh yes!
I just refer to #3960 (comment) and I think my colleague @AlexanderYastrebov (hey welcome my colleague!) was trying to say that you can go for it to create KEP and PR.
Just say if you need our help.

[aojea]

Example of a KEP to relax validation https://github.com/kubernetes/enhancements/tree/master/keps/sig-network/4427-relaxed-dns-search-validation

[sreeram-venkitesh]

@szuecs Ah sorry! I thought you were talking about adding the change in the same KEP! Thanks for clarifying! I will ping you both once I have something ready.

@aojea Since we're past the enhancements freeze, this new offshoot KEP can target for alpha in v1.32 right? I can put something together soon and keep the KEP ready before the next release cycle starts. What do you think?

[szuecs]

@sreeram-venkitesh we don't need to rush this, v1.32 looks fine.

[thockin]

Are we allowed to have a KEP that has multiple Gates? I think so right? If so, adding it to this KEP is probably a bit simpler, but it messes a little bit with the life cycle tracking where we tend to think of a single KEP as alpha, beta, GA.

…

On Mon, Jul 8, 2024, 6:50 AM Sandor Szücs ***@***.***> wrote: @sreeram-venkitesh <https://github.com/sreeram-venkitesh> we don't need to rush this, v1.32 looks fine. — Reply to this email directly, view it on GitHub <#3960 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABKWAVDNEFFYXTN4BNJ5TP3ZLKKLXAVCNFSM6AAAAAAXHWZB7WVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDEMJUGEZTGOBXGY> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[sreeram-venkitesh]

@thockin Since this KEP would be graduating from beta to GA in 1.32, would adding another gate in the same KEP require another round of beta then? Else the new feature gate would be going straight to GA right?

[thockin]

In theory, the one gate could go GA while the other did not. It's not COMMON, and this one is small, so I guess a new KEP is easy enough.

[kannon92]

For the new KEP, let's get the issue created so we can discuss separate from this one.

Looking at this KEP, it sounds like someone wants to promote this to stable in 1.32.

I will move this to considered for release for sig-node.

[sreeram-venkitesh]

@kannon92 I'll tag in the issue for the new KEP once I open it. I've been drafting the KEP doc and was planning to open the issue soon.

[thockin]

Why not GA this? I mean, it's useful as is, the "allow 0" KEP can follow.