Add a clean shutdown option

firecracker-microvm / firectl

firectl is a command-line tool to run Firecracker microVMs

Apache License 2.0

477 stars 72 forks source link

Add a clean shutdown option #24

Closed nmeyerhans closed 5 years ago

nmeyerhans commented 5 years ago

*Issue #16

Description of changes:

Update the firecracker SDK to a version that includes support for Machine.Shutdown().
Issue a clean shutdown request upon receipt of SIGINT.

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

sipsma commented 5 years ago

For 1, I feel like an SDK setting up signal handlers should be an anti-pattern. In general, I don't want a library I'm importing to do anything that affects what's essentially global process state (the signal handlers in this case). If I'm writing some program that currently sets up signal handlers in an init() of a package, once I import the SDK I would have to migrate those somewhere else just so they don't get overridden by the SDK.

Maybe the SDK can provide a utility function that sets up the signal forwarding currently set up by the SDK, i.e. ForwardSignals()? Then importers of the library can at least opt-in.

nmeyerhans commented 5 years ago

For 1, I feel like an SDK setting up signal handlers should be an anti-pattern. In general, I don't want a library I'm importing to do anything that affects what's essentially global process state (the signal handlers in this case). If I'm writing some program that currently sets up signal handlers in an init() of a package, once I import the SDK I would have to migrate those somewhere else just so they don't get overridden by the SDK.

The problem is that there's implicit signal handling behavior in Linux, and it does the wrong thing in most cases. The SDK may be the right place to override these defaults that more closely match the process hierarchy involved.

samuelkarp commented 5 years ago

The SDK may be the right place to override these defaults that more closely match the process hierarchy involved.

I think @sipsma's suggestion of a ForwardSignals() method makes a lot of sense; it provides the common capability that we would care about and means that changing Linux's default signal handling is opt-in instead of opt-out. I think for me this better fits the principle of least surprise; it's somewhat surprising that Linux wouldn't do what I want by default but it would be more surprising that an imported library would change my signal handling automatically.

samuelkarp commented 5 years ago

Should we fall back to a hard kill via StopVM() if the guest doesn't respond to CtrlAltDel within some timeout? Or should we leave this to whoever is sending the signals? They can always escalate to SIGTERM/SIGKILL, etc. Again, any option is OK, I'm just seeking opinions.

For firectl I don't really have a problem with using a timeout and falling-back to a hard kill if the timeout is exceeded; I think that's a pretty familiar UX for anyone used to docker stop. I don't think I would want that behavior by default in the SDK, but it would be nice to have it as an opt-in utility function.

nmeyerhans commented 5 years ago

Squashed the fixup commits and merged.