Keep directories when SIGINT sent to daemon

[sondavidb]

Issue #, if available:
#275, #832

Description of changes:
On SIGINT, the daemon will perform some cleanup functions, including removing the directories for each of the snapshot layers. However, the metadata does not reflect this change. This meant that, upon restarting the snapshot, the snapshot would not start unless allow_invalid_mounts_on_restart=true was set in the config. Even with the variable set, one had to manually remove the snapshot to get the daemon in a working state.

This was pretty frustrating behavior. What should be a graceful kill ends up being more work than using a non-graceful kill (i.e. SIGKILL/SIGTERM, which allow the snapshots to instantly start working again, no questions asked). Additionally, it was unintuitive (to me) that killing the daemon also wipes all the snapshotter data, forcing one to re-pull an image every time the daemon is killed.

This change removes the cleanup step of wiping the snapshot directories. It will still unmount the FUSE mounts, but the snapshotter already tries to restore snapshots on startup, which just unmounts any mounts leftover and remounts existing mounts in the metadata, so it ties in very well to snapshot startup

Testing performed:
I performed some basic testing, largely including pulling an image + running a container with it after a SIGINT and confirmed normal behavior.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

[austinvazquez]

Should we have some of these manual tests as automated tests with this change?

[sondavidb]

Should we have some of these manual tests as automated tests with this change?

I tried, but I don't actually know how to kill the snapshotter with a SIGINT. I see that we have KillMatchingProcess, which is called by rebootContainerd, but this sends a SIGKILL, not a SIGINT. Even if I could pass it in, rebootContainerd also wipes the content stores so I don't believe calling that is a viable option regardless.

I tried to manually kill but killall isn't a binary in the container created. I can technically fudge this by banking on the grpc being a specific pid every time the container starts, but from past experiences relying on an arbitrary value far out of the scope of what I'm trying to achieve is...not ideal, to say the least.

[austinvazquez]

Should we have some of these manual tests as automated tests with this change?

I tried, but I don't actually know how to kill the snapshotter with a SIGINT. I see that we have KillMatchingProcess, which is called by rebootContainerd, but this sends a SIGKILL, not a SIGINT. Even if I could pass it in, rebootContainerd also wipes the content stores so I don't believe calling that is a viable option regardless.

I tried to manually kill but killall isn't a binary in the container created. I can technically fudge this by banking on the grpc being a specific pid every time the container starts, but from past experiences relying on an arbitrary value far out of the scope of what I'm trying to achieve is...not ideal, to say the least.

Give https://man7.org/linux/man-pages/man1/kill.1.html a try and if still stuck lmk and we can look together.

[sondavidb]

I ended up using pkill instead of kill since for some reason kill doesn't work with a name even though the man page seems to imply that.

I added the test and ensured that it crashed with the previous version and passed with this one.

EDIT: I was wrong, but now I should be right (I hope!) I tried to do a goroutine at first but they weren't guaranteed to terminate with its encapsulating testing function. Hopefully timeout works fine for this.

Just also realized I will need to update the documentation to specify this new behavior. Should that be a separate PR? I think the docs (aka just debug.md) would only reflect slightly extraneous steps after this PR, maybe it's ok to keep as is?