Fix rayjob will not resume after preempted #1156

kerthcet · 2023-09-23T02:00:57Z

What type of PR is this?

/kind bug

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Fix resuming of RayJob after preempted.

netlify · 2023-09-23T02:01:07Z

✅ Deploy Preview for kubernetes-sigs-kueue canceled.

Name	Link
🔨 Latest commit	`7623656`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-kueue/deploys/6522d1108019cc000809bc96

mimowo

/lgtm
Just a non-blocking nit
/assign @tenzen-y

test/integration/controller/jobs/rayjob/rayjob_controller_test.go

k8s-ci-robot · 2023-09-25T06:52:05Z

LGTM label has been added.

Git tree hash: 6edd292534a5db3e477d0162dc94221b9c20e053

mimowo · 2023-09-25T06:53:19Z

@kerthcet please also set the release note, maybe "Fix resuming of RayJob after preempted".

tenzen-y

We should probably add unit tests for RayJob in the case of finished.
However, I think we can work on that at a follow-up.

tenzen-y · 2023-09-25T08:26:32Z

pkg/controller/jobs/rayjob/rayjob_controller.go

-	return condition, rayjobapi.IsJobTerminal(j.Status.JobStatus)
+
+	return condition, j.Status.JobStatus == rayjobapi.JobStatusFailed || j.Status.JobStatus == rayjobapi.JobStatusSucceeded


I'm not sure if this is a bug or the specification on the kuberay side.
My concern is this behavior might be changed according to the kuberay versions.

Are there any documentation or API comments?

it sounds like it should be fixed on the rayjob side.

In kuberay, when stopping a job, it will check whether the job is stopped, succeeded or failed, if no, kuberay will stop the job, so IsJobTerminal includes stop status. But here, we should only consider the terminated status. The name is puzzling anyway.

Ah, I see. The stopped means suspended.

It still sounds like the name or the logic could be fixed in kuberay.

tenzen-y · 2023-09-25T08:46:57Z

Fix resuming of RayJob after preempted"

It sounds like a great suggestion!

/release-note edit Fix resuming of RayJob after preempted.

tenzen-y · 2023-09-25T08:48:04Z

/release-note-edit Fix resuming of RayJob after preempted.

mimowo · 2023-09-25T08:48:04Z

/release-note edit Fix resuming of RayJob after preempted.

Let me try:
/release-note-edit Fix resuming of RayJob after preempted.

tenzen-y · 2023-09-25T08:49:39Z

This command wouldn't be work fine 😞

kerthcet · 2023-09-26T15:37:17Z

I'm back to this pr later as I'm on kubecon these days.

andrewsykim · 2023-09-28T14:50:23Z

/cc

Signed-off-by: kerthcet <kerthcet@gmail.com>

kerthcet · 2023-10-08T15:56:08Z

Updated based on the comments.

kerthcet · 2023-10-08T15:57:53Z

I think we should cherry-pick this bug fix.

mimowo · 2023-10-09T09:23:52Z

/lgtm
@assign @tenzen-y

k8s-ci-robot · 2023-10-09T09:23:58Z

LGTM label has been added.

Git tree hash: 2943b24fbfc79d0c3d964391a2edb2f9ef3e36bc

mimowo · 2023-10-09T09:24:04Z

/assign @tenzen-y

tenzen-y

Thanks!
/lgtm
/approve

tenzen-y · 2023-10-09T09:42:05Z

pkg/controller/jobs/rayjob/rayjob_controller.go

-	return condition, rayjobapi.IsJobTerminal(j.Status.JobStatus)
+
+	return condition, j.Status.JobStatus == rayjobapi.JobStatusFailed || j.Status.JobStatus == rayjobapi.JobStatusSucceeded


Ah, I see. The stopped means suspended.

k8s-ci-robot · 2023-10-09T09:49:22Z

[APPROVALNOTIFIER] This PR is APPROVED

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

~~OWNERS~~ [tenzen-y]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

