Add RayJob CRD and controller logic #303

harryge00 · 2022-06-14T11:43:49Z

Why are these changes needed?

Add RayJob CRD and its controller.
Includes:

Get or create corresponding Rayclusters for ray jobs.
Connect ray dashboard
Submit ray job and check job status.

This PR borrows a lot from @brucez-anyscale , thanks so much.

Related issue number

#106

Checks

I've made sure the tests are passing.
Testing Strategy
- Unit tests
- Manual tests
- This PR is not tested :(

Jeffwan · 2022-06-14T23:18:35Z

Thanks for the contribution! I will have a check today

Jeffwan

Seems some core logic in job controller is not ready yet. I left some comments and I will come back later for another round of review.

ray-operator/apis/ray/v1alpha1/rayjob_types.go

ray-operator/config/crd/bases/ray.io_rayservices.yaml

ray-operator/controllers/ray/utils/dashboard_httpclient.go

ray-operator/controllers/ray/rayservice_controller.go

ray-operator/controllers/ray/rayjob_controller.go

brucez-anyscale

Thanks. Please update the naming in job controller and do some manual integration test to ensure it works.

ray-operator/apis/ray/v1alpha1/rayjob_types.go

brucez-anyscale · 2022-06-15T19:05:37Z

ray-operator/config/crd/patches/cainjection_in_ray_rayjobs.yaml

@@ -0,0 +1,7 @@
+# The following patch adds a directive for certmanager to inject CA into the CRD
+apiVersion: apiextensions.k8s.io/v1


ray-operator/config/crd/patches/webhook_in_ray_rayjobs.yaml

ray-operator/controllers/ray/rayjob_controller.go

ray-operator/controllers/ray/utils/dashboard_httpclient_test.go

Jeffwan · 2022-07-02T06:13:34Z

@harryge00 Did you get a chance to update the PR and address the comments? We are close to v0.3.0 release and we plan to include this feature in the release.

harryge00 · 2022-07-12T14:13:49Z

@Jeffwan @brucez-anyscale I have just updated this PR, could u take another review?

Jeffwan · 2022-07-12T20:21:17Z

@harryge00 Could you help fix the linter and build issue?

ray-operator/controllers/ray/rayjob_controller.go

ray-operator/apis/ray/v1alpha1/rayjob_types.go

ray-operator/controllers/ray/common/service.go

ray-operator/controllers/ray/rayjob_controller.go

ray-operator/controllers/ray/utils/dashboard_httpclient.go

Jeffwan · 2022-07-14T04:37:54Z

I think the polished version is close to complete. Great work! Can you help address my above comments @harryge00

ray-operator/config/crd/patches/cainjection_in_ray_rayjobs.yaml

ray-operator/config/rbac/role.yaml

ray-operator/controllers/ray/common/service.go

ray-operator/controllers/ray/rayjob_controller.go

brucez-anyscale · 2022-07-15T05:09:44Z

We should at least add unit tests. Also please manually test it in EKS or GKE or kind cluster.

Jeffwan · 2022-07-18T17:47:00Z

@harryge00 Please help fix the build issue and lint issues. The logics look good to me and we can file follow up PRs to address potential issue.

harryge00 · 2022-07-18T17:51:33Z

ray-operator/controllers/ray/rayjob_controller.go

+
+	// Set rayClusterName and rayJobId first, to avoid duplicate submission
+	err = r.setRayJobIdAndRayClusterNameIfNeed(ctx, rayJobInstance)
+	if err != nil {


@brucez-anyscale
#303 (comment)
I now save cluster name and job Id before reconciling rayJob. In this case, rayCluster will not be created duplicately because only the first updating of status will succeed

If you update status in setRayJobIdAndRayClusterNameIfNeed, you should better just return this reconcile loop.
Or you should get rayJobInstance again, since the resource version of rayJobInstance has changed.

harryge00 · 2022-07-18T18:39:05Z

@Jeffwan @brucez-anyscale Could you take another review? I have address the remaining issues

Jeffwan · 2022-07-18T19:02:58Z

@harryge00 Awesome! There're few issues need to be improved later, I can help on it

jobDeploymentStatus seems stay at Running stage for successful runs
spec.jobId is not being used to custom job ids
spec.shutdownAfterJobFinishes is not implemented yet
provide easier way to fetch job failure message from CR
I do see frequent state update which can be optimized by comparing the before and after status.
log consistency problem.

brucez-anyscale · 2022-07-18T22:52:47Z

ray-operator/controllers/ray/utils/dashboard_httpclient.go

+		dashboardAgentService.Name,
+		dashboardAgentService.Namespace,
+		dashboardPort)
+	log.V(1).Info("fetchDashboardAgentURL ", "dashboardURL", dashboardAgentURL)


log.V(1).Info("fetchDashboardAgentURL ", "dashboardAgentURL", dashboardAgentURL)

@brucez-anyscale Seem this is the key change dashboardURL -> dashboardAgentURL? I can help update it in follow up PRs and let's merge this one. (We need this change in downstream to unblock other stories..)

brucez-anyscale · 2022-07-18T22:57:45Z

Great! Generally looks good. Pls address my comments and then feel free to merge.

* Generated RayJob CRD * Add the RayJob controller * Update vendor * Update generated code * Add unit tests * Refactor rayJob controller * Update to pass CI

harryge00 force-pushed the job_controller branch from f190e01 to 8473077 Compare June 14, 2022 11:47

Jeffwan requested review from brucez-anyscale, akanso, DmitriGekhtman, jiaodong and pcmoritz June 14, 2022 23:18

harryge00 force-pushed the job_controller branch from 8473077 to 10248a0 Compare June 15, 2022 00:58

DmitriGekhtman requested review from simon-mo and edoakes June 15, 2022 02:18

Jeffwan reviewed Jun 15, 2022

View reviewed changes

brucez-anyscale reviewed Jun 15, 2022

View reviewed changes

brucez-anyscale reviewed Jun 21, 2022

View reviewed changes

ray-operator/controllers/ray/rayjob_controller.go Outdated Show resolved Hide resolved

brucez-anyscale reviewed Jun 21, 2022

View reviewed changes

ray-operator/controllers/ray/rayjob_controller.go Show resolved Hide resolved

brucez-anyscale reviewed Jun 21, 2022

View reviewed changes

ray-operator/controllers/ray/rayjob_controller.go Outdated Show resolved Hide resolved

brucez-anyscale reviewed Jun 21, 2022

View reviewed changes

ray-operator/controllers/ray/utils/dashboard_httpclient_test.go Show resolved Hide resolved

harryge00 force-pushed the job_controller branch from 7e0391e to d3bbce7 Compare July 8, 2022 08:55

harryge00 changed the title ~~[WIP] Add RayJob CRD and controller logic~~ Add RayJob CRD and controller logic Jul 12, 2022

harryge00 force-pushed the job_controller branch from 434f624 to 591246c Compare July 13, 2022 07:51

DmitriGekhtman reviewed Jul 13, 2022

View reviewed changes

ray-operator/controllers/ray/rayjob_controller.go Outdated Show resolved Hide resolved

harryge00 force-pushed the job_controller branch from 591246c to b53dce1 Compare July 13, 2022 08:24

Jeffwan reviewed Jul 14, 2022

View reviewed changes

brucez-anyscale reviewed Jul 15, 2022

View reviewed changes

ray-operator/config/crd/patches/cainjection_in_ray_rayjobs.yaml Outdated Show resolved Hide resolved

brucez-anyscale reviewed Jul 15, 2022

View reviewed changes

ray-operator/config/rbac/role.yaml Outdated Show resolved Hide resolved

brucez-anyscale reviewed Jul 15, 2022

View reviewed changes

ray-operator/controllers/ray/common/service.go Outdated Show resolved Hide resolved

brucez-anyscale reviewed Jul 15, 2022

View reviewed changes

ray-operator/controllers/ray/rayjob_controller.go Outdated Show resolved Hide resolved

Jeffwan mentioned this pull request Jul 15, 2022

[1/3] Add job spec in raycluster CRD #153

Closed

4 tasks

harryge00 force-pushed the job_controller branch from 1272c3d to 9c724da Compare July 18, 2022 13:23

harryge00 and others added 5 commits July 19, 2022 01:37

Generated RayJob CRD

e37675d

Add the RayJob controller

1735400

Update vendor

fe02de1

Update generated code

1d5349e

Add unit tests

9b7970d

harryge00 force-pushed the job_controller branch from 9c724da to 9b7970d Compare July 18, 2022 17:41

harryge00 commented Jul 18, 2022

View reviewed changes

harryge00 force-pushed the job_controller branch from 0d4a827 to 85ca646 Compare July 18, 2022 18:10

Refactor rayJob controller

d715ffc

harryge00 force-pushed the job_controller branch from 85ca646 to d715ffc Compare July 18, 2022 18:17

Update to pass CI

6fd234e

harryge00 force-pushed the job_controller branch from 9b668f2 to 6fd234e Compare July 18, 2022 18:28

Jeffwan approved these changes Jul 18, 2022

View reviewed changes

brucez-anyscale reviewed Jul 18, 2022

View reviewed changes

Jeffwan merged commit e1e41c3 into ray-project:master Jul 18, 2022

Jeffwan mentioned this pull request Jul 20, 2022

[Feature] Improve stability of RayJob controller #393

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add RayJob CRD and controller logic #303

Add RayJob CRD and controller logic #303

harryge00 commented Jun 14, 2022 •

edited

Loading

Jeffwan commented Jun 14, 2022

Jeffwan left a comment

brucez-anyscale left a comment

brucez-anyscale Jun 15, 2022

Jeffwan commented Jul 2, 2022 •

edited

Loading

harryge00 commented Jul 12, 2022

Jeffwan commented Jul 12, 2022

Jeffwan commented Jul 14, 2022

brucez-anyscale commented Jul 15, 2022

Jeffwan commented Jul 18, 2022

harryge00 Jul 18, 2022 •

edited

Loading

brucez-anyscale Jul 18, 2022

harryge00 commented Jul 18, 2022

Jeffwan commented Jul 18, 2022 •

edited

Loading

brucez-anyscale Jul 18, 2022

Jeffwan Jul 18, 2022

brucez-anyscale commented Jul 18, 2022

		@@ -0,0 +1,7 @@
		# The following patch adds a directive for certmanager to inject CA into the CRD
		apiVersion: apiextensions.k8s.io/v1

Add RayJob CRD and controller logic #303

Add RayJob CRD and controller logic #303

Conversation

harryge00 commented Jun 14, 2022 • edited Loading

Why are these changes needed?

Related issue number

Checks

Jeffwan commented Jun 14, 2022

Jeffwan left a comment

Choose a reason for hiding this comment

brucez-anyscale left a comment

Choose a reason for hiding this comment

brucez-anyscale Jun 15, 2022

Choose a reason for hiding this comment

Jeffwan commented Jul 2, 2022 • edited Loading

harryge00 commented Jul 12, 2022

Jeffwan commented Jul 12, 2022

Jeffwan commented Jul 14, 2022

brucez-anyscale commented Jul 15, 2022

Jeffwan commented Jul 18, 2022

harryge00 Jul 18, 2022 • edited Loading

Choose a reason for hiding this comment

brucez-anyscale Jul 18, 2022

Choose a reason for hiding this comment

harryge00 commented Jul 18, 2022

Jeffwan commented Jul 18, 2022 • edited Loading

brucez-anyscale Jul 18, 2022

Choose a reason for hiding this comment

Jeffwan Jul 18, 2022

Choose a reason for hiding this comment

brucez-anyscale commented Jul 18, 2022

harryge00 commented Jun 14, 2022 •

edited

Loading

Jeffwan commented Jul 2, 2022 •

edited

Loading

harryge00 Jul 18, 2022 •

edited

Loading

Jeffwan commented Jul 18, 2022 •

edited

Loading