Benchmark job with backoff limit per index

[mimowo]

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

Which issue(s) this PR fixes:

Part of: kubernetes/enhancements#3850

Special notes for your reviewer:

1. Experiments on my local machine running the Benchmark integration test for jobs with failures using the BenchmarkLargeFailureHandling test. Here we have

b.Cleanup(setDurationDuringTest(&jobcontroller.DefaultJobPodFailureBackOff, fastPodFailureBackoff))
b.Cleanup(setDurationDuringTest(&jobcontroller.MaxJobPodFailureBackOff, fastPodFailureBackoff))

to minimize the timing differences due to different backoff strategies.

The results of the benchmark are put the KEP update PR: kubernetes/enhancements#4321.

Also, lowered the nPods parameter to 10 and 100 to avoid issues with long running time on CI. The parameters can be easily increased for local testing.

Does this PR introduce a user-facing change?

NONE

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

https://github.com/kubernetes/enhancements/tree/master/keps/sig-apps/3850-backoff-limits-per-index-for-indexed-jobs

[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.

[mimowo]

/test pull-kubernetes-unit

[mimowo]

@alculquicondor PTAL at the benchmark results.

1. Do you think this warrants further investigation at this point?
2. Do you think we want something like this merged, or the one time experiment on-demand is enough: Benchmark job with backoff limit per index #121393? I'm a little bit worried about merging the benchmark given our issues with the runtime for integration tests.

[alculquicondor]

Why is this benchmark relevant?
If there are no Pod failures, why would having backoffLimitPerIndex have an impact?

[mimowo]

Why is this benchmark relevant?
If there are no Pod failures, why would having backoffLimitPerIndex have an impact?

This is a sanity benchmark to make sure there is no significant penalty for stepping into some new branches of code like here or here.

However, to verify the new branches don't add significant penalty at scale we would need to have many failures, probably >1000. At this point, delay due to expotential backoff would take over. Since the delay patterns are different between global and perIndex backoffLimit I'm not sure how to design the proper full benchmark.

[mimowo]

One idea that occurs to me now is to use an integration test. Mark 1000 pods as failed, and measure just the duration of syncJob execution for comparison. This approach would allow to abstract out the expotential delay differences. Even if be not very representative for user experience, it will get us some clues about performance.

EDIT: I'm trying yet another approach with setting the backoffDelay to 10ms, or less only to make it less relevant.
EDIT: pushed one more benchmark with failing pods (with nPods failing pods). To minimize the differences with backoff strategies I use:

b.Cleanup(setDurationDuringTest(&jobcontroller.DefaultJobPodFailureBackOff, fastPodFailureBackoff))
b.Cleanup(setDurationDuringTest(&jobcontroller.MaxJobPodFailureBackOff, fastPodFailureBackoff))

I will publish the results when have them.

[mimowo]

I think it's valuable to keep this for a bit.
You should probably copy some of the results to the KEP, for future reference when considering the graduation to stable.

SGTM. I have prepared the KEP update: kubernetes/enhancements#4321

[alculquicondor]

/approve

[alculquicondor]

/retitle Benchmark job with backoff limit per index

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alculquicondor, mimowo

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:

• test/integration/job/OWNERS [alculquicondor]

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

[mimowo]

cc @kannon92 @pohly
would you like to review as well?

[mimowo]

/test pull-kubernetes-unit
retry as this is failure unrelated with this PR, but it might be good to look into this deeper as this is a job failure: https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/121393/pull-kubernetes-unit/1719392853026672640
FYI @alculquicondor @dejanzele

[mimowo]

/test pull-kubernetes-unit retry as this is failure unrelated with this PR, but it might be good to look into this deeper as this is a job failure: https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/121393/pull-kubernetes-unit/1719392853026672640 FYI @alculquicondor @dejanzele

Ticketed: #121652

[mimowo]

/retest

[seans3]

/remove-sig api-machinery

[dims]

/retest

[alculquicondor]

oops
/lgtm

[k8s-ci-robot]

LGTM label has been added.

Git tree hash: d25535bb9583f54359ab98c41aa974c115131fde

[pacoxu]

This is a test change. I suppose that we can add milestone v1.29 for this.

[mimowo]

Yes, I think it makes sense to add the milestone.

[mimowo]

/assign @soltysh

[pacoxu]

/milestone v1.29