Repository: skypilot-org/skypilot
Pull request: Fix Azure resource leak
Unfortunately I cannot reproduce the bug with my laptop. But it looks like the valid fix. @infwinston Could you try this simple fix?

BTW, we are still leaking other resources for all clouds (because of design of Ray autoscaler), e.g., credentials and security configurations. Not sure if there are limits of these resources. Check https://portal.azure.com/#blade/HubsExtension/BrowseResourceGroups for details. The fundamental fix would be deleting the resource group, which turns out incompatible with Ray autoscaler (raise exceptions). Also deleting the resource group is the correct way of cleaning up azure resources.

Closes #426
Closes #508

Problem statement:
[Azure] Is our Azure resouce clean up correct? 
I've started running into this error on Azure EastUS:
```
W 02-23 09:15:54 cloud_vm_ray_backend.py:505]   Exception Details:      (ResourceCountExceedsLimitDueToTemplate) Subscription aa86df77-e703-453e-b2f4-955c3b33e534 has a quota of 100 for resources of type P
ublicIpAddress with sku Basic. Subscription currently has 100 resources and the template contains 1 new resources of the this type which exceeds the quota. Please contact support to increase the quota for
resource type PublicIpAddress
```
Auto-failover provisions me a VM in EastUS2. 

However, I'm concerned we (or ray) are not correctly cleaning up resources (e.g., deleting public IPs when we do `sky down`). Our subscription seems to be littered with public IPs allocated to non-existent VMs.

**Current VMs: 4**
```
(base) romilb@romilbx1yoga:/mnt/d/Softwares/cmder$ az vm list -o table --query "[?location=='eastus'].{Name:name, Location:location}"
Name               Location
-----------------  ----------
Graph              eastus
bao-dev            eastus
bao-experiments-2  eastus
zongheng-ds        eastus
```

