[Provisioner] Open ports on AWS & GCP #2210

cblmemo · 2023-07-11T00:39:39Z

This PR resolves #1255 on AWS & GCP.

Interface:

resources:
  cloud: gcp
  ports:
    - 33828
    - 18000-18100

Tested (run the relevant ones):

Code formatting: bash format.sh
Any manual or new tests for this PR (please specify below)
- sky launch --cloud gcp; sky down <cluster-name> and check firewall rules on the GCP console
- sky launch --cloud aws; sky down <cluster-name> and check firewall rules on the AWS console
- Launching HTTP server on 2 node cluster
- Stop & restart cluster
- TPU-related tests: launch the following YAMLs and access http://<head_ip>:33828 in a web browser

name: mnist-tpu-node-port

resources:
  ports:
    - 33828
  instance_type: n1-highmem-8
  accelerators: tpu-v2-8
  accelerator_args:
    runtime_version: 2.7.0

workdir: examples/http_server_with_custom_ports  # For the server.py

run: python3 server.py

name: mnist-tpu-vm-port

resources:
  ports:
    - 33828
  accelerators: tpu-v2-8
  accelerator_args:
    runtime_version: tpu-vm-base
    tpu_vm: True

workdir: examples/http_server_with_custom_ports  # For the server.py

run: python3 server.py

All smoke tests: pytest tests/test_smoke.py
Relevant individual smoke tests: pytest tests/test_smoke.py::test_gcp_http_server_with_custom_ports
Relevant individual smoke tests: pytest tests/test_smoke.py::test_aws_http_server_with_custom_ports
- Somehow all pytest marked with aws will be skipped in my machine, so I manually run the script and it performs as expected.
Backward compatibility tests: bash tests/backward_comaptibility_tests.sh

cblmemo · 2023-07-11T00:44:25Z

API design discussion: Currently I use a section in ~/.sky/config.yaml since the port will open for all VM launched by skypilot, but not VMs launched for a specified task. We might want to discuss on this since configuring ~/.sky/config.yaml is not intuitive and will destroy our one-click solution for a single task.yaml.

concretevitamin · 2023-07-11T00:51:18Z

API design discussion: Currently I use a section in ~/.sky/config.yaml since the port will open for all VM launched by skypilot, but not VMs launched for a specified task. We might want to discuss on this since configuring ~/.sky/config.yaml is not intuitive and will destroy our one-click solution for a single task.yaml.

Agreed that it's not intuitive. If we put ports in task YAML, can we make it work as follows: create or reuse a port-specific security group (AWS concept; for other clouds we need to check) that opens the requested ports, then let this cluster use this specific security group?

cblmemo · 2023-07-11T02:53:11Z

API design discussion: Currently I use a section in ~/.sky/config.yaml since the port will open for all VM launched by skypilot, but not VMs launched for a specified task. We might want to discuss on this since configuring ~/.sky/config.yaml is not intuitive and will destroy our one-click solution for a single task.yaml.

Agreed that it's not intuitive. If we put ports in task YAML, can we make it work as follows: create or reuse a port-specific security group (AWS concept; for other clouds we need to check) that opens the requested ports, then let this cluster use this specific security group?

In my vague memory, the overhead of creating a vpc network in GCP is not neglectable; gonna do some benchmark later.

BTW, the concept of a port-specific security group might become complex when considering users might want to open 2 or more ports in one single task. For example, in task1.yaml we have

ports:
  - TCP:10022
  - UDP:10023

and in task2.yaml we have

ports:
  - TCP:10022
  - UDP:10024

IIUC, these two tasks cannot use the same security group and it is likely that none of the tasks could share one security group. Considering that checking whether a security group meets the requirements (and likely none of the existing security groups satisfy the requirements) takes some time (and it is not neglectable), we might want to consider task-specific security groups too.

cblmemo · 2023-07-13T05:57:48Z

Current implementation logic: each cluster have its own 'Security Group' (for GCP, a series of fire wall rules).

For GCP, we create a new firewall rule for each port specified. This rule only applies to the VM inside the current cluster and is deleted on termination of the cluster.

