-
Notifications
You must be signed in to change notification settings - Fork 288
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.
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
Extend documentation on pod group support [pod groups] #1684
Conversation
✅ Deploy Preview for kubernetes-sigs-kueue ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
cff3917
to
2f6fea0
Compare
/assign @tenzen-y @alculquicondor |
lgtm |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, should we rephrase Line 97 with "Example single Pod"?
Note that, this design choice impacts the scenario of | ||
[preemption](/docs/concepts/cluster_queue/#preemption). | ||
When a workload represented by the pod group is preempted all of its pods | ||
are killed by Kueue (by delete requests). However, later, when the workload is | ||
re-admitted, Kueue will not re-create the terminated pods. This task is left to | ||
the user (or the external controller). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great note
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I forgot to send my comments yesterday :(
466c735
to
eee3acf
Compare
eee3acf
to
4d0e5a6
Compare
Co-authored-by: Yuki Iwai <[email protected]> Co-authored-by: Patryk Bundyra <[email protected]>
3cb5660
to
317ac6a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Otherwise, lgtm
/approve
leave lgtm on @alculquicondor
**NOTE:** We recommend using the kubernetes Job API or similar CRDs such as | ||
JobSet, MPIJob, RayJob, etc. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tied https://kueue.sigs.k8s.io/docs/tasks/#batch-user here would be better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated. PTAL.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/approve
/hold for comments
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alculquicondor, mimowo, tenzen-y 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 |
/hold cancel Comments addressed, PTAL. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
LGTM label has been added. Git tree hash: 17e670abb3552cbea3c6a5e1c15d760489a2c931
|
…gs#1684) * Extend documentation on pod group support * Review remarks Co-authored-by: Yuki Iwai <[email protected]> Co-authored-by: Patryk Bundyra <[email protected]> * Align casing of pods and pod groups for consistency * review remarks * Fix indentation --------- Co-authored-by: Yuki Iwai <[email protected]> Co-authored-by: Patryk Bundyra <[email protected]>
What type of PR is this?
/kind documentation
What this PR does / why we need it:
Which issue(s) this PR fixes:
Part of #976
Special notes for your reviewer:
Does this PR introduce a user-facing change?