Skip to content

[bundles] Add range requests to bundle download API #8183

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

Merged
merged 14 commits into from
May 22, 2025
Merged

Conversation

smklein
Copy link
Collaborator

@smklein smklein commented May 16, 2025

This PR adds range request support to the support_bundle_download endpoint in the internal and external API.
It adds a RangeRequest header to both of these endpoints, and forwards this request to the underlying Sled Agent.

Additionally, this PR updates the Sled Agent API to identify that range requests are supported. They were supported before this PR, but not encoded into the openapi spec until now.

This PR also adds:

  • An integration test for Nexus, validating that range requests work for bundle downloading
  • omdb support for range requests when downloading bundles, including an option to resume a bundle that has only partially downloaded

Base automatically changed from dropshot-update to main May 16, 2025 20:49
@smklein smklein changed the title [wip] Working towards range requests for download API [bundles] Add range requests to bundle download API May 21, 2025
@smklein smklein marked this pull request as ready for review May 21, 2025 20:07
// Note that we'll still stream data in packets which are smaller than this,
// but we won't keep a single connection to Nexus open for longer than a 100
// MiB download.
const CHUNK_SIZE: u64 = 100 * (1 << 20);
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we're happy with this, I'll add something similar to the CLI once this PR merges

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the thought here that at some threshold we're testing our luck with regard to a timeout duration for a single query?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, exactly, 100 MiB seems small enough to not hit our 60 sec timeout. I could change this arbitrarily, but there's a cost with setting up each stream, so I didn't want to make it too small either.

(but this is semi-arbitrary; I can tweak the value if we feel strongly)

@smklein smklein requested review from wfchandler and ahl May 21, 2025 20:09
Copy link
Contributor

@ahl ahl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good to me!

// Note that we'll still stream data in packets which are smaller than this,
// but we won't keep a single connection to Nexus open for longer than a 100
// MiB download.
const CHUNK_SIZE: u64 = 100 * (1 << 20);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the thought here that at some threshold we're testing our luck with regard to a timeout duration for a single query?

Copy link
Contributor

@wfchandler wfchandler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, thanks!

@smklein smklein enabled auto-merge (squash) May 22, 2025 16:42
@smklein smklein merged commit 1f376f8 into main May 22, 2025
19 checks passed
@smklein smklein deleted the range-request-sb branch May 22, 2025 21:53
@sunshowers sunshowers mentioned this pull request May 23, 2025
sunshowers added a commit that referenced this pull request May 23, 2025
Semantic merge conflict between #8177 and #8183. The test appears to
pass locally.
smklein added a commit that referenced this pull request May 23, 2025
#8183 plumbed range request
support through Nexus for the "download entire support bundle" API.

This PR continues in that direction, plumbing range request support for
all other "download portions of the bundle" APIs.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants