Enable the "Retriable and non-retriable pod failures for jobs" feature into beta #113360

mimowo · 2022-10-26T13:31:44Z

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:

It depends on the following PRs:

Does this PR introduce a user-facing change?

Enable the "Retriable and non-retriable pod failures for jobs" feature into beta

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-triage-robot · 2022-10-26T13:39:22Z

This PR may require API review.

If so, when the changes are ready, complete the pre-review checklist and request an API review.

Status of requested reviews is tracked in the API Review project.

alculquicondor · 2022-10-26T15:53:15Z

Can you link all the PRs that this depends on?

mimowo · 2022-10-27T06:53:05Z

/retest

mimowo · 2022-10-27T06:59:48Z

Can you link all the PRs that this depends on?

Done

mimowo · 2022-10-28T10:05:06Z

/retest

liggitt

API constant change lgtm

a couple questions about the patch tests and one request on the default RBAC role if possible

liggitt · 2022-11-08T18:28:08Z

pkg/controller/nodelifecycle/scheduler/taint_manager_test.go

@@ -327,6 +327,7 @@ func TestCreateNode(t *testing.T) {
 		description  string
 		pods         []v1.Pod
 		node         *v1.Node
+		expectPatch  bool


should this verify anything about the content of the patch?

Answered below

liggitt · 2022-11-08T18:28:12Z

pkg/controller/nodelifecycle/scheduler/taint_manager_test.go

@@ -766,6 +770,7 @@ func TestEventualConsistency(t *testing.T) {
 		newPod       *v1.Pod
 		oldNode      *v1.Node
 		newNode      *v1.Node
+		expectPatch  bool


should this verify anything about the content of the patch?

This a unit test and the logic to generate the patch content is beyond Taint Manager, which prepares and sends the request:

kubernetes/pkg/controller/nodelifecycle/scheduler/taint_manager.go

Line 138 in c288251

if _, err := c.CoreV1().Pods(pod.Namespace).ApplyStatus(ctx, podApply, metav1.ApplyOptions{FieldManager: fieldManager, Force: true}); err != nil {

. On the other hand preparing the apply request is the responsibility of Taint Manager so testing the patch would verify the logic. However, this is also a pattern in other Taint Manager tests, already ticketed: #111612. If you think this is the right approach I would work on this as a follow up PR.

Test coverage for the feature would not change much as we already have integration tests which verifies the condition is added:

kubernetes/test/integration/node/lifecycle_test.go

Line 171 in c288251

_, cond := podutil.GetPodCondition(&testPod.Status, v1.AlphaNoCompatGuaranteeDisruptionTarget)

. An e2e test for the path is also introduced in this PR.

The patch would be a slice of bytes. I'm fine not checking it, given that we have integration tests.

Ok, closed there related issue with a comment: #111612 (comment)

liggitt · 2022-11-08T19:26:35Z

pkg/scheduler/framework/plugins/defaultpreemption/default_preemption_test.go

@@ -337,6 +337,9 @@ func TestPostFilter(t *testing.T) {
 			}
 			// As we use a bare clientset above, it's needed to add a reactor here
 			// to not fail Victims deletion logic.
+			cs.PrependReactor("patch", "pods", func(action clienttesting.Action) (bool, runtime.Object, error) {


it's surprising these don't need to handle the patch/delete requests in any way

This unit test focuses asserts on nominating the node for preemption. It does not assert the modifications done to the pod so it ignores the DELETE (and now also PATCH) action.

As a proposal I pushed a new commit implementing another approach (I find simple and more powerful) to index the potential victim pods in the fake client so that the test does not fail trying to patch non-existing pods (proposed in the new commit).

@alculquicondor @soltysh PTAL

plugin/pkg/auth/authorizer/rbac/bootstrappolicy/testdata/controller-roles.yaml

…victim pod

…pods

mimowo · 2022-11-09T11:30:10Z

/retest

liggitt · 2022-11-09T13:27:55Z

RBAC and API changes lgtm

will defer to area approvers for ack of patch test at #113360 (comment)

alculquicondor · 2022-11-09T13:27:16Z

pkg/controller/nodelifecycle/scheduler/taint_manager_test.go

@@ -766,6 +770,7 @@ func TestEventualConsistency(t *testing.T) {
 		newPod       *v1.Pod
 		oldNode      *v1.Node
 		newNode      *v1.Node
+		expectPatch  bool


The patch would be a slice of bytes. I'm fine not checking it, given that we have integration tests.

alculquicondor · 2022-11-09T13:28:47Z

pkg/scheduler/framework/plugins/defaultpreemption/default_preemption_test.go

+			for _, pod := range tt.pods {
+				podItems = append(podItems, *pod)
+			}
+			cs := clientsetfake.NewSimpleClientset(&v1.PodList{Items: podItems})


great, this seems better.

alculquicondor · 2022-11-09T13:29:08Z

/lgtm

liggitt · 2022-11-09T13:31:49Z

/approve

k8s-ci-robot · 2022-11-09T13:32:23Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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/apis/OWNERS~~ [liggitt]
~~pkg/controller/OWNERS~~ [liggitt,soltysh]
~~pkg/features/OWNERS~~ [liggitt,soltysh]
~~pkg/kubelet/OWNERS~~ [liggitt]
~~pkg/registry/OWNERS~~ [liggitt]
~~pkg/scheduler/OWNERS~~ [alculquicondor,liggitt]
~~plugin/pkg/auth/authorizer/OWNERS~~ [liggitt]
~~staging/src/k8s.io/api/OWNERS~~ [liggitt]
~~test/OWNERS~~ [liggitt,soltysh]

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

liggitt · 2022-11-09T13:32:28Z

adding to milestone per code-freeze exception granted at https://groups.google.com/g/kubernetes-sig-release/c/tZPjSWW_g30/m/_FzB7nrUAAAJ?utm_medium=email&utm_source=footer

/milestone v1.26

leonardpahlke · 2022-11-09T13:52:57Z

exception request approved: email

Your updated deadline to make any changes to your PR is 18:00 PST Friday 11th November 2022.

/milestone v1.26

mimowo · 2022-11-09T14:34:40Z

/retest

k8s-ci-robot requested review from alculquicondor and aojea October 26, 2022 13:32

alculquicondor mentioned this pull request Oct 27, 2022

Retriable and non-retriable Pod failures for Jobs kubernetes/enhancements#3329

Open

8 tasks

k8s-ci-robot added the area/e2e-test-framework Issues or PRs related to refactoring the kubernetes e2e test framework label Oct 28, 2022

mimowo force-pushed the handling-pod-failures-beta-enable branch from 36edb2b to d3e7057 Compare October 28, 2022 08:05

k8s-ci-robot added the sig/node Categorizes an issue or PR as relevant to SIG Node. label Oct 28, 2022

mimowo force-pushed the handling-pod-failures-beta-enable branch 4 times, most recently from bf4309a to 8927d37 Compare October 28, 2022 09:19

mimowo force-pushed the handling-pod-failures-beta-enable branch from 8927d37 to 94009c9 Compare October 28, 2022 12:03

k8s-ci-robot added the sig/auth Categorizes an issue or PR as relevant to SIG Auth. label Oct 28, 2022

liggitt self-assigned this Nov 8, 2022

liggitt added this to In progress in API Reviews Nov 8, 2022

liggitt reviewed Nov 8, 2022

View reviewed changes

liggitt moved this from In progress to API review completed, 1.26 in API Reviews Nov 8, 2022

mimowo added 2 commits November 9, 2022 09:02

Enable the feature into beta

c803892

Add e2e test for adding DisruptionTarget condition to the preemption …

818e180

…victim pod

mimowo force-pushed the handling-pod-failures-beta-enable branch from 5726091 to 818e180 Compare November 9, 2022 08:03

mimowo added 2 commits November 9, 2022 10:26

Cleanup the default_preemption_test by indexing the potential victim …

b3e9d8e

…pods

Add the "patch" permission to the "pods/status" unconditionally

ac4da3f

alculquicondor reviewed Nov 9, 2022

View reviewed changes

k8s-ci-robot assigned alculquicondor Nov 9, 2022

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

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

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

leonardpahlke removed this from the v1.26 milestone Nov 9, 2022

mimowo mentioned this pull request Nov 9, 2022

Extend tests in taint_manager_test.go to assert on deleted and patched pods, not just actions #111612

Closed

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

k8s-ci-robot merged commit 7e0e0c8 into kubernetes:master Nov 9, 2022

This was referenced Nov 10, 2022

Add e2e test for job pod failure policy used to match pod disruption #113812

Merged

Self-nominate mimowo as a reviewer for pkg/controller/job & test/integration/job packages #113196

Merged

atiratree mentioned this pull request Mar 13, 2023

API-initiated eviction: handle deleteOptions correctly #116554

Merged

mimowo deleted the handling-pod-failures-beta-enable branch March 18, 2023 18:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable the "Retriable and non-retriable pod failures for jobs" feature into beta #113360

Enable the "Retriable and non-retriable pod failures for jobs" feature into beta #113360

mimowo commented Oct 26, 2022 •

edited

k8s-triage-robot commented Oct 26, 2022

alculquicondor commented Oct 26, 2022

mimowo commented Oct 27, 2022

mimowo commented Oct 27, 2022

mimowo commented Oct 28, 2022

liggitt left a comment

liggitt Nov 8, 2022

mimowo Nov 9, 2022 •

edited

liggitt Nov 8, 2022

mimowo Nov 9, 2022 •

edited

alculquicondor Nov 9, 2022

mimowo Nov 9, 2022

liggitt Nov 8, 2022

mimowo Nov 9, 2022 •

edited

mimowo Nov 9, 2022

mimowo commented Nov 9, 2022

liggitt commented Nov 9, 2022

alculquicondor Nov 9, 2022

alculquicondor Nov 9, 2022

alculquicondor commented Nov 9, 2022

liggitt commented Nov 9, 2022

k8s-ci-robot commented Nov 9, 2022

liggitt commented Nov 9, 2022

leonardpahlke commented Nov 9, 2022

mimowo commented Nov 9, 2022

Enable the "Retriable and non-retriable pod failures for jobs" feature into beta #113360

Enable the "Retriable and non-retriable pod failures for jobs" feature into beta #113360

Conversation

mimowo commented Oct 26, 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-triage-robot commented Oct 26, 2022

alculquicondor commented Oct 26, 2022

mimowo commented Oct 27, 2022

mimowo commented Oct 27, 2022

mimowo commented Oct 28, 2022

liggitt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mimowo Nov 9, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mimowo Nov 9, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mimowo Nov 9, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mimowo commented Nov 9, 2022

liggitt commented Nov 9, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alculquicondor commented Nov 9, 2022

liggitt commented Nov 9, 2022

k8s-ci-robot commented Nov 9, 2022

liggitt commented Nov 9, 2022

leonardpahlke commented Nov 9, 2022

mimowo commented Nov 9, 2022

mimowo commented Oct 26, 2022 •

edited

mimowo Nov 9, 2022 •

edited

mimowo Nov 9, 2022 •

edited

mimowo Nov 9, 2022 •

edited