Skip to content

[support-bundle] include for large log files #8771

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.

Sign up for GitHub

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

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

[support-bundle] include for large log files #8771

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1b682a8
[jj-spr] initial version
papertigers Aug 5, 2025
df99f09
also enable large file in the nexus task
papertigers Aug 5, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion nexus/src/app/background/tasks/support_bundle_collector.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1153,7 +1153,7 @@ fn recursively_add_directory_to_zipfile(

let file_type = entry.file_type()?;
if file_type.is_file() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Also: I wonder if we should pick an explicit compression scheme here, so we aren't at the whim of zip changing defaults.

E.g., we expect it'll be supported by customers, maybe we just pick zstd-3 here too

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's probably not a good idea to use zstd for the customer-accessible file because the standard zip tools on all major platforms don't support it. How would we access the file on Illumos, for example?

Copy link
Collaborator

Choose a reason for hiding this comment

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

according to https://docs.rs/zip/latest/zip/write/struct.FileOptions.html I think the default is "Deflate", and the level is either 24 or 6? https://docs.rs/zip/latest/zip/write/struct.FileOptions.html#method.compression_level

Good point re: zstd - just want to make sure an update of the zip crate doesn't change this for us

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah yeah, agreed it makes sense to define which algorithm we want to use.

let opts = FullFileOptions::default();
let opts = FullFileOptions::default().large_file(true);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is not a blocker for this PR , but it has me wondering:

We basically want to pull out the contents of the "zipfile from the sled-agent" and just rename things within the user-visible zipfile that we're creating. I wonder if this would be possible with a slightly more advanced version of zip, that lets us transfer the "contents" of zipfiles around without unpacking them.

(Basically, move the contents of a zipfile entry to a new name, if the compression scheme is an exact match)

This seems like a dangerous operation, but could be a possible optimization if this is taking a while. It's potentially eating a chunk of disk space, but as long as we're using streaming operations to write to the zipfile, I think it's not (currently) consuming 30+ GiB of memory.

TL;DR: this current implementation is fine with me, as long as it doesn't have too-bad performance implications.

let src = entry.path();

zip.start_file_from_path(dst, opts)?;
Expand Down
8 changes: 7 additions & 1 deletion sled-diagnostics/src/logs.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -720,7 +720,13 @@ fn write_log_to_zip<W: Write + Seek>(
zip_path,
FullFileOptions::default()
.compression_method(zip::CompressionMethod::Zstd)
.compression_level(Some(3)),
.compression_level(Some(3))
// NB: From the docs
// If set to false and the file exceeds the
// limit, an I/O error is thrown and the file is aborted. If set to
// true, readers will require ZIP64 support and if the file does not
// exceed the limit, 20 B are wasted.
.large_file(true),
)?;
if let Err(e) = std::io::copy(&mut src, zip) {
// If we fail here the `ZipWriter` is an unknown state and we are forced
Expand Down
Loading