Repository: skypilot-org/skypilot
Pull request: Clean up generated config and scripts on cluster termination
Closes #264 

## Tested
```
sky tpunode
sky down sky-tpunode-gautam # cleanup
sky cpunode
sky stop sky-cpunode-gautam
sky start sky-cpunode-gautam
sky down sky-cpunode-gautam # clean up
```

Problem statement:
Placement and cleanup of generated cluster configs (`config/user/`)
Issue: A generated cluster config should be removed on `sky down` / `backend.teardown(terminate=True)`

To discuss: `<sky package dir>/config/user/` is currently used; but should probably (?) be changed to, say, `~/.sky/...`.  
  - Pro: it seems weird to have a pip package dir growing in size.  
  - Con: we may not want users to interact with these configs by hand.  Even as a developer, I haven't interacted with them manually for a while.

Hints from issue discussion:
I think it makes sense to move to `~/.sky/...` since all our other user state lives there. We also don't really want users interacting with the `state.db` and `jobs.db` stored there (even though they could), so not sure if putting things in `.sky` necessarily encourages user interaction.

I would encourage the move to `.sky`, doesn't really make sense to store user-associated state within the package tree.
This should be reopened, because cleanups on "sky down" are not included in
that PR.

On Fri, Feb 11, 2022 at 1:38 PM Gautam Mittal ***@***.***>
wrote:

> Closed #264
> <https://github.com/concretevitamin/sky-experiments/issues/264> via #278
> <https://github.com/concretevitamin/sky-experiments/pull/278>.
>
> —
> Reply to this email directly, view it on GitHub
> <https://github.com/concretevitamin/sky-experiments/issues/264#event-6048473086>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AAEQWHUJ3FAH7H652CMY6WDU2SOHBANCNFSM5NDSLSHQ>
> .
> Triage notifications on the go with GitHub Mobile for iOS
> <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
> or Android
> <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
>
> You are receiving this because you authored the thread.Message ID:
> ***@***.***
> com>
>

Commit message: clean up on terminate

Patch to review:
diff --git a/prototype/sky/backends/backend_utils.py b/prototype/sky/backends/backend_utils.py
index fd798cd4114..10e389d5b5b 100644
--- a/prototype/sky/backends/backend_utils.py
+++ b/prototype/sky/backends/backend_utils.py
@@ -47,12 +47,6 @@
 run_with_log = log_lib.run_with_log
 
 
-def get_rel_path(path: str) -> str:
-    cwd = os.getcwd()
-    common = os.path.commonpath([path, cwd])
-    return os.path.relpath(path, common)
-
-
 def _fill_template(template_path: str,
                    variables: Dict,
                    output_path: Optional[str] = None) -> str:
diff --git a/prototype/sky/backends/cloud_vm_ray_backend.py b/prototype/sky/backends/cloud_vm_ray_backend.py
index e891a452d0c..6364e091bba 100644
--- a/prototype/sky/backends/cloud_vm_ray_backend.py
+++ b/prototype/sky/backends/cloud_vm_ray_backend.py
@@ -650,6 +650,7 @@ def _retry_region_zones(self, task: Task, to_provision: Resources,
                 launched_nodes=num_nodes,
                 # OK for this to be shown in CLI as status == INIT.
                 launched_resources=to_provision,
+                tpu_create_script=config_dict.get('tpu-create-script'),
                 tpu_delete_script=config_dict.get('tpu-delete-script'))
             global_user_state.add_or_update_cluster(cluster_name,
                                                     cluster_handle=handle,
@@ -960,12 +961,14 @@ def __init__(self,
                      head_ip: Optional[str] = None,
                      launched_nodes: Optional[int] = None,
                      launched_resources: Optional[Resources] = None,
+                     tpu_create_script: Optional[str] = None,
                      tpu_delete_script: Optional[str] = None) -> None:
             self.cluster_name = cluster_name
             self.cluster_yaml = cluster_yaml
             self.head_ip = head_ip
             self.launched_nodes = launched_nodes
             self.launched_resources = launched_resources
+            self.tpu_create_script = tpu_create_script
             self.tpu_delete_script = tpu_delete_script
 
         def __repr__(self):
@@ -973,9 +976,10 @@ def __repr__(self):
                     f'\n\tcluster_name={self.cluster_name},'
                     f'\n\thead_ip={self.head_ip},'
                     '\n\tcluster_yaml='
-                    f'{backend_utils.get_rel_path(self.cluster_yaml)}, '
+                    f'{self.cluster_yaml}, '
                     f'\n\tlaunched_resources={self.launched_nodes}x '
                     f'{self.launched_resources}, '
+                    f'\n\ttpu_create_script={self.tpu_create_script}, '
                     f'\n\ttpu_delete_script={self.tpu_delete_script})')
 
         def get_cluster_name(self):
@@ -1111,6 +1115,7 @@ def provision(self,
             launched_nodes=launched_nodes,
             launched_resources=provisioned_resources,
             # TPU.
+            tpu_create_script=config_dict.get('tpu-create-script'),
             tpu_delete_script=config_dict.get('tpu-delete-script'))
         global_user_state.add_or_update_cluster(cluster_name,
                                                 handle,
@@ -1547,6 +1552,19 @@ def teardown(self, handle: ResourceHandle, terminate: bool) -> None:
         auth_config = backend_utils.read_yaml(handle.cluster_yaml)['auth']
         _remove_cluster_from_ssh_config(handle.head_ip, auth_config)
         name = global_user_state.get_cluster_name_from_handle(handle)
+
+        if terminate:
+            # Clean up generated config
+            # No try-except is needed since Ray will fail to teardown the
+            # cluster if the cluster_yaml is missing.
+            os.remove(handle.cluster_yaml)
+
+            # Clean up TPU creation/deletion scripts
+            if handle.tpu_delete_script is not None:
+                assert handle.tpu_create_script is not None
+                os.remove(handle.tpu_create_script)
+                os.remove(handle.tpu_delete_script)
+
         global_user_state.remove_cluster(name, terminate=terminate)
 
     def _get_node_ips(self,


Code review comments:
diff_hunk: @@ -1547,6 +1552,19 @@ def teardown(self, handle: ResourceHandle, terminate: bool) -> None:
         auth_config = backend_utils.read_yaml(handle.cluster_yaml)['auth']
         _remove_cluster_from_ssh_config(handle.head_ip, auth_config)
         name = global_user_state.get_cluster_name_from_handle(handle)
+
+        if terminate:
+            # Clean up generated config
+            # No try-except is needed since Ray will fail to teardown the
+            # cluster if the cluster_yaml is missing.
+            os.remove(handle.cluster_yaml)
+
+            # Clean up TPU creation/deletion scripts
+            if handle.tpu_delete_script is not None:
+                assert handle.tpu_create_script is not None
+                os.remove(handle.tpu_create_script)
+                os.remove(handle.tpu_delete_script)
+
         global_user_state.remove_cluster(name, terminate=terminate)
path: prototype/sky/backends/cloud_vm_ray_backend.py
text: @user1:
I'd put this line next to 1554.  They are logically together.