Switch feature flag to beta for pod replacement policy and add e2e test #121491

dejanzele · 2023-10-24T21:18:25Z

What type of PR is this?

/sig apps
/kind feature

What this PR does / why we need it:

Promotes feature flag for PodReplacementPolicy to beta and adds e2e test for the Failed PodReplacementPolicy.

Which issue(s) this PR fixes:

Part of kubernetes/enhancements#3939

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Promote PodReplacementPolicy to beta.

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

k8s-ci-robot · 2023-10-24T21:18:35Z

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.

k8s-ci-robot · 2023-10-24T21:18:37Z

Hi @dejanzele. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

kannon92 · 2023-10-24T21:20:17Z

/ok-to-test

test/e2e/apps/job.go

kannon92 · 2023-10-24T21:34:53Z

You should add a release note for this.
/release-note-edit Promote PodReplacementPolicy to beta.

pkg/features/kube_features.go

test/e2e/apps/job.go

alculquicondor · 2023-10-25T16:02:03Z

test/e2e/apps/job.go

+		pods, err := e2ejob.GetJobPods(ctx, f.ClientSet, f.Namespace.Name, job.Name)
+		framework.ExpectNoError(err, "failed to get pod list for job in namespace: %s", f.Namespace.Name)
+
+		framework.ExpectNoError(e2epod.DeletePodsWithGracePeriod(ctx, f.ClientSet, pods.Items, 0), "failed to delete pods in namespace: %s", f.Namespace.Name)


Actually let's redesign this test:

Set -termination-grace-period to something long (1min should be fine).

Change DeletePodsWithGracePeriod to have a grace period (not zero).

At this point (line 364, ), check that the Job status has one terminating pod and no active pods.

Once we have verified the desired status, issue another DeletePodsWithGracePeriod, but this time with zero grace period. This will cause a SIGKILL and immediately delete the pods.

Verify pods recreated.

pkg/features/kube_features.go

test/e2e/apps/job.go

alculquicondor

/retitle Switch feature flag to beta for pod replacement policy and add e2e test

k8s-ci-robot · 2023-10-26T15:46:58Z

LGTM label has been added.

Git tree hash: e96ce45c0275349a0d1c0d68dd6116896b4a23e5

test/e2e/apps/job.go

alculquicondor · 2023-10-26T18:06:09Z

test/e2e/apps/job.go

