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

Add ray head service endpoints in status for expose raycluster's head node endpoints #341

Merged

Conversation

scarlet25151
Copy link
Collaborator

Why are these changes needed?

Related issue number

Closes #223

Checks

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

@Jeffwan Jeffwan mentioned this pull request Jul 6, 2022
4 tasks
@Jeffwan
Copy link
Collaborator

Jeffwan commented Jul 6, 2022

Let's try to make the PR title self explainable. add service port is too vague and people doesn't know more details from the title.

@scarlet25151 scarlet25151 changed the title add service port Add ray head service endpoints in status for expose raycluster's head node endpoints Jul 6, 2022
@Jeffwan
Copy link
Collaborator

Jeffwan commented Jul 6, 2022

@scarlet25151 please rebase the master changes. What's wrong with the compatibility tests?

@scarlet25151 scarlet25151 force-pushed the enhancement/add-service-port branch 2 times, most recently from ae8f98a to cce9d1d Compare July 6, 2022 23:44
@Jeffwan Jeffwan merged commit 3093d78 into ray-project:master Jul 7, 2022
@scarlet25151
Copy link
Collaborator Author

scarlet25151 commented Jul 7, 2022

/cc @davidxia @daikeshi ,I've pushed the feature of adding service endpoints in rayclusters' status in the repos, PTAL.

davidxia added a commit to davidxia/kuberay that referenced this pull request Jul 15, 2022
instead of NodePort which is usually "0"
for updating RayCluster's `status.endpoints`.

follow up to ray-project#341
@davidxia
Copy link
Contributor

davidxia commented Jul 16, 2022

@scarlet25151 thanks! I think there's a bug and made a draft #383.

Are there plans to include the head node's pod IP in the API Cluster response too? Happy to make a PR for that if we're OK with it.

@scarlet25151
Copy link
Collaborator Author

@scarlet25151 thanks! I think there's a bug and made a draft #383.

Are there plans to include the head node's pod IP in the API Cluster response too? Happy to make a PR for that if we're OK with it.

@davidxia Great, I think the draft is reasonable and thank you for the draft. I think it's a good idea to include the head node's pod IP in the API cluster response. It would be greate that you would make a PR. Btw, from the point of extensibility, I would recommend we add a string map like: map<string, string> metadata in the protobuf definition so in the future we would not change the protobuf so much if we would like to add some more information, and we may restore the head node pod IP in some way of key value pairs. WDYT? /cc @Jeffwan

davidxia added a commit to davidxia/kuberay that referenced this pull request Jul 16, 2022
instead of NodePort which is usually "0"
for updating RayCluster's `status.endpoints`.

follow up to ray-project#341
davidxia added a commit to davidxia/kuberay that referenced this pull request Jul 16, 2022
instead of NodePort which is usually "0"
for updating RayCluster's `status.endpoints`.

follow up to ray-project#341
davidxia added a commit to davidxia/kuberay that referenced this pull request Jul 16, 2022
instead of NodePort which is usually "0"
for updating RayCluster's `status.endpoints`.

follow up to ray-project#341
davidxia added a commit to davidxia/kuberay that referenced this pull request Jul 16, 2022
instead of NodePort which is usually "0"
for updating RayCluster's `status.endpoints`.

follow up to ray-project#341
@davidxia
Copy link
Contributor

@scarlet25151 nice, #383 is now ready for review.

I've also drafted davidxia#1 for adding a status.metadata object. Lmk if that approach looks good enough to proceed.

davidxia added a commit to davidxia/kuberay that referenced this pull request Jul 21, 2022
instead of NodePort which is usually "0"
for updating RayCluster's `status.endpoints`.

follow up to ray-project#341
davidxia added a commit to davidxia/kuberay that referenced this pull request Jul 21, 2022
instead of NodePort which is usually "0"
for updating RayCluster's `status.endpoints`.

follow up to ray-project#341
davidxia added a commit to davidxia/kuberay that referenced this pull request Jul 21, 2022
if NodePort is "0" for updating RayCluster's `status.endpoints`.

follow up to ray-project#341
davidxia added a commit to davidxia/kuberay that referenced this pull request Jul 21, 2022
if NodePort is "0" for updating RayCluster's `status.endpoints`.

follow up to ray-project#341
Jeffwan pushed a commit that referenced this pull request Jul 25, 2022
if NodePort is "0" for updating RayCluster's `status.endpoints`.

follow up to #341
lowang-bh pushed a commit to lowang-bh/kuberay that referenced this pull request Sep 24, 2023
lowang-bh pushed a commit to lowang-bh/kuberay that referenced this pull request Sep 24, 2023
if NodePort is "0" for updating RayCluster's `status.endpoints`.

follow up to ray-project#341
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 Cluster State via cluster.status and api response
3 participants