Graduate JobTrackingWithFinalizers to stable #113510

alculquicondor · 2022-11-01T15:50:18Z

What type of PR is this?

/kind feature
/kind api-change

/sig apps

What this PR does / why we need it:

Graduate JobTrackingWithFinalizers to stable:

Fix feature gate to true
Declare job tracking annotation as deprecated
Allow adding job tracking annotation but add it back if users try to remove it.
Simplify tests where possible.

Which issue(s) this PR fixes:

Refs kubernetes/enhancements#2307

Special notes for your reviewer:

The other graduation criteria are completed in #113478 and #113176.

Legacy tracking is still supported. However, now it can only be tested in unit tests and not integration tests, because apiserver disallows creating a job without job tracking annotation.

Does this PR introduce a user-facing change?

Graduate JobTrackingWithFinalizers to stable.
Jobs created before the feature was enabled are still tracked without finalizers. Jobs tracked with finalizers have the annotation batch.kubernetes.io/job-tracking.
If the annotation is present and the user attempts to remove it, the control plane adds it back.
The annotation batch.kubernetes.io/job-tracking is deprecated. The control plane will ignore it and stop adding it for new Jobs in v1.27.

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

- [KEP]: https://github.com/kubernetes/enhancements/tree/a4b5f5c8e9aa857f9bf1f7df2a9a8b9ee660ed1e/keps/sig-apps/2307-job-tracking-without-lingering-pods#beta---ga-graduation
- [Usage]: https://kubernetes.io/docs/concepts/workloads/controllers/job/#job-tracking-with-finalizers

k8s-triage-robot · 2022-11-01T16:47:04Z

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-11-01T18:11:57Z

/assign @soltysh

alculquicondor · 2022-11-01T19:05:42Z

test/integration/apiserver/admissionwebhook/admission_test.go

