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

Rename RayCluster folder to Ray since the group is Ray #275

Merged
merged 1 commit into from
May 28, 2022

Conversation

brucez-anyscale
Copy link
Contributor

@brucez-anyscale brucez-anyscale commented May 25, 2022

Why are these changes needed?

The kubebuilder layout is that the CRD and controllers should under the group named packages.
This pr relocates those files into the Ray folder.

Related issue number

Checks

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

@Jeffwan
Copy link
Collaborator

Jeffwan commented May 26, 2022

em. Do you plan to enable multi group support?

There's some history I think you should know. Technically, we use ray.io as the group name and we use command kubebuilder create api --group ray.io --version v1alpha1 --kind RayCluster with empty domain to generate skeleton code originally . So the PR title Rename RayCluster folder to Ray since the group is Ray is not accurate..

I kind of forget whether kubebuilder supports multi-group at that time. We did use it to generate some codes but organize layout on our own. We thought even we have new CRDs, rayserve.ray.io rayjob.ray.io should be good enough(kubernetes doesn't have the domain concept natively, it's introduced in kubebuilder) , the trick thing is kubebuilder force user to write kind.group.domain. We can only use ray.io as the group to achieve format like rayxxx.ray.io.

I think currently we have two options

  1. Adopt latest multi-group pattern and regenerate crds which brings migration efforts and bring changes. It would be a new crd. It's better to bump api version if we plan to do this.
  2. Use our own layout. use kind as the folder name.
  3. mixed. Seems https://github.com/openkruise/kruise follow the multi group pattern only on apis but not controllers

@brucez-anyscale
Copy link
Contributor Author

brucez-anyscale commented May 26, 2022

em. Do you plan to enable multi group support?

The multigroup is already enabled. In PROJECT: multigroup: true

There's some history I think you should know. Technically, we use ray.io as the group name and we use command kubebuilder create api --group ray.io --version v1alpha1 --kind RayCluster with empty domain to generate skeleton code originally . So the PR title Rename RayCluster folder to Ray since the group is Ray is not accurate..

I kind of forget whether kubebuilder supports multi-group at that time. We did use it to generate some codes but organize layout on our own. We thought even we have new CRDs, rayserve.ray.io rayjob.ray.io should be good enough(kubernetes doesn't have the domain concept natively, it's introduced in kubebuilder) , the trick thing is kubebuilder force user to write kind.group.domain. We can only use ray.io as the group to achieve format like rayxxx.ray.io.

I think currently we have two options

  1. Adopt latest multi-group pattern and regenerate crds which brings migration efforts and bring changes. It would be a new crd. It's better to bump api version if we plan to do this.
  2. Use our own layout. use kind as the folder name.
  3. mixed. Seems https://github.com/openkruise/kruise follow the multi group pattern only on apis but not controllers

Thanks for the context. I think since domain is just the concept of kubebuilder, then we can make io as domain and ray as group. Just like the PROJECT file I add in the other pr.
https://github.com/ray-project/kuberay/pull/270/files#diff-352882674ddfc4f4c10da459d85128ca2875734a244436e86e42665db543e0b4

Then we can use
kubebuilder create api --group ray --version v1alpha1 --kind RayXXX to init RayService or RayJob in the future and keep the folder as Ray.

Then RayCluster, RayService, and RayJob will be all in group ray and domain io.

This method leads to:

  1. Adopt the latest kubebuilder, we can maintain everything with kubebuilder now
  2. RayCluster, RayService, and RayJob in the same group.

Otherwise:
If we want to apply multi-group pattern.
Then we can do two options:

  1. Keep RayCluster in group ray domain io, and create RayService in group RayService domain io.
  2. Rename RayCluster group as raycluster and create RayService in group RayService domain io.

First of all, I would like to know if we want the same group name: all crds like rayXXX.ray.io
Or we want multi-group and crds like rayXXX.rayXXX.io

If we prefer the same group, this pr makes sense following by #270

@Jeffwan @DmitriGekhtman @simon-mo

@simon-mo
Copy link
Collaborator

For me, rayXXX.ray.io is preferred but if it leads to more effort and migration pain rayXXX.rayXXX.io is also acceptable.

@DmitriGekhtman
Copy link
Collaborator

For me, rayXXX.ray.io is preferred but if it leads to more effort and migration pain rayXXX.rayXXX.io is also acceptable.

Rephrasing more or less the same:
It sounds like rayXXX.rayXXX.io may be the simpler and more incremental thing to do right now.
We could switch to rayXXX.ray.io down the line and bump all of the versions together. There's precedent for doing that -- K8s itself sometimes moves resource kinds between groups.

@Jeffwan
Copy link
Collaborator

Jeffwan commented May 28, 2022

@brucez-anyscale
I just notice one thing we forget to discuss, once you regenerate files using v3 with domain io and group ray. Did you see any updates on the ray apis side? Technically, it should update

// GroupVersion is group version used to register these objects
GroupVersion = schema.GroupVersion{Group: "ray.io", Version: "v1alpha1"}
.

@Jeffwan
Copy link
Collaborator

Jeffwan commented May 28, 2022

Overall the change looks good to me. Thanks for driving this refactor effort. Please help check my last question.

@brucez-anyscale
Copy link
Contributor Author

@brucez-anyscale I just notice one thing we forget to discuss, once you regenerate files using v3 with domain io and group ray. Did you see any updates on the ray apis side? Technically, it should update

// GroupVersion is group version used to register these objects
GroupVersion = schema.GroupVersion{Group: "ray.io", Version: "v1alpha1"}

.

schema.GroupVersion{Group: "ray.io", Version: "v1alpha1"} for kubebuilder, the Group here is actually auto generated asgroup + domain.
So even if we change to group ray and domain io, the generated code is still
schema.GroupVersion{Group: "ray.io", Version: "v1alpha1"}

@Jeffwan
Copy link
Collaborator

Jeffwan commented May 28, 2022

I think we get consensus on this and this PR is clean and straightforward. Let me merge this one

@Jeffwan Jeffwan merged commit 80f19eb into master May 28, 2022
@Jeffwan Jeffwan deleted the brucez/renameRayCluster branch May 28, 2022 02:21
lowang-bh pushed a commit to lowang-bh/kuberay that referenced this pull request Sep 24, 2023
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.

4 participants