**Current IPs allocated: 100**
```
(base) romilb@romilbx1yoga:/mnt/d/Softwares/cmder$ az network public-ip list -o table --query "[?location=='eastus'].{Name:name, Location:location, ProvisioningState:ProvisioningState}"
Name                                         Location
-------------------------------------------  ----------
ray-az-head-915c634a-ip0                     eastus
ray-azure-start-stop-head-0052c3d8-ip0       eastus
ray-azure-start-stop-head-125ac22f-ip0       eastus
ray-azure-start-stop-head-1f5f39ab-ip0       eastus
ray-azure-start-stop-head-23adb3ca-ip0       eastus
ray-azure-start-stop-head-3b0c6ded-ip0       eastus
ray-azure-start-stop-head-48026d47-ip0       eastus
ray-azure-start-stop-head-4a5e3726-ip0       eastus
ray-azure-start-stop-head-536c8a9d-ip0       eastus
ray-azure-start-stop-head-5471bedd-ip0       eastus
ray-azure-start-stop-head-6be5d5f9-ip0       eastus
ray-azure-start-stop-head-6e498a71-ip0       eastus
ray-azure-start-stop-head-7ef54f2a-ip0       eastus
ray-azure-start-stop-head-b403e468-ip0       eastus
ray-azure-start-stop-head-b73a3472-ip0       eastus
ray-azure-start-stop-worker-01f3b57d-ip0     eastus
ray-azure-start-stop-worker-14215426-ip0     eastus
ray-azure-start-stop-worker-81c074b9-ip0     eastus
ray-azure-start-stop-worker-c5066909-ip0     eastus
ray-fm-head-83f77e16-ip0                     eastus
Graph-ip                                     eastus
bao-dev-ip                                   eastus
bao-experiments-2-ip                         eastus
oracle-base-from-image-ip                    eastus
oracle-basePublicIP                          eastus
oracle-east-2-ip                             eastus
oracle-east-3-ip                             eastus
oracle-large-2PublicIP                       eastus
oracle-largePublicIP                         eastus
oracle-next-ip                               eastus
oraclenextip755                              eastus
ray-min-head-04c1e59c-ip0                    eastus
ray-min-head-1e11f303-ip0                    eastus
ray-min-head-480cc5c7-ip0                    eastus
ray-min2-head-30e2fd95-ip0                   eastus
ray-min3-head-be1f60fb-ip0                   eastus
accvm-ip                                     eastus
ray-sky-bd5c-zhwu-head-7843e745-ip0          eastus
ray-sky-cpunode-weichiang-head-1550ecfd-ip0  eastus
ray-sky-cpunode-weichiang-head-2a42d74d-ip0  eastus
ray-sky-cpunode-weichiang-head-3174e945-ip0  eastus
ray-sky-cpunode-weichiang-head-355b080a-ip0  eastus
ray-sky-cpunode-weichiang-head-3f81ad1d-ip0  eastus
ray-sky-cpunode-weichiang-head-ca24a280-ip0  eastus
ray-sky-cpunode-weichiang-head-ee399ff0-ip0  eastus
ray-sky-cpunode-weichiang-head-fe72968e-ip0  eastus
ray-dtf-head-0e2f1b7a-ip0                    eastus
ray-dtf-head-3fb12610-ip0                    eastus
ray-dtf-head-67d757de-ip0                    eastus
ray-dtf-head-728a0e9e-ip0                    eastus
ray-dtf-head-91eb15d3-ip0                    eastus
ray-dtf-head-b104f1ce-ip0                    eastus
ray-dtf-head-c43bfe14-ip0                    eastus
ray-dtf-head-e1c26047-ip0                    eastus
ray-dtf-worker-47562036-ip0                  eastus
ray-dtf-worker-48cdbbbb-ip0                  eastus
ray-dtf-worker-4d2021e8-ip0                  eastus
ray-dtf-worker-685922f0-ip0                  eastus
ray-dtf-worker-686ec6b6-ip0                  eastus
ray-dtf-worker-78c489ed-ip0                  eastus
ray-dtf-worker-7db47a07-ip0                  eastus
ray-dtf-worker-cb462272-ip0                  eastus
ray-multi_echo-head-0ccb4808-ip0             eastus
ray-rn-head-3638e5ca-ip0                     eastus
ray-sky-0b5e-zhwu-head-0ca2e85a-ip0          eastus
ray-sky-229d-zhwu-head-9507cc64-ip0          eastus
ray-sky-2ffb-zhwu-head-9d99aa97-ip0          eastus
ray-sky-2ffb-zhwu-head-d3e56232-ip0          eastus
ray-sky-2ffb-zhwu-head-d8fcfccb-ip0          eastus
ray-sky-3397-zhwu-head-ee839767-ip0          eastus
ray-sky-36df-zhwu-head-76a8b1ec-ip0          eastus
ray-sky-36df-zhwu-worker-2ac2e20f-ip0        eastus
ray-sky-4a4c-zongheng-head-3f205a73-ip0      eastus
ray-sky-5f87-zhwu-head-00a8caa0-ip0          eastus
ray-sky-613f-zhwu-head-02e121b3-ip0          eastus
ray-sky-615b-zhwu-head-2b438725-ip0          eastus
ray-sky-61f3-zhwu-head-1858b245-ip0          eastus
ray-sky-6d4e-zhwu-head-b6c0b2af-ip0          eastus
ray-sky-80b9-zhwu-head-ade626e8-ip0          eastus
ray-sky-91bb-zhwu-head-b6edb25c-ip0          eastus
ray-sky-91bb-zhwu-worker-20b9aca0-ip0        eastus
ray-sky-9c9b-zhwu-head-3cd6cdd8-ip0          eastus
ray-sky-9c9b-zhwu-head-a92207d5-ip0          eastus
ray-sky-ab4b-zhwu-head-6c8cc1f1-ip0          eastus
ray-sky-azure-head-3b6cef59-ip0              eastus
ray-sky-azure-head-52c289e8-ip0              eastus
ray-sky-azure-head-cafcc301-ip0              eastus
ray-sky-azure-head-f18d1113-ip0              eastus
ray-sky-bc78-zhwu-head-98ac3831-ip0          eastus
ray-sky-d4e4-zhwu-head-ca5211cd-ip0          eastus
ray-sky-dbfe-zhwu-head-652c94e2-ip0          eastus
ray-sky-dbfe-zhwu-worker-e9016d6f-ip0        eastus
ray-sky-f214-zhwu-head-c3e818bc-ip0          eastus
ray-sky-f310-zhwu-head-27cf634a-ip0          eastus
ray-sky-fb28-zhwu-head-3ad7a8e7-ip0          eastus
ray-sky-fa1f-zhwu-head-a69f968e-ip0          eastus
oracle-test19-2PublicIP                      eastus
oracle-test19PublicIP                        eastus
zongheng-ds-ip                               eastus
zongheng-vnet-gateway-pip                    eastus
```

Am I missing something or is our azure clean up broken? If this is a transient bug, please feel free to close this issue.

[Bug] Sky down only terminates head node not worker nodes on Azure
It looks like there's a bug with `ray down` on Azure that only terminates head node. Worker nodes are still alive after `sky down`. Not sure if it's fixable on our end.

Reproducible example:
```
sky launch examples/azure_start_stop.yaml -c multinode-azure
sky down multinode-azure
```

