[API Server] Workdir is not updated after deleting files / folders, even after terminating the cluster and relaunching

[Michaelvll]

To reproduce:

1. sky launch -c cluster-name --workdir . task.yaml
2. Remove some code / files
3. sky launch -c cluster-name --workdir . task.yaml: failed to see the updated workdir
4. sky down cluster-name
5. sky launch -c cluster-name --workdir . task.yaml: still failed to see the updated workdir

[dan-blanchard]

@Michaelvll Just a quick clarifying question: When you say "Remove some code / files", do you mean locally or on the cluster?

My understanding of the issue as written is that if you launch a cluster with --workdir and then remove some files from you local version of that working directory, those deletions are not synced to the cluster. I just wanted to make sure the issue was not the other way around.

[Michaelvll]

Yes, it is deleted in the local path :)

[dan-blanchard]

Okay, in that case I am pretty sure the issue is that when we rsync the working directory in CommandRunner._rsync the rsync command doesn't have the --delete flag set, so it won't delete files that do not exist in the destination.

The data persisting after sky down is called feels like an additional problem though, as it's my understanding that sky down is supposed to delete all of the resources (including storage) associated with the cluster, so I would expect that launching a fresh cluster and rsyncing would be fine then.

What kind of cluster were you creating when you ran into the issue with sky down not working? Was it a development sky local cluster or a real one?

[Michaelvll]

Okay, in that case I am pretty sure the issue is that when we rsync the working directory in CommandRunner._rsync the rsync command doesn't have the --delete flag set, so it won't delete files that do not exist in the destination.

The data persisting after sky down is called feels like an additional problem though, as it's my understanding that sky down is supposed to delete all of the resources (including storage) associated with the cluster, so I would expect that launching a fresh cluster and rsyncing would be fine then.

What kind of cluster were you creating when you ran into the issue with sky down not working? Was it a development sky local cluster or a real one?

Ahh, good question! I think the issue mainly happens when the SkyPilot API server is remotely deployed: https://docs.skypilot.co/en/latest/reference/api-server/api-server.html#remote-api-server-multi-user-teams

A remote SkyPilot API server has some special logic to handle the uploads : )

[dan-blanchard]

Thanks for the additional info. After looking around some more, I think I know what the additional issue is for the remotely deployed API server.

1. The client uploads a zipped version of the working directory in upload_mounts_to_api_server
2. The server unzips the working directory to client_file_mounts_dir / original_path:

   skypilot/sky/server/server.py

   Line 1033 in 5f39acf

   new_path = client_file_mounts_dir / original_path.lstrip(

3. Since client_file_mounts_dir and original_path stay the same when launching the same cluster multiple times with the same working directory path, unzip_file just keeps overwriting the existing paths on the server.

   skypilot/sky/server/server.py

   Line 1068 in 5f39acf

   shutil.copyfileobj(member_file, f)

   
   Deletions locally are not synchronized since we are only overwriting existing files.
4. When the API server launches the cluster, it uses the same rsync codepath as the local client, (just using the directory on the server as what it is rsyncing to the cluster), so without the rsync --delete fix, it would also fail to delete the files from the cluster.

The remote API server's file system is an extra layer between the client and the cluster that is not synchronizing deletions. The simplest fix would be to wipe the target directory on the API server in unzip_file, but before starting with that, I wanted to make sure that that seems reasonable. For example, if results are written to that folder or things like that, that would probably not be what we want.

[dan-blanchard]

I just put up #8132 which fixes this issue. I've verified it by running the relevant smoke tests locally.