For AWS, we create a security group for each cluster and delete it on termination of the cluster.

sky/provision/aws/instance.py

cblmemo · 2023-07-14T01:04:51Z

UPD:

Now it will raise an error if both ports and cloud that is not GCP, AWS are specified;
Auto failover is enabled: when no cloud is specified, it will only choose from GCP and AWS if port is used;
Post cleanup after failover is handled here

skypilot/sky/backends/cloud_vm_ray_backend.py

Line 1573 in 1501708

CloudVmRayBackend().teardown_no_lock(handle,

…ation

cblmemo · 2023-07-25T22:34:22Z

@Michaelvll The PR is ready for another review 🙂

Michaelvll

Thanks for the update of this PR @cblmemo! The code looks mostly good to me, but an issue with TPUVM.

Michaelvll · 2023-07-28T07:20:20Z

sky/provision/gcp/instance.py

    # We don't wait for the instances to be terminated, as it can take a long
    # time (same as what we did in ray's node_provider).


On GCP, does it need to wait the instance to be actually terminated before cleaning up the ports or it is fine to clean up the ports during the instance in the status of TERMINATING?

Here, we don't wait the instance to be actually terminated (deleted).

The firewall rules have no dependency on the VM; I think it is safe to clean up the ports during the instance when terminating.

Michaelvll · 2023-07-28T07:23:37Z

sky/provision/aws/instance.py

+    sg_name = (f'sky-sg-{common_utils.user_and_hostname_hash()}'
+               f'-{common_utils.hash_cluster_name(cluster_name)}')


How about the following for better readability from the console?

Suggested change

sg_name = (f'sky-sg-{common_utils.user_and_hostname_hash()}'

f'-{common_utils.hash_cluster_name(cluster_name)}')

sg_name = (f'sky-sg-{common_utils.user_and_hostname_hash()}'

f'-{cluster_name[:10]}-{common_utils.hash_cluster_name(cluster_name)}')

The 10 may need to be changed based on the total length of the string.

In fact, the common_utils.hash_cluster_name function already implements this: it will check cluster name length and truncate and append a hash if too long, otherwise we just return the original cluster name. The function name is a little bit confusing 🤧 I'll consider other name

Now renamed to truncate_and_hash_cluster_name. If you have another better name, please let me know 👀

sky/skylet/providers/gcp/config.py

Michaelvll · 2023-07-28T19:27:12Z

sky/skylet/providers/gcp/config.py

+        if "tags" not in node_config:
+            node_config["tags"] = {"items": []}
+        # Add cluster name to the tags so that firewall rules will apply to
+        # the created VM.
+        node_config["tags"]["items"].append(config["cluster_name"])


This seems not working with the TPUVM. To reproduce:

sky launch -c test-tpuvm examples/tpu/tpuvm_mnist.yaml --use-spot -i 0 --down

The following error occurs even the user does not specify the ports.

Got googleapiclient.errors.HttpError: <HttpError 400 when requesting https://tpu.googleapis.com/v2alpha1/projects/skypilot-375900/locations/us-central1-c/nodes?nodeId=ray-test-tpuvm-head-480658f3-tpu&alt=json returned "Invalid value at 'node' (tags), Starting an object on a scalar field". Details: "[{'@type': 'type.googleapis.com/google.rpc.BadRequest', 'fieldViolations': [{'field': 'node', 'description': "Invalid value at 'node' (tags), Starting an object on a scalar field"}]}]">

It would be nice the run the smoke test as well. : )

Resolved. Plus, this version that disabled TPU with ports has passed all smoke tests. 🫡

Hi,
When I use this feature to down a cluster got an error as below:

