Skip to content
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

KEP-3998: Job success/completion policy #4062

Merged
merged 11 commits into from Feb 8, 2024
Merged

KEP-3998: Job success/completion policy #4062

merged 11 commits into from Feb 8, 2024

Conversation

tenzen-y
Copy link
Member

@tenzen-y tenzen-y commented Jun 6, 2023

  • One-line PR description: KEP for an API to define when a Job can be declared as succeeded.
  • Other comments:

@k8s-ci-robot k8s-ci-robot added the kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory label Jun 6, 2023
@k8s-ci-robot k8s-ci-robot added sig/apps Categorizes an issue or PR as relevant to SIG Apps. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jun 6, 2023
@tenzen-y tenzen-y force-pushed the kep-3998 branch 2 times, most recently from 24a688c to 4fc68f1 Compare June 6, 2023 13:10
@tenzen-y
Copy link
Member Author

tenzen-y commented Jun 6, 2023

cc @alculquicondor @mimowo

This was referenced Jun 6, 2023
Open
Closed
@tenzen-y
Copy link
Member Author

tenzen-y commented Jun 8, 2023

@alculquicondor @mimowo I updated the API design. Can you check this?

@tenzen-y
Copy link
Member Author

tenzen-y commented Jun 8, 2023

@soltysh Could you take a look at this proposal?

@tenzen-y
Copy link
Member Author

tenzen-y commented Jun 8, 2023

/wg batch

@k8s-ci-robot k8s-ci-robot added the wg/batch Categorizes an issue or PR as relevant to WG Batch. label Jun 8, 2023
@kannon92
Copy link
Contributor

kannon92 commented Jun 10, 2023 •
edited

Don’t forget to get the ci checks green. Looks like the linter is saying you are missing some fields

@tenzen-y
Copy link
Member Author

Don’t forget to get the ci checks green. Looks like the linter is saying you are missing some fields

Thanks @kannon92! I will check the CI error.

@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jun 11, 2023
@tenzen-y
Copy link
Member Author

Don’t forget to get the ci checks green. Looks like the linter is saying you are missing some fields

Thanks @kannon92! I will check the CI error.

Done.

@tenzen-y
Fix nits
acb4ac5 
Signed-off-by: Yuki Iwai <yuki.iwai.tz@gmail.com>
@tenzen-y
If SuccessPolicy is null, the SuccessCriteriaMet isn't added to the Job
0239e3a 
Signed-off-by: Yuki Iwai <yuki.iwai.tz@gmail.com>
@tenzen-y
Add the optional second alpha stage for Job whenCriterionAchived and …
ec4464e 
…CronJob ForbidUntilJobSuccessful

Signed-off-by: Yuki Iwai <yuki.iwai.tz@gmail.com>
@tenzen-y
Update history
3876c49 
Signed-off-by: Yuki Iwai <yuki.iwai.tz@gmail.com>
@tenzen-y
Rephrase 'version skew strategy'
021a1ea 
Signed-off-by: Yuki Iwai <yuki.iwai.tz@gmail.com>
@tenzen-y
Add TODO to sections that we should be done befre promoting to beta
217adef 
Signed-off-by: Yuki Iwai <yuki.iwai.tz@gmail.com>
@tenzen-y
Copy link
Member Author

tenzen-y commented Feb 7, 2024

@wojtek-t I addressed all comments.
Please take another look. Thank you!

@tenzen-y
Add the possibility of increasing consumed resources
97fd057 
Signed-off-by: Yuki Iwai <yuki.iwai.tz@gmail.com>
alculquicondor
alculquicondor reviewed Feb 8, 2024
View reviewed changes
Copy link
Member

@alculquicondor alculquicondor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still LGTM

keps/sig-apps/3998-job-success-completion-policy/README.md Outdated Show resolved Hide resolved
keps/sig-apps/3998-job-success-completion-policy/README.md Outdated
However, both Job level and JobSet level successPolicies would be valuable
since there are some cases in which we want to launch a Job by a single podTemplate.

<<[UNRESOLVED @tenzen-y: Do we need to make configurable actions? ]>>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the purpose of this iteration, these are not unresolved questions. We have concluded that they might be re-evaluated in a future version. You can mention this in the alpha/beta criteria, but you shouldn't leave the UNRESOLVED tags.

@wojtek-t
Copy link
Member

wojtek-t commented Feb 8, 2024

Just one more comment from the PRR.

I will approve though to not block it and ad hold to have it addressed before submitting.

/approve PRR
/hold

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 8, 2024
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: soltysh, tenzen-y, 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:

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 k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 8, 2024
@tenzen-y
Move unresolved sections to the future work section
394c33e 
Signed-off-by: Yuki Iwai <yuki.iwai.tz@gmail.com>
@tenzen-y
Update enablement/disablement tests
e6a78ff 
Signed-off-by: Yuki Iwai <yuki.iwai.tz@gmail.com>
@tenzen-y
Update the definition of the Complete condition
a41593c 
Signed-off-by: Yuki Iwai <yuki.iwai.tz@gmail.com>
@wojtek-t
Copy link
Member

wojtek-t commented Feb 8, 2024

/lgtm
/hold cancel

@k8s-ci-robot k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Feb 8, 2024
alculquicondor
alculquicondor reviewed Feb 8, 2024
View reviewed changes
Copy link
Member

@alculquicondor alculquicondor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm
/hold cancel

@alculquicondor
Copy link
Member

race condition :)

@tenzen-y
Copy link
Member Author

tenzen-y commented Feb 8, 2024

race condition :)

The browser is loading this PR so slower, so race conditions often occur :)

@k8s-ci-robot k8s-ci-robot merged commit 6d05986 into kubernetes:master Feb 8, 2024
4 checks passed
@k8s-ci-robot k8s-ci-robot added this to the v1.30 milestone Feb 8, 2024
@tenzen-y tenzen-y deleted the kep-3998 branch February 8, 2024 19:58
@tenzen-y
Copy link
Member Author

tenzen-y commented Feb 8, 2024

Thank you to everyone!

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

@vsoch vsoch vsoch left review comments

@alculquicondor alculquicondor alculquicondor left review comments

@atiratree atiratree atiratree left review comments

@kannon92 kannon92 kannon92 left review comments

@deads2k deads2k deads2k left review comments

@mimowo mimowo mimowo left review comments

@wojtek-t wojtek-t wojtek-t left review comments

@sftim sftim sftim left review comments

@ahg-g ahg-g ahg-g left review comments

@danielvegamyhre danielvegamyhre danielvegamyhre left review comments

@soltysh soltysh soltysh approved these changes

@kow3ns kow3ns Awaiting requested review from kow3ns

Assignees

@alculquicondor alculquicondor

@wojtek-t wojtek-t

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory lgtm "Looks good to me", indicates that a PR is ready to be merged. sig/apps Categorizes an issue or PR as relevant to SIG Apps. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. wg/batch Categorizes an issue or PR as relevant to WG Batch.
Projects
SIG Apps
Archived in project
Milestone
v1.30
Development

Successfully merging this pull request may close these issues.

None yet