[GCP] Fix `--disk-size` for Custom Machine Images #2718

jackyk02 · 2023-10-18T04:44:20Z

Issue Addressed: #2488

As reported in the issue, it's clear that when launching a GCP cluster with a machine image originating from a 150 GB disk, specifying the --disk-size 200 parameter results in an instance retaining the original 150 GB size.

Based on GCP's documentation on resizing persistent disks, when we create a custom Linux image, we have to manually increase the size of the boot and non-boot disks. To achieve this, I used the Compute Engine API to modify disk sizes within our custom GCP node provider. Comments have been added to the code within the commits to provide additional context into the changes made. Please feel free to comment if further modifications are required.

Custom Image Test:

Launch the custom image with a specified disk size:
sky launch -c custom-image --cloud gcp --image-id [image id path] --disk-size 200
sky launch -c custom-image --cloud gcp --image-id [image id path] --disk-size 150
Confirmed the disk size has been increased to 200GB from the original 150GB for the first test, and ensured that users are able to create an instance with the image's original size.

Multi-Node Test:

Launch 8 nodes with a specified disk size:
sky launch -c my-cluster --num-nodes 8 --image-id [image id path] --cloud gcp --disk-size 200
Verified the disk of each node has been resized to 200GB.

TPU Test:

Launch the custom image with a specified TPU accelerator and disk size:
sky launch tpu_node.yaml -c custom-image --cloud gcp --image-id [image id path] --disk-size 200, where the YAML file is provided below:
```
resources:
  instance_type: n1-highmem-8
  accelerators: tpu-v2-8
  accelerator_args:
    tpu_vm: False
```
Verified that the disk has been resized to 200GB for TPU node.

Note: TPU VMs include a 100GB boot disk that cannot be resized. Users have to add a Persistent Disk to expand their local disk capacity according to the documentation. This requirement may be associated with a feature request at: #2387

cblmemo

Thanks for contributing to SkyPilot! This is exciting ; ) Left a suggestion 🙂
Just want to make sure, from your PR description it seems like the resize will happen only after a restart? Is that true? If this is the case, shall we add some CLI hints to notify the user?

sky/skylet/providers/gcp/node.py

…mages only, Address Formatting Issues

cblmemo

Thanks for the prompt fix! Left several comments 😉

docs/source/cloud-setup/cloud-permissions/gcp.rst

sky/skylet/providers/gcp/node.py

docs/source/cloud-setup/cloud-permissions/gcp.rst

cblmemo

Thanks for the explanation! Left several comments 👀

sky/skylet/providers/gcp/node.py

jackyk02 · 2023-11-06T23:00:13Z

Every remaining issue we talked about in the meeting should have been resolved. Please let me know if any additional clarification is needed.

cblmemo · 2023-11-06T23:02:49Z

Every remaining issue we talked about in the meeting should have been resolved. Please let me know if any additional clarification is needed.

Could you provide all the tests you have done? I don't see any TPUVM test in your PR description 🤔

jackyk02 · 2023-11-07T00:37:15Z

Could you provide all the tests you have done? I don't see any TPUVM test in your PR description 🤔

Updated the PR descriptions to include additional tests for GCPTPUNode. As the TPU VMs issue is unrelated to custom machine images (refer to the above description), it would be better to address it in a separate PR. Additionally, another issue discussed in our recent meeting is documented at: #2760.

cblmemo · 2023-11-07T00:59:36Z

Could you provide all the tests you have done? I don't see any TPUVM test in your PR description 🤔

Updated the PR descriptions to include additional tests for GCPTPUNode. As the TPU VMs issue is unrelated to custom machine images (refer to the above description), it would be better to address it in a separate PR. Additionally, another issue discussed in our recent meeting is documented at: #2760.

If this is the case, what if we specify a custom (machine) image with a size that is more than 100GB to a TPU VM? Will it directly fail? If so, we should remove related code in the GCPTPU class since it will never get executed; also, we should check in resources._try_validate_image_id.

jackyk02 · 2023-11-07T02:11:08Z

If this is the case, what if we specify a custom (machine) image with a size that is more than 100GB to a TPU VM? Will it directly fail?

Thanks for the prompt reply! No, it will not fail directly. This issue isn't specifically tied to the custom image itself or #2488. Even when launching a TPU VM with sky launch tpu_vm.yaml -c mycluster --cloud gcp --disk-size 300, the resulting VM is still initialized with a disk size of 100 GB. Users have to add a persistent disk to expand the capacity.

Here is the output of the df -h on the created TPU VM:

Filesystem      Size  Used Avail Use% Mounted on
/dev/root        97G   11G   87G  11% /

