/test pull-kubernetes-e2e-kind-ipv6

/triage accepted
/priority important-soon
/assign @bobbypage

/test pull-kubernetes-e2e-gce-serial
/test pull-kubernetes-cos-cgroupv2-containerd-node-e2e-serial
/test pull-kubernetes-node-kubelet-serial-containerd

@bobbypage @smarterclayton ready for another pass. I've tested the latest version with 50 repeats of the new tests locally and there were no failures.

/test pull-kubernetes-e2e-kind-ipv6

/test pull-kubernetes-node-kubelet-serial-containerd

@mimowo: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Looks good to me. I don't have any substantitive comments on the tests, other than "you saw them fail if your fix is commented out"?

Looks good to me. I don't have any substantitive comments on the tests, other than "you saw them fail if your fix is commented out"?

The first test, for the main scenario, fails consistently without the fix.

The second, for the non-admissible runtime pod, passes as it was already covered by HandlePodCleanups, but it seems valuable to test this non-obvious modification of the main scenario.

Also lgtm on the changes, the change to use pod worker update with SyncPodKill is much cleaner then having to call into TerminatePod from within HandlePodCleanups.

The only thing I saw when testing out the changes here is there is a ~10s delay for the pod from the pod being rejected at kubelet to the pod being deleted on the apiserver. This is because the following flow happens:

(1) The pod is rejected by kubelet at admission and the local status manager is updated with terminal phase
(2) We wait 10s until the next syncBatch completes in status_manager
(3) status manager sees that the local phase status is different then api status, it updates phase to failed
(4) HandlePodCleanups will run, pod is filtered as part of filterTerminalPodsToDelete and the SyncPodKill will be delivered to it.

IMO, the 10s latency is not big deal... it will reconcile eventually, but if we wanted it to update faster and reduce latency, I think if we also called kl.podWorkers.UpdatePod(UpdatePodOptions with UpdateType: kubetypes.SyncPodKill in rejectPod it would update the status ~instantly, without waiting for the 10sec sync batch (i.e. as https://github.com/kubernetes/kubernetes/pull/118614/files).

So maybe long term, we should do both, i.e. call UpdatePod in HandlePodCleanups and rejectPod for these types of pods that are rejected at admission and don't exist in pod worker. But I don't think should be blocking the PR (and can be handled as a followup) as the case here is not super common to hit in the first place, and the latency is not critical IMO.

/lgtm

LGTM label has been added.

Thanks @bobbypage for the summary. The scenario is very unlikely, so I don't think 10s makes a difference, but it is a judgment call.

I have looked if there are other scenarios possible to arrive with pods which satisfy the conditions: not runtime pods, terminal, with deletion timestamp and not in pod workers, but the scenario reported is the only I could come up with. This is probably because rejectPod is the only path in HandlePodAdditions which doesn't register the pod in pod workers.

So, this is why the fix https://github.com/kubernetes/kubernetes/pull/118614/files (to call SyncPodKIll from rejectPod( should also work. As for which one is better, the margins are small. The other fix reduces latency, but is specific to rejected pods, so potentially there might be a scenario we couldn't think about, which somehow bypasses HandlePodAdditions, but I couldn't find despite attempts.

IIUC if we combine the two fixes, then the code in HandlePodCleanups is dead, so I would prefer to decide on one of the two approaches (unless we have a scenario to support the need for both). If we are fine with latency then the generic one is preferred for me, otherwise the fix on rejectPod.

Let me know what you think, I'm out next week and will be back on the 5th of July.

Thanks @mimowo for the explanation, +1, I agree this scenario is unlikely to be hit and the 10s latency is not critical, so I'm lgtm to go with this approach.

Since we plan on pulling admission closer to pod worker anyway, rejection probably becomes a pod worker responsibility and that should fix the issue (but we still need resync for restarts anyway).

/lgtm too
/approve

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mimowo, smarterclayton

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Hi Team,

Same issue here, may i know the issue fixed in which release?

Same issue here, may i know the issue fixed in which release?

This was fixed in 1.28 (this PR), and cherry-picked on 1.27.4 in this PR #118841.

See in release notes: https://github.com/kubernetes/kubernetes/blob/master/CHANGELOG/CHANGELOG-1.27.md#changelog-since-v1273

"Fix deletion of non-admissible pods that are deleted during Kubelet restart (#118841, @bobbypage) [SIG Node and Testing]"

Same issue here, may i know the issue fixed in which release?

This was fixed in 1.28 (this PR), and cherry-picked on 1.27.4 in this PR #118841.

See in release notes: https://github.com/kubernetes/kubernetes/blob/master/CHANGELOG/CHANGELOG-1.27.md#changelog-since-v1273

"Fix deletion of non-admissible pods that are deleted during Kubelet restart (#118841, @bobbypage) [SIG Node and Testing]"

Got it, thanks for your time and have a good day.