[sled-agent] Replace `du(1)` with a Rust function #6482

hawkw · 2024-08-29T18:39:09Z

Currently, the zone_bundle::disk_usage function in sled-agent shells
out to du(1) and parses its output. This works fine, but it's a bit
flaky, especially when running the corresponding tests on "weird"
platforms such as NixOS, where the du(1) binary doesn't live in
/usr/bin/du (see #6479).

This commit replaces the code that shells out to /usr/bin/du with a
pure-Rust rewrite. There's a unit test that asserts that the disk usage
calculated by the new implementation matches the value calculated by
du(1), with code for handling the potential inaccuracy due to du(1)
operating in terms of 512B blocks on macOS.

Closes #6479

Currently, the `zone_bundle::disk_usage` function in sled-agent shells out to `du(1)` and parses its output. This works fine, but it's a bit flaky, especially when running the corresponding tests on "weird" platforms such as NixOS, where the `du(1)` binary doesn't live in `/usr/bin/du` (see #6479). This commit replaces the code that shells out to `/usr/bin/du` with a pure-Rust rewrite. There's a unit test that asserts that the disk usage calculated by the new implementation matches the value calculated by `du(1)`, with code for handling the potential inaccuracy due to `du(1)` operating in terms of 512B blocks on macOS. Closes #6479

bnaecker

Nice thanks! A few nits, but looks good to me.

sled-agent/src/zone_bundle.rs

hawkw · 2024-08-29T19:26:03Z

@bnaecker ab6aaf0 addresses both of your suggestions, thank you!

davepacheco · 2024-08-29T19:50:24Z

sled-agent/src/zone_bundle.rs

-        } else {
-            compile_error!("unsupported target OS");
+    let path = path.clone();
+    // We could, alternatively, just implement this using `tokio::fs` to read


I have not looked at this much yet but I wonder if we should just use the walkdir crate that we already have? I feel like there are so many edge cases in doing traversal that it's worth using a crate (but I don't feel strongly).

ah, i didn't realize that was in our dependency tree, I'll take a look at it.

b4e8731 changes this to use walkdir. I will add that, on my machine, the walkdir implementation takes ~100 ms to calculate the size of sled-agent/src, while my previous implementation did it in ~8 ms, and du(1) only takes about 6 ms. I'm not sure if we care about performance here that much, though, so that may not matter a whole lot --- making sure we're handling edge cases is definitely important, but we're also not using all that much of walkdir's functionality (e.g. traversing symlinks).

Looking at https://github.com/uutils/coreutils/blob/8fe93ded741f8257e9d0a5bce12f904e09227a59/src/uu/du/src/du.rs#L319, at least du looks pretty simple. There are definitely issues around traversing symlinks and bind mounts (loops in general) but I guess once we're not traversing symlinks, the only other check one could do is to ensure we don't cross filesystems.

Also that code's recursive, huh. I'd definitely write this using a heap-allocated stack personally.

Also that code's recursive, huh. I'd definitely write this using a heap-allocated stack personally.

That was what my original implementation did! :)

jclulow · 2024-08-29T21:27:34Z

sled-agent/src/zone_bundle.rs

