-
Notifications
You must be signed in to change notification settings - Fork 127
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Make hardcoded timeouts configurable #164
Comments
@sipsma Totally make sense to me, but how do you plan to configure it? |
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. |
What config? |
@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. |
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:
firecracker-go-sdk/machine.go
Line 495 in 9e2ff62
firecracker-go-sdk/firecracker.go
Line 29 in 9e2ff62
firecracker-go-sdk/firecracker.go
Line 173 in 9e2ff62
The text was updated successfully, but these errors were encountered: