[rayservice] Add support for getting multi-app status #1136

zcin · 2023-06-02T06:26:30Z

Why are these changes needed?

Add a field serveConfigV2 in the RayService CRD which will be filled in in a follow-up PR.
serveConfigV2 corresponds to the Serve multi-application schema. If the user specifies the serve config using serveConfigV2, we should read the status from the multi-app endpoint (GET /api/serve/applications/).

This PR only adds the support for pulling the status from the multi-app endpoint. A follow up PR will add support for submitting the serve config to the multi-app endpoint.

Example Service Status:

Status:
  Active Service Status:
    Application Statuses:
      Default:
        Health Last Update Time:  2023-05-26T20:15:01Z
        Last Update Time:         2023-05-26T20:15:01Z
        Serve Deployment Statuses:
          DAG Driver:
            Health Last Update Time:  2023-05-26T20:15:01Z
            Last Update Time:         2023-05-26T20:15:01Z
            Status:                   HEALTHY
          Fruit Market:
            Health Last Update Time:  2023-05-26T20:15:01Z
            Last Update Time:         2023-05-26T20:15:01Z
            Status:                   HEALTHY
          Mango Stand:
            Health Last Update Time:  2023-05-26T20:15:01Z
            Last Update Time:         2023-05-26T20:15:01Z
            Status:                   HEALTHY
          Orange Stand:
            Health Last Update Time:  2023-05-26T20:15:01Z
            Last Update Time:         2023-05-26T20:15:01Z
            Status:                   HEALTHY
          Pear Stand:
            Health Last Update Time:  2023-05-26T20:15:01Z
            Last Update Time:         2023-05-26T20:15:01Z
            Status:                   HEALTHY
        Status:                       RUNNING
    Dashboard Status:
      Health Last Update Time:  2023-05-26T20:15:01Z
      Is Healthy:               true
      Last Update Time:         2023-05-26T20:15:01Z
    Ray Cluster Name:           rayservice-sample-raycluster-2tqs5
    Ray Cluster Status:
      Head:
  Observed Generation:  1
  Pending Service Status:
    Dashboard Status:
    Ray Cluster Status:
      Head:
  Service Status:  Running

Other changes:

Removed lastupdatetime and healthlastupdatetime from the return types of the dashboard client functions, because those values are completely untouched by the dashboard client, and instead only filled in inside rayservice controller methods. I've separated the return types of dashboard client functions from the rayservice status types to make this possible. As a result, I have also removed the time input from the testing function generateServeStatus, since it is (1) unrealistic because in reality the dashboard client would never fill in those values and (2) ineffective because the values are overwritten in the reconcile loop of the rayservice controller anyways.

Related issue number

Checks

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

zcin · 2023-06-06T01:56:16Z

@kevin85421 The tests should be passing, please take a look when you get the chance!

kevin85421

Others: Could you ensure that all variables in the tests are valid and avoid hardcoding the values? For example, both Status: "unhealthy" and deploymentStatus.Status != "HEALTHY" are present in the codebase.

ray-operator/config/manager/kustomization.yaml

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

kevin85421 · 2023-06-06T04:32:29Z

ray-operator/controllers/ray/rayservice_controller.go

