fix: respect cleanPodPolicy when job exceeds backoffLimit

[AviadHayumi]

What is the problem?

When a training job exceeds its backoffLimit, all pods are deleted regardless of cleanPodPolicy: None. This is caused by a condition-setting ordering bug in ReconcileJobs() where DeletePodsAndServices() is called before the JobFailed condition is set on jobStatus.

DeletePodsAndServices guards pod deletion with IsFinished(jobStatus), which checks for JobFailed/JobSucceeded conditions — not CompletionTime. Since the condition is set after the delete call in the jobExceedsLimit block, IsFinished() returns false and the cleanPodPolicy: None guard is bypassed.

Affects all V1 job types using ReconcileJobs with backoffLimit: PyTorchJob, TFJob, XGBoostJob, PaddleJob, MPIJob.

Ref: #3419

Changes

Moved UpdateJobConditions(JobFailed) and Recorder.Event before DeletePodsAndServices() in the jobExceedsLimit block (pkg/controller.v1/common/job.go lines 216-247), so that IsFinished() returns true at the time of cleanup and cleanPodPolicy is correctly respected.

Added a test case to TestDeletePodsAndServices documenting the bug scenario (unfinished job + cleanPodPolicy: None).

Before (buggy ordering)

if jobExceedsLimit {
    jobStatus.CompletionTime = &now
    jc.DeletePodsAndServices(...)  // IsFinished() == false, cleanPodPolicy ignored
    // ...
    commonutil.UpdateJobConditions(&jobStatus, apiv1.JobFailed, ...)  // too late
}

After (fixed ordering)

if jobExceedsLimit {
    jobStatus.CompletionTime = &now
    commonutil.UpdateJobConditions(&jobStatus, apiv1.JobFailed, ...)  // set first
    jc.DeletePodsAndServices(...)  // IsFinished() == true, cleanPodPolicy respected
}

Testing

• All unit tests pass (go test ./pkg/controller.v1/common/)
• Verified on a live cluster (v1.8.1 based image): deployed the fix and created a PyTorchJob with cleanPodPolicy: None and backoffLimit: 1 — pods are now correctly preserved after the job fails due to backoff limit. Before the fix, all pods were unconditionally deleted.

/kind bug

[andreyvelich]

Thanks for the fix, that looks good!
/assign @astefanutti @tenzen-y @kubeflow/wg-training-leads
/lgtm

[astefanutti]

@AviadHayumi thanks, could you please sign the commit?

[google-oss-prow]

New changes are detected. LGTM label has been removed.

[google-oss-prow]

[APPROVALNOTIFIER] This PR is NOT APPROVED

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

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

Details

Needs approval from an approver in each of these files:

• OWNERS

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

[AviadHayumi]

@AviadHayumi thanks, could you please sign the commit?

Done, signed

[andreyvelich]

/ok-to-test

[AviadHayumi]

@astefanutti

The failing integration tests are pre-existing - the last merged PR (#3221) has the exact same failures. These are JAXJob e2e timeouts on older K8s versions (v1.28-v1.30).