firecracker-microvm
Make hardcoded timeouts configurable
When attempting to reproduce some rare errors in firecracker-containerd, I've at times increased load quite a bit which has caused me to hit various hardcoded timeouts in the go sdk. When I increase the timeouts in my fork the operations succeed, so the timeouts are just set too low for the tests I'm running. The current values seem fine as defaults, but they should be configurable.
Ones I've encountered:
- https://github.com/firecracker-microvm/firecracker-go-sdk/blob/9e2ff62839b228665976be79191e33cffce5f760/machine.go#L495
- https://github.com/firecracker-microvm/firecracker-go-sdk/blob/9e2ff62839b228665976be79191e33cffce5f760/firecracker.go#L29
- https://github.com/firecracker-microvm/firecracker-go-sdk/blob/9e2ff62839b228665976be79191e33cffce5f760/firecracker.go#L173
It's probably reasonable for this sort of thing to be settable via environment variables.
I think I much prefer these to live in the config, rather than as environment variables. We can support both, but I think having them inside the config at the bare minimum is preferable. And honestly, this can probably just use the request timeout.
@nmeyerhans - The config structure, https://github.com/firecracker-microvm/firecracker-go-sdk/blob/master/machine.go#L52
Right. That's exactly why I think we should use environment variables. That structure comes from the client application, meaning that if we require these settings to come from there, then they can only be adjusted by a client application that knows about them. Since this is specifically useful for debugging, and affects the internal behavior of the SDK, there's no need for the client to know anything about them. Environment variables let us manipulate the SDK settings directly, independent of whatever client is in use. This makes SDK debugging much more practical.
Unless there are use-cases for adjusting these settings in production deployments, we should not. require clients to handle this.
Hm, how do you feel about supporting both then? I mean that definitely adds a level of complexity of figuring out what value to use when both are set, but I can definitely see users wanting to adjust the timeouts in the config.
I'd start with environment variables, since that functionality will address the use cases we know about now and is simple to implement.
If at a later time it makes sense for these values to be settable by the calling application, we can do that. I would not want to add support for that now, though, given a lack of any documented need for it.