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

Add JobSuccessPolicy Doc #45135

Merged
merged 7 commits into from Mar 26, 2024
Merged

Add JobSuccessPolicy Doc #45135

merged 7 commits into from Mar 26, 2024

Conversation

tenzen-y
Copy link
Member

@tenzen-y tenzen-y commented Feb 14, 2024 •
edited

In this PR, I added documentation for the kubernetes/enhancements#3998.

@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 14, 2024
@netlify Netlify
Copy link

netlify bot commented Feb 14, 2024 •
edited

👷 Deploy Preview for kubernetes-io-vnext-staging processing.

Name Link
🔨 Latest commit 7465256
🔍 Latest deploy log https://app.netlify.com/sites/kubernetes-io-vnext-staging/deploys/6601b93f0a22380008b3c864

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Feb 14, 2024
@tenzen-y tenzen-y mentioned this pull request Feb 14, 2024
Open
15 tasks
@k8s-ci-robot k8s-ci-robot added language/en Issues or PRs related to English language sig/docs Categorizes an issue or PR as relevant to SIG Docs. labels Feb 14, 2024
@dipesh-rawat
Copy link
Member

/sig apps

@k8s-ci-robot k8s-ci-robot added the sig/apps Categorizes an issue or PR as relevant to SIG Apps. label Feb 14, 2024
@reylejano
Copy link
Member

/milestone 1.30

@k8s-ci-robot k8s-ci-robot added this to the 1.30 milestone Feb 16, 2024
@fsmunoz
Copy link
Contributor

fsmunoz commented Mar 6, 2024

Hello @tenzen-y 👋 !

Please take a look at Documenting for a release - PR Ready for Review to get your PR ready for review before the deadline Tuesday March 12th 2024 18:00 PST.

Thank you!

@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Mar 8, 2024
@tenzen-y tenzen-y changed the title WIP: Add JobSuccessPolicy Doc Add JobSuccessPolicy Doc Mar 8, 2024
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 8, 2024
@tenzen-y
Copy link
Member Author

tenzen-y commented Mar 8, 2024 •
edited

This PR is ready for the review. Please take a look.
/assign @alculquicondor @soltysh @kannon92 @mimowo

@tenzen-y
Copy link
Member Author

tenzen-y commented Mar 8, 2024

/unassign @FillZpp

(Sorry, due to my bad, I assigned this to @FillZpp)

/assign @atiratree

@k8s-ci-robot k8s-ci-robot assigned atiratree and unassigned FillZpp Mar 8, 2024
atiratree
atiratree reviewed Mar 11, 2024
View reviewed changes
content/en/docs/concepts/workloads/controllers/job.md Outdated Show resolved Hide resolved
content/en/docs/concepts/workloads/controllers/job.md Outdated Show resolved Hide resolved
content/en/docs/concepts/workloads/controllers/job.md Outdated Show resolved Hide resolved
content/en/docs/concepts/workloads/controllers/job.md Outdated Show resolved Hide resolved
content/en/docs/concepts/workloads/controllers/job.md Outdated

In the example above, the rule of the success policy specifies that
the Job should be marked succeeded and terminate the lingering Pods
if one of the 0, 1, and 2 indexes succeeded.
Copy link
Member

Choose a reason for hiding this comment

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

Can we mention the behavior of SuccessCriteriaMet and Complete conditions?

Copy link
Member Author

Choose a reason for hiding this comment

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

That makes sense.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 11, 2024
@tenzen-y
Copy link
Member Author

Just rebased.

@sftim
Copy link
Contributor

sftim commented Mar 25, 2024