Hints from issue discussion:
The code appears to delete NIC, IP, storage on a best effort basis: https://github.com/sky-proj/sky/blob/master/sky/skylet/providers/azure/node_provider.py#L271.  cc @suquark 
We use azure cli to delete the stopped azure machines, where we only use `az vm delete`. Maybe we also need to clean up the IP resources as well?
https://github.com/sky-proj/sky/blob/2d033aab51416a7252ec70fd23f85347203112b6/sky/backends/cloud_vm_ray_backend.py#L1571
I suspect that the NIC deletion option is not being properly set when Azure VMs are created.

 According to https://docs.microsoft.com/en-us/azure/virtual-machines/delete?tabs=cli2, the user should specify the NIC delete option as "**Delete**" at the instance creation time (or update the option via REST API). However, when I checked my VM's option created by `sky gpunode --cloud azure`, I could see that the NIC delete option is "null".

```json
  "networkProfile": {
    "networkApiVersion": null,
    "networkInterfaceConfigurations": null,
    "networkInterfaces": [
      {
        "deleteOption": null,
        "id": "/subscriptions/aa86df77-e703-453e-b2f4-955c3b33e534/resourceGroups/sky-gpunode-ubuntu-northcentralus/providers/Microsoft.Network/networkInterfaces/ray-sky-gpunode-ubuntu-head-3e9524b6-nic-public0",
        "primary": null,
        "resourceGroup": "sky-gpunode-ubuntu-northcentralus"
      }
    ]
  },
```

Can this be a problem from `ray up`? cc @suquark .
@infwinston Unfortunately, I could not reproduce the bug. I will try a fix, but it has to be tested by others. (Thanks Wei-Lin for reproducing the bug on his own laptop!)
Aha, seems we observed the bug at the same time. I will close mine #509 ; )
I also observed the similar issue.

During the launching of the multi-node cluster on azure, I use `sky down` to terminate the cluster. There will sometimes be unterminated instances in the Azure console, though `sky` shows the cluster is terminated.

