Update KEP-3850 "Backoff Limit Per Index" for Beta #4228

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Merged

k8s-ci-robot merged 4 commits into kubernetes:master from mimowo:backoff-limit-per-index-beta

Oct 2, 2023

Contributor

mimowo commented Sep 21, 2023

One-line PR description: Update the KEP for Beta graduation for Backoff Limit Per Index

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

Other comments:

Contributor

k8s-ci-robot commented Sep 21, 2023

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

k8s-ci-robot added do-not-merge/work-in-progress cncf-cla: yes labels

k8s-ci-robot requested review from kow3ns and soltysh

September 21, 2023 10:56

k8s-ci-robot added kind/kep sig/apps size/XS labels

mimowo force-pushed the backoff-limit-per-index-beta branch from ac34804 to d16aea7 Compare

September 21, 2023 11:16

mimowo changed the title ~~Update KEP for Beta for "Backoff Limit Per Index"~~ Update KEP-3850 "Backoff Limit Per Index" for Beta

jensentanlo mentioned this pull request

Backoff Limit Per Index For Indexed Jobs #3850

Closed

12 tasks

mimowo force-pushed the backoff-limit-per-index-beta branch from d16aea7 to de0c18c Compare

September 22, 2023 16:51

k8s-ci-robot added size/M and removed size/XS labels

wojtek-t self-assigned this

mimowo force-pushed the backoff-limit-per-index-beta branch from de0c18c to 20e655e Compare

September 25, 2023 08:26

mimowo changed the title ~~Update KEP-3850 "Backoff Limit Per Index" for Beta~~ WIP: Update KEP-3850 "Backoff Limit Per Index" for Beta

mimowo marked this pull request as ready for review

September 25, 2023 08:28

Contributor Author

mimowo commented Sep 25, 2023

/assign @alculquicondor

k8s-ci-robot assigned alculquicondor

mimowo changed the title ~~WIP: Update KEP-3850 "Backoff Limit Per Index" for Beta~~ Update KEP-3850 "Backoff Limit Per Index" for Beta

k8s-ci-robot removed the do-not-merge/work-in-progress label

mimowo force-pushed the backoff-limit-per-index-beta branch from 45f0b4b to 20e655e Compare

September 27, 2023 09:51

alculquicondor reviewed

View reviewed changes

Member

alculquicondor left a comment

/lgtm

k8s-ci-robot added the lgtm label

Member

alculquicondor commented Sep 27, 2023

/assign @soltysh

k8s-ci-robot assigned soltysh

kannon92 reviewed

View reviewed changes

Contributor

kannon92 left a comment

Do you want to mention that in beta we should move these failure reasons to staging?

xref kubernetes/kubernetes@a62eb45#diff-a7577a7dede7864ff38c631319033714fdd0bed91108d976282e1099507e6ff0

wojtek-t reviewed

View reviewed changes

keps/sig-apps/3850-backoff-limits-per-index-for-indexed-jobs/README.md

    
              ###### What are the reasonable SLOs (Service Level Objectives) for the enhancement?

              This feature does not propose SLOs.

Member

wojtek-t Sep 28, 2023

Are there any tests for feature enablement/disablement?

Have those been added? Can you link? If not, can you please prioritize?

How can a rollout or rollback fail? Can it impact already running workloads?

it doesn't impact already running pods, but it actually impacts the way they are restarted in case of failures - seems worth clarifying.

also, please address "how can rollout/rollback fail"

Were upgrade and rollback tested? Was the upgrade->downgrade->upgrade path tested?

Please ensure that this will happen and the KEP will get updated before graduating FG in k/k.
Even better would be to describe a test scenario now, see
https://github.com/kubernetes/enhancements/pull/3658/files
as an example

Are there any missing metrics that would be useful to have to improve observability of this feature?

Are you still planning it? Can you update?

https://github.com/kubernetes/enhancements/tree/master/keps/NNNN-kep-template#can-enabling--using-this-feature-result-in-resource-exhaustion-of-some-node-resources-pids-sockets-inodes-etc

as a new question was added - please copy and fill-in

Contributor

soltysh Sep 29, 2023

I agree with Wojtek here, this feature is proposed to be promoted to beta, ie. on by default, so all the questions around rollback and monitoring when such should be performed are very important.

Contributor Author

mimowo Sep 29, 2023

