support using proxy subresources when connecting to Ray head node #1980

andrewsykim · 2024-03-10T04:10:46Z

Why are these changes needed?

There are some cases where Kuberay may not be able to directly connect to a Ray head node. For example, there might be a NetworkPolicy disallowing ingress from all Pods or KubeRay is running on a network with no connectivity to Pods. This PR allows Kuberay to use the services/proxy subresource to proxy HTTP requests to the Ray head node. This allows Kuberay to make requests to the head node without every connecting to it directly.

Here are some sample HTTP requests in apiserver from my testing using the proxy subresource:

I0310 14:30:19.596708       1 httplog.go:131] "HTTP" verb="GET" URI="/api/v1/namespaces/default/services/rayjob-sample-raycluster-phsj9-head-svc:dashboard/proxy/api/jobs/rayjob-sample-th44t" latency="7.239221ms" userAgent="manager/v0.0.0 (linux/amd64) kubernetes/$Format" audit-ID="c40152de-7a81-46b9-ac4a-f2ea296e44f0" srcIP="10.244.0.6:49524" resp=200

I0310 15:20:43.105571       1 httplog.go:131] "HTTP" verb="GET" URI="/api/v1/namespaces/default/pods/rayservice-sample-raycluster-qm2m2-head-xj44d:8000/proxy/-/healthz" latency="2.915789ms" userAgent="manager/v0.0.0 (linux/amd64) kubernetes/$Format" audit-ID="bad00c23-de01-45ed-9fb5-05bc8b2f6c2d" srcIP="10.244.0.6:47274" resp=200


I0310 15:21:07.446881       1 httplog.go:131] "HTTP" verb="GET" URI="/api/v1/namespaces/default/services/rayservice-sample-raycluster-qm2m2-head-svc:dashboard/proxy/api/serve/applications/" latency="15.347398ms" userAgent="manager/v0.0.0 (linux/amd64) kubernetes/$Format" audit-ID="54544346-b827-4f34-b593-bcbc78199611" srcIP="10.244.0.6:47274" resp=200

Checks

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

ray-operator/controllers/ray/rayservice_controller.go

kevin85421 · 2024-04-03T18:17:06Z

ray-operator/main.go

