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

[feat][RayCluster] shorten HeadlessServiceSuffix to have more space for CR names #3101

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

rueian
Copy link
Contributor

@rueian rueian commented Feb 25, 2025

Why are these changes needed?

Currently, KubeRay adds different suffixes to make different services for a RayCluster. It will also trim these service names if they are too long to fit the 63-character limitation by k8s. However, the silent cut is not user-friendly as users can't easily know the resulting service names beforehand.

As a part of #3076: we are going to add a length validation to RayCluster names in v1.4 to make sure the silent cut does not take place while keeping sufficient room for the names specified by users, this PR shortens the -headless-worker-svc suffix to just -headless, saving 11 characters for users to use.

Related issue number

#3076

Checks

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

@kevin85421
Copy link
Member

can you split this into two PRs, one for headless service and one for RayCluster?

@rueian rueian force-pushed the less-suffix-for-more-resource-name branch from cb99863 to d67af8c Compare February 25, 2025 03:57
@rueian rueian changed the title [feat] shorten HeadlessServiceSuffix and RayClusterSuffix to have more space for CR names [feat] shorten HeadlessServiceSuffix to have more space for CR names Feb 25, 2025
@rueian rueian changed the title [feat] shorten HeadlessServiceSuffix to have more space for CR names [feat][RayCluster] shorten HeadlessServiceSuffix to have more space for CR names Feb 25, 2025
@rueian rueian force-pushed the less-suffix-for-more-resource-name branch from d67af8c to 536d17a Compare February 25, 2025 03:58
@rueian rueian marked this pull request as ready for review February 25, 2025 05:51
@rueian
Copy link
Contributor Author

rueian commented Feb 25, 2025

can you split this into two PRs, one for headless service and one for RayCluster?

Sure, another PR is here: #3102

@rueian
Copy link
Contributor Author

rueian commented Feb 25, 2025

Hi @andrewsykim,

We'd like to add a length validation to RayCluster names in the next release to provide non-trimmed service names for a better user experience. But the current headless service suffix for the TPU webhook, -headless-worker-svc, is already too long, we'd like to shorten it to just -headless to leave more space for RayCluster names.

Please let me know if you have any concerns about this. Thanks!

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