[Feature][RayService] Set default ports #3262

machichima · 2025-04-01T13:26:41Z

Changes

Remove ports section in ray-service.*.yaml
Remove unnecessary loop in BuildServeService when assigning service port
Add test for setting default port in ray service, covering
- No ports set by user, set all default ports for them
- Some ports set but metrics does not, the default metrics port is set

Why are these changes needed?

Currently, many sample YAML files define the ports section, while in code the default ports will be set if no ports defined.

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

Lines 379 to 382 in b6db7a5

    
           // Assign default ports 
        
           if len(ports) == 0 { 
        
           	ports = getDefaultPorts() 
        
           }

Those ports section in ray-service.*.yaml file can be removed.

Related issue number

Closes #3246

Checks

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

-e Signed-off-by: machichima <[email protected]>

port can be set different name in yaml -e Signed-off-by: machichima <[email protected]>

MortalHappiness · 2025-04-04T11:31:40Z

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

+	for name, defaultPort := range defaultPorts {
+		// ensure client port value overwrite
+		if name == utils.ClientPortName {
+			assert.Equalf(t, 12345, int(svcPorts[name]), "Expected `%v` as `%v` port value but got `%v`", 12345, name, svcPorts[name])


Suggested change

assert.Equalf(t, 12345, int(svcPorts[name]), "Expected `%v` as `%v` port value but got `%v`", 12345, name, svcPorts[name])

assert.Equal(t, 12345, int(svcPorts[name]))

The message is not very useful. Simple assertion is enough.

MortalHappiness · 2025-04-04T11:34:55Z

ray-operator/config/samples/ray-service.sample.yaml

-              ports:
-                - containerPort: 6379
-                  name: gcs-server
-                - containerPort: 8265 # Ray dashboard
-                  name: dashboard
-                - containerPort: 10001
-                  name: client
-                - containerPort: 8000
-                  name: serve


Also remove this section in other ray-service.*.yaml files.

MortalHappiness · 2025-04-04T14:15:40Z

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

+			// Only assign defaultPort if the port is not already in use
+			if !usedPorts[defaultPort] {
+				ports[name] = defaultPort
+				usedPorts[defaultPort] = true // Mark this port as used
+			}


Should return error instead of do nothing if ports conflict with defaultPort.

What I was thinking here is that if user set the port with different name, the user's setting will be used.
For instance, when running tests, I encountered the issue that the default is:

- containerPort: 6379 name: gcs-server

While test set:

- containerPort: 6379 name: gcs

In this case, user's setting with name gcs will be applied.

~~Do we need error for this? Or probably a warning is enough?~~ Update on below comments

Update:
This problem is actually caused by different naming for gcs-server (some named it as gcs)

For example:

kuberay/ray-operator/controllers/ray/common/service_test.go

Lines 71 to 72 in e672376

ContainerPort: 6379,

Name: "gcs",

Default gcs-server name should be:

kuberay/ray-operator/controllers/ray/utils/constant.go

Line 74 in e672376

GcsServerPortName = "gcs-server"

I would like to confirm if it is ok to change all port name "gcs" into "gcs-server"?

Yes. We should use the same value across the codebase.

MortalHappiness · 2025-04-04T14:23:58Z

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

+	svcPorts := make(map[string]int32)
+	usedPorts := make(map[int32]bool)
 	for _, port := range ports {
 		if *port.AppProtocol != utils.DefaultServiceAppProtocol {
 			t.Fatalf("Expected `%v` but got `%v`", expectedResult, *port.AppProtocol)
 		}
+		svcPorts[port.Name] = port.Port
+		usedPorts[port.Port] = true
+	}
+
+	for name, defaultPort := range defaultPorts {
+		// ensure client port value overwrite
+		if name == utils.ClientPortName {
+			assert.Equalf(t, 12345, int(svcPorts[name]), "Expected `%v` as `%v` port value but got `%v`", 12345, name, svcPorts[name])
+		} else if !usedPorts[defaultPort] {
+			// Ensure the default port value is applied
+			t.Fatalf("Port `%v` not set", defaultPort)
+		}
 	}
+


In tests, we should simply assert the final result, and the expected outcome should be easy to construct. It's hard to read a test that loops over constructed values and makes assertions.

We may want to split the test into several test cases if the expected value is hard to construct. For example, one with duplicated ports and another without.

Thanks for the advice! I moved into a new test function so that it is easier to read

kevin85421 · 2025-04-05T01:24:16Z

cc @rueian for review

rueian · 2025-04-05T02:11:13Z

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

-		ports[utils.MetricsPortName] = utils.DefaultMetricsPort
+	// 1. If a value is provided in the YAML file, overwrite the default value
+	// 2. If no port value is provided, use the default name and port
+	// 3. If a port value is provided in the YAML file, retain the original name


Will k8s throw an error when there are duplicated port values? If not, can we just preserve them all?

Yes, I encountered the following error when there's duplicate port value 6379 (for gcs-server).

Warning FailedToCreateService 47s (x16 over 3m31s) raycluster-controller Failed creating service test-ns-hgtnx/raycluster-gcsft-head-svc, Service "raycluster-gcsft-head-svc" is invalid: spec.ports[3]: Duplicate value: core.ServicePort{Name:"", Protocol:"TCP", AppProtocol:(*string)(nil), Port:6379, TargetPort:intstr.IntOrString{Type:0, IntVal:0, StrVal:""}, NodePort:0}

…service-default-ports -e Signed-off-by: machichima <[email protected]>

-e Signed-off-by: machichima <[email protected]>

rueian · 2025-04-05T04:28:53Z

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

+	svcPort := corev1.ServicePort{Name: utils.ServingPortName, Port: portsInt[utils.ServingPortName]}
+	// Only include serve port
+	ports := []corev1.ServicePort{svcPort}


Should we still include the svcPort if utils.ServingPortName is not in the portsInt?

I think it will always be there as we will assign default value. And now if user trying to use name other than utils.ServingPortName for serving port, the error will be raised.

-e Signed-off-by: machichima <[email protected]>

This also raise error if the default port is set with name different to default name -e Signed-off-by: machichima <[email protected]>

-e Signed-off-by: machichima <[email protected]>

machichima · 2025-04-05T09:59:41Z

Hi @MortalHappiness ,
It seems like while running sampleyaml test, the BuildServiceForHeadPod will also be called to setup the service for ray head pod. I think we should also remove the ports section under ray-head container in files like ray-cluster.*.yaml .

Otherwise, the "gcs" port name need to be renamed as "gcs-server" (default port name) so that the error will not be raised.

MortalHappiness · 2025-04-07T11:17:52Z

Hi @MortalHappiness , It seems like while running sampleyaml test, the BuildServiceForHeadPod will also be called to setup the service for ray head pod. I think we should also remove the ports section under ray-head container in files like ray-cluster.*.yaml .

Otherwise, the "gcs" port name need to be renamed as "gcs-server" (default port name) so that the error will not be raised.

Yes. They'll be removed in #3245

-e Signed-off-by: machichima <[email protected]>

MortalHappiness · 2025-04-09T15:03:31Z

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

+				for name, port := range testCase.expectResult {
+					assert.Equal(t, port, svcPorts[name])
+				}


Suggested change

for name, port := range testCase.expectResult {

assert.Equal(t, port, svcPorts[name])

}

assert.Equal(t, testCase.expectResult, svcPorts)

Fixed. Thank you!

-e Signed-off-by: machichima <[email protected]>

kevin85421 · 2025-04-09T17:34:41Z

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

-			ports = append(ports, svcPort)
-			break
-		}
+	ports := []corev1.ServicePort{}


ports seems to have at most one element? If this is correct, maybe we can use var servePort corev1.ServicePort instead.

In the code below we need to assign ports to serveService.Spec.Ports, so I think it's better to keep it as slice here. I will set its capacity into 1 then.

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

Line 244 in 5416c6f

serveService.Spec.Ports = ports

kevin85421 · 2025-04-09T17:36:30Z

ray-operator/config/samples/ray-cluster.custom-head-service.yaml

@@ -40,10 +40,3 @@ spec:
            requests:
              cpu: 500m
              memory: 2Gi
-          ports:


Why should we update this RayCluster YAML?

Oh sorry, I modified this accidentally. Let me change it back.

kevin85421 · 2025-04-09T17:39:05Z

ray-operator/controllers/ray/raycluster_controller_unit_test.go

-			"dashboard": "8265",
-			"gcs":       "6379",
-			"metrics":   "8080",
+			"client":     "10001",


Not necessarily in this PR, but would you mind using constant variables instead of hard-coded strings?

No problem, I will add a new PR for this as there's multiple places to modify

kevin85421 · 2025-04-09T17:39:10Z

ray-operator/test/e2e/support.go

@@ -125,7 +125,7 @@ func headPodTemplateApplyConfiguration() *corev1ac.PodTemplateSpecApplyConfigura
 				WithName("ray-head").
 				WithImage(GetRayImage()).
 				WithPorts(
-					corev1ac.ContainerPort().WithName("gcs").WithContainerPort(6379),
+					corev1ac.ContainerPort().WithName("gcs-server").WithContainerPort(6379),


Not necessarily in this PR, but would you mind using constant variables instead of hard-coded strings?

kevin85421 · 2025-04-09T17:39:14Z

ray-operator/test/e2eautoscaler/support.go

@@ -98,7 +98,7 @@ func headPodTemplateApplyConfiguration() *corev1ac.PodTemplateSpecApplyConfigura
 				WithName("ray-head").
 				WithImage(GetRayImage()).
 				WithPorts(
-					corev1ac.ContainerPort().WithName("gcs").WithContainerPort(6379),
+					corev1ac.ContainerPort().WithName("gcs-server").WithContainerPort(6379),


Not necessarily in this PR, but would you mind using constant variables instead of hard-coded strings?

kevin85421 · 2025-04-09T17:39:39Z

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

 	// BuildServiceForHeadPod should generate a headless service for a Head Pod by default.
 	if svc.Spec.ClusterIP != corev1.ClusterIPNone {
 		t.Fatalf("Expected `%v` but got `%v`", corev1.ClusterIPNone, svc.Spec.ClusterIP)
 	}
 }

+// Test if default ports is applied and can be overwritten by config


What's the relationship between this test and this PR? The tests seem irrelevant and don't cover all cases. Perhaps we can remove this from the PR and open another one to add tests with sufficient coverage.

This test ensures the default ports are set if user does not specify ports in yaml. As we are trying to remove all ports section in rayservice sample yaml file, I think this test is needed.

-e Signed-off-by: machichima <[email protected]>

kevin85421

Left small comments. Otherwise, LGTM.

kevin85421 · 2025-04-11T19:31:39Z

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

+		name         string
+		expectResult map[string]int32
+		ports        []corev1.ContainerPort
+		expectError  bool


Currently, expectError is false in all tests, so we can remove it for now.

Fixed. Thanks!

kevin85421 · 2025-04-11T19:35:22Z

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

 	// BuildServiceForHeadPod should generate a headless service for a Head Pod by default.
 	if svc.Spec.ClusterIP != corev1.ClusterIPNone {
 		t.Fatalf("Expected `%v` but got `%v`", corev1.ClusterIPNone, svc.Spec.ClusterIP)
 	}
 }

+// Test if default ports is applied and can be overwritten by config
+func TestBuildServiceForHeadPodDefaultPorts(t *testing.T) {
+	type testCase struct {


Can you add comments here to summarize the behavior of setting ports?

Sure, I added:

"Test that default ports are applied when none are specified. The metrics port is always added if not explicitly set."

-e Signed-off-by: machichima <[email protected]>

machichima added 5 commits March 31, 2025 22:58

fix: ray-service sample yaml remove ports

d635aa5

-e Signed-off-by: machichima <[email protected]>

feat: use default port value if not set

e8bba1f

-e Signed-off-by: machichima <[email protected]>

test: check default port value is set

1b28e0a

-e Signed-off-by: machichima <[email protected]>

fix: prevent assign defaulPort that already defined

972aee6

-e Signed-off-by: machichima <[email protected]>

fix: test defaultPort value in use instead of based on port name

a166315

port can be set different name in yaml -e Signed-off-by: machichima <[email protected]>

MortalHappiness self-assigned this Apr 4, 2025

MortalHappiness reviewed Apr 4, 2025

View reviewed changes

rueian reviewed Apr 5, 2025

View reviewed changes

machichima added 2 commits April 5, 2025 11:39

Merge branch 'master' of github.com:ray-project/kuberay into 3246-ray…

a9676a7

…service-default-ports -e Signed-off-by: machichima <[email protected]>

fix: remove not helpful test assert message

9be033b

-e Signed-off-by: machichima <[email protected]>

rueian reviewed Apr 5, 2025

View reviewed changes

machichima added 6 commits April 5, 2025 13:49

test: separate test for default port behaviour

4110361

-e Signed-off-by: machichima <[email protected]>

fix: remove ports section for ray-service.*.yaml files

87eff99

-e Signed-off-by: machichima <[email protected]>

fix: raise error if default port in used

c47baa5

This also raise error if the default port is set with name different to default name -e Signed-off-by: machichima <[email protected]>

fix: gcs to gcs-server

4d96c3f

-e Signed-off-by: machichima <[email protected]>

test: fix custom port with diff name test

7b58937

-e Signed-off-by: machichima <[email protected]>

refactor: remove unnecessary comments

7791809

-e Signed-off-by: machichima <[email protected]>

fix: assign default ports only if no ports set

0bc1145

-e Signed-off-by: machichima <[email protected]>

MortalHappiness reviewed Apr 9, 2025

View reviewed changes

fix: remove unnecessary loop in test

5416c6f

-e Signed-off-by: machichima <[email protected]>

MortalHappiness approved these changes Apr 9, 2025

View reviewed changes

MortalHappiness assigned kevin85421 Apr 9, 2025

kevin85421 reviewed Apr 9, 2025

View reviewed changes

machichima added 2 commits April 10, 2025 21:05

refactor: set ports capacity to 1

796f783

-e Signed-off-by: machichima <[email protected]>

fix: add ports section back to RayCluster YAML

babc998

-e Signed-off-by: machichima <[email protected]>

kevin85421 approved these changes Apr 11, 2025

View reviewed changes

refactor: remove unused expectError

0f0c739

-e Signed-off-by: machichima <[email protected]>

docs: add test docstring for port setting behavior

2fc356d

-e Signed-off-by: machichima <[email protected]>

kevin85421 merged commit 1283a62 into ray-project:master Apr 12, 2025
21 checks passed

machichima mentioned this pull request Apr 12, 2025

[Refactor] use constant variable for port name and value #3341

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Feature][RayService] Set default ports #3262

[Feature][RayService] Set default ports #3262

machichima commented Apr 1, 2025 •

edited

Loading

MortalHappiness Apr 4, 2025

MortalHappiness Apr 4, 2025

MortalHappiness Apr 4, 2025

machichima Apr 5, 2025 •

edited

Loading

machichima Apr 5, 2025

MortalHappiness Apr 7, 2025

MortalHappiness Apr 4, 2025

machichima Apr 5, 2025

kevin85421 commented Apr 5, 2025

rueian Apr 5, 2025

machichima Apr 5, 2025

rueian Apr 5, 2025

rueian Apr 5, 2025

machichima Apr 5, 2025

machichima commented Apr 5, 2025 •

edited

Loading

MortalHappiness commented Apr 7, 2025

MortalHappiness Apr 9, 2025

machichima Apr 9, 2025

kevin85421 Apr 9, 2025

machichima Apr 10, 2025 •

edited

Loading

kevin85421 Apr 9, 2025

machichima Apr 10, 2025

kevin85421 Apr 9, 2025

machichima Apr 10, 2025

kevin85421 Apr 9, 2025

kevin85421 Apr 9, 2025

kevin85421 Apr 9, 2025

machichima Apr 10, 2025

kevin85421 left a comment

kevin85421 Apr 11, 2025

machichima Apr 12, 2025

kevin85421 Apr 11, 2025

machichima Apr 12, 2025

	// Assign default ports
	if len(ports) == 0 {
	ports = getDefaultPorts()
	}

	assert.Equalf(t, 12345, int(svcPorts[name]), "Expected `%v` as `%v` port value but got `%v`", 12345, name, svcPorts[name])
	assert.Equal(t, 12345, int(svcPorts[name]))

[Feature][RayService] Set default ports #3262

[Feature][RayService] Set default ports #3262

Conversation

machichima commented Apr 1, 2025 • edited Loading

Changes

Why are these changes needed?

Related issue number

Checks

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

machichima Apr 5, 2025 • edited Loading

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 commented Apr 5, 2025

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

machichima commented Apr 5, 2025 • edited Loading

MortalHappiness commented Apr 7, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

machichima Apr 10, 2025 • edited Loading

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

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

machichima commented Apr 1, 2025 •

edited

Loading

machichima Apr 5, 2025 •

edited

Loading

machichima commented Apr 5, 2025 •

edited

Loading

machichima Apr 10, 2025 •

edited

Loading