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

[AWS] Adopt new provisioner to query clusters #2288

Merged
merged 7 commits into from
Jul 26, 2023
Merged

[AWS] Adopt new provisioner to query clusters #2288

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
0277464
init
suquark Jul 21, 2023
065de2b
update
suquark Jul 21, 2023
9defb38
fix comments
suquark Jul 25, 2023
de727df
lint
suquark Jul 25, 2023
6c28cfc
fix
suquark Jul 26, 2023
c38c55d
lint
suquark Jul 26, 2023
a12053d
add comment
suquark Jul 26, 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
13 changes: 12 additions & 1 deletion sky/backends/backend_utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1685,7 +1685,12 @@ def tag_filter_for_cluster(cluster_name: str) -> Dict[str, str]:
def _query_cluster_status_via_cloud_api(
handle: 'cloud_vm_ray_backend.CloudVmRayResourceHandle'
) -> List[status_lib.ClusterStatus]:
"""Returns the status of the cluster."""
"""Returns the status of the cluster.

Raises:
exceptions.ClusterStatusFetchingError: the cluster status cannot be
fetched from the cloud provider.
"""
cluster_name = handle.cluster_name
# Use region and zone from the cluster config, instead of the
# handle.launched_resources, because the latter may not be set
Expand Down Expand Up @@ -1828,6 +1833,12 @@ def check_can_clone_disk_and_override_task(

def _update_cluster_status_no_lock(
cluster_name: str) -> Optional[Dict[str, Any]]:
"""Updates the status of the cluster.

Raises:
exceptions.ClusterStatusFetchingError: the cluster status cannot be
fetched from the cloud provider.
"""
record = global_user_state.get_cluster_from_name(cluster_name)
if record is None:
return None
Expand Down
1 change: 1 addition & 0 deletions sky/provision/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ def query_instances(
provider_name: str,
cluster_name: str,
provider_config: Optional[Dict[str, Any]] = None,
non_terminated_only: bool = True,
Copy link
Member

Choose a reason for hiding this comment

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

For discussion: Does it make sense to not expose this and always assume non_terminated_only=True? Will there be callers who would want this to be False?

stop() and terminate() for example already implicitly assume non-terminated, e.g.,

    filters = [
        {
            'Name': 'instance-state-name',
            # exclude 'shutting-down' or 'terminated' states
            'Values': ['pending', 'running', 'stopping', 'stopped'],
        },
        *_cluster_name_filter(cluster_name),
    ]

Also similar to node providers' design of get_nonterminated_nodes().

We can certainly leave this for the future.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

let me leave a comment about it

) -> Dict[str, Optional[status_lib.ClusterStatus]]:
"""Query instances.

Expand Down
7 changes: 5 additions & 2 deletions sky/provision/aws/instance.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ def _filter_instances(ec2, filters: List[Dict[str, Any]],
def query_instances(
cluster_name: str,
provider_config: Optional[Dict[str, Any]] = None,
non_terminated_only: bool = True,
) -> Dict[str, Optional[status_lib.ClusterStatus]]:
"""See sky/provision/__init__.py"""
assert provider_config is not None, (cluster_name, provider_config)
Expand All @@ -66,8 +67,10 @@ def query_instances(
}
statuses = {}
for inst in instances:
state = inst.state['Name']
statuses[inst.id] = status_map[state]
status = status_map[inst.state['Name']]
if non_terminated_only and status is None:
continue
statuses[inst.id] = status
return statuses


Expand Down