Expose head service ServiceSpec

[architkulkarni]

Exposes the ServiceSpec of the head pod service to the user. Previously, we only exposed the type field. This led to limitations such as, for example, the user specifying type: LoadBalancer and having no way to set the associated field LoadBalancerIPs.

After this PR, we will have two ways of specifying the service type: the legacy HeadGroupSpec.ServiceType, and the new HeadGroupSpec.HeadServiceSpec.Type. In this PR we make the former take precedence.

In the long term we should have only one way of specifying this. One plan might involve a few PRs:

1. Update all sample YAMLS and documentation examples that currently use HeadGroupSpec.ServiceType to use HeadGroupSpec.HeadServiceSpec.Type, if any
2. Print a deprecation warning for HeadGroupSpec.ServiceType in v0.6
3. Remove HeadGroupSpec.ServiceType in v0.7.

Or, if we think it's convenient and harmless, we can leave them both in forever.

Why are these changes needed?

Closes #877

Related issue number

Checks

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

[architkulkarni]

Everything on and below this line (properties:) was autogenerated by make helm. I'm not sure how it works, but it looks reasonable.

[architkulkarni]

Is https://github.com/ray-project/kuberay/blob/master/ray-operator/controllers/ray/raycluster_controller_fake_test.go the best place to add a test for this?

[architkulkarni]

There's one test failure:

                               Diff:
                                --- Expected
                                +++ Actual
                                @@ -8,3 +8,5 @@
                                   (string) (len=16) "rayClusterConfig": (map[string]interface {}) (len=3) {
                                -   (string) (len=13) "headGroupSpec": (map[string]interface {}) (len=3) {
                                +   (string) (len=13) "headGroupSpec": (map[string]interface {}) (len=4) {
                                +    (string) (len=15) "headServiceSpec": (map[string]interface {}) {
                                +    },
                                     (string) (len=14) "rayStartParams": (map[string]interface {}) (len=7) {
                Test:           TestMarshallingRayService
--- FAIL: TestMarshallingRayService (0.00s)

I'm trying to figure out why the new headServiceSpec field is being included but the existing serviceType field isn't included, even though they're both optional and have similar code handling them.

[kevin85421]

Add headServiceSpec in values.yaml.

[architkulkarni]

I was wondering about this, why does it need to be included? I used head autoscalerOptions: as an example, which is also not included in values.yaml.

Did you mean to add headServiceSpec with a default value of {}, or to add it commented out like autoscalerOptions?

[kevin85421]

v1.ServiceSpec is not enough for users. For example, users need to configure head service's annotations for some ingress controllers (#708). In addition, users also want to configure service's labels (#874, slack thread).

[architkulkarni]

Sorry, could you say a little more about what you mean here? Are you saying "expose the entire head service" is different from "expose the head service spec"?

[architkulkarni]

Resolved offline. I'll update this PR to expose the entire head service (which in particular includes ObjectMeta, which includes labels and annotations.)

[DmitriGekhtman]

In the long term we should have only one way of specifying this.

At some point, the API needs to be upgraded to, say, v1beta1. Then you're allowed to make backwards-incompatible changes, but there's some rigamarole with converting webhooks that you're supposed to do: https://book.kubebuilder.io/multiversion-tutorial/conversion-concepts.html

[architkulkarni]

Closing in favor of #1040