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

[dtran24]

Fixes #2709

• Will close previous PR [Core] Relax cluster name restrictions and process cluster names #2743 when this one gets merged
• Was easier to start new PR then work from PR started months ago

Tested (run the relevant ones):

• Code formatting: bash format.sh
• Any manual or new tests for this PR (please specify below)
  • pytest tests/unit_tests/test_cloud.py::TestCheckClusterNameIsValid
  • pytest tests/unit_tests/test_common_utils.py
  • sky launch -c mycluster@ hello_sky.yaml --cloud aws. Raised exception, as expected
  • sky launch -c mycluster hello_sky.yaml --cloud aws
  • sky launch -c Cuda_11.8 hello_sky.yaml --cloud aws. Processed cluster name, as expected. sky status displays the original cluster name
  • sky launch -c mycluster@ hello_sky.yaml --cloud aws --use-spot. Raised exception, as expected
  • sky launch -c mycluster hello_sky.yaml --cloud aws --use-spot
  • sky launch -c Cuda_11.8 hello_sky.yaml --cloud aws --use-spot. Processed cluster name, as expected. sky status displays the original cluster name
  • backward compatibility test (link to comment for more details)
• All smoke tests: pytest tests/test_smoke.py
• Relevant individual smoke tests: pytest tests/test_smoke.py::test_fill_in_the_name
• Backward compatibility tests: bash tests/backward_comaptibility_tests.sh

[dtran24]

Requesting review @Michaelvll

[Michaelvll]

Thanks for the fix @dtran24! Looks pretty good to me! Left some comments mostly for code style : )

[dtran24]

Appreciate the review @Michaelvll! Is helpful for me as I learn more about the codebase's patterns

Also, whenever this PR does get approved, feel free to merge it since I don't have permissions to merge myself

[Michaelvll]

Thanks for the quick update @dtran24! We should test a bit of the backward compatibility before merging, i.e., we need to make sure the cluster_name_on_cloud does not change for existing clusters after making the change for this function.

[dtran24]

Did a quick check on backward compatibility

1. From master branch, run sky launch -c mycluster hello_sky.yaml --cloud aws
2. Switched to feature branch (i.e. core/relax-cluster-name-constraint), and ran sky down mycluster

Let me know if something like this is what you had in mind @Michaelvll

[Michaelvll]

Thanks for the quick fix @dtran24! Some final comments:

We should update the docstr in the following function, by saying that dot and underscores are allowed, as we will handle it when mapping to the cluster_name_on_cloud in common_utils.make_cluster_name_on_cloud

skypilot/sky/utils/common_utils.py

Lines 553 to 574 in d10ec5a

	def get_cleaned_username(username: str = '') -> str:
	"""Cleans the username as some cloud provider have limitation on
	characters usage such as dot (.) is not allowed in GCP.
	Clean up includes:
	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
	Returns:
	A cleaned username.
	"""
	username = username or getpass.getuser()
	username = username.lower()
	username = re.sub(r'[^a-z0-9-]', '', username)
	username = re.sub(r'^[0-9-]+', '', username)
	username = re.sub(r'-$', '', username)
	return username

[dtran24]

PTAL @Michaelvll, appreciate the time and effort for reviewing this multiple times!

As per the comment for updating docstr for get_cleaned_username, I also updated the regex to also include dots and underscores.

skypilot/sky/utils/common_utils.py

Line 605 in 430ab05

username = re.sub(r'[^a-z0-9-._]', '', username)

Otherwise, the docstr update didn't seem to make sense to me

[Michaelvll]

Thank you for the update @dtran24! LGTM. Merging the PR now.