cblmemo · 2023-11-07T04:37:31Z

If this is the case, what if we specify a custom (machine) image with a size that is more than 100GB to a TPU VM? Will it directly fail?

Thanks for the prompt reply! No, it will not fail directly. This issue isn't specifically tied to the custom image itself or #2488. Even when launching a TPU VM with sky launch tpu_vm.yaml -c mycluster --cloud gcp --disk-size 300, the resulting VM is still initialized with a disk size of 100 GB. Users have to add a persistent disk to expand the capacity.

Here is the output of the df -h on the created TPU VM:
Filesystem      Size  Used Avail Use% Mounted on
/dev/root        97G   11G   87G  11% /

Then at least we should remove related code in the GCPTPU class since it will never get executed? Also, if we doesn't plan to address the problem in this PR, can we file an issue to keep track of this problem?

jackyk02 · 2023-11-07T09:34:59Z

Then at least we should remove related code in the GCPTPU class since it will never get executed? Also, if we doesn't plan to address the problem in this PR, can we file an issue to keep track of this problem?

Yes, that's a good point. Regarding the issue tracking, I've already filed it under issue #2387.

cblmemo

Almost done..! Left several nits
Also, it would be great to run the smoke test to make sure the PR doesn't break anything ; )

sky/skylet/providers/gcp/node.py

sky/skylet/providers/gcp/node_provider.py

sky/skylet/providers/gcp/node.py

Co-authored-by: Tian Xia <[email protected]>

…ame disk size

jackyk02 · 2023-11-07T21:08:54Z

Thank you very much for your contributions to this PR! Let me know if there are any more changes needed.

sky/resources.py

cblmemo

LGTM except for one nit! It should be good to go ;)

sky/resources.py

Michaelvll

Thanks for the great work @jackyk02! Left a comment. ; )

Michaelvll · 2023-11-13T20:05:45Z

sky/resources.py

@@ -834,13 +834,15 @@ def _try_validate_image_id(self) -> None:
            # Check the image exists and get the image size.
            # It will raise ValueError if the image does not exist.
            image_size = self.cloud.get_image_size(image_id, region)
-            if image_size > self.disk_size:
+            if image_size >= self.disk_size:


= seems unexpected. Do you mean you cannot create a machine with the same size as the image? I think we did try creating image with 256GB and launch a VM from the image with the same disk size?

Users should be able to create instances with the same size as the image in the latest commit. I’ve updated the PR description to include additional tests for it.

Michaelvll

Thanks for the fix @jackyk02! Just tried this PR:

sky launch -c test-machine-image --cloud gcp "echo hi > hello.txt"
create a machine image for test-machine-image on the dashboard
sky launch -c test-use-machine-image --cloud gcp --image-id projects/xxxx/global/machineIma ges/test-machine-image-resize --disk-size 500

It works well! I am going to merge this PR soon.

Michaelvll · 2023-11-15T20:33:56Z

sky/templates/gcp-ray.yml.j2

@@ -18,7 +18,7 @@ docker:
 {%- endif %}

 provider:
-  # We use a custom node provider for GCP to support instance stop and reuse.
+  # We use a custom node provider for GCP to create, stop and reuse instances.


initial commit for gcp resizing disk

91fb6b6

Michaelvll requested a review from cblmemo October 18, 2023 07:34

cblmemo reviewed Oct 18, 2023

View reviewed changes

sky/skylet/providers/gcp/node.py Outdated Show resolved Hide resolved

disk resizing for new instances

3162784

Michaelvll requested a review from cblmemo October 22, 2023 05:51

jackyk02 and others added 8 commits October 25, 2023 13:14

Merge branch 'skypilot-org:master' into master

2a7474d

Change from compute_v1 to googleapiclient, Disk Resizing for custom i…

92053b7

…mages only, Address Formatting Issues

Address Formatting Issues

5b845a9

Address Formatting Issues

0916d2c

update gcp-ray.yml comments

432fbfc

address reformatting

9c2771f

Create documentation for custom image cloud permissions

b95f717

[GCP] Minimal permissions for custom image

8d75181

cblmemo reviewed Oct 27, 2023

View reviewed changes

docs/source/cloud-setup/cloud-permissions/gcp.rst Show resolved Hide resolved

sky/skylet/providers/gcp/node.py Outdated Show resolved Hide resolved

docs/source/cloud-setup/cloud-permissions/gcp.rst Show resolved Hide resolved

jackyk02 added 2 commits October 27, 2023 20:52

Check exisiting disk size & Move implementation to GCPCompute

6358322

Fix Format

ac79d11

cblmemo reviewed Oct 28, 2023

View reviewed changes