Nudging Prow (doesn't need a rebase any more)

@tenzen-y
Copy link
Member Author

/remove-language zh
/remove-area blog

@k8s-ci-robot k8s-ci-robot removed language/zh Issues or PRs related to Chinese language area/blog Issues or PRs related to the Kubernetes Blog subproject labels Mar 25, 2024
@tenzen-y
Copy link
Member Author

Nudging Prow (doesn't need a rebase any more)

I got it.

@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 25, 2024
@tenzen-y
KEP-3998: move section to before Job termination and cleanup
105d90a 
Signed-off-by: Yuki Iwai <yuki.iwai.tz@gmail.com>
@tenzen-y
KEP-3998: Make the explanation for rules more clarify
32fd60c 
Signed-off-by: Yuki Iwai <yuki.iwai.tz@gmail.com>
@tenzen-y
KEP-3998: Apply Aldo's suggestons
cec3c3f 
Signed-off-by: Yuki Iwai <yuki.iwai.tz@gmail.com>
@tenzen-y
KEP-3998: Mention SuccessCriteriaMet condition
fcdb477 
Signed-off-by: Yuki Iwai <yuki.iwai.tz@gmail.com>
alculquicondor
alculquicondor reviewed Mar 25, 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 from a technical PoV

content/en/docs/concepts/workloads/controllers/job.md Outdated Show resolved Hide resolved
content/en/docs/concepts/workloads/controllers/job.md Outdated Show resolved Hide resolved
content/en/docs/concepts/workloads/controllers/job.md Outdated Show resolved Hide resolved
content/en/docs/concepts/workloads/controllers/job.md Outdated Show resolved Hide resolved
content/en/docs/concepts/workloads/controllers/job.md Outdated Show resolved Hide resolved
content/en/docs/concepts/workloads/controllers/job.md Outdated Show resolved Hide resolved
@tenzen-y
KEP-3998: Clarify the example
d79de02 
Signed-off-by: Yuki Iwai <yuki.iwai.tz@gmail.com>
@tenzen-y
KEP-3998: Aldo's second suggestions
7465256 
Signed-off-by: Yuki Iwai <yuki.iwai.tz@gmail.com>
@tenzen-y
Copy link
Member Author

@alculquicondor @sftim I addressed all comments.
Please take another look. Thank you for the review!

@alculquicondor
Copy link
Member

/lgtm
as approver for the job controller

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 25, 2024
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 6f9ac62721af1b55d81ddff1bae8e2a477299325

@tenzen-y
Copy link
Member Author

@tenzen-y hello, Release Lead Shadow for 1.30, acting as Docs shadow; as @drewhagen mentioned above, the Docs Freeze is fast approaching (Tuesday March 26th 18:00 PDT).

From what I've seen, the technical review is not done yet incorporated (@alculquicondor and @atiratree have made comments, and there is discussion around them, but they seem to be open).

There are some pending review items from SIG Docs - could you also take a look? I think that with that the lgtm and approve process can follow.

Thanks!

@fsmunoz If this PR doesn't get approval from Docs approver, do I need to submit any exception request?

@drewhagen
Copy link
Member

drewhagen commented Mar 26, 2024 •
edited

@tenzen-y hello, Release Lead Shadow for 1.30, acting as Docs shadow; as @drewhagen mentioned above, the Docs Freeze is fast approaching (Tuesday March 26th 18:00 PDT).
From what I've seen, the technical review is not done yet incorporated (@alculquicondor and @atiratree have made comments, and there is discussion around them, but they seem to be open).
There are some pending review items from SIG Docs - could you also take a look? I think that with that the lgtm and approve process can follow.
Thanks!

@fsmunoz If this PR doesn't get approval from Docs approver, do I need to submit any exception request?

@tenzen-y Technically yeah, this is the Exception process that needs to be filed after 18:00 PDT tomorrow. We're here to help you get this over the finish line before Docs Freeze tomorrow though!

@kubernetes/sig-docs-en-owners It looks like there was a cycle of feedback and new commits addressing them, and we need an approve label for merge. Does this look good to y'all?

divya-mohan0209
divya-mohan0209 reviewed Mar 26, 2024
View reviewed changes
Copy link
Contributor

@divya-mohan0209 divya-mohan0209 left a comment

Choose a reason for hiding this comment

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

@drewhagen While the overall doc is LGTM, I suggest we clarify further in a follow-up PR. Suggestions have been added herewith.

Biasing for action since this has received technical approval.

/approve

content/en/docs/concepts/workloads/controllers/job.md

You can configure a success policy, in the `.spec.successPolicy` field,
to meet the above use cases. This policy can handle Job success based on the
succeeded pods. After the Job meet success policy, the job controller terminates the lingering Pods.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit (can be addressed later).

Suggested change
succeeded pods. After the Job meet success policy, the job controller terminates the lingering Pods.
succeeded pods. After the Job meets the success policy, the job controller terminates the lingering Pods.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

content/en/docs/concepts/workloads/controllers/job.md

In the example above, the rule of the success policy specifies that
the Job should be marked succeeded and terminate the lingering Pods
if one of the 0, 2, and 3 indexes succeeded.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: We might want to rephrase this question for better understanding. Ideally, we should be saying something like

"In the example above, both succeededIndexes and succeededCount have been specified. Therefore, the job controller will mark the Job as succeeded and terminate the lingering Pods when either of the specified indexes, 0, 2, or 3, succeed."

P.S. We can do this in a follow-up PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you for this great suggestion!

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

content/en/docs/concepts/workloads/controllers/job.md
In the example above, the rule of the success policy specifies that
the Job should be marked succeeded and terminate the lingering Pods
if one of the 0, 2, and 3 indexes succeeded.
The Job that met the success policy gets the `SuccessCriteriaMet` condition.
Copy link
Contributor

Choose a reason for hiding this comment

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

Grammatical nit: Can be amended in a follow-up PR.

Suggested change
The Job that met the success policy gets the `SuccessCriteriaMet` condition.
The Job that meets the success policy gets the `SuccessCriteriaMet` condition.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

content/en/docs/concepts/workloads/controllers/job.md
the Job should be marked succeeded and terminate the lingering Pods
if one of the 0, 2, and 3 indexes succeeded.
The Job that met the success policy gets the `SuccessCriteriaMet` condition.
After the removal of the lingering Pods is issued, the Job gets the `Complete` condition.
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we clarify here?

The Job is considered complete after the Job Controller removes the lingering pods.

Copy link
Member Author

Choose a reason for hiding this comment

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

No, the job controller doesn't care if the lingering pods are actually removed.
Again, the job controller adds the Complete condition after the removal of the lingering Pods is issued.
So, the Job gets the Complete condition even if some lingering pods are still terminating state.

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: divya-mohan0209

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 Mar 26, 2024
@k8s-ci-robot k8s-ci-robot merged commit deb1be8 into kubernetes:dev-1.30 Mar 26, 2024
6 checks passed
@tenzen-y tenzen-y deleted the job-success-policy-doc branch March 26, 2024 09:29
@tenzen-y
Copy link
Member Author

tenzen-y commented Mar 26, 2024 •
edited

@divya-mohan0209 Thank you for reviewing this PR! I'm addressing your comments in a separate PR!

@tenzen-y tenzen-y mentioned this pull request Mar 26, 2024
Merged
@tenzen-y
Copy link
Member Author

@divya-mohan0209 I opened a follow-up PR: #45677
PTAL, thanks.

@drewhagen
Copy link
Member

/milestone 1.30

@k8s-ci-robot k8s-ci-robot added this to the 1.30 milestone Apr 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alculquicondor alculquicondor alculquicondor left review comments

@atiratree atiratree atiratree left review comments

@mimowo mimowo mimowo left review comments

@divya-mohan0209 divya-mohan0209 divya-mohan0209 left review comments

@sftim sftim sftim left review comments

@nate-double-u nate-double-u Awaiting requested review from nate-double-u

Assignees

@soltysh soltysh

@alculquicondor alculquicondor

@atiratree atiratree

@kannon92 kannon92

@mimowo mimowo

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. language/en Issues or PRs related to English language 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. sig/docs Categorizes an issue or PR as relevant to SIG Docs. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
SIG Apps
Status: Done
Milestone
1.30
Development

Successfully merging this pull request may close these issues.

None yet