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

[Core] Relax cluster name restriction and process cloud cluster name #3130

Merged
Merged
Show file tree
Hide file tree
Changes from 10 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions sky/exceptions.py
Original file line number Diff line number Diff line change
Expand Up @@ -213,6 +213,11 @@ class InvalidClusterNameError(Exception):
pass


class UnexpectedCharacterInClusterName(Exception):
"""Raised when cluster name has unexpected character."""
pass

dtran24 marked this conversation as resolved.
Show resolved Hide resolved

class CloudUserIdentityError(Exception):
"""Raised when the cloud identity is invalid."""
pass
Expand Down
2 changes: 1 addition & 1 deletion sky/skylet/constants.py
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@

# In most clouds, cluster names can only contain lowercase letters, numbers
# and hyphens. We use this regex to validate the cluster name.
CLUSTER_NAME_VALID_REGEX = '[a-z]([-a-z0-9]*[a-z0-9])?'
CLUSTER_NAME_VALID_REGEX = '[a-zA-Z]([-_.a-zA-Z0-9]*[a-zA-Z0-9])?'

# Used for translate local file mounts to cloud storage. Please refer to
# sky/execution.py::_maybe_translate_local_file_mounts_and_sync_up for
Expand Down
34 changes: 22 additions & 12 deletions sky/utils/common_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -117,44 +117,54 @@ def _base36_encode(num: int) -> str:
return _base36_encode(int_value)


def make_cluster_name_on_cloud(cluster_name: str,
def make_cluster_name_on_cloud(display_name: str,
max_length: Optional[int] = 15,
add_user_hash: bool = True) -> str:
"""Generate valid cluster name on cloud that is unique to the user.

This is to map the cluster name to a valid length for cloud providers, e.g.
GCP limits the length of the cluster name to 35 characters. If the cluster
name with user hash is longer than max_length:
This is to map the cluster name to a valid length and character set for
cloud providers,
- e.g. GCP limits the length of the cluster name to 35 characters. If the
cluster name with user hash is longer than max_length:
1. Truncate it to max_length - cluster_hash - user_hash_length.
2. Append the hash of the cluster name
dtran24 marked this conversation as resolved.
Show resolved Hide resolved
- e.g. Some cloud providers don't allow for uppercase letters, periods,
or underscores, so we map these to hyphens
dtran24 marked this conversation as resolved.
Show resolved Hide resolved

Args:
cluster_name: The cluster name to be truncated and hashed.
display_name: The cluster name to be truncated, hashed, and
transformed.
max_length: The maximum length of the cluster name. If None, no
truncation is performed.
add_user_hash: Whether to append user hash to the cluster name.
"""

cluster_name_on_cloud = re.sub(r'[._]', '-', display_name).lower()
if display_name != cluster_name_on_cloud:
logger.debug(f'Cluster name will be called {cluster_name_on_cloud} on '
f'the cloud. The user specified cluster name '
f'("{display_name}") would be invalid in the cloud.')
dtran24 marked this conversation as resolved.
Show resolved Hide resolved
user_hash = ''
if add_user_hash:
user_hash = get_user_hash()[:USER_HASH_LENGTH_IN_CLUSTER_NAME]
user_hash = f'-{user_hash}'
user_hash_length = len(user_hash)

if (max_length is None or
len(cluster_name) <= max_length - user_hash_length):
return f'{cluster_name}{user_hash}'
len(cluster_name_on_cloud) <= max_length - user_hash_length):
return f'{cluster_name_on_cloud}{user_hash}'
# -1 is for the dash between cluster name and cluster name hash.
truncate_cluster_name_length = (max_length - CLUSTER_NAME_HASH_LENGTH - 1 -
user_hash_length)
truncate_cluster_name = cluster_name[:truncate_cluster_name_length]
truncate_cluster_name = cluster_name_on_cloud[:truncate_cluster_name_length]
if truncate_cluster_name.endswith('-'):
truncate_cluster_name = truncate_cluster_name.rstrip('-')
assert truncate_cluster_name_length > 0, (cluster_name, max_length)
cluster_name_hash = hashlib.md5(cluster_name.encode()).hexdigest()
assert truncate_cluster_name_length > 0, (cluster_name_on_cloud, max_length)
local_cluster_name_hash = hashlib.md5(display_name.encode()).hexdigest()
dtran24 marked this conversation as resolved.
Show resolved Hide resolved
# Use base36 to reduce the length of the hash.
cluster_name_hash = base36_encode(cluster_name_hash)
local_cluster_name_hash = base36_encode(local_cluster_name_hash)
return (f'{truncate_cluster_name}'
f'-{cluster_name_hash[:CLUSTER_NAME_HASH_LENGTH]}{user_hash}')
f'-{local_cluster_name_hash[:CLUSTER_NAME_HASH_LENGTH]}{user_hash}')


def cluster_name_in_hint(cluster_name: str, cluster_name_on_cloud: str) -> str:
Expand Down
21 changes: 21 additions & 0 deletions tests/unit_tests/test_cloud.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import pytest

from sky import exceptions
from sky.clouds.cloud import Cloud


Expand All @@ -12,3 +13,23 @@ def test_cloud_get_reservations_available_resources(specific_reservations,
available_resources = Cloud().get_reservations_available_resources(
"instance_type", "region", "zone", specific_reservations)
assert available_resources == expected


class TestCheckClusterNameIsValid:

def test_check(self):
Cloud.check_cluster_name_is_valid("lora")
dtran24 marked this conversation as resolved.
Show resolved Hide resolved

def test_check_with_hyphen(self):
Cloud.check_cluster_name_is_valid("seed-1")

def test_check_with_characters_to_transform(self):
Cloud.check_cluster_name_is_valid("Cuda_11.8")

def test_check_when_starts_with_number(self):
with pytest.raises(exceptions.InvalidClusterNameError):
Cloud.check_cluster_name_is_valid("11.8cuda")

def test_check_with_invalid_characters(self):
with pytest.raises(exceptions.InvalidClusterNameError):
Cloud.check_cluster_name_is_valid("lor@")
25 changes: 25 additions & 0 deletions tests/unit_tests/test_common_utils.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
from unittest.mock import patch

from sky.utils import common_utils

MOCKED_USER_HASH = 'ab12cd34'


class TestMakeClusterNameOnCloud:

@patch('sky.utils.common_utils.get_user_hash')
def test_make(self, mock_get_user_hash):
mock_get_user_hash.return_value = MOCKED_USER_HASH
assert "lora-ab12" == common_utils.make_cluster_name_on_cloud("lora")

@patch('sky.utils.common_utils.get_user_hash')
def test_make_with_hyphen(self, mock_get_user_hash):
mock_get_user_hash.return_value = MOCKED_USER_HASH
assert "seed-1-ab12" == common_utils.make_cluster_name_on_cloud(
"seed-1")

@patch('sky.utils.common_utils.get_user_hash')
def test_make_with_characters_to_transform(self, mock_get_user_hash):
mock_get_user_hash.return_value = MOCKED_USER_HASH
assert "cuda-11-8-ab12" == common_utils.make_cluster_name_on_cloud(
"Cuda_11.8")
Loading