-
Notifications
You must be signed in to change notification settings - Fork 62
[sled-diagnostics] log collection should happen on u.2s #8438
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
[sled-diagnostics] log collection should happen on u.2s #8438
Conversation
Created using spr 1.3.6-beta.1
|
@smklein I will test bundle collection on a racklette once CI kicks out a TUF repo for me. The idea here is that we now use u.2 debug datasets and attempt to find the dataset with the most available storage. EditBundle collection was successful |
| match illumos_utils::zfs::Zfs::get_value( | ||
| path.as_str(), | ||
| "available", | ||
| true, | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function was expanded to include a parsable bool so that we get bytes rather than a value like "100G". I made it an option so that we don't change the behavior of any existing code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having to pass a bare boolean at all the call sites is a little gnarly. How much do the existing callers depend on the returned value not being parsable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea I agree that it's not ideal, I was mostly trying to minimize any unexpected behavior. The man page states -p Display numbers in parsable (exact) values., and all of the call sites should be in this PR. Looking through the call sites I think everything would be okay so we could likely just always pass the -p.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed in 0fabc0a
Created using spr 1.3.6-beta.1
| // Attempt to find a U.2 device with the most available free space | ||
| // for temporary storage to assemble a zip file made up of all of the | ||
| // discovered zone's logs. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Style nit I don't feel strongly about - I'd maybe move this chunk of code to a separate method? dataset_for_temporary_storage() or something
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry I had saw the approval on mobile when I stepped away from the laptop and marked it for auto merge, I didn't catch this nit in time so here's the follow up: #8454
|
|
||
| /// Return the directories that can be used for temporary sled-diagnostics | ||
| /// file storage. | ||
| pub fn all_sled_diagnostics_directories(&self) -> Vec<Utf8PathBuf> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This reminds me we should really prune a whole bunch of stuff from this crate now that the config reconciler has landed... A lot of it is only used in a couple tests (and I should update those tests to use the config reconciler instead!)
No objection to pruning this method though.
This fixes a missed style nit from #8438
Fixes #8197