Empty the acloud_cvd_temp dir on cvd reset. #1485
Conversation
The acloud_cvd_temp directory contains instance lock files. If we are resetting with cvd reset, we should remove these, since we are resetting state completely anyway. This will become more important later when we are making changes to the way instance locks are acquired, and starting from a clean slate with cvd reset is more convenient than manually removing lock files.
| /* clear_instance_dirs*/ options.clean_runtime_dir)); | ||
|
|
||
| if (DirectoryExists("/tmp/acloud_cvd_temp/")) { | ||
| CF_EXPECT(RecursivelyRemoveDirectory("/tmp/acloud_cvd_temp/")); |
There was a problem hiding this comment.
Does it seem worth it to delete the contents of this directory without deleting the directory itself?
I could imagine a user changing the directory permissions of this to facilitate some multi-user interaction, though it would have to be reset on every reboot anyway since /tmp is not going to be persistent. Maybe not that significant in that context.
There was a problem hiding this comment.
It may not be worth the effort. If we want to support the multi-user case better, then we probably need to improve the directory structure here and maybe put some of these files elsewhere, but that's a larger change.
But I'm happy to make the adjustment if you think it's worthwhile.
There was a problem hiding this comment.
Changing the structure has other downsides, it breaks compatibility with python acloud. Might as well delete it then.
| CF_EXPECT(KillAllCuttlefishInstances( | ||
| /* clear_instance_dirs*/ options.clean_runtime_dir)); | ||
|
|
||
| if (DirectoryExists("/tmp/acloud_cvd_temp/")) { |
There was a problem hiding this comment.
As there are already 3 non-test occurrences of this string, maybe worth making a shared constant in a new file somewhere?
There was a problem hiding this comment.
That'll be the next pr after this one. ;)
jemoreira
left a comment
jemoreira
left a comment
There was a problem hiding this comment.
That directory is shared among all users in the system. cvd reset is supposed to only reset devices for the current user.
A more "correct" way for handling this is to have cvd change the ownership of each file it takes over to the current user. For that to work these files need to be created with rw permission for all. At reset time, cvd will only delete those files that are owned by the current user.
The one problem the approach mentioned above doesn't solve is acloud. It doesn't change ownership of those files, it simply reuses them if they are present. But acloud and cvd create are rarely used in the same machines these days, so it should be ok.
4 participants
The acloud_cvd_temp directory contains instance lock files. If we are resetting with cvd reset, we should remove these, since we are resetting state completely anyway.
This will become more important later when we are making changes to the way instance locks are acquired, and starting from a clean slate with cvd reset is more convenient than manually removing lock files.