GCP: allow stop/autostop for spot VMs. #2877

concretevitamin · 2023-12-16T18:23:52Z

Tested (run the relevant ones):

Code formatting: bash format.sh
Any manual or new tests for this PR (please specify below)
All smoke tests: pytest tests/test_smoke.py
Relevant individual smoke tests: pytest tests/test_smoke.py::test_stop_gcp_spot
Backward compatibility tests: bash tests/backward_comaptibility_tests.sh

Michaelvll

Thanks for adding the support of stopping a spot instance on GCP @concretevitamin! The code looks mostly good to me! Left two nits.

Michaelvll · 2023-12-19T00:52:31Z

sky/core.py

+        if handle.launched_resources.use_spot and not cloud.is_same_cloud(
+                clouds.GCP()):


How about we make this as a CloudImplementationFeatures.STOP_SPOT_INSTANCE, so we don't have to do a cloud specific check here?

Good call, done.

Michaelvll · 2023-12-19T00:54:46Z

sky/core.py

+    elif (handle.launched_resources.use_spot and not down and not is_cancel and
+          not handle.launched_resources.cloud.is_same_cloud(clouds.GCP())):


Similarly, should we make this one of the CloudImplementationFeatures?

Good call, done.

Michaelvll

Thanks for the fix @concretevitamin! It looks mostly good to me with a comment for a refactoring of the check_features_are_supported function.

Michaelvll · 2023-12-22T03:53:39Z

sky/core.py

        cloud.check_features_are_supported(
            {clouds.CloudImplementationFeatures.STOP})
-        if handle.launched_resources.use_spot:
+        # Check cloud supports stopping spot instances
+        supports_stop_spot = True
+        try:
+            cloud.check_features_are_supported(
+                {clouds.CloudImplementationFeatures.STOP_SPOT_INSTANCE})
+        except exceptions.NotSupportedError:
+            supports_stop_spot = False
+        # Allow GCP spot to be stopped since it preserves disk:
+        #   https://cloud.google.com/compute/docs/instances/preemptible#preemption-process  # pylint: disable=line-too-long
+        if handle.launched_resources.use_spot and not supports_stop_spot:
            # Disable spot instances to be stopped.
