[refactor][operator]: make RayStartParams optional
#3202
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
a8efa58 to
a1e53d9
Compare
omitempty on RayStartParams
c9781e1 to
80eb350
Compare
andrewsykim
reviewed
Mar 17, 2025
davidxia
commented
Mar 17, 2025
There was a problem hiding this comment.
the important changes are in this file
Comment on lines
89
to
149
| // TODO: Look for better workaround/fixes for RayStartParams. Currently using `WithRayStartParams()` requires | ||
| // a non-empty map with valid key value pairs and will not populate the field with empty/nil values. This | ||
| // isn't ideal as it forces the generated RayCluster yamls to use those parameters. | ||
| headRayStartParams := map[string]string{ | ||
| "dashboard-host": "0.0.0.0", | ||
| } | ||
| workerRayStartParams := map[string]string{ | ||
| "metrics-export-port": "8080", | ||
| } |
There was a problem hiding this comment.
can remove this now since the map can be empty
98625b2 to
973af72
Compare
davidxia
commented
Mar 21, 2025
aa3243a to
8b0650b
Compare
omitempty on RayStartParamsRayStartParams optional
8b0650b to
6359479
Compare
davidxia
commented
Mar 23, 2025
Comment on lines
57
to
60
| expectedWorkerRayStartParams := map[string]string{ | ||
| "metrics-export-port": "8080", | ||
| } | ||
| maps.Copy(expectedWorkerRayStartParams, testRayClusterYamlObject.WorkerRayStartParams) |
There was a problem hiding this comment.
We can remove the hard-coded metrics-export-port now.
Comment on lines
-184
to
-346
| rayStartParams: | ||
|
|
There was a problem hiding this comment.
This shows that when rayStartParams aren't set in the RayClusterApplyConfiguration, the field is absent from the serialized YAML.
05f6a13 to
557b90a
Compare
davidxia
commented
Mar 24, 2025
hm this is still failing 🤔 |
rueian
approved these changes
8000
Apr 1, 2025
|
The changes look good to me. I got the failed test from logs: --- RUN TestAutoscalingRayService
[2025-03-31T19:12:16Z] rayservice_ha_test.go:82: [2025-03-31T19:06:25Z] Created ConfigMap test-ns-rtl5s/locust-runner-script successfully
[2025-03-31T19:12:16Z] rayservice_ha_test.go:85: [2025-03-31T19:06:25Z] Successfully applied testdata/rayservice.autoscaling.yaml to namespace test-ns-rtl5s
[2025-03-31T19:12:16Z] rayservice_ha_test.go:88: [2025-03-31T19:06:25Z] Created RayService test-ns-rtl5s/test-rayservice successfully
[2025-03-31T19:12:16Z] rayservice_ha_test.go:90: [2025-03-31T19:06:25Z] Waiting for RayService test-ns-rtl5s/test-rayservice to running
[2025-03-31T19:12:16Z] rayservice_ha_test.go:92:
[2025-03-31T19:12:16Z] Timed out after 300.001s.
[2025-03-31T19:12:16Z] Expected
[2025-03-31T19:12:16Z] <bool>: false
[2025-03-31T19:12:16Z] to be true
[2025-03-31T19:12:16Z] test.go:112: [2025-03-31T19:11:25Z] Retrieving Pod Container test-ns-rtl5s/test-rayservice-8dspv-head/ray-head logs
[2025-03-31T19:12:16Z] test.go:100: [2025-03-31T19:11:25Z] Creating ephemeral output directory as KUBERAY_TEST_OUTPUT_DIR env variable is unset
[2025-03-31T19:12:16Z] test.go:103: [2025-03-31T19:11:25Z] Output directory has been created at: /tmp/TestAutoscalingRayService3172182483/001
[2025-03-31T19:12:16Z] test.go:112: [2025-03-31T19:11:25Z] Retrieving Pod Container test-ns-rtl5s/test-rayservice-8dspv-head/autoscaler logs
[2025-03-31T19:12:16Z] ### FAIL: TestAutoscalingRayService (300.24s)Hope that logs help. |
MortalHappiness
approved these changes
Apr 2, 2025
cbe7c1f to
dd7033e
Compare
davidxia
added a commit
to davidxia/ray
that referenced
this pull request
Apr 3, 2025
We are planning on making `rayStartParams` optional and not present in the RayCluster K8s custom resource output if it's empty. ray-project/kuberay#3202 We need to make its autoscaler code not error when it's absent. This change is also needed for the integration tests in that PR to pass. Signed-off-by: David Xia <david@davidxia.com>
8 tasks
|
Need a review on ray-project/ray#51954. It should fix some of the buildkite/ray-ecosystem-ci-kuberay-ci/test-e2e-rayservice-nightly-operator failures |
davidxia
added a commit
to davidxia/ray
that referenced
this pull request
Apr 3, 2025
We are planning on making `rayStartParams` optional and not present in the RayCluster K8s custom resource output if it's empty. ray-project/kuberay#3202 We need to make its autoscaler code not error when it's absent. This change is also needed for the integration tests in that PR to pass. Signed-off-by: David Xia <david@davidxia.com>
|
PR is blocked until ray releases a new version with ray-project/ray#51954 and we upgrade to that version in this repo |
|
depends on #3547 |
|
@davidxia, how about adding a key-value pair so that the CI tests won’t fail and I can merge the PR? I’ll bump the Ray version for the v1.4.0 release, which is a pretty heavyweight process. We can remove the key-value pair 8000 after bumping the version but before the release. |
used in `kubectl ray create cluster`. Signed-off-by: David Xia <david@davidxia.com>
in `RayCluster` CRD by marking it with `// +optional`.
Make it not serialized when empty with `omitempty`.
This change is backwards compatible on K8s clusters with existing
RayCluster CRD and RayCluster CRs.
* RayClusters with non-empty RayStartParams will serialize the same as
before this change.
* RayClusters with empty RayStartParams will serialize the same as
before this change: as an empty map, `{}` in YAML.
* New behavior: we can `kubectl create|apply` RayClusters that lack
RayStartParams in both head and worker groups. Previously the K8s API
would return validation errors requiring this field to be present.
When we get these RayClusters back with `kubectl get raycluster -o
yaml`, there's no `rayStartParams` field present.
See [Optional vs. Required][1] in K8s API Conventions doc.
> Optional fields have the following properties:
> * They have the `+optional` comment tag in Go.
> * They are a pointer type in the Go definition (e.g. AwesomeFlag *SomeFlag)
> or have a built-in nil value (e.g. maps and slices).
> * The API server should allow POSTing and PUTing a resource with this field
> unset.
The required changes to the RayCluster CRD YAML can be achieved with only
`omitempty`, but we also add `// +optional` as recommended by the doc above.
[1]: https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api-conventions.md#optional-vs-required
Signed-off-by: David Xia <david@davidxia.com>
dd7033e to
e940dbe
Compare
|
@kevin85421 makes sense, updated in e940dbe |
|
Does this mean that the Autoscaler will fail if users specify |
|
Good point. Yes. We should make this backwards compatible. I’ll think a bit more on this. |
* `RayClusters` with `rayStartParams: {}` will continue to be serialized with
the empty map.
* `RayClusters` without `rayStartParams` will be serialized without the field.
This prevents a backwards-incompatible change with RayClusters that use the
autoscaler with Ray versions earlier than 2.45.0 which lack this change
ray-project/ray#51954.
Signed-off-by: David Xia <david@davidxia.com>
e940dbe to
b1c0d28
Compare
|
@kevin85421 I think b1c0d28 makes backwards compatible. |
kevin85421
approved these changes
May 11, 2025
2 tasks
|
Open a follow up issue: #3580 |
davidxia
added a commit
to davidxia/kuberay
that referenced
this pull request
May 13, 2025
No longer needed after ray-project#3202 Signed-off-by: David Xia <david@davidxia.com>
4 tasks
davidxia
added a commit
to davidxia/kuberay
that referenced
this pull request
May 13, 2025
No longer needed after ray-project#3202 Signed-off-by: David Xia <david@davidxia.com>
davidxia
added a commit
to davidxia/kuberay
that referenced
this pull request
May 13, 2025
No longer needed after ray-project#3202 Signed-off-by: David Xia <david@davidxia.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
in
RayClusterCRD by marking it with// +optional.Make it not serialized when empty with
omitempty.This change is backwards compatible on K8s clusters with existing
RayCluster CRD and RayCluster CRs.
before this change.
before this change: as an empty map,
{}in YAML.kubectl create|applyRayClusters that lackRayStartParams in both head and worker groups. Previously the K8s API
would return validation errors requiring this field to be present.
When we get these RayClusters back with
kubectl get raycluster -o yaml, there's norayStartParamsfield present.See Optional vs. Required in K8s API Conventions doc.
The required changes to the RayCluster CRD YAML can be achieved with only
omitempty, but we also add// +optionalas recommended by the doc above.Signed-off-by: David Xia david@davidxia.com