+			if job.Status.Active == 0 && job.Status.Failed == 0 && *job.Status.Terminating == 1 {
+				return ""


what prevents the pod from finishing too fast, so that it's in a terminating state before reaching Failed?

hmmm good question, kubelet needs some time to transition it, but maybe there is a small chance it happens super fast and test fails. I don't have a good answer, do you have ideas?

That's why we had the grace period in the pod itself... the tricky part is forcing a delete after sending a SIGTERM.

test/e2e/apps/job.go

alculquicondor · 2023-10-26T19:45:14Z

/lgtm
/approve
/assign @soltysh

k8s-ci-robot · 2023-10-26T19:45:21Z

LGTM label has been added.

Git tree hash: 16e1a0a28f9962a65ecf1c8efe4b654d1db9317a

alculquicondor · 2023-10-26T19:45:24Z

Can you squash?

update pod replacement policy feature flag comment and refactor the e2e test for pod replacement policy minor fixes for pod replacement policy and e2e test fix wrong assertions for pod replacement policy e2e test more fixes to pod replacement policy e2e test refactor PodReplacementPolicy e2e test to use finalizers fix unit tests when pod replacement policy feature flag is promoted to beta fix podgc controller unit tests when pod replacement feature is enabled fix lint issue in pod replacement policy e2e test assert no error in defer function for removing finalizer in pod replacement policy e2e test implement test using a sh trap for pod replacement policy reduce sleep after SIGTERM in pod replacement policy e2e test to 5s

dejanzele · 2023-10-26T19:51:59Z

Can you squash?

done

alculquicondor · 2023-10-26T20:20:36Z

Thanks!
/lgtm

mimowo · 2023-10-27T09:16:47Z

test/e2e/apps/job.go

+		job := e2ejob.NewTestJob("", "pod-recreate-failed", v1.RestartPolicyNever, 1, 1, nil, 1)
+		job.Spec.PodReplacementPolicy = ptr.To(batchv1.Failed)
+		job.Spec.Template.Spec.Containers[0].Command = []string{"/bin/sh", "-c", `_term(){
+	sleep 5


Is this necessary if we follow the design proposed here: #121491 (comment)? I think we could use the "notTerminate" image, and another call for DeletePodsWithGracePeriod with 0. Then, we don't need the 5s penalty. But I'm happy to do this improvement later in the test freeze not to risk the code freeze.

EDIT: Actually, I tested locally with kubectl, that when the first delete for a sleep pod has grace-period=30, then the next executed right after with grace-period=0 does not terminate the pod quickly (still takes ~30s). This was surprising, but in that case I don't see any easy improvement.

soltysh

/approve

k8s-ci-robot · 2023-10-27T09:23:03Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alculquicondor, dejanzele, 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/controller/OWNERS~~ [soltysh]
~~pkg/features/OWNERS~~ [soltysh]
~~test/e2e/apps/OWNERS~~ [soltysh]
~~test/integration/job/OWNERS~~ [alculquicondor,soltysh]

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

mimowo · 2023-10-27T11:04:33Z

/retest

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Oct 24, 2023

k8s-ci-robot requested review from kow3ns and smarterclayton October 24, 2023 21:18

k8s-ci-robot added area/test sig/testing Categorizes an issue or PR as relevant to SIG Testing. labels Oct 24, 2023

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Oct 24, 2023

kannon92 mentioned this pull request Oct 24, 2023

Allow for recreation of pods once fully terminated in the job controller kubernetes/enhancements#3939

Open

8 tasks

kannon92 reviewed Oct 24, 2023

View reviewed changes

test/e2e/apps/job.go Outdated Show resolved Hide resolved

kannon92 reviewed Oct 24, 2023

View reviewed changes

test/e2e/apps/job.go Outdated Show resolved Hide resolved

kannon92 reviewed Oct 24, 2023

View reviewed changes

test/e2e/apps/job.go Outdated Show resolved Hide resolved

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Oct 24, 2023

alculquicondor reviewed Oct 25, 2023

View reviewed changes

dejanzele force-pushed the pod-replacement-policy-e2e branch from fcaafc0 to 32d9375 Compare October 25, 2023 19:54

alculquicondor reviewed Oct 25, 2023

View reviewed changes

pkg/features/kube_features.go Show resolved Hide resolved

test/e2e/apps/job.go Outdated Show resolved Hide resolved

test/e2e/apps/job.go Outdated Show resolved Hide resolved

test/e2e/apps/job.go Outdated Show resolved Hide resolved

alculquicondor reviewed Oct 25, 2023

View reviewed changes

k8s-ci-robot changed the title ~~[WIP][DO NOT MERGE] Switch feature flag to beta for pod replacement policy and add e2e test~~ Switch feature flag to beta for pod replacement policy and add e2e test Oct 25, 2023

k8s-ci-robot assigned mimowo and kannon92 Oct 26, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 26, 2023

alculquicondor reviewed Oct 26, 2023

View reviewed changes

alculquicondor mentioned this pull request Oct 26, 2023

Improve execution time of job e2e tests #121538

Merged

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 26, 2023

k8s-ci-robot requested review from alculquicondor, kannon92 and mimowo October 26, 2023 19:07

alculquicondor reviewed Oct 26, 2023

View reviewed changes

test/e2e/apps/job.go Outdated Show resolved Hide resolved

alculquicondor mentioned this pull request Oct 26, 2023

kubelet does not honor shortened grace period on already-deleting pod #83916

Open

k8s-ci-robot assigned soltysh Oct 26, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 26, 2023

dejanzele force-pushed the pod-replacement-policy-e2e branch from f095ca7 to e98c33b Compare October 26, 2023 19:51

mimowo reviewed Oct 27, 2023

View reviewed changes

soltysh approved these changes Oct 27, 2023

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 27, 2023

k8s-ci-robot merged commit 6f4ad7a into kubernetes:master Oct 27, 2023
16 checks passed

k8s-ci-robot added this to the v1.29 milestone Oct 27, 2023

dejanzele mentioned this pull request Oct 30, 2023

REQUEST: New membership for dejanzele kubernetes/org#4561

Closed

9 tasks

dejanzele mentioned this pull request Dec 13, 2023

KEP-3973: Consider Terminating Pods in Deployments kubernetes/enhancements#4357

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Switch feature flag to beta for pod replacement policy and add e2e test #121491

Switch feature flag to beta for pod replacement policy and add e2e test #121491

dejanzele commented Oct 24, 2023 •

edited

k8s-ci-robot commented Oct 24, 2023

k8s-ci-robot commented Oct 24, 2023

kannon92 commented Oct 24, 2023

kannon92 commented Oct 24, 2023

alculquicondor Oct 25, 2023

alculquicondor left a comment

k8s-ci-robot commented Oct 26, 2023

alculquicondor Oct 26, 2023

dejanzele Oct 26, 2023

alculquicondor Oct 26, 2023

alculquicondor commented Oct 26, 2023

k8s-ci-robot commented Oct 26, 2023

alculquicondor commented Oct 26, 2023

dejanzele commented Oct 26, 2023

alculquicondor commented Oct 26, 2023

mimowo Oct 27, 2023 •

edited

soltysh left a comment

k8s-ci-robot commented Oct 27, 2023

mimowo commented Oct 27, 2023

		if job.Status.Active == 0 && job.Status.Failed == 0 && *job.Status.Terminating == 1 {
		return ""

Switch feature flag to beta for pod replacement policy and add e2e test #121491

Switch feature flag to beta for pod replacement policy and add e2e test #121491

Conversation

dejanzele commented Oct 24, 2023 • 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 Oct 24, 2023

k8s-ci-robot commented Oct 24, 2023

kannon92 commented Oct 24, 2023

kannon92 commented Oct 24, 2023

alculquicondor Oct 25, 2023

Choose a reason for hiding this comment

alculquicondor left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Oct 26, 2023

alculquicondor Oct 26, 2023

Choose a reason for hiding this comment

dejanzele Oct 26, 2023

Choose a reason for hiding this comment

alculquicondor Oct 26, 2023

Choose a reason for hiding this comment

alculquicondor commented Oct 26, 2023

k8s-ci-robot commented Oct 26, 2023

alculquicondor commented Oct 26, 2023

dejanzele commented Oct 26, 2023

alculquicondor commented Oct 26, 2023

mimowo Oct 27, 2023 • edited

Choose a reason for hiding this comment

soltysh left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Oct 27, 2023

mimowo commented Oct 27, 2023

dejanzele commented Oct 24, 2023 •

edited

mimowo Oct 27, 2023 •

edited