@wojtek-t @soltysh thanks for reviewing. I think all the questions are now addressed. PTAL.


          Update KEP for Beta for "Backoff Limit Per Index"

295014f

mimowo force-pushed the backoff-limit-per-index-beta branch from 20e655e to 099f0d1 Compare

September 29, 2023 12:44

k8s-ci-robot removed the lgtm label

mimowo force-pushed the backoff-limit-per-index-beta branch 3 times, most recently from 71409f6 to 576a4b2 Compare

September 29, 2023 14:16


          Remarks

31e1403

mimowo force-pushed the backoff-limit-per-index-beta branch from 576a4b2 to 31e1403 Compare

September 29, 2023 14:16

k8s-ci-robot added size/L and removed size/M labels

alculquicondor reviewed

View reviewed changes

keps/sig-apps/3850-backoff-limits-per-index-for-indexed-jobs/README.md Outdated

    
              2. Simulate downgrade by disabling the feature for api server and control-plane:

              ```sh

Member

alculquicondor Sep 29, 2023

For the sake of keeping the KEP small for future consumption, I find the commands unnecessary.

Contributor Author

mimowo Sep 29, 2023

Do you mean all commands in this section, or just those to edit the control-plane manifests?

Contributor Author

mimowo Sep 29, 2023

For now, removed the commands to edit the manifests for now, and made some descriptions more concise. PTAL

keps/sig-apps/3850-backoff-limits-per-index-for-indexed-jobs/README.md Outdated Show resolved Hide resolved

mimowo and others added 2 commits

September 29, 2023 18:55


          Update keps/sig-apps/3850-backoff-limits-per-index-for-indexed-jobs/R…

e2ab56e

…EADME.md

Co-authored-by: Aldo Culquicondor <[email protected]>


          Simplify the upgrade->downgrade->upgrade section

bcbfe7e

alculquicondor reviewed

View reviewed changes

Member

alculquicondor left a comment

/lgtm

k8s-ci-robot added the lgtm label

BenTheElder reviewed

View reviewed changes

keps/sig-apps/3850-backoff-limits-per-index-for-indexed-jobs/README.md

    
              - Address reviews and bug reports from Alpha users

              - Propose and implement metrics

              - Implement the `job_finished_indexes_total` metric

              - E2e tests are in Testgrid and linked in KEP

Member

BenTheElder Sep 29, 2023

I see integration but not e2e?

Contributor Author

mimowo Oct 2, 2023

The test will be added after the feature gate is enabled by default, during the implementation phase for Beta.

wojtek-t reviewed

View reviewed changes

keps/sig-apps/3850-backoff-limits-per-index-for-indexed-jobs/README.md

    
              No. The tests will be added in Alpha.

              Yes, there is an [integration test](https://github.com/kubernetes/kubernetes/blob/dc28eeaa3a6e18ef683f4b2379234c2284d5577e/test/integration/job/job_test.go#L763)

              which tests the following path: enablement -> disablement -> re-enablement.

Member

wojtek-t Oct 2, 2023

Awesome :)

keps/sig-apps/3850-backoff-limits-per-index-for-indexed-jobs/README.md

    
              ###### Are there any missing metrics that would be useful to have to improve observability of this feature?

              For Beta we will consider introduction of a new metric `job_finished_indexes_total`

              For Beta we will introduce a new metric `job_finished_indexes_total`

Member

wojtek-t Oct 2, 2023

OK - thanks!

Member

wojtek-t commented Oct 2, 2023

/lgtm
/approve PRR

soltysh approved these changes

View reviewed changes

Contributor

soltysh left a comment

/lgtm
/approve
/label tide/merge-method-squash

k8s-ci-robot added the tide/merge-method-squash label

Contributor

k8s-ci-robot commented Oct 2, 2023

[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

k8s-ci-robot added the approved label

k8s-ci-robot merged commit f3466c7 into kubernetes:master

k8s-ci-robot added this to the v1.29 milestone

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

BenTheElder BenTheElder left review comments

kannon92 kannon92 left review comments

wojtek-t wojtek-t left review comments

soltysh soltysh approved these changes

kow3ns Awaiting requested review from kow3ns

+1 more reviewer

alculquicondor alculquicondor left review comments

Labels

approved cncf-cla: yes kind/kep lgtm sig/apps size/L tide/merge-method-squash