Job: Support for the SuccessPolicy

[tenzen-y]

What type of PR is this?

/kind feature
/kind api-change

What this PR does / why we need it:

I implemented the following items to support the JobSuccessPolicy:

• Extended Job Controller and Uni Tests
• Extended Job Validation Webhooks and Uni Tests
• Added Integration Test for this feature.

Please see more details in the KEP.

Which issue(s) this PR fixes:

Tracking issue kubernetes/enhancements#3998

Special notes for your reviewer:

New Condition Name

ref: #123412 (comment)

Votes:

• SuccessCriteriaMet: @atiratree @deads2k
• SuccessPolicyMet: @atiratree @deads2k
• SuccessTarget:

Neutral: @mimowo

Difference from KEP

Most of the implementation completely follows specifications decided in KEP.
There are 3 differences from the KEP.

1. The type of .spec.successPolicy.criteria[*].succeededCount. I replaced the int with int32 because we used to select the int32 for the number of indexes like the .spec.completions.
2. I added validations when updating Job Conditions. I added the following validations based on Support for the Job managedBy field (alpha) #123273 (comment):
   • RejectSuccessCriteriaMetWithFailedCondition: Reject transitions of conditions Failed <-> SuccessCriteriaMet
   • RejectSuccessCriteriaMetWithFailureTargetCondition: Reject transitions of conditions FailureTarget <-> SuccessCriteriaMet
   • RejectSuccessCriteriaMetForAlreadyCompleteJob: Reject transitions of conditions Complete -> SuccessCriteria
   • RejectSuccessCriteriaMetForNonIndexedJob: Reject the addition of SuccessCriteria for NonIndexed Job.
3. I replaced the API name, SuccessPolicyCriteria (JSON=criteria) with SuccessPolicyRule (JSON=rules) based on Job: Support for the SuccessPolicy #123412 (comment).

Additionally, this PR depends on #123273 in the validateIndexesFormat:

kubernetes/pkg/apis/batch/validation/validation.go