Traceback (most recent call last):
File "/usr/local/bin/sky", line 8, in
sys.exit(cli())
File "/usr/local/lib/python3.8/dist-packages/click/core.py", line 1128, in call
return self.main(*args, **kwargs)
File "/usr/local/lib/python3.8/dist-packages/click/core.py", line 1053, in main
rv = self.invoke(ctx)
File "/usr/local/lib/python3.8/dist-packages/sky/utils/common_utils.py", line 239, in _record
return f(*args, **kwargs)
File "/usr/local/lib/python3.8/dist-packages/sky/cli.py", line 1110, in invoke
return super().invoke(ctx)
File "/usr/local/lib/python3.8/dist-packages/click/core.py", line 1659, in invoke
return _process_result(sub_ctx.command.invoke(sub_ctx))
File "/usr/local/lib/python3.8/dist-packages/click/core.py", line 1395, in invoke
return ctx.invoke(self.callback, **ctx.params)
File "/usr/local/lib/python3.8/dist-packages/click/core.py", line 754, in invoke
return __callback(*args, **kwargs)
File "/usr/local/lib/python3.8/dist-packages/sky/utils/common_utils.py", line 260, in _record
return f(*args, **kwargs)
File "/usr/local/lib/python3.8/dist-packages/sky/cli.py", line 2500, in down
_down_or_stop_clusters(clusters,
File "/usr/local/lib/python3.8/dist-packages/sky/cli.py", line 2755, in _down_or_stop_clusters
subprocess_utils.run_in_parallel(_down_or_stop, clusters)
File "/usr/local/lib/python3.8/dist-packages/sky/utils/subprocess_utils.py", line 54, in run_in_parallel
return list(p.imap(func, args))
File "/usr/lib/python3.8/multiprocessing/pool.py", line 868, in next
raise value
File "/usr/lib/python3.8/multiprocessing/pool.py", line 125, in worker
result = (True, func(*args, **kwds))
File "/usr/local/lib/python3.8/dist-packages/sky/cli.py", line 2727, in _down_or_stop
core.down(name, purge=purge)
File "/usr/local/lib/python3.8/dist-packages/sky/utils/common_utils.py", line 260, in _record
return f(*args, **kwargs)
File "/usr/local/lib/python3.8/dist-packages/sky/core.py", line 365, in down
backend.teardown(handle, terminate=True, purge=purge)
File "/usr/local/lib/python3.8/dist-packages/sky/utils/common_utils.py", line 260, in _record
return f(*args, **kwargs)
File "/usr/local/lib/python3.8/dist-packages/sky/utils/common_utils.py", line 239, in _record
return f(*args, **kwargs)
File "/usr/local/lib/python3.8/dist-packages/sky/backends/backend.py", line 110, in teardown
self._teardown(handle, terminate, purge)
File "/usr/local/lib/python3.8/dist-packages/sky/backends/cloud_vm_ray_backend.py", line 3122, in _teardown
self.teardown_no_lock(
File "/usr/local/lib/python3.8/dist-packages/sky/backends/cloud_vm_ray_backend.py", line 3404, in teardown_no_lock
self.post_teardown_cleanup(handle, terminate, purge)
File "/usr/local/lib/python3.8/dist-packages/sky/backends/cloud_vm_ray_backend.py", line 3622, in post_teardown_cleanup
provision_api.cleanup_ports(repr(cloud), handle.cluster_name,
File "/usr/local/lib/python3.8/dist-packages/sky/provision/init.py", line 29, in _wrapper
return impl(*args, **kwargs)
File "/usr/local/lib/python3.8/dist-packages/sky/provision/aws/instance.py", line 121, in cleanup_ports
list(sgs)[0].delete()
File "/usr/local/lib/python3.8/dist-packages/boto3/resources/factory.py", line 580, in do_action
response = action(self, *args, **kwargs)
File "/usr/local/lib/python3.8/dist-packages/boto3/resources/action.py", line 88, in call
response = getattr(parent.meta.client, operation_name)(*args, **params)
File "/usr/local/lib/python3.8/dist-packages/botocore/client.py", line 534, in _api_call
return self._make_api_call(operation_name, kwargs)
File "/usr/local/lib/python3.8/dist-packages/botocore/client.py", line 976, in _make_api_call
raise error_class(parsed_response, operation_name)
botocore.exceptions.ClientError: An error occurred (DependencyViolation) when calling the DeleteSecurityGroup operation: resource sg-01d0e8429fdc77751 has a dependent object

Seems cleanup security group making some errors?

Thanks.

Which commit are you at? This is a bug that should have been fixed, and I cannot reproduce this error in the latest commit.
And could you provide some information? Such as the provisioning log, teardown log, task.yaml you used and the security group name shown in the cloud console?

Thanks for the reply. The environment has been destroyed. I found that this change was not committed to master. So I used this repo: https://github.com/skypilot-org/skypilot/tree/f3ffaaf25744a8fe0926e89dcc0a8ee69febd690. I am not sure if it is the suitable.

For the corrupted environment, maybe you could use sky down -p <cluster_name> to force teardown the cluster and manually cleanup resources on the cloud console; Then you could use the stable version on the current master branch.

So you mean stable version on master is support open port feature now?

Yes, we just merged this PR. By checkout to the master's HEAD and installing from source, you could use the open port features now.

Oh, that's very nice. Thanks a lot for the great job that you have done.

Michaelvll

Thanks for adding this feature @cblmemo! Many users have requested it.

(Maybe another PR) We need to add that in our document for minimal permission for AWS/GCP, we probably should add another item about the permission for opening the ports. https://skypilot.readthedocs.io/en/latest/cloud-setup/cloud-permissions/gcp.html#user

I just tried to launch juypter lab on VMs on AWS, GCP, and also TPU node / TPU VM. All works well.

Here is my testing yaml, I guess we can have a jupyter example in the future.

resources:
  # accelerators: tpu-v2-8
  # accelerator_args:
  #   runtime_version: 2.12.1
  # use_spot: true
  ports:
    - 29324


run: |
  pip install jupyter
  myip=`curl ifconfig.me`
  echo $myip
  jupyter lab --port 29324 --no-browser --ip=0.0.0.0

sky/skylet/providers/gcp/config.py

Michaelvll · 2023-08-01T20:27:57Z

examples/http_server_with_custom_ports/task.yaml

@@ -0,0 +1,7 @@
+resources:


This example is nice. It would be great to have an end-to-end example for jupyter notebook in a next PR. : )

Michaelvll · 2023-08-01T20:29:37Z

examples/http_server_with_custom_ports/task.yaml

+
+workdir: ./examples/http_server_with_custom_ports
+
+run: python3 server.py


Is it possible to provide an env var in the run and setup section that the user can do something like the following:

run: | # python serve.py --port $SKYPILOT_PORT jupyter notebook --port $SKYPILOT_PORT

What is the expected behavior when multiple ports are specified? Undefined?

Oh nvm. I guess I could reference what SKYPILOT_NODE_IPS do: multiple lines and each line of them represent one item in our task.yaml

Co-authored-by: Zhanghao Wu <[email protected]>

* successfully launched on GCP * update image with rsync installed && fix name error in conda command * support dockerimage on task yaml && reformat code * recoup sudo in setup commands * reformat code * support direct ssh to docker && fix mesg warning && reformat * fix gcp port issue * aws successfully launched * fix port range * successfullt launched on Azure * remove unused exception class * reformat setup command to azure.py in plain text for readbility * fix job queue cannot cancel * reformat code * switch to port 22 for docker ssh * move docker image to resources: image_id: docker:<image> and change to an optional function * minor fix for ray yaml j2 * fix error when image_id is a dict * support muli-node * mode docker user setup after handle is created * support images without rsync * add aws && azure support * remove redundant pip3 install * remove error merging * move docker image to resources * minor fixes * format * adjust extrack docker image * change back to port 22 for host and port 10022 for docker, passed stop-start recovery test * add ulimit and gcp 10022 enable outside oslogin * fix gcp suthentication & move ssh authorized keys setup to run_init * fix len(image_id) when image_id is None * use docker stop & start to recover * temporary remove conflict * add back docker user * fix wrong username in add job * use ssh jump server to access docker * remove inbloud rules of 10022 * update comment * now ssh into docker * ux: raise rather than assert * move some setup commands to SkyDockerCommandRunner * monir fix * format * fix multinode ssh config * Update sky/backends/backend_utils.py Co-authored-by: Zhanghao Wu <[email protected]> * Update sky/backends/backend_utils.py Co-authored-by: Zhanghao Wu <[email protected]> * Update sky/backends/backend_utils.py Co-authored-by: Zhanghao Wu <[email protected]> * Update sky/clouds/azure.py Co-authored-by: Zhanghao Wu <[email protected]> * Update sky/skylet/providers/gcp/config.py Co-authored-by: Zhanghao Wu <[email protected]> * minor fixes * format * move get_docker_user to backend_utils.py * move two constants to skylet and move resources vars to make deployment vars * move SkyDockerCommandRunner to skylet/providers * format * support proxy command with docker * Update sky/backends/backend_utils.py Co-authored-by: Zhanghao Wu <[email protected]> * Update sky/clouds/aws.py Co-authored-by: Zhanghao Wu <[email protected]> * add comment * quote docker ssh proxy command * explicit checking for Optional object * fix credentials * remove -m in bash script * fix job queue owner * fix job owner and username * add job queue smoke test * add test_docker_preinstalled_package * fix restart error * nit: code style for proxy command * update CloudVmRayResourceHandle version * disable unattended-upgrade with cloud-init * fix UnboundLocalError * fix variable shadow * move checking for targetTags to #2210 * rename * restore some deprecated changes * format * disable tpu with docker * fix acc inexisting problem * add progress bar to docker image pulling * Apply suggestions from code review Co-authored-by: Zhanghao Wu <[email protected]> * Apply suggestions from code review * Try cloud init without base64 encode Co-authored-by: Zhanghao Wu <[email protected]> * Revert "Try cloud init without base64 encode" This reverts commit 418c912. * add comment in cmd runner * add failover for clous that not support docker yet * temporary remove check of docker image * add docker to resources.get_required_cloud_features * format * install pip in run_init * format * disable ssh control when docker is used * add docker user to resource handle's repr * stash some changes for easier merge * minor * add back previously stashed function * disable docker with proxy for now * change constants.DEFAULT_DOCKER_PORT to int * rename NATIVE_DOCKER_SUPPORT to DOCKER_IMAGE * add todo for only support debian-based images * add check for proxy command * fix docker_config={} * upd docker test * upd docker test * move proxy command check to cloud.check_features_are_supported --------- Co-authored-by: Zhanghao Wu <[email protected]>

finished AWS & GCP, test passed on GCP

18bbc13

cblmemo added 4 commits July 11, 2023 15:43

GCP now create rules with cluster-specified tags

07f0e9e

Creating new SGs on AWS and configuring user specified rules

3718934

new SG name

3934bbf

move to resources & allow tcp only

b3bcdc7

cblmemo added a commit to cblmemo/skypilot that referenced this pull request Jul 12, 2023

move checking for targetTags to skypilot-org#2210

ef82d9d

cblmemo added 5 commits July 12, 2023 11:23

disable request of newer ports in task.yaml

e4d5832

lint

e868033

delete firewall rules & aws SGs after teardown

87e3dcb

add smoke test & cluster name length limit

9bbdf30

format

840f93a

cblmemo mentioned this pull request Jul 13, 2023

Python interface for getting cluster metadata (e.g., IP) #1779

Closed

add doc

7f0c439

concretevitamin reviewed Jul 13, 2023

View reviewed changes

sky/provision/aws/instance.py Show resolved Hide resolved

cblmemo added 2 commits July 13, 2023 17:35

add error when not using aws & gcp

d7f2760

add failover

dae364a

remove redundant length checking

d8735ea

cblmemo mentioned this pull request Jul 15, 2023

Refactor failover when resources have some unsupported features on certain cloud #2245

Merged

5 tasks

cblmemo added 7 commits July 15, 2023 18:55

temporary remove failover and wait for skypilot-org#2245

28bbd79

Merge remote-tracking branch 'upstream/master' into open-ports

a7cbd2f

move to new provisioner api

0c892de

Merge remote-tracking branch 'upstream/master' into open-ports

8ae6144

add unsupported feature

726792f

add ports to resources.get_required_cloud_features

37993bc

use CloudImplementationFeatures to validate port

d64b9d5

remove redundant error message since check is done in resources valid…

e78ab4c

…ation

cblmemo force-pushed the open-ports branch from cad3949 to e78ab4c Compare July 25, 2023 19:31

cblmemo added 6 commits July 25, 2023 13:57

support port range

4492df4

refactor GCP implementation

7cba540

remove unused function

a1318c2

rename gcp rule name to avoid dependency and name too long

d44b0ff

remove redundant tags

a9559d7

remove redundant silent argument

c9edcdd

change cluster name hash to truncate + hash

9cbf217

concretevitamin requested a review from Michaelvll July 27, 2023 21:27

cblmemo added 2 commits July 27, 2023 15:02

merge to master

6e708ac

format

9e7eec7

Michaelvll reviewed Jul 28, 2023

View reviewed changes

cblmemo added 8 commits July 28, 2023 13:32

remove create firewall wait function

01309c7

rename hash_cluster_name to truncate_and_hash_cluster_name

9c467b3

fix error when launching TPU node

02a9fb5

disable TPU with ports for now

b25ced4

format

ca8d40b

enable tpu nodes

438d4c4

enable tpu vm

f017c49

raise error instead of assert

b4e89b2

Michaelvll approved these changes Aug 1, 2023

View reviewed changes

Update sky/skylet/providers/gcp/config.py

b9ce954

Co-authored-by: Zhanghao Wu <[email protected]>

cblmemo merged commit fa4dd65 into skypilot-org:master Aug 1, 2023

cblmemo deleted the open-ports branch August 1, 2023 22:58

cblmemo mentioned this pull request Aug 1, 2023

Update minimal permission for open ports #2339

Closed

This was referenced Aug 4, 2023

fix tpu bug #2350

Merged

[Provisioner] enable ports opening on Azure #2373

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Provisioner] Open ports on AWS & GCP #2210

[Provisioner] Open ports on AWS & GCP #2210

cblmemo commented Jul 11, 2023 •

edited

Loading

cblmemo commented Jul 11, 2023

concretevitamin commented Jul 11, 2023

cblmemo commented Jul 11, 2023 •

edited

Loading

cblmemo commented Jul 13, 2023

cblmemo commented Jul 14, 2023

cblmemo commented Jul 25, 2023

Michaelvll left a comment

Michaelvll Jul 28, 2023

cblmemo Jul 28, 2023 •

edited

Loading

Michaelvll Jul 28, 2023

cblmemo Jul 28, 2023

cblmemo Jul 28, 2023

Michaelvll Jul 28, 2023

cblmemo Jul 29, 2023

zhangr1008 Aug 1, 2023

cblmemo Aug 1, 2023

zhangr1008 Aug 2, 2023

cblmemo Aug 2, 2023

zhangr1008 Aug 2, 2023

cblmemo Aug 2, 2023 •

edited

Loading

zhangr1008 Aug 2, 2023

Michaelvll left a comment

Michaelvll Aug 1, 2023

Michaelvll Aug 1, 2023

cblmemo Aug 1, 2023

cblmemo Aug 1, 2023

		# We don't wait for the instances to be terminated, as it can take a long
		# time (same as what we did in ray's node_provider).

		sg_name = (f'sky-sg-{common_utils.user_and_hostname_hash()}'
		f'-{common_utils.hash_cluster_name(cluster_name)}')


		workdir: ./examples/http_server_with_custom_ports

		run: python3 server.py

[Provisioner] Open ports on AWS & GCP #2210

[Provisioner] Open ports on AWS & GCP #2210

Conversation

cblmemo commented Jul 11, 2023 • edited Loading

cblmemo commented Jul 11, 2023

concretevitamin commented Jul 11, 2023

cblmemo commented Jul 11, 2023 • edited Loading

cblmemo commented Jul 13, 2023

cblmemo commented Jul 14, 2023

cblmemo commented Jul 25, 2023

Michaelvll left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cblmemo Jul 28, 2023 • edited Loading

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

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

Choose a reason for hiding this comment

cblmemo Aug 2, 2023 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cblmemo commented Jul 11, 2023 •

edited

Loading

cblmemo commented Jul 11, 2023 •

edited

Loading

cblmemo Jul 28, 2023 •

edited

Loading

cblmemo Aug 2, 2023 •

edited

Loading