I think there might be a bug with how terminating pods are counted in the Job status when PodReplacementPolicy is TerminatingOrFailed. The .Status.Terminating field never decreases to 0 unlike for Failed policy where it decreases to 0.
Unless I am mistaken?
cc @mimowo @alculquicondor @kannon92

I’ll look into this tomorrow but my first thought is that this is expected.

Failed is essentially saying I want all terminating pods to be fully terminated before starting new pods.

TerminatingOrFailed says create new pods as soon as they are deleted. It takes time to create new ones but it’s pretty close to almost after deletion is registered.

so in your case what I think is happening is that you are waiting for active pods in both but in TerminatingOrFailed you are getting active pods while the terminating ones are still being deleted.

Hmmm but I expected Terminating to drop to 0 after deletion & fail pods, but it didn't decrease from the value 2.

I agree with @dejanzele. I would expect Terminating to drop to 0. Can you investigate why it's not dropping?

They do drop on kind.

Again, deletion & fail for TerminatingOrFailed doesn't mean that it will happen immediately. My point is that I think you would need to wait for the terminating pods to go away separately from waiting for active pods.

We delete pods and we have this check which asserts how many pods should be terminating - https://github.com/kubernetes/kubernetes/pull/121342/files#diff-ace35385ec6f5400e8ccbfe8e9f35c2f9573d0fc05889071dc9c4b36e5e975f7R1804

Then we fail pods and do the assertion here to verify the status after faiiling them - https://github.com/kubernetes/kubernetes/pull/121342/files#diff-ace35385ec6f5400e8ccbfe8e9f35c2f9573d0fc05889071dc9c4b36e5e975f7R1815

Both checks use validateJobsPodsStatusOnly which does a poll and timeout is 30 seconds. I'd expect the 2 terminating pods to drop to 0 in 30 seconds.

it works as expected for Failed policy, after deletion I assert 2 pods are terminating and 0 active, after failing pods I assert 0 are terminating and 2 are active.

I agree with @dejanzele. I would expect Terminating to drop to 0. Can you investigate why it's not dropping?

did you investigate why this stays at 2?

To some degree, this line https://github.com/kubernetes/kubernetes/blob/master/pkg/controller/job/job_controller.go#L672 returns different results between the TerminatingOrFailed and Failed policy test cases, but I still cannot fully understand what is going on

But even if it does, they should be filtered out here, no? https://github.com/kubernetes/kubernetes/blob/baea2df9d6f02935849e00390e0e8a5fad596f51/pkg/controller/job/job_controller.go#L790C46-L790C46

here is the bug https://github.com/kubernetes/kubernetes/blob/master/pkg/controller/job/job_controller.go#L925-L933

we set job.Status.Terminating = jobCtx.terminating on line 929 and then do the check needsStatusUpdate = needsStatusUpdate || !ptr.Equal(job.Status.Terminating, jobCtx.terminating) but at this point !ptr.Equal(job.Status.Terminating, jobCtx.terminating) will always return false.

If we just do a simple reorder, all tests work correctly and terminating count correctly drops to 0 for TerminatingOrFailed policy.

needsStatusUpdate := suspendCondChanged || active != job.Status.Active || !ptr.Equal(ready, job.Status.Ready)
needsStatusUpdate = needsStatusUpdate || !ptr.Equal(job.Status.Terminating, jobCtx.terminating)
job.Status.Active = active
job.Status.Ready = ready
job.Status.Terminating = jobCtx.terminating
err = jm.trackJobStatusAndRemoveFinalizers(ctx, jobCtx, needsStatusUpdate)
if err != nil {
	return fmt.Errorf("tracking status: %w", err)
}

/cc @mimowo @kannon92

Is the only reason why this worked for Failed is because we had other updates around the same time?

In case of TerminatingOrFailed, we had a case where terminating was the only field changing. While Failed had the failed or active field set?

This change looks good to me. I think you can update it in this PR if you want. We don't cherry-pick alpha features so I don't think we have to backport this.

I guess it got decremented when the Job status got updated for some other reason, and the side-effect was the terminating count dropping to 0, but it was missing the update because of the terminating field being 0 earlier