@@ -689,7 +689,9 @@ func testResourceUpdate(c *testContext) {
 		if err != nil {
 			return err
 		}
-		obj.SetAnnotations(map[string]string{"update": "true"})
+		annotations := obj.GetAnnotations()


If somebody had a buggy mutating webhook that did something like this, it will start failing on upgrade to 1.26.

I would consider this WAI: we don't want users to disable tracking with finalizers.

Changed the behavior to always add the annotation back

alculquicondor · 2022-11-01T19:35:18Z

/retest

alculquicondor · 2022-11-02T13:57:16Z

/label api-review

soltysh · 2022-11-04T12:07:03Z

/triage accepted
/priority important-longterm

soltysh

/lgtm
/approve

soltysh · 2022-11-04T12:11:57Z

pkg/apis/batch/validation/validation.go

@@ -546,8 +546,8 @@ func ValidateJobTemplateSpec(spec *batch.JobTemplateSpec, fldPath *field.Path, o

 type JobValidationOptions struct {
 	apivalidation.PodValidationOptions
-	// Allow Job to have the annotation batch.kubernetes.io/job-tracking
-	AllowTrackingAnnotation bool
+	// Allow Job to not have the annotation batch.kubernetes.io/job-tracking


Suggested change

// Allow Job to not have the annotation batch.kubernetes.io/job-tracking

// Allow Job to not to have the annotation batch.kubernetes.io/job-tracking

ok, I didn't like either option, so I changed to logic to be RequireTrackingAnnotation=true by default, and false if the old job didn't have it.

liggitt · 2022-11-04T17:21:33Z

if the goal is to make sure new jobs have the tracking annotation and updates to jobs don't remove it, is there a reason not to add/preserve the annotation in PrepareForCreate/PrepareForUpdate, rather than rejecting previously working API requests?

alculquicondor · 2022-11-04T18:13:53Z

is there a reason not to add/preserve the annotation in PrepareForCreate/PrepareForUpdate, rather than rejecting previously working API requests?

I just didn't think of that, thanks! That's much simpler.

staging/src/k8s.io/api/batch/v1/types.go

pkg/registry/batch/job/strategy.go

Change-Id: Ifc749a85b1270c0155ac511b91d4681d53236820

alculquicondor

Reverted the validation to disallow adding the tracking annotation

alculquicondor · 2022-11-04T20:49:16Z

pkg/registry/batch/job/strategy_test.go

@@ -418,17 +423,6 @@ func testJobStrategy(t *testing.T) {
 		t.Errorf("Expected a validation error")
 	}

-	// Ensure going from legacy tracking Job to tracking with finalizers is


This is now tested in TestJobStrategyValidateUpdate

staging/src/k8s.io/api/batch/v1/types.go

pkg/registry/batch/job/strategy.go

liggitt

only comment was on PrepareForUpdate

liggitt · 2022-11-07T14:02:41Z

/lgtm
/approve

k8s-ci-robot · 2022-11-07T14:03:11Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alculquicondor, liggitt, 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:

~~api/OWNERS~~ [liggitt]
~~pkg/apis/OWNERS~~ [liggitt]
~~pkg/controller/job/OWNERS~~ [alculquicondor,liggitt,soltysh]
~~pkg/features/OWNERS~~ [liggitt,soltysh]
~~pkg/generated/openapi/OWNERS~~ [liggitt]
~~pkg/registry/OWNERS~~ [liggitt]
~~staging/src/k8s.io/api/OWNERS~~ [liggitt]
~~test/integration/job/OWNERS~~ [alculquicondor,liggitt,soltysh]

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

alculquicondor mentioned this pull request Nov 1, 2022

Job tracking without lingering Pods kubernetes/enhancements#2307

Closed

12 tasks

k8s-ci-robot added area/code-generation area/test sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/testing Categorizes an issue or PR as relevant to SIG Testing. labels Nov 1, 2022

alculquicondor force-pushed the finalizers-stable branch from 0fcd992 to 72269e3 Compare November 1, 2022 15:51

k8s-ci-robot requested review from dchen1107 and deads2k November 1, 2022 15:51

alculquicondor force-pushed the finalizers-stable branch from 72269e3 to a2743f2 Compare November 1, 2022 17:45

k8s-ci-robot assigned soltysh Nov 1, 2022

alculquicondor force-pushed the finalizers-stable branch from a2743f2 to 379dc13 Compare November 1, 2022 19:02

alculquicondor commented Nov 1, 2022

View reviewed changes

alculquicondor force-pushed the finalizers-stable branch 2 times, most recently from 932e894 to b17dae2 Compare November 1, 2022 21:50

k8s-ci-robot added api-review Categorizes an issue or PR as actively needing an API review. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Nov 2, 2022

k8s-ci-robot added the triage/accepted Indicates an issue or PR is ready to be actively worked on. label Nov 4, 2022

k8s-ci-robot removed the needs-priority Indicates a PR lacks a `priority/foo` label and requires one. label Nov 4, 2022

soltysh approved these changes Nov 4, 2022

View reviewed changes

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

alculquicondor force-pushed the finalizers-stable branch from b17dae2 to 3cbde55 Compare November 4, 2022 15:31

k8s-ci-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Nov 4, 2022

liggitt added this to Assigned in API Reviews Nov 4, 2022

liggitt self-assigned this Nov 4, 2022

liggitt moved this from Assigned to In progress in API Reviews Nov 4, 2022

alculquicondor force-pushed the finalizers-stable branch 2 times, most recently from 19bb0d1 to cdeb40c Compare November 4, 2022 18:08

liggitt added this to the v1.26 milestone Nov 4, 2022

liggitt reviewed Nov 4, 2022

View reviewed changes

staging/src/k8s.io/api/batch/v1/types.go Show resolved Hide resolved

pkg/registry/batch/job/strategy.go Show resolved Hide resolved

liggitt moved this from In progress to Changes requested in API Reviews Nov 4, 2022

Graduate JobTrackingWithFinalizers to stable

4948918

Change-Id: Ifc749a85b1270c0155ac511b91d4681d53236820

alculquicondor force-pushed the finalizers-stable branch from cdeb40c to 4948918 Compare November 4, 2022 21:06

alculquicondor commented Nov 4, 2022

View reviewed changes

liggitt moved this from Changes requested to In progress in API Reviews Nov 4, 2022

liggitt reviewed Nov 5, 2022

View reviewed changes

pkg/registry/batch/job/strategy.go Show resolved Hide resolved

liggitt reviewed Nov 5, 2022

View reviewed changes

pkg/registry/batch/job/strategy.go Show resolved Hide resolved

liggitt reviewed Nov 5, 2022

View reviewed changes

liggitt moved this from In progress to Changes requested in API Reviews Nov 5, 2022

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

liggitt moved this from Changes requested to API review completed, 1.26 in API Reviews 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 ac95e5b into kubernetes:master Nov 7, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Graduate JobTrackingWithFinalizers to stable #113510

Graduate JobTrackingWithFinalizers to stable #113510

alculquicondor commented Nov 1, 2022 •

edited

k8s-triage-robot commented Nov 1, 2022

alculquicondor commented Nov 1, 2022

alculquicondor Nov 1, 2022

alculquicondor Nov 4, 2022

alculquicondor commented Nov 1, 2022

alculquicondor commented Nov 2, 2022

soltysh commented Nov 4, 2022

soltysh left a comment

soltysh Nov 4, 2022

alculquicondor Nov 4, 2022

liggitt commented Nov 4, 2022

alculquicondor commented Nov 4, 2022

alculquicondor left a comment

alculquicondor Nov 4, 2022

liggitt left a comment

liggitt commented Nov 7, 2022

k8s-ci-robot commented Nov 7, 2022

	// Allow Job to not have the annotation batch.kubernetes.io/job-tracking
	// Allow Job to not to have the annotation batch.kubernetes.io/job-tracking

Graduate JobTrackingWithFinalizers to stable #113510

Graduate JobTrackingWithFinalizers to stable #113510

Conversation

alculquicondor commented Nov 1, 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 Nov 1, 2022

alculquicondor commented Nov 1, 2022

alculquicondor Nov 1, 2022

Choose a reason for hiding this comment

alculquicondor Nov 4, 2022

Choose a reason for hiding this comment

alculquicondor commented Nov 1, 2022

alculquicondor commented Nov 2, 2022

soltysh commented Nov 4, 2022

soltysh left a comment

Choose a reason for hiding this comment

soltysh Nov 4, 2022

Choose a reason for hiding this comment

alculquicondor Nov 4, 2022

Choose a reason for hiding this comment

liggitt commented Nov 4, 2022

alculquicondor commented Nov 4, 2022

alculquicondor left a comment

Choose a reason for hiding this comment

alculquicondor Nov 4, 2022

Choose a reason for hiding this comment

liggitt left a comment

Choose a reason for hiding this comment

liggitt commented Nov 7, 2022

k8s-ci-robot commented Nov 7, 2022

alculquicondor commented Nov 1, 2022 •

edited