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

[Provisioner] Generate valid cluster names when username has invalid characters #1526

Merged
merged 6 commits into from
Dec 19, 2022

Conversation

romilbhardwaj
Copy link
Collaborator

@romilbhardwaj romilbhardwaj commented Dec 16, 2022

Closes #1161.

We now clean up the username (used as suffix in the cluster name) by:

  1. Making all characters lowercase
  2. Removing any non-alphanumeric characters (excluding hyphens)
  3. Removing any numbers and/or hyphens at the start of the username.
  4. Removing any hyphens at the end of the username

e.g. 1SkY-PiLot2- becomes sky-pilot2 when it is used for generating cluster name.

Tested:

Manual tests by overriding the username read by getpass.getuser():

  • LOGNAME=RomilB sky launch - uppercase characters
  • LOGNAME=romil-b sky launch - hypen
  • LOGNAME=romil_b sky launch - underscore
  • LOGNAME=1roMil-b123 sky launch - number at start gets removed. Becomes romilb123
  • LOGNAME=r#omIl_B sky launch

Copy link
Member

@concretevitamin concretevitamin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @romilbhardwaj!

  1. How about keeping hyphens in a username? Since they are used and allowed by clouds.
  2. One other place where non-conforming username may be picked up as part of a cluster name is _default_interactive_node_name() in cli.py. Should we fix that too?
  3. There are a few other usage of getpass.getuser() but I think only the above two are for cluster names purposes.

@romilbhardwaj
Copy link
Collaborator Author

These are great points:

  1. How about keeping hyphens in a username? Since they are used and allowed by clouds.

I have updated the code to allow hyphens in the middle the of the username, but not at start or end. Thinking more about it, maybe it's better to have a simpler scheme (disallow hyphens everywhere), but I don't feel strongly either way.

  1. One other place where non-conforming username may be picked up as part of a cluster name is _default_interactive_node_name() in cli.py. Should we fix that too?

Great catch! I have updated _default_interactive_node_name()

  1. There are a few other usage of getpass.getuser() but I think only the above two are for cluster names purposes.

Yeah, there are some used in hashing job ids, but perhaps they don't need to be changed. Changing them would require some thought about backward compatibility.

Copy link
Member

@concretevitamin concretevitamin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @romilbhardwaj. LGTM.

sky/backends/backend_utils.py Show resolved Hide resolved
sky/backends/backend_utils.py Outdated Show resolved Hide resolved
romilbhardwaj and others added 2 commits December 19, 2022 11:51
Co-authored-by: Zongheng Yang <[email protected]>
@romilbhardwaj romilbhardwaj merged commit 678821e into master Dec 19, 2022
@romilbhardwaj romilbhardwaj deleted the clustername_cleaning branch December 19, 2022 06:47
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.

Invalid cluster name is generated if username has uppercase or invalid characters
2 participants