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.

Sign up for GitHub

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] Fix status refresh for INIT cluster #2491

Merged
merged 6 commits into from
Aug 31, 2023
Merged

[Core] Fix status refresh for INIT cluster #2491

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
e17c595
Fix ports for existing cluster
Michaelvll Aug 30, 2023
d7463c6
refresh ports
Michaelvll Aug 30, 2023
892ffc9
gracefully handle the head_ssh_port to be None
Michaelvll Aug 30, 2023
8495134
changed to runtime error
Michaelvll Aug 31, 2023
15cdcc9
Merge branch 'master' of github.com:skypilot-org/skypilot into fix-ports
Michaelvll Aug 31, 2023
6f75c06
address comments
Michaelvll Aug 31, 2023
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
14 changes: 12 additions & 2 deletions sky/backends/backend_utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2014,14 +2014,24 @@ def run_ray_status_to_check_ray_cluster_healthy() -> bool:
# triggered.
if external_ips is None or len(external_ips) == 0:
logger.debug(f'Refreshing status ({cluster_name!r}): No cached '
f'IPs found. External IPs: {external_ips}')
f'IPs found. Handle: {handle}')
raise exceptions.FetchIPError(
reason=exceptions.FetchIPError.Reason.HEAD)

if handle.head_ssh_port is None:
# Refresh the ssh ports. It is ok to refresh as it is fast.
handle.external_ssh_ports()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shall we use update_ssh_ports for better readability?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good point! Actually, we don't need to handle this None or refresh case, as we already have it in the external_ssh_ports call. changed to that instead.

if handle.head_ssh_port is None:
logger.debug(
f'Refreshing status ({cluster_name!r}): failed '
f'to get the ssh ports. Handle: {handle}')
raise exceptions.FetchIPError(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shall we use a new exception named FetchPortError instead?

Copy link
Collaborator Author

@Michaelvll Michaelvll Aug 31, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Changed to RuntimeError, as this error is only internal facing and should not be raised in most cases, we probably don't want to add a new exception. Wdyt?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sounds good!

reason=exceptions.FetchIPError.Reason.HEAD)

# Check if ray cluster status is healthy.
ssh_credentials = ssh_credential_from_yaml(handle.cluster_yaml,
handle.docker_user)
assert handle.head_ssh_port is not None, handle

runner = command_runner.SSHCommandRunner(external_ips[0],
**ssh_credentials,
port=handle.head_ssh_port)
Expand Down
12 changes: 7 additions & 5 deletions sky/backends/cloud_vm_ray_backend.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1524,9 +1524,10 @@ def _retry_zones(
if zones and len(zones) == 1:
launched_resources = launched_resources.copy(zone=zones[0].name)

prev_cluster_ips = None
prev_cluster_ips, prev_ssh_ports = None, None
if prev_handle is not None:
prev_cluster_ips = prev_handle.stable_internal_external_ips
prev_ssh_ports = prev_handle.stable_ssh_ports
# Record early, so if anything goes wrong, 'sky status' will show
# the cluster name and users can appropriately 'sky down'. It also
# means a second 'sky launch -c <name>' will attempt to reuse.
Expand All @@ -1543,10 +1544,11 @@ def _retry_zones(
launched_resources=launched_resources,
tpu_create_script=config_dict.get('tpu-create-script'),
tpu_delete_script=config_dict.get('tpu-delete-script'),
# Use the previous cluster's IPs if available to optimize
# the case where the cluster is restarted, i.e., no need to
# query the IPs from the cloud provider.
stable_internal_external_ips=prev_cluster_ips)
# Use the previous cluster's IPs and ports if available to
# optimize the case where the cluster is restarted, i.e., no
# need to query the IPs from the cloud provider.
stable_internal_external_ips=prev_cluster_ips,
stable_ssh_ports=prev_ssh_ports)
usage_lib.messages.usage.update_final_cluster_status(
status_lib.ClusterStatus.INIT)

Expand Down