Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RayCluster controller] Add headServiceAnnotations field to RayCluster CR #841

Merged

Conversation

cskornel-doordash
Copy link
Contributor

@cskornel-doordash cskornel-doordash commented Dec 14, 2022

Why are these changes needed?

Related issue number

Closes #708

Checks

  • I've made sure the tests are passing.
  • Testing Strategy
    • Unit tests
    • Manual tests
    • Configuration tests
$ kubectl apply -f ray_v1alpha1_rayservice.yaml
rayservice.ray.io/rayservice-sample created
$ kubectl describe svc rayservice-sample-raycluster-7m5b7-head-svc
Name:              rayservice-sample-raycluster-7m5b7-head-svc
Namespace:         default
Labels:            app.kubernetes.io/created-by=kuberay-operator
                   app.kubernetes.io/name=kuberay
                   ray.io/cluster=rayservice-sample-raycluster-7m5b7
                   ray.io/identifier=rayservice-sample-raycluster-7m5b7-head
                   ray.io/node-type=head
Annotations:       service_key: service_value
...

@kevin85421
Copy link
Member

Maybe it is better to have a new field in CRD for users to specify service annotation (e.g. spec.ServiceAnnotations) as mentioned in the #708 issue description.

cc @DmitriGekhtman any thoughts?

@DmitriGekhtman
Copy link
Collaborator

DmitriGekhtman commented Dec 14, 2022

Maybe it is better to have a new field in CRD for users to specify service annotation (e.g. spec.ServiceAnnotations) as mentioned in the #708 issue description.

cc @DmitriGekhtman any thoughts?

That would be much better. Please do that instead.

In a future API version, we could move all service configuration into its own field spec.headServiceConfig, or whatever you want to call it.

@cskornel-doordash cskornel-doordash marked this pull request as ready for review December 15, 2022 01:38
@cskornel-doordash
Copy link
Contributor Author

cskornel-doordash commented Dec 15, 2022

Changed the field to spec.ServiceAnnotations.

@DmitriGekhtman
Copy link
Collaborator

This looks pretty good to me.

There's a lint failure due to a funky side-effect make build that changes imports in zz_generated_deepcopy.go.
Revert the change to the imports in that file to fix this.
(Fixing this needs to be tracked.)

Copy link
Member

@kevin85421 kevin85421 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank @cskornel-doordash for your contribution! Overall looks pretty good to me.

Copy link
Member

@kevin85421 kevin85421 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thank you for your contribution!

@cskornel-doordash
Copy link
Contributor Author

Thank you for the quick review @kevin85421 @DmitriGekhtman . Would you be able to merge this? I don't have write permissions.

@DmitriGekhtman DmitriGekhtman changed the title pass annotations from RayService to Service [RayCluster controller] Add headServiceAnnotations field to RayCluster CR Dec 16, 2022
@DmitriGekhtman DmitriGekhtman merged commit d4784a5 into ray-project:master Dec 16, 2022
lowang-bh pushed a commit to lowang-bh/kuberay that referenced this pull request Sep 24, 2023
…r CR (ray-project#841)

Adds headServiceAnnotations field to RayCluster CR.

Signed-off-by: Kornel Csernai <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature] Expose RayCluster service annotations
3 participants