WIP: Job: calculate the job completion before calculating the activeDeadlineSeconds

[tenzen-y]

What type of PR is this?

/kind bug

What this PR does / why we need it:

The job controller should calculate the completion before calculating the activeDeadlineSeconds.

Which issue(s) this PR fixes:

Fixes #117303

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Fix a bug in which the complete condition isn't recorded in jobs when the job reaches activeDeadlineSeconds.

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

[k8s-ci-robot]

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]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: tenzen-y
Once this PR has been reviewed and has the lgtm label, please assign janetkuo for approval. For more information see the Kubernetes Code Review Process.

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

Needs approval from an approver in each of these files:

• pkg/controller/job/OWNERS

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

[tenzen-y]

I'm implementing unit tests.

[alculquicondor]

@mimowo or @kannon92 would you like to take a first pass?

[tenzen-y]

I have pushed all commits. If I'm missing anything, please let me know.

[tenzen-y]

/wg batch

[kannon92]

/cc

[kannon92]

For these test cases, do they reproduce the bug if this code is removed?

[tenzen-y]

Yes, we can reproduce issues like the following:

$ go test -run "TestSyncJobPastDeadline" ./pkg/controller/job/...
?       k8s.io/kubernetes/pkg/controller/job/config     [no test files]
?       k8s.io/kubernetes/pkg/controller/job/config/v1alpha1    [no test files]
?       k8s.io/kubernetes/pkg/controller/job/metrics    [no test files]
--- FAIL: TestSyncJobPastDeadline (0.00s)
    --- FAIL: TestSyncJobPastDeadline/nonIndexed_job_succeeded_and_exceeded_activeDeadlineSeconds (0.00s)
        job_controller_test.go:2094: Expected fail condition.  Got []v1.JobCondition{v1.JobCondition{Type:"Failed", Status:"True", LastProbeTime:time.Date(2023, time.November, 14, 13, 40, 12, 766800000, time.Local), LastTransitionTime:time.Date(2023, time.November, 14, 13, 40, 12, 766800000, time.Local), Reason:"DeadlineExceeded", Message:"Job was active longer than specified deadline"}}
    --- FAIL: TestSyncJobPastDeadline/indexed_job_succeeded_and_exceeded_activeDeadlineSeconds (0.00s)
        job_controller_test.go:2094: Expected fail condition.  Got []v1.JobCondition{v1.JobCondition{Type:"Failed", Status:"True", LastProbeTime:time.Date(2023, time.November, 14, 13, 40, 12, 767083000, time.Local), LastTransitionTime:time.Date(2023, time.November, 14, 13, 40, 12, 767083000, time.Local), Reason:"DeadlineExceeded", Message:"Job was active longer than specified deadline"}}
FAIL
FAIL    k8s.io/kubernetes/pkg/controller/job    0.817s
FAIL

[kannon92]

Sounds good.

[tenzen-y]

Rebased.

[tenzen-y]

/retest

[tenzen-y]

/test pull-kubernetes-unit

[mimowo]

Do we need this if? Maybe, if needed, it could be inside the wantActivePods function?

[tenzen-y]

I think we should put this if here since wantActivePods is also called in L1478.
@mimowo WDYT?

[tenzen-y]

@mimowo Friendly ping :)

[mimowo]

My question is motivated by the fact that this two code paths could result setting different values for wantActive. So for example if satisfiedExpectations == false, feature.DefaultFeatureGate.Enabled(features.ElasticIndexedJob)==true, and job.Deletion==nil you will have wantActive=active here, but wantActive = wantActivePods(&job, jobCtx) inside manageJob.

I'm wondering if we can / should avoid it. No specific failure scenario at the moment.

[tenzen-y]

Ah, you're right. We should call inside wantActive() if we need this if. Thanks!

[mimowo]

I'm also wondering if we could take a step back and make some minimal fix. The starting point would be to introduce finishedConditionForActiveDeadlineExceeded, and set it line 826, instead of setting jobCtx.finishedCondition. Then, proceeed with evaluation of complete, and finally set jobCtx.finishedCondition = finishedConditionForActiveDeadlineExceeded if complete=false. I'm not sure this is 100% simpler, but I think worth exploring to compare the options.

[mimowo]

My question is motivated by the fact that this two code paths could result setting different values for wantActive. So for example if satisfiedExpectations == false, feature.DefaultFeatureGate.Enabled(features.ElasticIndexedJob)==true, and job.Deletion==nil you will have wantActive=active here, but wantActive = wantActivePods(&job, jobCtx) inside manageJob.

I'm wondering if we can / should avoid it. No specific failure scenario at the moment.

[tenzen-y]

I'm also wondering if we could take a step back and make some minimal fix. The starting point would be to introduce finishedConditionForActiveDeadlineExceeded, and set it line 826, instead of setting jobCtx.finishedCondition. Then, proceeed with evaluation of complete, and finally set jobCtx.finishedCondition = finishedConditionForActiveDeadlineExceeded if complete=false. I'm not sure this is 100% simpler, but I think worth exploring to compare the options.

Thanks for your suggestion! I'll try to evaluate the suggested way, carefully.

[k8s-triage-robot]

The Kubernetes project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

• Mark this PR as fresh with /remove-lifecycle stale
• Close this PR with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

[tenzen-y]

I will come back here after JobSuccessPolicy initial implementation.

/remove-lifecycle stale

[tenzen-y]

Rebased.

[alculquicondor]

This fix is not enough. I should have been more clear: the issue is not just about activeDeadlineSeconds, but any failure.

So the order should be:

1. Check if there is a FailureTarget condition.
2. Check if we have all the succeeded pods.
3. Check any failures: pod failure policy, backoffLimit, activeDeadline (order here is irrelevant).

[tenzen-y]

This fix is not enough. I should have been more clear: the issue is not just about activeDeadlineSeconds, but any failure.

So the order should be:

1. Check if there is a FailureTarget condition.
2. Check if we have all the succeeded pods.
3. Check any failures: pod failure policy, backoffLimit, activeDeadline (order here is irrelevant).

I came back from JobSuccessPolicy. Let me try to finalize this until Test Freeze.

[k8s-ci-robot]

@tenzen-y: The following tests 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-conformance-kind-ga-only-parallel	9fcf8b5	link	true	/test pull-kubernetes-conformance-kind-ga-only-parallel
pull-kubernetes-integration	9fcf8b5	link	true	/test pull-kubernetes-integration

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.