Add pod disruption conditions for kubelet-initiated failures #112360

mimowo · 2022-09-09T15:29:29Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

Which issue(s) this PR fixes:

Tracking issue: Retriable and non-retriable Pod failures for Jobs enhancements#3329

Special notes for your reviewer:

We limit this PR to cover a subset of pod failure scenarios described in the KEP. In particular, the following scenarios are excluded:

DisruptionTarget for admission failures (draft implementation in PR: Handling pod failures due to kubelet admissions failures #113399)
ResourceExhausted for oom killer and exceeding of local storage limits (draft implementation in PR: Add ResourceExhausted pod condition for oom killer and exceeding of local storage limits #113436)

This is due to the following complications:

admission failures are non-retriable as all nodes in a cluster would have the same configuration, resulting in admission failures. A dedicated pod condition AdmissionFailures can be considered the future.
ResourceExhausted pod condition, based on OOM killer might be unreliable in some environments, because:
- OOM killer may potentially kill a container which does not exceed the memory limits
- on some environments (CRI-O with cgroupsv2) the reason field for the oom killed container is set as Error rather than the expected OOMKilled.

The KEP will be updated to reflect the decisions made during implementation phase for Beta: kubernetes/enhancements#3646

Does this PR introduce a user-facing change?

Kubelet adds the following pod failure conditions:
- DisruptionTarget (graceful node shutdown, node pressure eviction)

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

- [KEP]: https://github.com/kubernetes/enhancements/tree/master/keps/sig-apps/3329-retriable-and-non-retriable-failures

k8s-ci-robot · 2022-09-09T15:29:31Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

k8s-ci-robot · 2022-09-09T15:29:37Z

@mimowo: This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

fedebongio · 2022-10-04T20:21:24Z

/remove-sig api-machinery

mimowo · 2022-10-10T16:55:12Z

/ok-to-test

mimowo · 2022-11-07T15:06:48Z

/retest

k8s-ci-robot · 2022-11-07T15:18:53Z

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

Test name	Commit	Details	Required	Rerun command
pull-kubernetes-node-kubelet-serial-crio-cgroupv1	193210fe945f9e9f531602887e5078a6590971fb	link	false	`/test pull-kubernetes-node-kubelet-serial-crio-cgroupv1`

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.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

… skip condition update instead.

alculquicondor · 2022-11-07T15:36:26Z

Update to pkg/controller LGTM

bobbypage · 2022-11-07T21:17:42Z

pkg/kubelet/status/status_manager.go

+				// there might still be running containers to avoid sending an unnecessary PATCH request if the
+				// actual transition is delayed (see below)
+				if transitioningToTerminalPhase && !couldHaveRunningContainers {
+					// update the LastTransitionTime


nit: consider to note that the reason of why we need to update LastTransitionTime for this condition again here is because the older transition time set in updateStatusInternal is likely stale because the status update was not sent until the all the containers have terminated.

Added a comment in the follow up PR: #113718

pkg/kubelet/types/pod_status.go

bobbypage · 2022-11-07T21:22:45Z

pkg/kubelet/status/status_manager.go

 	for _, c := range newPodStatus.Conditions {
 		if kubetypes.PodConditionByKubelet(c.Type) {
 			podConditions = append(podConditions, c)
+		} else if kubetypes.PodConditionSharedByKubelet(c.Type) {


Thanks for updating the logic like this -- this is much cleaner than the previous approach of "reverting" the condition.

The logic now is: we replace/append all the "shared kubelet" conditions, and guard the AlphaNoCompatGuaranteeDisruptionTarget condition with transitioningToTerminalPhase && !couldHaveRunningContainers to ensure it will only be sent once all containers have terminated.

Thanks for point this out with the details.

nit: Can we include above the logic as the comment for future maintainence?

Added a comment in the follow up PR: #113718

bobbypage · 2022-11-07T21:24:25Z

Thanks for all the updates and refactoring the status updates in status manager, the updated logic is much more clear.

LGTM from me for SIG node related areas for pkg/kubelet/ and test/e2e_node/

/lgtm

dchen1107 · 2022-11-07T21:57:30Z

/lgtm
/approve from SIG Node

You can send the followup PR for the nits.

liggitt · 2022-11-07T22:09:22Z

/approve

for API constant and doc change

k8s-ci-robot · 2022-11-07T22:09:32Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dchen1107, liggitt, mimowo

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~pkg/OWNERS~~ [dchen1107,liggitt]
~~pkg/apis/OWNERS~~ [liggitt]
~~staging/src/k8s.io/api/OWNERS~~ [liggitt]
~~test/OWNERS~~ [dchen1107,liggitt]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot requested review from ncdc and tallclair September 9, 2022 15:30

mimowo force-pushed the handling-pod-failures-beta-kubelet branch 2 times, most recently from c870a7a to 1658d03 Compare September 12, 2022 07:36

SergeyKanzhelev added this to Triage in SIG Node PR Triage Sep 19, 2022

mimowo force-pushed the handling-pod-failures-beta-kubelet branch from 1658d03 to e994b27 Compare September 30, 2022 13:47

k8s-ci-robot added area/apiserver sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. labels Sep 30, 2022

k8s-ci-robot removed the sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. label Oct 4, 2022

mimowo force-pushed the handling-pod-failures-beta-kubelet branch from e994b27 to f3310f9 Compare October 10, 2022 11:58

k8s-ci-robot added the sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. label Oct 10, 2022

k8s-ci-robot added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label Oct 10, 2022

mimowo force-pushed the handling-pod-failures-beta-kubelet branch from b8b2ba4 to 1e2b674 Compare October 11, 2022 08:24

mimowo force-pushed the handling-pod-failures-beta-kubelet branch 2 times, most recently from eb3b989 to 07365e4 Compare November 7, 2022 07:46

Add pod disruption conditions for kubelet initiated failures

52cd675

mimowo force-pushed the handling-pod-failures-beta-kubelet branch 4 times, most recently from 2158b29 to e1c146f Compare November 7, 2022 14:27

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Nov 7, 2022

Do not revert the pod condition if there might be running containers,…

4e732e2

… skip condition update instead.

mimowo force-pushed the handling-pod-failures-beta-kubelet branch from e1c146f to 4e732e2 Compare November 7, 2022 15:22

bobbypage reviewed Nov 7, 2022

View reviewed changes

k8s-ci-robot assigned bobbypage Nov 7, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 7, 2022

k8s-ci-robot assigned dchen1107 Nov 7, 2022

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 7, 2022

k8s-ci-robot merged commit 47952e0 into kubernetes:master Nov 8, 2022

SIG Node CI/Test Board automation moved this from Archive-it to Done Nov 8, 2022

SIG Node PR Triage automation moved this from Triage to Done Nov 8, 2022

k8s-ci-robot added this to the v1.26 milestone Nov 8, 2022

mimowo mentioned this pull request Nov 8, 2022

Add comments to clarify the updated logic in kubelet's status_manager #113718

Merged

mimowo deleted the handling-pod-failures-beta-kubelet branch March 18, 2023 18:41

mikekap mentioned this pull request Jul 4, 2023

Mark terminal pods not ready before they finish shut down #117822

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add pod disruption conditions for kubelet-initiated failures #112360

Add pod disruption conditions for kubelet-initiated failures #112360

mimowo commented Sep 9, 2022 •

edited

k8s-ci-robot commented Sep 9, 2022

k8s-ci-robot commented Sep 9, 2022

fedebongio commented Oct 4, 2022

mimowo commented Oct 10, 2022

mimowo commented Nov 7, 2022

k8s-ci-robot commented Nov 7, 2022 •

edited

alculquicondor commented Nov 7, 2022

bobbypage Nov 7, 2022 •

edited

mimowo Nov 8, 2022

bobbypage Nov 7, 2022 •

edited

dchen1107 Nov 7, 2022

mimowo Nov 8, 2022

bobbypage commented Nov 7, 2022 •

edited

dchen1107 commented Nov 7, 2022 •

edited

liggitt commented Nov 7, 2022

k8s-ci-robot commented Nov 7, 2022

Add pod disruption conditions for kubelet-initiated failures #112360

Add pod disruption conditions for kubelet-initiated failures #112360

Conversation

mimowo commented Sep 9, 2022 • edited

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

k8s-ci-robot commented Sep 9, 2022

k8s-ci-robot commented Sep 9, 2022

fedebongio commented Oct 4, 2022

mimowo commented Oct 10, 2022

mimowo commented Nov 7, 2022

k8s-ci-robot commented Nov 7, 2022 • edited

alculquicondor commented Nov 7, 2022

bobbypage Nov 7, 2022 • edited

Choose a reason for hiding this comment

mimowo Nov 8, 2022

Choose a reason for hiding this comment

bobbypage Nov 7, 2022 • edited

Choose a reason for hiding this comment

dchen1107 Nov 7, 2022

Choose a reason for hiding this comment

mimowo Nov 8, 2022

Choose a reason for hiding this comment

bobbypage commented Nov 7, 2022 • edited

dchen1107 commented Nov 7, 2022 • edited

liggitt commented Nov 7, 2022

k8s-ci-robot commented Nov 7, 2022

mimowo commented Sep 9, 2022 •

edited

k8s-ci-robot commented Nov 7, 2022 •

edited

bobbypage Nov 7, 2022 •

edited

bobbypage Nov 7, 2022 •

edited

bobbypage commented Nov 7, 2022 •

edited

dchen1107 commented Nov 7, 2022 •

edited