-            # TODO(suquark): enable GCP+spot to be stopped in the future.
            raise exceptions.NotSupportedError(
                f'{colorama.Fore.YELLOW}Stopping cluster '
                f'{cluster_name!r}... skipped.{colorama.Style.RESET_ALL}\n'


How about the following for simplicity?

features = {clouds.CloudImplementationFeatures.STOP} if handle.launched_resources.use_spot: features.insert(clouds.CloudImplementationFeatures.STOP_SPOT_INSTANCE) cloud.check_features_are_supported(features)

Since check_features_are_supported() raises an error I found it harder to read. For example, if a cloud doesn't support stopping spot and here the cluster is an on-demand one, we do not want to perform a check on STOP_SPOT_INSTANCE or raise.

sky/core.py

concretevitamin

Made some changes. PTAL.

concretevitamin · 2023-12-22T04:42:05Z

sky/core.py

        cloud.check_features_are_supported(
            {clouds.CloudImplementationFeatures.STOP})
-        if handle.launched_resources.use_spot:
+        # Check cloud supports stopping spot instances
+        supports_stop_spot = True
+        try:
+            cloud.check_features_are_supported(
+                {clouds.CloudImplementationFeatures.STOP_SPOT_INSTANCE})
+        except exceptions.NotSupportedError:
+            supports_stop_spot = False
+        # Allow GCP spot to be stopped since it preserves disk:
+        #   https://cloud.google.com/compute/docs/instances/preemptible#preemption-process  # pylint: disable=line-too-long
+        if handle.launched_resources.use_spot and not supports_stop_spot:
            # Disable spot instances to be stopped.
-            # TODO(suquark): enable GCP+spot to be stopped in the future.
            raise exceptions.NotSupportedError(
                f'{colorama.Fore.YELLOW}Stopping cluster '
                f'{cluster_name!r}... skipped.{colorama.Style.RESET_ALL}\n'


Since check_features_are_supported() raises an error I found it harder to read. For example, if a cloud doesn't support stopping spot and here the cluster is an on-demand one, we do not want to perform a check on STOP_SPOT_INSTANCE or raise.

sky/core.py

Michaelvll

Thank you for the fix @concretevitamin! The code looks mostly good to me, except for when we raise an error for the STOP_SPOT_INSTANCE being unsupported.

sky/backends/cloud_vm_ray_backend.py

Michaelvll · 2023-12-27T04:57:33Z

@concretevitamin I refactored the handling for checking supported features. Please take a look : )

concretevitamin

Thanks @Michaelvll for the refactoring! Failover works correctly now.

I pushed some minor changes in messages. PTAL.

sky/clouds/cloud.py

concretevitamin · 2023-12-27T06:39:47Z

sky/clouds/gcp.py

@@ -809,8 +817,6 @@ def need_cleanup_after_preemption(self,
        # you must delete it and create a new one ..."
        # See: https://cloud.google.com/tpu/docs/preemptible#tpu-vm

-        # pylint: disable=import-outside-toplevel


Q: Was there a reason this was imported inlined?

I suppose it was for avoiding circular import, but since we have updated a lot of places, it seems fine to import it at the top level

sky/core.py

…spot

Michaelvll

LGTM. :)

GCP: allow stop/autostop for spot VMs.

dd76f04

Michaelvll reviewed Dec 19, 2023

View reviewed changes

concretevitamin added 2 commits December 18, 2023 21:11

CloudImplementationFeatures.STOP_SPOT_INSTANCE

64a4f9b

update core.py

6eb7ed8

concretevitamin requested a review from Michaelvll December 19, 2023 05:18

Michaelvll approved these changes Dec 22, 2023

View reviewed changes

changes

2466952

concretevitamin commented Dec 22, 2023

View reviewed changes

concretevitamin requested a review from Michaelvll December 22, 2023 06:07

format

43613a5

Michaelvll reviewed Dec 24, 2023

View reviewed changes

sky/backends/cloud_vm_ray_backend.py Outdated Show resolved Hide resolved

Michaelvll added 4 commits December 27, 2023 04:00

refactoring for feature supported check

390ad73

Merge branch 'master' of github.com:skypilot-org/skypilot into gcp-spot

a6bdf9b

remove special handling for TPU VM pod

3975109

format

11ec2b7

Message

e247a75

concretevitamin commented Dec 27, 2023

View reviewed changes

Michaelvll added 3 commits December 28, 2023 13:11

add docstr

1688099

Merge branch 'gcp-spot' of github.com:skypilot-org/skypilot into gcp-…

1c570d3

…spot

Merge branch 'master' of github.com:skypilot-org/skypilot into gcp-spot

5ff7c94

Michaelvll force-pushed the gcp-spot branch from 21aab05 to 5ff7c94 Compare December 28, 2023 13:17

Michaelvll approved these changes Dec 28, 2023

View reviewed changes

concretevitamin merged commit 20013e5 into master Dec 28, 2023
38 checks passed

concretevitamin deleted the gcp-spot branch December 28, 2023 17:38

concretevitamin mentioned this pull request Jan 5, 2024

Consider allowing sky stop on GCP spot clusters #2837

Closed

Michaelvll mentioned this pull request Feb 17, 2024

Investigate making unmanaged spot instances (auto-)stoppable & resumable #1448

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GCP: allow stop/autostop for spot VMs. #2877

GCP: allow stop/autostop for spot VMs. #2877

concretevitamin commented Dec 16, 2023

Michaelvll left a comment

Michaelvll Dec 19, 2023

concretevitamin Dec 19, 2023

Michaelvll Dec 19, 2023

concretevitamin Dec 19, 2023

Michaelvll left a comment

Michaelvll Dec 22, 2023

concretevitamin Dec 22, 2023

concretevitamin left a comment

concretevitamin Dec 22, 2023

Michaelvll left a comment

Michaelvll commented Dec 27, 2023

concretevitamin left a comment

concretevitamin Dec 27, 2023

Michaelvll Dec 28, 2023

Michaelvll left a comment

		if handle.launched_resources.use_spot and not cloud.is_same_cloud(
		clouds.GCP()):

		elif (handle.launched_resources.use_spot and not down and not is_cancel and
		not handle.launched_resources.cloud.is_same_cloud(clouds.GCP())):

GCP: allow stop/autostop for spot VMs. #2877

GCP: allow stop/autostop for spot VMs. #2877

Conversation

concretevitamin commented Dec 16, 2023

Michaelvll left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Michaelvll left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

concretevitamin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Michaelvll left a comment

Choose a reason for hiding this comment

Michaelvll commented Dec 27, 2023

concretevitamin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Michaelvll left a comment

Choose a reason for hiding this comment