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

Do not default to suspending a job whose parent is already managed by Kueue #1846

Merged
merged 2 commits into from Mar 15, 2024
Merged

Do not default to suspending a job whose parent is already managed by Kueue #1846

merged 2 commits into from Mar 15, 2024

Conversation

astefanutti
Copy link
Member

@astefanutti astefanutti commented Mar 14, 2024 •
edited

What type of PR is this?

/kind regression

Which issue(s) this PR fixes:

Fixes #1844

This is a regression introduced in #1747.

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Do not default to suspending a job whose parent is already managed by Kueue

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. kind/regression Categorizes issue or PR as related to a regression from a prior release. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Mar 14, 2024
@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Mar 14, 2024
@netlify Netlify
Copy link

netlify bot commented Mar 14, 2024 •
edited

✅ Deploy Preview for kubernetes-sigs-kueue canceled.

Name Link
🔨 Latest commit a322a19
🔍 Latest deploy log https://app.netlify.com/sites/kubernetes-sigs-kueue/deploys/65f4127f3326e6000864756e

@tenzen-y
Copy link
Member

/remove-kind regression
/kind bug

@k8s-ci-robot k8s-ci-robot added kind/bug Categorizes issue or PR as related to a bug. and removed kind/regression Categorizes issue or PR as related to a regression from a prior release. labels Mar 14, 2024
@tenzen-y
Copy link
Member

@astefanutti Could you update a release note?

@tenzen-y tenzen-y mentioned this pull request Mar 14, 2024
Closed
26 tasks
@astefanutti
Copy link
Member Author

@tenzen-y I wasn't sure what's your convention here. I think that's a regression that has only been introduced in main at the moment, is it still relevant to mention it in the release notes?

@tenzen-y
Copy link
Member

@tenzen-y I wasn't sure what's your convention here. I think that's a regression that has only been introduced in main at the moment, is it still relevant to mention it in the release notes?

Oh, I didn't find that. So, I'm ok with not having any release notes. Thanks!

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Mar 14, 2024
@astefanutti
Copy link
Member Author

/retitle Do not default to suspending a job whose parent is already managed by Kueue

@k8s-ci-robot k8s-ci-robot changed the title Do not default to suspending RayClusters managed by RayJobs Do not default to suspending a job whose parent is already managed by Kueue Mar 14, 2024
alculquicondor
alculquicondor reviewed Mar 14, 2024
View reviewed changes
Copy link
Contributor

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

/approve

@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 14, 2024
@alculquicondor
Copy link
Contributor

@tenzen-y anything to add?

tenzen-y
tenzen-y reviewed Mar 14, 2024
View reviewed changes
Copy link
Member

@tenzen-y tenzen-y left a comment •
edited

Choose a reason for hiding this comment

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

Generally, lgtm

This error could happen in the case of MPIJob (MPIJob uses batch/v1 Job) as well.
Thank you!

I left some nit comments to improve visibility.

test/integration/controller/jobs/raycluster/raycluster_webhook_test.go Outdated Show resolved Hide resolved
test/integration/controller/jobs/raycluster/raycluster_webhook_test.go Outdated Show resolved Hide resolved
tenzen-y
tenzen-y reviewed Mar 15, 2024
View reviewed changes
Copy link
Member

@tenzen-y tenzen-y left a comment

Choose a reason for hiding this comment

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

Thank you!
/lgtm
/approve

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

LGTM label has been added.

Git tree hash: 26c46fae6b89aaee5d555b4dfe31c52a2ce0d208

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alculquicondor, astefanutti, 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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot merged commit c296cea into kubernetes-sigs:main Mar 15, 2024
14 checks passed
@k8s-ci-robot k8s-ci-robot added this to the v0.7 milestone Mar 15, 2024
vsoch pushed a commit to researchapps/kueue that referenced this pull request Apr 18, 2024
@astefanutti @vsoch
Do not default to suspending a job whose parent is already managed by…
7689031 
… Kueue (kubernetes-sigs#1846)

* Do not default to suspending a job whose parent is already managed by Kueue

* Better test assertions
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

@tenzen-y tenzen-y tenzen-y left review comments

@mimowo mimowo Awaiting requested review from mimowo

@trasc trasc Awaiting requested review from trasc

Assignees

@tenzen-y tenzen-y

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/bug Categorizes issue or PR as related to a bug. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
v0.7
Development

Successfully merging this pull request may close these issues.

RayClusters managed by RayJobs stay in suspended state
4 participants
@astefanutti @tenzen-y @alculquicondor @k8s-ci-robot