force-delete pod execute prestop hook about kubernetes HOT 8 OPEN

I think it's the change in #115835 that causes to break the original implementation of the feature.

Although we didn't explain it (preStopHook will not be executed when gracePeriod is 0) in Forced Pod termination and Container Lifecycle Hooks documentation.

I'll fix this and amend the documentation to state that preStopHook is not run when we force delete pod using --grace-period=0.

/triage accepted
/priority important-soon

from kubernetes.

I think it's the change in #115835 that causes to break the original implementation of the feature.

Although we didn't explain it (preStopHook will not be executed when gracePeriod is 0) in Forced Pod termination and Container Lifecycle Hooks documentation.

I'll fix this and amend the documentation to state that preStopHook is not run when we force delete pod using --grace-period=0.

/triage accepted /priority important-soon

Hi @HirazawaUi and all, I am also concerned about what the expected behavior is when the --force parameter is given.

• Based on the description in document Container Lifecycle Hooks as below, my understanding is that the invocation of PreStop is unconditional:

PreStop

This hook is called immediately before a container is terminated due to an API request or management event such as a liveness/startup probe failure, preemption, resource contention, and others....

Btw, no changes on the statement from 1.25 to 1. 29.

• In addition, in document pod-termination-forced it is said that "...still be given a small grace period before being force killed":

When a force deletion is performed, the API server does not wait for confirmation from the kubelet that the Pod has been terminated on the node it was running on. It removes the Pod in the API immediately so a new Pod can be created with the same name. On the node, Pods that are set to terminate immediately will still be given a small grace period before being force killed.

It is not mentioned that how long the “small grace period” is. Here I am just thinking that the behavior prior to fix implemented in #115835 did not align with this statement while the new behavior does, or at least to some extent.

Looking forward to clarification from the expert group.

Many thanks.

from kubernetes.

/cc

from kubernetes.

/sig node

from kubernetes.

Let me do some research.
/assign

from kubernetes.

In PR #115835, we use gracePeriodOverride to override gracePeriod before executing the preStop hook, which brings two destructive changes.

a). When the pod is force delete using --grace-period=0, we will not execute the preStop hook.
b). The result of the calculateEffectiveGracePeriod function will always be applied to the preStop hook.

For a, we should fix it, it breaks the original logic,
We always set a value greater than or equal to 1 for gracePeriod in the calculateEffectiveGracePeriod function.
ref:

Although I saw an explanation for this behavior, I'm still curious as to why we need to set gracePeriod to 1, because I saw in killContainer that when gracePeriod is less than minimumGracePeriodInSeconds, we always set it to the value of minimumGracePeriodInSeconds
ref:

For me, the solution to this issue is to have calculateEffectiveGracePeriod not set gracePeriod to a value greater than or equal to 1, or we don't override gracePeriod when pod.DeletionGracePeriodSeconds=0

For b, I think we did the right thing, but now there seems to be some bugs in the calculateEffectiveGracePeriod function (when the pod is evicted or force deleted, the value of terminationGracePeriodSeconds is used as the time to delete the pod). I have seen a PR in fix it, like: #119570

@smarterclayton @bobbypage May I ask if I'm right?

from kubernetes.

/remove-priority important-soon
/priority important-longterm

from kubernetes.

@HirazawaUi: The label(s) priority/ cannot be applied, because the repository doesn't have them.

from kubernetes.