search

firecracker-microvm / firecracker-go-sdk

An SDK in Go for the Firecracker microVM API
Apache License 2.0
466 stars 123 forks source link

Make hardcoded timeouts configurable #164

Open sipsma opened 4 years ago

sipsma commented 4 years ago

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:

Zyqsempai commented 4 years ago

@sipsma Totally make sense to me, but how do you plan to configure it?

nmeyerhans commented 4 years ago

It's probably reasonable for this sort of thing to be settable via environment variables.

xibz commented 4 years ago

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 commented 4 years ago

What config?

xibz commented 4 years ago

@nmeyerhans - The config structure, https://github.com/firecracker-microvm/firecracker-go-sdk/blob/master/machine.go#L52

nmeyerhans commented 4 years ago

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.

xibz commented 4 years ago

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.

nmeyerhans commented 4 years ago

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.