[sled-agent] Ensure Support Bundles datasets are mounted #7938

smklein · 2025-04-08T20:41:26Z

Related to #7887 - we have seen that datasets nested within
encrypted datasets are not automatically mounted on reboot.

To mitigate this issue: whenever we perform an operation which attempts to read the dataset, ensure
that it is mounted. To do this operation, some minor refactors were made to mounting-related code.

Fixes #7884

smklein · 2025-04-08T20:42:10Z

illumos-utils/src/zfs.rs

 #[derive(Debug, Clone)]
-pub enum Mountpoint {
-    #[allow(dead_code)]
-    Legacy,


It simplifies logic elsewhere if this is "always a path". I went ahead and updated this, since there appear to be no users of "legacy".

The dataset mounted at "/" will actually show up as a legacy mountpoint, however I did a quick spot check and it doesn't look like we use this anywhere when reading from zfs to get a mountpoint.

smklein · 2025-04-08T20:42:43Z

illumos-utils/src/zfs.rs

 }

+impl CanMount {
+    fn wants_mounting(&self) -> bool {


This is just to make the code more readable below. Should be the same functionality as before.

smklein · 2025-04-08T20:42:58Z

illumos-utils/src/zfs.rs

    return Ok(result);
 }

+struct DatasetMountInfo {


We used to return a tuple of bools, I'm just making this explicit for readability.

smklein · 2025-04-08T20:45:34Z

sled-agent/src/support_bundle/storage.rs

    // Returns a dataset that the sled has been explicitly configured to use.
-    async fn get_configured_dataset(
+    //
+    // Returns an error if this dataset is not mounted.


This is used to access the "parent dataset" on which the "support bundle dataset" gets placed.

E.g., "Look up the crypt/debug dataset by UUIDs".

I'm adding these checks to also validate: ALSO confirm that this dataset is mounted.

This is tested in cannot_create_bundle_on_unmounted_parent

smklein · 2025-04-08T20:49:46Z

sled-agent/src/support_bundle/storage.rs

            let support_bundle_path = dataset
                .name
-                .mountpoint(&self.storage.zpool_mountpoint_root())
+                .ensure_mounted_and_get_mountpoint(


Every spot where we read the mountpoint of the support bundle dataset: Instead, ensure it's actually mounted first.

This is tested in listing_bundles_mounts_them

smklein · 2025-04-08T20:50:29Z

sled-agent/src/support_bundle/storage.rs


+        // The mounted root of the support bundle dataset
+        let support_bundle_dir = dataset
+            .ensure_mounted_and_get_mountpoint(


This technically is not tested - the call to dyn_nested_dataset_ensure above should actually do the mounting, so this call shouldn't actually be necessary. However, I included it here out of consistency with the other two callsites.

(This isn't easy to test; I'd basically need to stop this function from running halfway through. But it seems like a "paranoid check" anyway).

smklein · 2025-04-08T20:51:09Z

sled-agent/src/support_bundle/storage.rs

-        let support_bundle_dir =
-            dataset.mountpoint(&self.storage.zpool_mountpoint_root());
+        let support_bundle_dir = dataset
+            .ensure_mounted_and_get_mountpoint(


This is tested in getting_bundles_mounts_them.

papertigers · 2025-04-09T20:17:03Z

illumos-utils/src/zfs.rs

 #[derive(Debug, Clone)]
-pub enum Mountpoint {
-    #[allow(dead_code)]
-    Legacy,


The dataset mounted at "/" will actually show up as a legacy mountpoint, however I did a quick spot check and it doesn't look like we use this anywhere when reading from zfs to get a mountpoint.

papertigers · 2025-04-09T20:41:31Z

illumos-utils/src/zfs.rs

-            }
+        if wants_mounting {
+            let path = &mountpoint.0;
+            ensure_empty_immutable_mountpoint(&path).map_err(|err| {


Should this be ensure_dataset_mounted()?

No, this is on the creation path. The dataset does not exist yet.

Ah okay, the "wants_mounting" is a little confusing but I understand now. Maybe add a comment that mentions creating the immutable mountpoint for future mounting.

So, I have a comment on lines 1012 - 1019 which tries to cover this:

// Non-zoned datasets with an explicit mountpoint and the // "canmount=on" property should be mounted within the global zone. // // We'll ensure they have an empty immutable mountpoint before // creating the dataset itself, which will also mount it. // // Zoned datasets are mounted when their zones are booted, so // we don't do this mountpoint manipulation for them.

Do you have recommendations for how to make this more clear? My read of this is that it already does mention "creating the immutable mountpoint for future mounting", in particular:

// We'll ensure they have an empty immutable mountpoint before // creating the dataset itself, which will also mount it.

I guess I can re-shuffle the comment to go alongside the wants_mounting variable.

papertigers · 2025-04-09T20:52:11Z

sled-agent/src/support_bundle/storage.rs


+    async fn is_mounted(dataset: &str) -> bool {
+        let mut command = tokio::process::Command::new(illumos_utils::zfs::ZFS);
+        let cmd = command.args(&["list", "-Hpo", "mounted", dataset]);


Not really necessary and I don't know how much care about this function being async, but you could call illumos_utils::zfs::Zfs::get_value(dataset, "mounted"), which will return a String in the Ok path.

It's a module-local test-only function, I don't have a strong preference tbh. Do you care strongly?

I don't, feel free to skip it.

papertigers · 2025-04-09T20:57:29Z

sled-agent/src/support_bundle/storage.rs

+                }),
+            )
+            .await
+            .expect_err("Should not have been able to create support bundle");


Thanks for adding this test!

papertigers · 2025-04-09T21:38:45Z

illumos-utils/src/zfs.rs

-            }
+        if wants_mounting {
+            let path = &mountpoint.0;
+            ensure_empty_immutable_mountpoint(&path).map_err(|err| {


Ah okay, the "wants_mounting" is a little confusing but I understand now. Maybe add a comment that mentions creating the immutable mountpoint for future mounting.

papertigers · 2025-04-09T21:39:30Z

sled-agent/src/support_bundle/storage.rs


+    async fn is_mounted(dataset: &str) -> bool {
+        let mut command = tokio::process::Command::new(illumos_utils::zfs::ZFS);
+        let cmd = command.args(&["list", "-Hpo", "mounted", dataset]);


I don't, feel free to skip it.

wfchandler

LGTM, thanks for fixing this!

smklein added 3 commits April 8, 2025 12:11

[sled-agent] Ensure Support Bundle datasets get mounted

6925aed

Removing legacy path, trying to simplify code

4a7341c

Add tests

ff7a399

smklein changed the title ~~[sled-agent] Ensure Support Bundles are mounted~~ [sled-agent] Ensure Support Bundles datasets are mounted Apr 8, 2025

smklein commented Apr 8, 2025

View reviewed changes

Less confusing internal functions, more error handling

c94d8de

smklein requested review from papertigers and wfchandler April 8, 2025 21:25

Abstract mounting in sim-sled-agent for Nexus tests

be88715

papertigers reviewed Apr 9, 2025

View reviewed changes

papertigers approved these changes Apr 9, 2025

View reviewed changes

wfchandler approved these changes Apr 10, 2025

View reviewed changes

smklein added 2 commits April 10, 2025 10:08

Merge branch 'main' into support-bundle-mounting

026700a

comment

6213a86

smklein enabled auto-merge (squash) April 10, 2025 17:12

smklein merged commit 1a6317c into main Apr 10, 2025
16 checks passed

smklein deleted the support-bundle-mounting branch April 10, 2025 19:46

smklein mentioned this pull request May 9, 2025

[dogfood] Support bundles not mounted on reboot #8129

Closed

[sled-agent] Ensure Support Bundles datasets are mounted #7938

[sled-agent] Ensure Support Bundles datasets are mounted #7938

Uh oh!

Conversation

smklein commented Apr 8, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smklein Apr 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wfchandler left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

smklein Apr 10, 2025 •

edited

Loading