Feature request for StopVM API

[haikuoliu]

Hi,

Currently the “StopVM” API does the following in order:

1. Shutdown fc-agent
2. Send a SIGTERM to VM
3. Remove shim dir

Any of them could produce an error, the error message is good but the error type is not well defined, we don't know if the failure is from shim dir removal or stopping VM. To integrate with firecracker-containerd, we want to take different actions on different errors, if the error comes from removing shim dir, that's probably fine and we can continue, but if the VM itself is not stopped, we may want to trigger an alarm or something because it's a more critical failure. So is it possible for firecracker-containerd to define error type for "StopVM" API?

Also, reading from code, I see the SIGTERM is sent to VM to gracefully stop it, is it possible to set a timeout for SIGTERM, and SIGKILL (not sure if you are already doing it) will be sent by firecracker-containerd in case SIGTERM is not working?

Let me know if it makes sense.
Thanks,
Haikuo

[nmeyerhans]

I agree with the suggestion to add more specific error types.

I think the timeout + SIGKILL logic should be left to the application. Based on the type of error returned upon failure, you may want to respond in various ways. Falling back to a call to StopVM with a "force" flag set, or passing a custom signal (e.g. KILL) might be reasonable ways to implement this from the SDK side.

[haikuoliu]

I think the timeout + SIGKILL logic should be left to the application. Based on the type of error returned upon failure, you may want to respond in various ways. Falling back to a call to StopVM with a "force" flag set, or passing a custom signal (e.g. KILL) might be reasonable ways to implement this from the SDK side.

Yeah it would be good as long as there is a parameter in the "StopVM" API to stop VM forcefully (send SIGKILL).

[sipsma]

Also agree with using more specific error types.

For the SIGTERM vs. SIGKILL issue, the current behavior actually should be that the VM gets a SIGKILL in the event that the SIGTERM and graceful shutdown fails. This is due to the fact that the VM process's context get cancelled, see this line and the cmd.Exec docs

[haikuoliu]

@sipsma Thanks for the link, but I think it's still worthy providing options to caller. Let's say I define a 1 min timeout for StopVM, when the timeout happens, there might be two cases:

1. The SIGTERM fails and SIGKILL is stuck, this is fine because I can't do anything for a stuck SIGKILL.
2. The SIGTERM is stuck, and SIGKILL is not executed yet, in this case I may want to use SIGKILL but I don't really have the choice.

[sipsma]

@haikuoliu I think adding a configurable timeout to StopVM is a good idea.

For case 2 you listed, the current behavior is to cancel the VM process's context in a defer statement, so it will be canceled no matter what (whether StopVM succeeds or not). When the context is cancelled, the VM process will receive a SIGKILL if it is still running. So I don't think you can get into a situation where SIGKILL will just not be sent, it's what we always fallback to if the graceful shutdown doesn't work.

If we add a timeout to StopVM that shouldn't change any of the above behavior.

[sipsma]

To clarify one part of my suggestion that may be causing confusion, I'm imagining that a new timeout for StopVM actually be enforced as part of the StopVM implementation itself (i.e. server-side, not client-side). This is what would allow the SIGKILL to happen no matter what after the timeout occurrs.

[Zyqsempai]

Hey guys, I would like to take this issue.
@sipsma Can you please summarize what should be done?

[sipsma]

Hi @Zyqsempai thanks for helping out! I think what we need here is:

1. A timeout enforced server-side for shutting down the VM in StopVM api calls. This should mean wrapping these lines in a timeout and ensuring the existing error handling logic will run if that timeout is hit.
2. A specific error type returned when a timeout is hit. I think DeadlineExceeded in the GRPC error code definitions would be a good candidate.
3. The ability to configure that timeout via a parameter to the StopVM call (defined in protobuf here)

If you'd like to address this in parts as opposed to doing all of the above at once, please feel free to structure that however you'd like.

Let me know if that's clear and please feel free to reach out if we can provide any more context or pointers along the way.

[sipsma]

Fixed via #272 by @Zyqsempai

[kzys]

I think we haven't addressed 2 (returning DeadlineExceeded) yet.