This test relies on sending a Delete request and wait for the pod not present in the namespace, this sounds very brittle to me, and the solution to consider very large windows of absorb any possible CI latency makes that we'll not be completely sure that the feature is working as expected.

@thockin don't we have any other method to validate this feature than relying on the overall time for the pod to disappear?

The main logic of the sleep hook is in the container lifecycle management code within kubelet, where it waits for a certain period when a container terminates. This means we may not have a way to monitor this process other than by observing the pod status.

However, instead of waiting for the pod to disappear from the namespace after deletion, there might be an alternative approach. This approach doesn't require the full lifecycle of creating and deleting the pod, which could yield higher accuracy than the current method.

We create a pod with a restartPolicy of Always and add a livenessProbe that will fail quickly. When the livenessProbe fails, kubelet will send a TERM signal to the container and start executing the sleep hook.

After the sleep hook executes, kubelet will increment the container's restart count by one. We only need to check the time interval between when the restart count reaches 1 and when the container enters the Running state.

An additional benefit of this method is that we don't need to verify if the time we capture falls between the sleep hook execution and the TerminationGracePeriodSeconds(This greatly reduces the probability of test failure). We don’t need to worry about the value of TerminationGracePeriodSeconds; it can be an arbitrarily large value.

A possible pod is:

apiVersion: v1
kind: Pod
metadata:
  name: auto-restart-pod
spec:
  terminationGracePeriodSeconds: 90
  containers:
  - name: auto-restart-container
    image: k8s.gcr.io/pause:3.2
    livenessProbe:
      tcpSocket:
        port: 80
      periodSeconds: 1
    lifecycle:
      preStop:
        sleep:
          seconds: 10
  restartPolicy: Always

/cc @SergeyKanzhelev @kannon92 WDYT?

Can we reuse the logic in https://github.com/kubernetes/kubernetes/blob/master/test/e2e/common/node/container_probe.go?

I think your sleep timelines are too short for a resource heavy CI and I think this will be brittle with 10s.

I think your sleep timelines are too short for a resource heavy CI and I think this will be brittle with 10s.

Yes, we can redesign this test based on this code.

I think your sleep timelines are too short for a resource heavy CI and I think this will be brittle with 10s.

10s is the time I set for local testing. We can make it longer in CI.

I don't have a better answer, sadly. This feature is fundamentally about sleeping. That said, the implementation of this is pretty trivial, so I think it's low risk of false-pass.

We can set the sleep pretty short (5-10s), the grace period very long (2-5m) and assert that the observed time was between sleep (O(seconds) and grace (O(minutes)). It doesn't need to be super tight, we just don't want to waste time.

Currently my implementation is same as @thockin suggest.
@HirazawaUi has given a great idea here, but I'm not sure if it's worth to implement this way here. We still need to check the duration in this way and can also show flaky?

Notice that although now we make a super long grace period, these tests generally don't last that long.
In the first test, we verify a normal sleepAction (15s), so deletion starts immediately after 15 seconds.
In the second test, we change the grace period to 15s, so deletion also starts immediately after 15 seconds.
In the third test, we test an exceptional case where sleepAction is ignored, and deletion proceeds immediately.

adding a finalizer and checking pod conditions can help us here ?
the finalizer will block the deletion of the pod object IIRC, is there any condition in the pod status that can serve us to calculate the sleep time?

is there any condition in the pod status that can serve us to calculate the sleep time?

I think only the .metadata.deletionTimestamp can help here?

commented on kubernetes/enhancements#3960 (comment)

why is this 40?

This should be little longer than sleep_seconds?
Changed to sleep_seconds*2.