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

Adding capability to create ray cluster with serve support #1582

Closed
wants to merge 7 commits into from

Conversation

blublinsky
Copy link
Contributor

Why are these changes needed?

Currently, RayCluster CRD does not provide capabilities to create a cluster that can be used for Ray Serve. The only way to create such a cluster is to use RayServe CRD. This PR overcomes this by creating a cluster supporting Ray serve if an annotation "ray.io/enableAgentService": "true" is added to the cluster.

Related issue number

closes https://github.com/ray-project/kuberay

Checks

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

@blublinsky
Copy link
Contributor Author

@kevin85421 I was waiting for a while to implement this and it turned out to be extremely simple. The amount of additional code is minimal, so I assume this is not a big deal.
@z103cb, please take a look as well. I added a apiserver/test/cluster/cluster/cluster_with_serve to be able to test it from the API server

@kevin85421 kevin85421 self-assigned this Nov 15, 2023
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 you for the contributions! I've left some comments. Would you or someone from IBM/Red Hat mind manually testing this PR to ensure that the Kubernetes service is created successfully and selects the correct Ray Pods after addressing the comments?

ray-operator/controllers/ray/common/pod.go Outdated Show resolved Hide resolved
ray-operator/controllers/ray/common/service.go Outdated Show resolved Hide resolved
ray-operator/controllers/ray/common/service.go Outdated Show resolved Hide resolved
ray-operator/controllers/ray/common/service.go Outdated Show resolved Hide resolved
ray-operator/controllers/ray/common/service_test.go Outdated Show resolved Hide resolved
ray-operator/controllers/ray/raycluster_controller.go Outdated Show resolved Hide resolved
ray-operator/controllers/ray/raycluster_controller.go Outdated Show resolved Hide resolved
@blublinsky
Copy link
Contributor Author

I have also double-checked that everything works and added a little readme for using this from the API server

@blublinsky
Copy link
Contributor Author

The error here is due to the age of the PR. I can rebase it, but this will bloat the PR with a lot of things, that are not relevant

@kevin85421
Copy link
Member

The commit 8bfc8d6 only has a new doc. Maybe you forgot to git add some changes to this repo?

I can rebase it, but this will bloat the PR with a lot of things, that are not relevant

If you rebase it correctly, it should not bloat with irrelevant things.

@blublinsky
Copy link
Contributor Author

@kevin85421 PR is rebased.

@kevin85421
Copy link
Member

Your rebase process seems to have some issues. It adds a lot of irrelevant things to this PR. Maybe we can open a new PR instead?

@blublinsky
Copy link
Contributor Author

Thats how rebase works, sorry. These additional things are completely harmless, as they come from git. They are already there

@kevin85421
Copy link
Member

I guess you did not resolve the conflicts correctly before continuing with rebase --continue, but it does not matter. Would you mind opening a new PR instead? I cannot merge this PR with a lot of irrelevant things. If you do not want to do that, I can open a PR to implement this when I have time. Thanks!

@blublinsky blublinsky closed this Nov 22, 2023
@blublinsky blublinsky deleted the kuberay-serve branch November 22, 2023 08:30
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.

2 participants