-// - Parsing stdout fails
-// - Parsing the actual size as a u64 fails
+// This returns an error if reading any file or directory within the provided
+// directory fails.
 async fn disk_usage(path: &Utf8PathBuf) -> Result<u64, BundleError> {


I think it's probably worth considering a different name here, given the function appears to be assessing the apparent size of a tree of files rather than the actual disk usage. I definitely would have guessed the other way based on the name!

Ugh, that's a good point --- du(1) was actually reporting physical disk usage. We should probably change this to be more faithful to du rather than reporting logical size...

We probably want std::fs::unix::MetadataExt::blocks?

Ugh, that's a good point --- du(1) was actually reporting physical disk usage.

I don't believe it was, because we were using the -A flag. To be clear I think the name was already confusing haha

Ahh, okay, I can just rename the function, then. Unless we want the other behavior --- I'm not totally sure. @bnaecker, this is your code, right?

It is, yeah. I have no preference on the name, that was chosen since it was calling out to du. Whatever you want to call it is fine by me! Josh is correct that the calling code is using that to find the size of a tree of files, during a check against the zone bundle dataset quota.

772cd16 renames it to dir_size, but I'm happy to change the name again if there's something y'all think would be clearer.

Can that be &Utf8Path btw? I know you didn't touch that specific line

It can, yeah — I just didn't want to mess with the surrounding code as part of a quick fix. Happy to go and change it as well.

hawkw · 2024-08-30T18:55:43Z

Hmm, it looks like the disk_usage_matches_du test has been pretty consistently failing on CI because the Rust function reports a few fewer bytes than du: https://buildomat.eng.oxide.computer/wg/0/details/01J6J0P8D5V1YZR7AVT2R5F6DV/XrNrdk6kN2AdU90PUSVGK8gFhIwk9AmyWUCN1apAyz3vMdVU/01J6J0PPKMWXBWZ7SK1QQV9QVE#S5290

Interestingly, on my Linux machine, I've been pretty consistently seeing the exact same sizes. I wonder what the difference in behavior is caused by...

hawkw · 2024-08-30T19:14:24Z

Maybe the problem is that I'm skipping directories, and du isn't? Commit 8fe21ff changes it to include the size of dirs in the sum...on my machine, this causes the test to fail:

--- STDERR:              omicron-sled-agent zone_bundle::tests::test_dir_size_matches_du ---
du --version:
du (GNU coreutils) 9.5
Packaged by https://nixos.org
Copyright (C) 2024 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <https://gnu.org/licenses/gpl.html>.
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.

Written by Torbjörn Granlund, David MacKenzie, Paul Eggert,
and Jim Meyering.


[sled-agent/src/zone_bundle.rs:1725:13] dir_size(&path).await = Ok(
    1340678,
)
dir_size(/home/eliza/Code/oxide/omicron/sled-agent/src) took 865.155µs
[sled-agent/src/zone_bundle.rs:1730:13] dir_size_du(&path).await = Ok(
    1340591,
)
du -s /home/eliza/Code/oxide/omicron/sled-agent/src took 1.866788ms
thread 'zone_bundle::tests::test_dir_size_matches_du' panicked at sled-agent/src/zone_bundle.rs:1742:9:
assertion `left == right` failed: expected `dir_size(/home/eliza/Code/oxide/omicron/sled-agent/src)` to == `du -s /home/eliza/Code/oxide/omicron/sled-agent/src`
1340678B (`dir_size`) != 1340591B (`du`)
  left: 1340678
 right: 1340591
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

But perhaps it'll start passing on CI? And then we can, once again, return to where we started: this test always fails on my computer specifically. :)

(as you can see above, I also made the test run du --version and print the output, so we can see what's running on the CI machines and how it might differ from my du)

hawkw · 2024-08-30T20:19:16Z

Welp. It looks like including directories in the size calculation makes the behavior match du on the Ubuntu test runners: https://github.com/oxidecomputer/omicron/pull/6482/checks?check_run_id=29492777413

But, it makes it no longer match the version of du on my computer. That's so cool! I hate everything and want to die!!!

jclulow · 2024-08-31T08:31:36Z

FWIW, I would just make the test pass with illumos and turn it off on other platforms. We only really run the sled agent there anyway.

hawkw · 2024-08-31T16:06:34Z

FWIW, I would just make the test pass with illumos and turn it off on other platforms. We only really run the sled agent there anyway.

Yeah...I had really hoped rewriting the function in Rust would mean that we could run the tests everywhere, but, well, the original goal of this change was to make the test stop failing on my Linux box, so...

hawkw · 2024-09-03T17:24:46Z

@jclulow @davepacheco are either of you interested in weighing in on this again before I merge it? Hopefully at this point it's fairly uncontroversial...

davepacheco · 2024-09-03T17:52:46Z

Go for it!

aapoalas

Peanut gallery checking in with one question that popped to mind reading this.

Definitely uncontroversial :) I would press "Approve" but I expect that's not supposed to be available to me. It currently is, though. Only hubris repo's settings were previously changed to not allow approval and request-changes from outsiders.

EDIT: Oop, missed by seconds :D

aapoalas · 2024-09-03T17:55:08Z

sled-agent/src/zone_bundle.rs

-            const DU_ARG: &str = "-k";
-        } else {
-            compile_error!("unsupported target OS");
+// This returns an error if reading any file or directory within the provided


question: Is there a reason that these are not doc comments?

aapoalas · 2024-09-03T17:55:53Z

sled-agent/src/zone_bundle.rs

@@ -582,6 +582,11 @@ pub enum BundleError {

    #[error("Instance is terminating")]
    InstanceTerminating,
+
+    /// The `walkdir` crate's errors already include the path which could not be


praise: Good explanation <3

hawkw added 2 commits August 29, 2024 11:10

just stick the whole thing in one big blocking task

a265af8

hawkw requested review from davepacheco, smklein and bnaecker August 29, 2024 18:39

hawkw mentioned this pull request Aug 29, 2024

Skip test_disk_usage if there's no /usr/bin/du #6479

Closed

bnaecker approved these changes Aug 29, 2024

View reviewed changes

sled-agent/src/zone_bundle.rs Outdated Show resolved Hide resolved

sled-agent/src/zone_bundle.rs Outdated Show resolved Hide resolved

review feedback

ab6aaf0

davepacheco reviewed Aug 29, 2024

View reviewed changes

just use walkdir

b4e8731

hawkw requested a review from davepacheco August 29, 2024 21:17

jclulow reviewed Aug 29, 2024

View reviewed changes

rename disk_usage to dir_size

772cd16

hawkw added 2 commits August 30, 2024 12:07

print du version in tests

92718e0

what happens if we include dirs

8fe21ff

only run du test on illumos

dcf0764

hawkw merged commit b6a098d into main Sep 3, 2024
23 checks passed

hawkw deleted the eliza/du-rust branch September 3, 2024 18:05

aapoalas reviewed Sep 3, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[sled-agent] Replace `du(1)` with a Rust function #6482

[sled-agent] Replace `du(1)` with a Rust function #6482

hawkw commented Aug 29, 2024

bnaecker left a comment

hawkw commented Aug 29, 2024

davepacheco Aug 29, 2024

hawkw Aug 29, 2024

hawkw Aug 29, 2024

sunshowers Aug 29, 2024 •

edited

Loading

sunshowers Aug 29, 2024

hawkw Aug 29, 2024

jclulow Aug 29, 2024

hawkw Aug 29, 2024

hawkw Aug 29, 2024

jclulow Aug 29, 2024

hawkw Aug 29, 2024

bnaecker Aug 29, 2024 •

edited

Loading

hawkw Aug 30, 2024

sunshowers Aug 30, 2024

hawkw Aug 30, 2024

hawkw commented Aug 30, 2024

hawkw commented Aug 30, 2024

hawkw commented Aug 30, 2024

jclulow commented Aug 31, 2024

hawkw commented Aug 31, 2024

hawkw commented Sep 3, 2024

davepacheco commented Sep 3, 2024

aapoalas left a comment •

edited

Loading

aapoalas Sep 3, 2024

aapoalas Sep 3, 2024

[sled-agent] Replace du(1) with a Rust function #6482

[sled-agent] Replace du(1) with a Rust function #6482

Conversation

hawkw commented Aug 29, 2024

bnaecker left a comment

Choose a reason for hiding this comment

hawkw commented Aug 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sunshowers Aug 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bnaecker Aug 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hawkw commented Aug 30, 2024

hawkw commented Aug 30, 2024

hawkw commented Aug 30, 2024

jclulow commented Aug 31, 2024

hawkw commented Aug 31, 2024

hawkw commented Sep 3, 2024

davepacheco commented Sep 3, 2024

aapoalas left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

[sled-agent] Replace `du(1)` with a Rust function #6482

[sled-agent] Replace `du(1)` with a Rust function #6482

sunshowers Aug 29, 2024 •

edited

Loading

bnaecker Aug 29, 2024 •

edited

Loading

aapoalas left a comment •

edited

Loading