Removed containers are not always waiting

[dashpole]

fixes #54499
The issue was that a container that is removed (during pod deletion, for example), is assumed to be in a "waiting" state.
Instead, we should use the previous container state.
Fetching the most recent status is required to ensure that we accurately reflect the previous state. The status attached to a pod object is often stale.

I verified this by looking through the kubelet logs during a deletion, and verifying that the status updates do not transition from terminated -> pending.

cc @kubernetes/sig-node-bugs @sjenning @smarterclayton @derekwaynecarr @dchen1107

Fix an issue where pods were briefly transitioned to a "Pending" state during the deletion process.

[dashpole]

/assign @dchen1107 @sjenning

[dashpole]

/unassign @feiskyer

[derekwaynecarr]

commented on the issue where i meant the pr...

can we also do the following:
"it would be good if syncPod in status manager protected against an invalid transfer of phase..."

prior to calling updateStatus? if it detects an invalid state transition, panic?

[derekwaynecarr]

/approve
/lgtm

[sjenning]

confirmed fix, lgtm

[k8s-cherrypick-bot]

Removing label cherrypick-candidate because no release milestone was set. This is an invalid state and thus this PR is not being considered for cherry-pick to any release branch. Please add an appropriate release milestone and then re-add the label.

[k8s-github-robot]

Automatic merge from submit-queue (batch tested with PRs 54597, 54593, 54081, 54271, 54600). If you want to cherry-pick this change to another branch, please follow the instructions here.

[joelsmith]

I think k8s-merge-robot misinterpreted @sjenning's intent and closed this when #54597 merged. Could you re-open this @dashpole?

[smarterclayton]

can we also do the following: "it would be good if syncPod in status manager protected against an invalid transfer of phase..."

I would like to also potentially guard or flag an error on the API server. If we prevent the transition, we could break components in unexpected ways, but if we don't prevent the transition, we can break EVERY workload controller.

[sjenning]

@smarterclayton

I would like to also potentially guard or flag an error on the API server. If we prevent the transition, we could break components in unexpected ways, but if we don't prevent the transition, we can break EVERY workload controller.

I don't have a problem reopening #54530 (actually have to create a new PR now because branch changed 😞)

Did you see @dchen1107 concern there?
#54530 (comment)

[derekwaynecarr]

i think @dchen1107 concern was not that validation was bad, but it alone was not sufficient as the kubelet would still have been unhappy. now that we have the kubelet checking, the fix identified, adding another guard on the apiserver itself also seems fine to me.

[dchen1107]

+1 on #54593 (comment) Once validation code is included, both API server and Kubelet will perform the same checking.

[dchen1107]

/lgtm

[k8s-github-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dashpole, dchen1107, derekwaynecarr

Associated issue: 54499

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

Needs approval from an approver in each of these OWNERS Files:

• pkg/kubelet/OWNERS [dchen1107,derekwaynecarr]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

[k8s-github-robot]

Automatic merge from submit-queue (batch tested with PRs 54593, 54607, 54539, 54105). If you want to cherry-pick this change to another branch, please follow the instructions here.