Lines 445 to 447 in 134a16f

	// Extended the [validateIndexesFormat](https://github.com/kubernetes/kubernetes/pull/123273)
	// TODO: Once #123273 is merged into the master, we should rebase these commits.
	func validateIndexesFormat(indexesStr string, completions int) (*int, error) {

, IsJobComplete, IsJobFailed, IsConditionTrue:

kubernetes/pkg/apis/batch/validation/validation.go

Lines 790 to 805 in 1bff570

	func IsJobComplete(job *batch.Job) bool {
	return IsConditionTrue(job.Status.Conditions, batch.JobComplete)
	}
	func IsJobFailed(job *batch.Job) bool {
	return IsConditionTrue(job.Status.Conditions, batch.JobFailed)
	}
	func IsConditionTrue(list []batch.JobCondition, cType batch.JobConditionType) bool {
	for _, c := range list {
	if c.Type == cType && c.Status == api.ConditionTrue {
	return true
	}
	}
	return false
	}

, and JobStatusValidationOptions:

kubernetes/pkg/apis/batch/validation/validation.go

Line 855 in 1bff570

type JobStatusValidationOptions struct {

So, we need to merge #123273, first.

Does this PR introduce a user-facing change?

Add alpha-level support for the SuccessPolicy in Jobs

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

https://github.com/kubernetes/enhancements/tree/master/keps/sig-apps/3998-job-success-completion-policy

[kannon92]

/triage accepted
/priority important-soon

[kannon92]

/cc

[mimowo]

The job controller changes generally lgtm, left mostly readability comments.

One place I'm not yet understanding the change is in this comment: https://github.com/kubernetes/kubernetes/pull/123412/files#r1516065458. A comment would do, or drop the check if not needed.

[tenzen-y]

@atiratree @mimowo I addressed all your comments. PTAL, thanks!

[tenzen-y]

I'm still working on addressing Aldo's comments.

[tenzen-y]

/test pull-kubernetes-node-e2e-containerd

[mimowo]

The changes in pkg/controller/job LGTM.

[tenzen-y]

@alculquicondor I addressed all comments! PTAL, thanks!

[alculquicondor]

I'm not sure if this is too different from my last review or I just missed it.

We had agreed in the KEP that if, in a given reconcile loop, a Job qualifies for both SuccessCriteriaMet and FailureTarget (or Backoff or any other error), the success would win. It doesn't look like it, because we are checking matchSuccessPolicy last.

This is somewhat important because the job controller might be busy at the time when a Pod finishes successfully, so by the time it has a chance to run, multiple other Pods might have failed. But again, the success policy should win.

But I'll consider the above as part of #117303 and we can fix it before the test freeze.

[tenzen-y]

I'm not sure if this is too different from my last review or I just missed it.

We had agreed in the KEP that if, in a given reconcile loop, a Job qualifies for both SuccessCriteriaMet and FailureTarget (or Backoff or any other error), the success would win. It doesn't look like it, because we are checking matchSuccessPolicy last.

I changed this based on #123412 (comment)

[alculquicondor]

/lgtm
/hold cancel
Please work on #117303 before the test freeze.

[k8s-ci-robot]

LGTM label has been added.

Git tree hash: fb204ff04b33a16575ea7f27ca616e95d1942860

[tenzen-y]

/lgtm /hold cancel Please work on #117303 before the test freeze.

Sure.

[tenzen-y]

Kubernetes e2e suite: [It] [sig-network] Services should complete a service status lifecycle [Conformance]
{ failed [FAILED] failed to locate Service test-service-sq55n in namespace services-2473: timed out waiting for the condition
In [It] at: k8s.io/kubernetes/test/e2e/network/service.go:3530 @ 03/07/24 21:09:02.702
}

This is an irrelevant error.
/test pull-kubernetes-conformance-kind-ipv6-parallel

[alculquicondor]

As per Exception approval

/milestone v1.30

[tenzen-y]

Kubernetes e2e suite: [It] [sig-node] Mount propagation should propagate mounts within defined scopes
{ failed [FAILED] Told to stop trying after 2.030s. The phase of Pod slave is Failed which is unexpected. In [It] at: k8s.io/kubernetes/test/e2e/framework/pod/pod_client.go:106 @ 03/07/24 21:09:00.22 }

This is an irrelevant error.
/test pull-kubernetes-e2e-kind-ipv6

[tenzen-y]

Kubernetes e2e suite: [It] [sig-network] Services should complete a service status lifecycle [Conformance] expand_less | 1m2s
-- | --
{ failed [FAILED] failed to locate Service test-service-sq55n in namespace services-2473: timed out waiting for the condition In [It] at: k8s.io/kubernetes/test/e2e/network/service.go:3530 @ 03/07/24 21:09:02.702 }

/test pull-kubernetes-conformance-kind-ipv6-parallel

[tenzen-y]

Kubernetes e2e suite: [It] [sig-apps] Job should apply changes to a job status [Conformance] expand_less | 12s
-- | --
{ failed [FAILED] patched object should have the applied condition with LastTransitionTime time.Date(2024, time.March, 7, 21, 31, 16, 0, time.UTC), got time.Date(2024, time.March, 7, 21, 31, 15, 0, time.Local) instead In [It] at: k8s.io/kubernetes/test/e2e/apps/job.go:901 @ 03/07/24 21:31:16.371 }

Let me check it.

[tenzen-y]

Kubernetes e2e suite: [It] [sig-apps] Job should apply changes to a job status [Conformance] expand_less	12s
{ failed [FAILED] patched object should have the applied condition with LastTransitionTime time.Date(2024, time.March, 7, 21, 31, 16, 0, time.UTC), got time.Date(2024, time.March, 7, 21, 31, 15, 0, time.Local) instead In [It] at: k8s.io/kubernetes/test/e2e/apps/job.go:901 @ 03/07/24 21:31:16.371 }

Let me check it.

This already has been tracked by #123799

/test pull-kubernetes-e2e-gce