[nexus] Create a longer-lived connection from Nexus -> Sled Agent #8149
Conversation
| // However, the bundle itself may be large. As long as we're | ||
| // continuing to make progress (see: read_timeout) we should be | ||
| // willing to keep transferring the bundle for a while longer. | ||
| .timeout(long_timeout) |
There was a problem hiding this comment.
This may still be too short for a truly large support bundle, say 50 GiB. At a download speed of 10 MiB/s, that would take roughly 85 minutes to complete.
There was a problem hiding this comment.
I hear you - I think range requests is how I want to solve this in the limit; I'm not sure I really want to allow connections to stay open in Nexus for longer than an hour.
WDYT - should we still try to do this PR as-is? Do you think we should aim for a higher number? Or should we accept this limitation until range requests are fully plumbed through?
There was a problem hiding this comment.
My initial concern was a customer being completely blocked by this timeout, but we could extract individual files via the API pretty easily, so that's less of an issue. An hour seems like a reasonable limit until we can get range requests in.
| let short_timeout = std::time::Duration::from_secs(60); | ||
| let long_timeout = std::time::Duration::from_secs(3600); |
There was a problem hiding this comment.
Should these be in the config file or something? It seems like it could, potentially, be useful to change them in the field if we see the long timeout hit...
There was a problem hiding this comment.
I'm not opposed to this, but I only wonder: is plumbing them into nexus config enough to make this modifiable? Seems like callers would still need to change the config among all nexuses, which would either require storing this value in the DB or deploying a new configuration via reconfigurator, right?
nexus/networking/src/sled_client.rs
Outdated
| pub fn default_reqwest_client() -> reqwest::Client { | ||
| let dur = std::time::Duration::from_secs(60); | ||
| reqwest::ClientBuilder::new() | ||
| .connect_timeout(dur) | ||
| .timeout(dur) | ||
| .build() | ||
| .expect("Failed to build reqwest Client") | ||
| } |
There was a problem hiding this comment.
Here's a thought: what if we had a function that returned the client builder with the default values, which this (and other functions in this module) would call to get a builder? That way, code that wants to set additional builder options could use that , and call additional methods on the "base" builder, but getting the "default" settings for anything it doesn't care to override.
Currently, we have the "short" timeout for connections/requests defined as 60s both here and in src/app/support_bundle.rs. If we wanted to add other "default client settings" here, we'd have to remember to update support_bundle.rs as well. On the other hand, if we had a "default builder" function that support_bundle.rs called and then added its own additional configurations to, we wouldn't have to remember to add new default settings in two or more places.
Just a thought.
There was a problem hiding this comment.
Sure, updated to using the default builder
Fixes #8144