Backoff limit per Job Index

[mimowo]

• One-line PR description: Support for executing all indexes in case of failures using backoff limit per Index

• Issue link: Backoff Limit Per Index For Indexed Jobs #3850

• Other comments:
  • superseeds: Backoff Limit Per Job #3774

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: jensentanlo (44769f5)
• ✅ login: mimowo / name: Michał Woźniak (cd0d8a0, 30f964f)

[mimowo]

FYI @jensentanlo @alculquicondor @soltysh @kerthcet

[alculquicondor]

/wg batch

[soltysh]

/lgtm

[wojtek-t]

From PRR perspective I'm fine - I wanted to follow up on scalability aspect.

[wojtek-t]

This is not strictly related to PRR, rather wearing my scalability hat now.
[I would also be happy to discuss it separately if preferred.]

My concern is purely code-related - you have this magic immediate parameter in:
https://github.com/kubernetes/kubernetes/blob/6195f96e56ee1e9f52986a0e768e22ca0d1949d6/pkg/controller/job/job_controller.go#L499
I have concerns:

1. why we want to use a different separate backoff in those cases (that effectively starts from 0 instead of 1s)
2. why we don't do any batching when Job object is changing itself, but I think the new PR (that doesn't do batching only when the job generation changes) seems to address this part

[mimowo]

(1.) I agree. IIUC the idea of backoff is to throttle pod creation in case of consecutive failures. This is covered already here: https://github.com/kubernetes/kubernetes/blob/6195f96e56ee1e9f52986a0e768e22ca0d1949d6/pkg/controller/job/job_controller.go#L1415-L1419. However, currently it is also used for throttling in other places for non-obvious reasons. It also might be a left over after a relatively recent refactoring of how backoff works. I think this was always a little bit underspecified, +1 to clean it up at some point. Also, I think it makes sense to modify the final backoff as max(1s, computed backoff), but this is a detail. @alculquicondor @soltysh wdyt? Maybe we should open a separate Issue to clean up the backoff status?

(2.) yes, I don't think we want to batch (delay) job updates in case of spec updates. For example, spec updates are used to update suspend status, which could result in delayed job start. The PR covers the distinction.

[alculquicondor]

1. Backoff is used for errors from API calls (like conflicts). It is not used for events (pod creation, updates, etc). I guess a better name for the variable would be useBackoff or have separate functions altogether.
2. We want to be able to respond to changes to the spec ASAP (new parallelism, suspend, etc).

[alculquicondor]

Maybe we should open a separate Issue to clean up the backoff status?

Just open a PR with your proposal of what a clean code looks like :)
There is a lot of heritage in the backoff code.

[mimowo]

Just open a PR with your proposal of what a clean code looks like :)

We have one PR currently open for the fix + potentially small cleanup. I will consider another in the future, but some changes may not be just cosmetic, but also touching on semantics (so requiring discussion). Still, I suggest we move this discussion out of KEP to offline, under PRs or under Issues.

[wojtek-t]

For (1) - clearly backoff isn't used only for errors from API calls, because errors from API calls don't generate events and we have those in event handlers:
https://github.com/kubernetes/kubernetes/blob/6195f96e56ee1e9f52986a0e768e22ca0d1949d6/pkg/controller/job/job_controller.go#L318-L321

@mimowo - while opening PR is certainly good thing, can you open an issue to so that we won't forget about it?

For (2) - I agree.

[mimowo]

@wojtek-t sure, opened: kubernetes/kubernetes#118527

[wojtek-t]

From PRR perspective I'm fine - I wanted to follow up on scalability aspect.

[soltysh]

/lgtm

[wojtek-t]

/lgtm
/approve PRR

/hold
@mimowo - feel free to cancel hold once the issue is opened (and please cc me to that issue)

[mimowo]

/hold cancel
as the issue is now open: kubernetes/kubernetes#118527

[wojtek-t]

@soltysh - it needs your approval

[soltysh]

@soltysh - it needs your approval

oh, I thought I already did
/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mimowo, soltysh, wojtek-t

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:

• keps/prod-readiness/OWNERS [wojtek-t]
• keps/sig-apps/OWNERS [soltysh]

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

[deads2k]

spoke on slack and live prior. The API looked good too.