@@ -218,9 +222,9 @@ func main() {
 	ctx := ctrl.SetupSignalHandler()
 	exitOnError(ray.NewReconciler(ctx, mgr, rayClusterOptions).SetupWithManager(mgr, config.ReconcileConcurrency),
 		"unable to create controller", "controller", "RayCluster")
-	exitOnError(ray.NewRayServiceReconciler(ctx, mgr, utils.GetRayDashboardClient, utils.GetRayHttpProxyClient).SetupWithManager(mgr),


Will it be better if we pass configapi.Configuration to the NewRayServiceReconciler and NewRayJobReconciler functions?

I don't think it's necessary at the moment since we're only using a single field there. But we should revisit this in the future and when config API graduates to beta. What do you think?

I recently worked on an open-source event for newbies. This would be a good first issue for the event. I will open an issue later.

kevin85421 · 2024-04-03T18:21:37Z

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

 	r.dashboardURL = "http://" + url
 }

+func (r *RayDashboardClient) WithKubernetesServiceProxy(svcNamespace, svcName string) {


The function initializes the dashboard client. Perhaps we could update InitClient instead of creating a new function?

Good point -- I'll try to update InitClient to support both proxy / non-proxy clients and see if it make sense to combine them.

kevin85421 · 2024-04-03T20:40:57Z

ray-operator/controllers/ray/rayjob_controller.go

@@ -640,6 +664,21 @@ func (r *RayJobReconciler) getOrCreateRayClusterInstance(ctx context.Context, ra
 	return rayClusterInstance, nil
 }

+func (r *RayJobReconciler) getRayClusterInstance(ctx context.Context, rayJobInstance *rayv1.RayJob) (*rayv1.RayCluster, error) {


I am not sure if we need this function. Perhaps we could directly use RayJobRayClusterNamespacedName and r.Get(...)?

Great catch, will update to just call RayJobRayClusterNamespacedName

kevin85421 · 2024-04-03T20:42:07Z

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

 		Timeout: 20 * time.Millisecond,
 	}
 }

+func (r *RayHttpProxyClient) WithKubernetesPodProxy(podNamespace, podName string, port int) {


The function initializes the client. Perhaps we could update InitClient instead of creating a new function?

andrewsykim

@kevin85421 addressed your comments, please take another look

andrewsykim · 2024-04-11T20:43:58Z

ray-operator/main.go

@@ -218,9 +222,9 @@ func main() {
 	ctx := ctrl.SetupSignalHandler()
 	exitOnError(ray.NewReconciler(ctx, mgr, rayClusterOptions).SetupWithManager(mgr, config.ReconcileConcurrency),
 		"unable to create controller", "controller", "RayCluster")
-	exitOnError(ray.NewRayServiceReconciler(ctx, mgr, utils.GetRayDashboardClient, utils.GetRayHttpProxyClient).SetupWithManager(mgr),


I don't think it's necessary at the moment since we're only using a single field there. But we should revisit this in the future and when config API graduates to beta. What do you think?

kevin85421

Note for myself
- GetHTTPClient(): controller-runtime
- GetConfig().Host: client-go

kevin85421 · 2024-04-16T16:16:06Z

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

 	httpProxyURL string
+
+	mgr      ctrl.Manager
+	useProxy bool


Nit: It's better to use UseKubernetesProxy for consistency.

No need to update this in this PR. I recently worked on an open-source event for newbies. This would be a good first issue for the event. I will open an issue later.

kevin85421 · 2024-04-16T16:18:28Z

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

 }

 func (r *RayHttpProxyClient) InitClient() {
-	r.client = http.Client{
+	r.client = &http.Client{


Is there any reason for this change?

It's mainly because GetHTTPClient() returns a pointer

kevin85421 · 2024-04-16T16:22:57Z

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

 	dashboardURL string
 }

-func GetRayDashboardClient() RayDashboardClientInterface {
-	return &RayDashboardClient{}
+func GetRayDashboardClientFunc(mgr ctrl.Manager, useProxy bool) func() RayDashboardClientInterface {


Is there any reason to change from GetRayDashboardClient() to GetRayDashboardClientFunc()?

Yes, because the function signature is changed to now return a func that returns a RayDashboardClientInterface

kevin85421 · 2024-04-16T16:24:43Z

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

-func (r *RayDashboardClient) InitClient(url string) {
-	r.client = http.Client{
+func (r *RayDashboardClient) InitClient(url, svcNamespace, svcName string) {
+	if r.useProxy {


Nit: It's better to use UseKubernetesProxy for consistency.

No need to update this in this PR. I recently worked on an open-source event for newbies. This would be a good first issue for the event. I will open an issue later.

ray-operator/controllers/ray/rayjob_controller.go

kevin85421 · 2024-04-16T16:32:22Z

ray-operator/controllers/ray/rayservice_controller.go

@@ -1028,8 +1030,13 @@ func (r *RayServiceReconciler) updateStatusForActiveCluster(ctx context.Context,
 		return err
 	}

+	headSvcName, err := utils.GenerateHeadServiceName(utils.RayServiceCRD, rayClusterInstance.Spec, rayClusterInstance.Name)


We should have a more elegant method to initialize the dashboard client. Calling GenerateHeadServiceName every time we want to initialize the client makes the codebase complex and hard to maintain.

Agreed, I thought this was kind of ugly too. Maybe we can pass the entire RayCluster to InitClient?

Maybe we can pass the entire RayCluster to InitClient?

I can make this change in this PR if you want

I can make this change in this PR if you want

Thanks! It is helpful! Maybe we can consider adding the head service's name to RayCluster's status in the future.

I updated InitClient to receive a RayCluster so we can call GenerateHeadServiceName from inside InitClient. This requires InitClient to now return an error

…ad node Signed-off-by: Andrew Sy Kim <andrewsy@google.com>

Signed-off-by: Andrew Sy Kim <andrewsy@google.com>

kevin85421

LGTM. Thanks! I will test it manually before I merge this PR.

kevin85421 · 2024-04-19T02:06:49Z

I tested this PR manually.

Step 0: Create a Kind cluster with the following configurations:

kind: Cluster
apiVersion: kind.x-k8s.io/v1alpha4
nodes:
- role: control-plane
  kubeadmConfigPatches:
  - |
    kind: ClusterConfiguration
    apiServer:
        extraArgs:
          v: "4"

Step 1: Update deployment.yaml (helm-chart/kuberay-operator/templates/deployment.yaml) to enable this feature.
```
{{- $argList = append $argList "--use-kubernetes-proxy" -}}
```
Step 2: Install the KubeRay operator.
Step 3: Create a RayJob CR.

Step 4: Check the API server log

I0419 01:57:09.080550       1 httplog.go:132] "HTTP" verb="GET" URI="/api/v1/namespaces/default/services/rayjob-sample-raycluster-rxhpc-head-svc:dashboard/proxy/api/jobs/rayjob-sample-zcjbj" latency="4.394603ms" userAgent="kuberay-operator/nightly" audit-ID="db0c2d98-a5d0-479f-81f8-7e9d7e3dc38b" srcIP="10.244.0.5:47796" resp=200

andrewsykim · 2024-04-19T02:16:38Z

thanks @kevin85421! I'll look into adding an e2e test as well

…#1980 Signed-off-by: TingYi <a75896453@gmail.com>

Signed-off-by: TingYi <a75896453@gmail.com>

…#1980 Signed-off-by: TingYi <a75896453@gmail.com>

Signed-off-by: TingYi <a75896453@gmail.com>

andrewsykim force-pushed the service-proxy branch 3 times, most recently from b599778 to 530e237 Compare March 10, 2024 15:30

andrewsykim changed the title ~~support using services/proxy when connecting to Ray head node~~ support using proxy subresources when connecting to Ray head node Mar 10, 2024

kevin85421 self-requested a review March 11, 2024 00:31

kevin85421 self-assigned this Mar 11, 2024

andrewsykim force-pushed the service-proxy branch 2 times, most recently from 0117668 to 1a03e48 Compare March 11, 2024 02:05

kevin85421 reviewed Apr 3, 2024

View reviewed changes

andrewsykim force-pushed the service-proxy branch 3 times, most recently from 34b43a6 to df8a3aa Compare April 11, 2024 15:33

andrewsykim commented Apr 11, 2024

View reviewed changes

kevin85421 reviewed Apr 16, 2024

View reviewed changes

andrewsykim force-pushed the service-proxy branch from df8a3aa to a44271e Compare April 17, 2024 20:35

support using services/proxy and pods/proxy when connecting to Ray he…

2b7cf4d

…ad node Signed-off-by: Andrew Sy Kim <andrewsy@google.com>

andrewsykim force-pushed the service-proxy branch from a44271e to 6dc84d3 Compare April 17, 2024 20:38

address review comments from Kai-Hsun

ef5db3d

Signed-off-by: Andrew Sy Kim <andrewsy@google.com>

andrewsykim force-pushed the service-proxy branch from 6dc84d3 to ef5db3d Compare April 17, 2024 20:48

kevin85421 approved these changes Apr 17, 2024

View reviewed changes

kevin85421 merged commit ff008a1 into ray-project:master Apr 19, 2024
24 checks passed

Xiao75896453 pushed a commit to Xiao75896453/kuberay that referenced this pull request Apr 21, 2024

feat (httpproxy_httpclient): rename UseProxy ray-project#1980

775f943

Xiao75896453 pushed a commit to Xiao75896453/kuberay that referenced this pull request Apr 21, 2024

refactor (httpproxy_httpclient): rename UseProxy ray-project#1980

936ed17

bugorz mentioned this pull request Apr 21, 2024

Init dashboardClientFunc and httpProxyClientFunc by the config arg #2092

Merged

3 tasks

Xiao75896453 pushed a commit to Xiao75896453/kuberay that referenced this pull request Apr 21, 2024

refactor (httpproxy_httpclient): rename UserProxy ray-project#1980

e699ff1

Xiao75896453 mentioned this pull request Apr 21, 2024

[Refactor] Renaming RayHttpProxyClient attribute UseProxy #1980 #2093

Merged

4 tasks

Xiao75896453 pushed a commit to Xiao75896453/kuberay that referenced this pull request Apr 21, 2024

refactor (DEVELOPMENT): reset DEVELOPMENT ray-project#1980

c80606b

Xiao75896453 added a commit to Xiao75896453/kuberay that referenced this pull request Apr 23, 2024

refactor (GetRayHttpProxyClientFunc): rename arg UseProxy ray-project…

2e8fd74

…#1980 Signed-off-by: TingYi <a75896453@gmail.com>

Xiao75896453 added a commit to Xiao75896453/kuberay that referenced this pull request Apr 23, 2024

feat (httpproxy_httpclient): modify field to private ray-project#1980

f0d6f69

Xiao75896453 added a commit to Xiao75896453/kuberay that referenced this pull request Apr 23, 2024

feat (httpproxy_httpclient): modify field to private ray-project#1980

daa741e

Signed-off-by: TingYi <a75896453@gmail.com>

Xiao75896453 added a commit to Xiao75896453/kuberay that referenced this pull request Apr 24, 2024

refactor (dashboard_httpclient>): rename field ray-project#1980

5db4ce8

Signed-off-by: TingYi <a75896453@gmail.com>

Xiao75896453 pushed a commit to Xiao75896453/kuberay that referenced this pull request Apr 25, 2024

refactor (httpproxy_httpclient): rename UserProxy ray-project#1980

6f827dc

Xiao75896453 pushed a commit to Xiao75896453/kuberay that referenced this pull request Apr 25, 2024

refactor (DEVELOPMENT): reset DEVELOPMENT ray-project#1980

72745b4

Xiao75896453 added a commit to Xiao75896453/kuberay that referenced this pull request Apr 25, 2024

refactor (GetRayHttpProxyClientFunc): rename arg UseProxy ray-project…

63706b8

…#1980 Signed-off-by: TingYi <a75896453@gmail.com>

Xiao75896453 added a commit to Xiao75896453/kuberay that referenced this pull request Apr 25, 2024

feat (httpproxy_httpclient): modify field to private ray-project#1980

d992c8c

Xiao75896453 added a commit to Xiao75896453/kuberay that referenced this pull request Apr 25, 2024

feat (httpproxy_httpclient): modify field to private ray-project#1980

1f0a63f

Signed-off-by: TingYi <a75896453@gmail.com>

Xiao75896453 added a commit to Xiao75896453/kuberay that referenced this pull request Apr 25, 2024

refactor (dashboard_httpclient>): rename field ray-project#1980

1f153fc

Signed-off-by: TingYi <a75896453@gmail.com>

kevin85421 pushed a commit that referenced this pull request Apr 25, 2024

[Refactor] Renaming RayHttpProxyClient attribute UseProxy #1980 (#2093)

4836d01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

support using proxy subresources when connecting to Ray head node #1980

support using proxy subresources when connecting to Ray head node #1980

andrewsykim commented Mar 10, 2024 •

edited

Loading

kevin85421 Apr 3, 2024

andrewsykim Apr 11, 2024

kevin85421 Apr 16, 2024

kevin85421 Apr 3, 2024

andrewsykim Apr 7, 2024

kevin85421 Apr 3, 2024

andrewsykim Apr 7, 2024

kevin85421 Apr 3, 2024

andrewsykim left a comment

andrewsykim Apr 11, 2024

kevin85421 left a comment

kevin85421 Apr 16, 2024

kevin85421 Apr 16, 2024

andrewsykim Apr 16, 2024

kevin85421 Apr 16, 2024

andrewsykim Apr 16, 2024

kevin85421 Apr 16, 2024

kevin85421 Apr 16, 2024

andrewsykim Apr 16, 2024

andrewsykim Apr 16, 2024

kevin85421 Apr 16, 2024

andrewsykim Apr 17, 2024

kevin85421 left a comment

kevin85421 commented Apr 19, 2024

andrewsykim commented Apr 19, 2024

support using proxy subresources when connecting to Ray head node #1980

support using proxy subresources when connecting to Ray head node #1980

Conversation

andrewsykim commented Mar 10, 2024 • edited Loading

Why are these changes needed?

Checks

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

andrewsykim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevin85421 left a comment

Choose a reason for hiding this comment

kevin85421 commented Apr 19, 2024

andrewsykim commented Apr 19, 2024

andrewsykim commented Mar 10, 2024 •

edited

Loading