…mpted (#1190) * Fix rayjob will not resume after preempted Signed-off-by: kerthcet <kerthcet@gmail.com> * Fix integration test error Signed-off-by: kerthcet <kerthcet@gmail.com> --------- Signed-off-by: kerthcet <kerthcet@gmail.com>

kerthcet · 2023-10-26T07:59:14Z

Title: Fix rayjob will not resume after preempted
Summary: |-
 This PR fixes an issue where a RayJob would not resume after it was preempted.
 The issue was caused by a bug in the RayJob controller, which prevented the job from being resumed after it was preempted.
 This PR introduces a fix for this issue, which ensures that a preempted RayJob is resumed correctly.
Types:
 - bugfix
Main Files Walkthrough:
 - filename: pkg/controller/jobs/rayjob/rayjob_controller.go
   changes in file:
     - Fixes a bug in the RayJob controller that prevented preempted RayJobs from being resumed correctly.
     - Adds a new function to the RayJob controller that handles the resumption of preempted RayJobs.
     - Updates the existing function to use the new function to handle preempted RayJobs.
 - filename: test/integration/controller/jobs/rayjob/rayjob_controller_test.go
   changes in file:
     - Adds a new test case that verifies the fix for the issue.
     - Updates the existing test cases to use the new test case.

alculquicondor · 2023-10-26T16:55:05Z

@kerthcet what's the above?

kerthcet · 2023-10-27T00:54:42Z

@kerthcet what's the above?

Sorry, I'm benchmarking the code LM, forgot to delete the comment.🥺

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. kind/bug Categorizes issue or PR as related to a bug. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Sep 23, 2023

k8s-ci-robot requested review from ArangoGutierrez and denkensk September 23, 2023 02:01

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Sep 23, 2023

mimowo reviewed Sep 25, 2023

View reviewed changes

test/integration/controller/jobs/rayjob/rayjob_controller_test.go Show resolved Hide resolved

k8s-ci-robot assigned tenzen-y and mimowo Sep 25, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 25, 2023

tenzen-y reviewed Sep 25, 2023

View reviewed changes

k8s-ci-robot requested a review from andrewsykim September 28, 2023 14:50

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 Oct 8, 2023

Fix rayjob will not resume after preempted

7623656

Signed-off-by: kerthcet <kerthcet@gmail.com>

kerthcet force-pushed the fix/rayjob-resume branch from 73b72b4 to 7623656 Compare October 8, 2023 15:55

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 8, 2023

k8s-ci-robot requested review from mimowo and tenzen-y October 8, 2023 15:56

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 9, 2023

tenzen-y reviewed Oct 9, 2023

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 9, 2023

k8s-ci-robot merged commit 938d118 into kubernetes-sigs:main Oct 9, 2023
14 checks passed

k8s-ci-robot added this to the v0.5 milestone Oct 9, 2023

kerthcet deleted the fix/rayjob-resume branch October 10, 2023 01:42

This was referenced Oct 10, 2023

Automated cherry pick of #1156: Fix rayjob will not resume after preempted #1189

Closed

Automated cherry pick of #1156: Fix rayjob will not resume after preempted #1190

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix rayjob will not resume after preempted #1156

Fix rayjob will not resume after preempted #1156

kerthcet commented Sep 23, 2023 •

edited

netlify bot commented Sep 23, 2023 •

edited

mimowo left a comment

k8s-ci-robot commented Sep 25, 2023

mimowo commented Sep 25, 2023

tenzen-y left a comment

tenzen-y Sep 25, 2023

alculquicondor Sep 27, 2023

kerthcet Oct 8, 2023

tenzen-y Oct 9, 2023

alculquicondor Oct 10, 2023

tenzen-y commented Sep 25, 2023

tenzen-y commented Sep 25, 2023

mimowo commented Sep 25, 2023

tenzen-y commented Sep 25, 2023

kerthcet commented Sep 26, 2023

andrewsykim commented Sep 28, 2023

kerthcet commented Oct 8, 2023

kerthcet commented Oct 8, 2023

mimowo commented Oct 9, 2023

k8s-ci-robot commented Oct 9, 2023

mimowo commented Oct 9, 2023

tenzen-y left a comment

tenzen-y Oct 9, 2023

k8s-ci-robot commented Oct 9, 2023

kerthcet commented Oct 26, 2023

alculquicondor commented Oct 26, 2023

kerthcet commented Oct 27, 2023

		return condition, rayjobapi.IsJobTerminal(j.Status.JobStatus)

		return condition, j.Status.JobStatus == rayjobapi.JobStatusFailed \|\| j.Status.JobStatus == rayjobapi.JobStatusSucceeded

Fix rayjob will not resume after preempted #1156

Fix rayjob will not resume after preempted #1156

Conversation

kerthcet commented Sep 23, 2023 • edited

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

netlify bot commented Sep 23, 2023 • edited

✅ Deploy Preview for kubernetes-sigs-kueue canceled.

mimowo left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Sep 25, 2023

mimowo commented Sep 25, 2023

tenzen-y left a comment

Choose a reason for hiding this comment

tenzen-y Sep 25, 2023

Choose a reason for hiding this comment

alculquicondor Sep 27, 2023

Choose a reason for hiding this comment

kerthcet Oct 8, 2023

Choose a reason for hiding this comment

tenzen-y Oct 9, 2023

Choose a reason for hiding this comment

alculquicondor Oct 10, 2023

Choose a reason for hiding this comment

tenzen-y commented Sep 25, 2023

tenzen-y commented Sep 25, 2023

mimowo commented Sep 25, 2023

tenzen-y commented Sep 25, 2023

kerthcet commented Sep 26, 2023

andrewsykim commented Sep 28, 2023

kerthcet commented Oct 8, 2023

kerthcet commented Oct 8, 2023

mimowo commented Oct 9, 2023

k8s-ci-robot commented Oct 9, 2023

mimowo commented Oct 9, 2023

tenzen-y left a comment

Choose a reason for hiding this comment

tenzen-y Oct 9, 2023

Choose a reason for hiding this comment

k8s-ci-robot commented Oct 9, 2023

kerthcet commented Oct 26, 2023

alculquicondor commented Oct 26, 2023

kerthcet commented Oct 27, 2023

kerthcet commented Sep 23, 2023 •

edited

netlify bot commented Sep 23, 2023 •

edited