sky/skylet/providers/gcp/node.py Show resolved Hide resolved

sky/skylet/providers/gcp/node.py Outdated Show resolved Hide resolved

sky/skylet/providers/gcp/node.py Outdated Show resolved Hide resolved

sky/skylet/providers/gcp/node.py Outdated Show resolved Hide resolved

jackyk02 added 3 commits October 27, 2023 22:03

Removed Unnecessary Imports & Return type

91234b8

remove unnecessary check and resizing for restart

f480eda

add disk resizing for TPUVMs

30059e0

Add TODO for TPUVM resize

a3e3311

cblmemo reviewed Nov 7, 2023

View reviewed changes

Update reference to github issue for TPUVM

caf9846

Co-authored-by: Tian Xia <[email protected]>

jackyk02 and others added 4 commits November 7, 2023 11:52

Return None for TPU Resize

f0c0699

Co-authored-by: Tian Xia <[email protected]>

shorten comments for TPU function

df4adff

Remove redundant check

f409bb9

Updated try_validate_image_id to raise error when users specify the s…

ae554f7

…ame disk size

cblmemo reviewed Nov 7, 2023

View reviewed changes

sky/resources.py Outdated Show resolved Hide resolved

jackyk02 added 3 commits November 7, 2023 15:10

update try_validate_image_id

3eb0778

update resources.py

b639adb

Fix formatting for resources.py

a193406

cblmemo approved these changes Nov 8, 2023

View reviewed changes

sky/resources.py Outdated Show resolved Hide resolved

jackyk02 added 2 commits November 8, 2023 11:28

Update resources.py

c3f4fe9

Update resources.py

9cce23f

Michaelvll reviewed Nov 13, 2023

View reviewed changes

Allowing users to create instances with the same size as the image

b73f9a9

Michaelvll reviewed Nov 15, 2023

View reviewed changes

Michaelvll merged commit 7d9bcec into skypilot-org:master Nov 15, 2023
19 checks passed

Michaelvll mentioned this pull request Nov 16, 2023

[GCP] Machine image does not work with --disk-size #2488

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[GCP] Fix `--disk-size` for Custom Machine Images #2718

[GCP] Fix `--disk-size` for Custom Machine Images #2718

jackyk02 commented Oct 18, 2023 •

edited

Loading

cblmemo left a comment

cblmemo left a comment

cblmemo left a comment

jackyk02 commented Nov 6, 2023

cblmemo commented Nov 6, 2023

jackyk02 commented Nov 7, 2023

cblmemo commented Nov 7, 2023

jackyk02 commented Nov 7, 2023 •

edited

Loading

cblmemo commented Nov 7, 2023

jackyk02 commented Nov 7, 2023

cblmemo left a comment

jackyk02 commented Nov 7, 2023 •

edited

Loading

cblmemo left a comment

Michaelvll left a comment

Michaelvll Nov 13, 2023

jackyk02 Nov 14, 2023

Michaelvll left a comment •

edited

Loading

Michaelvll Nov 15, 2023

[GCP] Fix --disk-size for Custom Machine Images #2718

[GCP] Fix --disk-size for Custom Machine Images #2718

Conversation

jackyk02 commented Oct 18, 2023 • edited Loading

cblmemo left a comment

Choose a reason for hiding this comment

cblmemo left a comment

Choose a reason for hiding this comment

cblmemo left a comment

Choose a reason for hiding this comment

jackyk02 commented Nov 6, 2023

cblmemo commented Nov 6, 2023

jackyk02 commented Nov 7, 2023

cblmemo commented Nov 7, 2023

jackyk02 commented Nov 7, 2023 • edited Loading

cblmemo commented Nov 7, 2023

jackyk02 commented Nov 7, 2023

cblmemo left a comment

Choose a reason for hiding this comment

jackyk02 commented Nov 7, 2023 • edited Loading

cblmemo left a comment

Choose a reason for hiding this comment

Michaelvll left a comment

Choose a reason for hiding this comment

Michaelvll Nov 13, 2023

Choose a reason for hiding this comment

jackyk02 Nov 14, 2023

Choose a reason for hiding this comment

Michaelvll left a comment • edited Loading

Choose a reason for hiding this comment

Michaelvll Nov 15, 2023

Choose a reason for hiding this comment

[GCP] Fix `--disk-size` for Custom Machine Images #2718

[GCP] Fix `--disk-size` for Custom Machine Images #2718

jackyk02 commented Oct 18, 2023 •

edited

Loading

jackyk02 commented Nov 7, 2023 •

edited

Loading

jackyk02 commented Nov 7, 2023 •

edited

Loading

Michaelvll left a comment •

edited

Loading