@@ -648,68 +672,99 @@ func (r *RayServiceReconciler) updateServeDeployment(ctx context.Context, raySer
 // updates health timestamps, and checks if the RayCluster is overall healthy.
 // It's return values should be interpreted as
 // (Serve app healthy?, Serve app ready?, error if any)
-func (r *RayServiceReconciler) getAndCheckServeStatus(ctx context.Context, dashboardClient utils.RayDashboardClientInterface, rayServiceServeStatus *rayv1alpha1.RayServiceStatus, unhealthySecondThreshold *int32) (bool, bool, error) {
+func (r *RayServiceReconciler) getAndCheckServeStatus(ctx context.Context, dashboardClient utils.RayDashboardClientInterface, rayServiceServeStatus *rayv1alpha1.RayServiceStatus, serveConfigType utils.RayServeConfigType, unhealthySecondThreshold *int32) (bool, bool, error) {
 	serviceUnhealthySecondThreshold := ServiceUnhealthySecondThreshold


Although the variable names are self-explanatory, it would be helpful to have comments for them.

Where is the comments?

I added comments for serveConfigType above the function

Oh, I am talking about ServiceUnhealthySecondThreshold and serveConfigTypeForTesting.

Oh I see! Added comments now for them too :)

kevin85421 · 2023-06-06T04:40:10Z

ray-operator/controllers/ray/rayservice_controller.go

+		}
+
+		// Check app status
+		if app.Status != rayv1alpha1.ApplicationStatusEnum.RUNNING {


What is the definition of RUNNING for a Serve application?

RUNNING = all deployments are HEALTHY

Is it correct that isHealthy must be true if app.Status is not RUNNING?

ray-operator/controllers/ray/rayservice_controller.go

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

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

Signed-off-by: Cindy Zhang <[email protected]>

Signed-off-by: cindyz <[email protected]>

zcin · 2023-06-06T17:51:51Z

Others: Could you ensure that all variables in the tests are valid and avoid hardcoding the values? For example, both Status: "unhealthy" and deploymentStatus.Status != "HEALTHY" are present in the codebase.

@kevin85421 I've changed all of the hardcoded strings to the enums.

Signed-off-by: cindyz <[email protected]>

kevin85421

Leave some minor comments. By the way, I have not reviewed changes in apiserver and proto. We can either ping some KubeRay API Server folks to review them or revert the changes and do it in a follow-up PR, or ask KubeRay API Server folks for help.

kevin85421 · 2023-06-06T20:38:48Z

ray-operator/controllers/ray/rayservice_controller.go

+// If the user has set a value for `ServeConfigV2`, the config type is MULTI_APP
+// Otherwise, the user should have set a value for `ServeConfig`, in which case the config type is SINGLE_APP
+func (r *RayServiceReconciler) determineServeConfigType(ctx context.Context, rayServiceInstance *rayv1alpha1.RayService) utils.RayServeConfigType {
+	if rayServiceInstance.Spec.ServeConfigV2 == (rayv1alpha1.ServeConfigV2{}) {


Is it safe to use == to compare two structs or should we use DeepEqual instead?

kevin85421 · 2023-06-06T20:40:23Z

ray-operator/controllers/ray/rayservice_controller.go

@@ -648,68 +672,99 @@ func (r *RayServiceReconciler) updateServeDeployment(ctx context.Context, raySer
 // updates health timestamps, and checks if the RayCluster is overall healthy.
 // It's return values should be interpreted as
 // (Serve app healthy?, Serve app ready?, error if any)
-func (r *RayServiceReconciler) getAndCheckServeStatus(ctx context.Context, dashboardClient utils.RayDashboardClientInterface, rayServiceServeStatus *rayv1alpha1.RayServiceStatus, unhealthySecondThreshold *int32) (bool, bool, error) {
+func (r *RayServiceReconciler) getAndCheckServeStatus(ctx context.Context, dashboardClient utils.RayDashboardClientInterface, rayServiceServeStatus *rayv1alpha1.RayServiceStatus, serveConfigType utils.RayServeConfigType, unhealthySecondThreshold *int32) (bool, bool, error) {
 	serviceUnhealthySecondThreshold := ServiceUnhealthySecondThreshold


Where is the comments?

kevin85421 · 2023-06-06T20:49:18Z

ray-operator/controllers/ray/rayservice_controller.go

+		}
+
+		// Check app status
+		if app.Status != rayv1alpha1.ApplicationStatusEnum.RUNNING {


Is it correct that isHealthy must be true if app.Status is not RUNNING?

ray-operator/controllers/ray/rayservice_controller.go

kevin85421 · 2023-06-06T20:54:12Z

ray-operator/controllers/ray/rayservice_controller.go

+				HealthLastUpdateTime: &timeNow,
+			}
+
+			if deployment.Status != rayv1alpha1.DeploymentStatusEnum.HEALTHY {


If a replica is not healthy, it is unable to serve any requests. Is it correct?

ray-operator/controllers/ray/rayservice_controller_test.go

ray-operator/controllers/ray/rayservice_controller_unit_test.go

zcin · 2023-06-06T21:23:53Z

Leave some minor comments. By the way, I have not reviewed changes in apiserver and proto. We can either ping some KubeRay API Server folks to review them or revert the changes and do it in a follow-up PR, or ask KubeRay API Server folks for help.

Hmm, I don't think it's possible to revert the changes (correct me if I'm wrong), because the code in apiserver references the rayservice types. Since I changed the rayservice types, I believe the code would error.
Could you ping someone who can review these changes? Should be very minor.

Signed-off-by: cindyz <[email protected]>

kevin85421

LGTM (for changes not under apiserver/ and proto/)

cc @Jeffwan @scarlet25151 @blublinsky would you mind taking a look at the changes for apiserver/ and proto/? Thanks!

kevin85421 · 2023-06-08T21:27:39Z

Tomorrow, I will review the changes made under the proto/ and apiserver/ directories. @zcin, could you please provide more details on how to test the modifications in these two directories? In addition, could you rebase with the master branch to resolve the conflicts? Thanks!

zcin · 2023-06-09T01:28:04Z

Tomorrow, I will review the changes made under the proto/ and apiserver/ directories. @zcin, could you please provide more details on how to test the modifications in these two directories? In addition, could you rebase with the master branch to resolve the conflicts? Thanks!

@kevin85421 Resolved the conflicts! As for the changes under apiserver and proto, I only made the bare minimum changes to make the code compile because the structs changed in rayservice_types.go. I am not sure how to test it.

blublinsky · 2023-06-09T07:38:05Z

Changes to proto/ and apiserver/ LGTM

scarlet25151 · 2023-06-09T17:30:04Z

proto/ and apiserver/ parts LGTM.

kevin85421 · 2023-06-09T17:44:59Z

Thank @blublinsky and @scarlet25151 for the review!

kevin85421 · 2023-06-09T17:46:20Z

CC: @architkulkarni, this pull request should not be included in the minor release v0.5.2. Is it acceptable to merge this pull request at this time?

architkulkarni · 2023-06-09T17:48:09Z

@kevin85421 Sounds good, fine with me.

Add support for getting multi-app status

zcin force-pushed the multi-app-status branch 5 times, most recently from 1c75391 to 755c41c Compare June 2, 2023 07:04

zcin marked this pull request as ready for review June 2, 2023 22:32

zcin requested a review from kevin85421 June 2, 2023 22:32

zcin force-pushed the multi-app-status branch 2 times, most recently from d2372cd to 18195e6 Compare June 6, 2023 01:53

kevin85421 reviewed Jun 6, 2023

View reviewed changes

wip

1b75e0f

Signed-off-by: Cindy Zhang <[email protected]>

zcin force-pushed the multi-app-status branch from 18195e6 to 1b75e0f Compare June 6, 2023 06:05

zcin requested a review from sihanwang41 June 6, 2023 17:26

use enum instead of hardcoded status strings

c1bfc7a

Signed-off-by: cindyz <[email protected]>

cindyz added 2 commits June 6, 2023 20:25

improve unit test

3cf1805

Signed-off-by: cindyz <[email protected]>

lint

256f7cc

Signed-off-by: cindyz <[email protected]>

kevin85421 reviewed Jun 6, 2023

View reviewed changes

cindyz added 2 commits June 6, 2023 21:28

remove hardcoded strings

75e63d5

Signed-off-by: cindyz <[email protected]>

add comments

494944b

Signed-off-by: cindyz <[email protected]>

Jeffwan added the rayservice label Jun 6, 2023

Jeffwan assigned kevin85421 Jun 6, 2023

kevin85421 approved these changes Jun 7, 2023

View reviewed changes

Merge branch 'master' into multi-app-status

fd5b9d6

kevin85421 merged commit 0ae9fc1 into ray-project:master Jun 9, 2023

zcin mentioned this pull request Jun 17, 2023

[Service] Multi app support in Kuberay #985

Closed

2 tasks

zcin deleted the multi-app-status branch August 25, 2023 17:35

lowang-bh pushed a commit to lowang-bh/kuberay that referenced this pull request Sep 24, 2023

[rayservice] Add support for getting multi-app status (ray-project#1136)

bf24da1

Add support for getting multi-app status

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[rayservice] Add support for getting multi-app status #1136

[rayservice] Add support for getting multi-app status #1136

zcin commented Jun 2, 2023 •

edited

Loading

zcin commented Jun 6, 2023

kevin85421 left a comment

kevin85421 Jun 6, 2023

zcin Jun 6, 2023

kevin85421 Jun 6, 2023

zcin Jun 6, 2023

kevin85421 Jun 6, 2023

zcin Jun 6, 2023

kevin85421 Jun 6, 2023

zcin Jun 6, 2023

kevin85421 Jun 6, 2023

zcin commented Jun 6, 2023

kevin85421 left a comment

kevin85421 Jun 6, 2023

kevin85421 Jun 6, 2023

kevin85421 Jun 6, 2023

kevin85421 Jun 6, 2023

zcin commented Jun 6, 2023 •

edited

Loading

kevin85421 left a comment

kevin85421 commented Jun 8, 2023

zcin commented Jun 9, 2023

blublinsky commented Jun 9, 2023

scarlet25151 commented Jun 9, 2023 •

edited

Loading

kevin85421 commented Jun 9, 2023

kevin85421 commented Jun 9, 2023

architkulkarni commented Jun 9, 2023

[rayservice] Add support for getting multi-app status #1136

[rayservice] Add support for getting multi-app status #1136

Conversation

zcin commented Jun 2, 2023 • edited Loading

Why are these changes needed?

Related issue number

Checks

zcin commented Jun 6, 2023

kevin85421 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zcin commented Jun 6, 2023

kevin85421 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zcin commented Jun 6, 2023 • edited Loading

kevin85421 left a comment

Choose a reason for hiding this comment

kevin85421 commented Jun 8, 2023

zcin commented Jun 9, 2023

blublinsky commented Jun 9, 2023

scarlet25151 commented Jun 9, 2023 • edited Loading

kevin85421 commented Jun 9, 2023

kevin85421 commented Jun 9, 2023

architkulkarni commented Jun 9, 2023

zcin commented Jun 2, 2023 •

edited

Loading

zcin commented Jun 6, 2023 •

edited

Loading

scarlet25151 commented Jun 9, 2023 •

edited

Loading