<img width="1711" alt="image" src="https://user-images.githubusercontent.com/6753189/157388751-d306f376-4413-443c-8e9c-b47afa06cd18.png">
Let me confirm whether it's a Ray or Sky bug first.
Hmm it seems to be a Ray bug.
I try `pip install -U ray[default]==1.10.0 azure-cli azure-core` (because Azure doesn't work on `ray==1.9.2`)
And I `ray up` a multi-node azure yml. I waited until `ray status` showed head/worker node are all set up.
However after `ray down` I can still see the worker node alive on Azure web portal even though ray showed `No nodes remaining.`
```
(ray-azure) weichiang@blaze:~/repos/sky/examples$ ray down /tmp/multi-azure.yml                                  
2022-03-08 23:49:44,585 INFO util.py:282 -- setting max workers for head node type to 0
Loaded cached provider configuration                                                                                       
If you experience issues with the cloud provider, try re-running the command with --no-config-cache.
Destroying cluster. Confirm [y/N]: y                                                                                                               
2022-03-08 23:49:45,838 INFO util.py:282 -- setting max workers for head node type to 0
** Message: 23:49:46.415: Remote error from secret service: org.freedesktop.DBus.Error.ServiceUnknown: The name org.freedesktop.secrets was not provided by any .service files
Fetched IP: 20.185.188.36 
Stopped all 14 Ray processes.                                                                                       
Shared connection to 20.185.188.36 closed.                                       
2022-03-08 23:49:54,878 INFO node_provider.py:158 -- NodeProvider: ray-sky-df42-weichiang-head-045671fc0: Terminating node
2022-03-08 23:50:37,710 INFO node_provider.py:158 -- NodeProvider: ray-sky-df42-weichiang-worker-868ae2dd0: Terminating node
Requested 2 nodes to shut down. [interval=1s]                                                       
0 nodes remaining after 5 second(s).                                                                                                                                          
No nodes remaining.       
```
cc @suquark.  Maybe #501's approaches (adding `.wait()`; then changing to using CLI to delete the whole resource group) can solve this?

By the way, `ray down` uses ray's azure node provider, while ours is the slightly different patched version.

Also: Do these worker nodes live there for a long time (e.g., dozens of minutes?), or is it just because of a delay on Azure's part?
I also have a `ray-sky-6d11-zongheng-worker-75e34b430` orphaned right now, whose head was `sky down`'d Monday 10pm (36 hours ago).  Clicking on the bell button on Azure portal to look for activity log (search for `6d11`), I see:

![](https://i.imgur.com/vGsmkqb.png)

Clicking into each of these, and then click JSON, I only see the head node being initiated a termination request:
```json
...
        "scope": "/subscriptions/aa86df77-e703-453e-b2f4-955c3b33e534/resourceGroups/sky-6d11-zongheng-eastus/providers/Microsoft.Compute/virtualMachines/ray-sky-6d11-zongheng-head-208fba130"
...
```
It seems like we definitely want to delete the whole resource group.
> By the way, `ray down` uses ray's azure node provider, while ours is the slightly different patched version.

Yes, I think I checked both and they all failed. 

> Also: Do these worker nodes live there for a long time (e.g., dozens of minutes?), or is it just because of a delay on Azure's part?

No I think those worker nodes never received termination signals.
Yes https://github.com/sky-proj/sky/pull/501 that relying on `azure-cli` tool to delete VMs may be a better solution. I dug into this problem (it's hard to reproduce as each trial takes 30mins+). I think this bug is either related to Ray's version or Azure CLI version. I'll update more later.

Commit message: fix

Patch to review:
diff --git a/sky/backends/cloud_vm_ray_backend.py b/sky/backends/cloud_vm_ray_backend.py
index e5aa99aeac8..0b9f474e97e 100644
--- a/sky/backends/cloud_vm_ray_backend.py
+++ b/sky/backends/cloud_vm_ray_backend.py
@@ -1693,7 +1693,20 @@ def teardown(self, handle: ResourceHandle, terminate: bool) -> None:
         prev_status = global_user_state.get_status_from_cluster_name(
             handle.cluster_name)
         cluster_name = config['cluster_name']
-        if (terminate and
+        if terminate and isinstance(cloud, clouds.Azure):
+            # Here we handle termination of Azure by ourselves instead of Ray
+            # autoscaler.
+            resource_group = config['provider']['resource_group']
+            terminate_cmd = f'az group delete -y --name {resource_group}'
+            with console.status(f'[bold cyan]Terminating '
+                                f'[green]{cluster_name}'):
+                returncode, stdout, stderr = log_lib.run_with_log(
+                    terminate_cmd,
+                    log_abs_path,
+                    shell=True,
+                    stream_logs=False,
+                    require_outputs=True)
+        elif (terminate and
                 prev_status == global_user_state.ClusterStatus.STOPPED):
             if isinstance(cloud, clouds.AWS):
                 # TODO(zhwu): Room for optimization. We can move these cloud
@@ -1735,19 +1748,6 @@ def teardown(self, handle: ResourceHandle, terminate: bool) -> None:
                         shell=True,
                         stream_logs=False,
                         require_outputs=True)
-            elif isinstance(cloud, clouds.Azure):
-                resource_group = config['provider']['resource_group']
-                query_cmd = (f'az vm list -g {resource_group} '
-                             '--query "[].id" -o tsv')
-                terminate_cmd = f'az vm delete --yes --ids $({query_cmd})'
-                with console.status(f'[bold cyan]Terminating '
-                                    f'[green]{cluster_name}'):
-                    returncode, stdout, stderr = log_lib.run_with_log(
-                        terminate_cmd,
-                        log_abs_path,
-                        shell=True,
-                        stream_logs=False,
-                        require_outputs=True)
             else:
                 raise ValueError(f'Unsupported cloud {cloud} for stopped '
                                  f'cluster {cluster_name!r}.')
diff --git a/sky/skylet/providers/azure/node_provider.py b/sky/skylet/providers/azure/node_provider.py
index fec9404f911..32cce9bef2f 100644
--- a/sky/skylet/providers/azure/node_provider.py
+++ b/sky/skylet/providers/azure/node_provider.py
@@ -56,6 +56,9 @@ class AzureNodeProvider(NodeProvider):
 
     def __init__(self, provider_config, cluster_name):
         NodeProvider.__init__(self, provider_config, cluster_name)
+        # TODO(suquark): This is a temporary patch for creating the non-existing resource group.
+        from sky.skylet.providers.azure.config import _configure_resource_group
+        _configure_resource_group({"provider": provider_config})
         subscription_id = provider_config["subscription_id"]
         self.cache_stopped_nodes = provider_config.get("cache_stopped_nodes", True)
         # AWS provides managed identity for Azure, but it is not setup properly by


Code review comments:
diff_hunk: @@ -56,6 +56,9 @@ class AzureNodeProvider(NodeProvider):
 
     def __init__(self, provider_config, cluster_name):
         NodeProvider.__init__(self, provider_config, cluster_name)
+        # TODO(suquark): This is a temporary patch for creating the non-existing resource group.
+        from sky.skylet.providers.azure.config import _configure_resource_group
+        _configure_resource_group({"provider": provider_config})
path: sky/skylet/providers/azure/node_provider.py
text: @user1:
Why is this needed?  `azure/config.py` has a `bootstrap_azure()` that calls something like this; is that setup somehow not called / wrong?  Should document more on L59.

@author:
I add more comments in the code.

@user2:
Can we add one more sentence like "Without this patch, sky launch with the same yaml file would fail."?