install-to-disk: Check for mounts before install #805

djach7 · 2024-09-25T19:53:39Z

Works to improve bootc install to-disk flow by checking for mounts on the device to be installed to. If mounts are present then install will abort.

Resolves #720

cgwalters

Thanks for picking this up!

.dockerignore

cgwalters · 2024-09-26T06:43:16Z

lib/src/blockdev.rs

@@ -37,6 +38,9 @@ pub(crate) struct Device {
    // Filesystem-related properties
    pub(crate) label: Option<String>,
    pub(crate) fstype: Option<String>,
+
+    // VFS properties
+    pub(crate) mountpoints: Option<Vec<String>>,


cgwalters · 2024-09-26T06:46:12Z

lib/src/blockdev.rs

@@ -50,6 +54,30 @@ impl Device {
        self.children.as_ref().map_or(false, |v| !v.is_empty())
    }

+    // Find first mounted device along with mountpoints
+    pub(crate) fn find_first_mounted(&self) -> Option<(&Device, String)> {
+        let output = Command::new("findmnt")


So we already have code in mount.rs that's wrapping findmnt. An overall challenge here is because the install code is in a mount namespace, we won't see the device as mounted to start when we scan the block device.

That's why the mountpoints property that we can get from lsblk will be empty.

I think what we want here is a new api in mount.rs like this:

pub(crate) fn is_mounted_in_pid_mountns(device: &Device, pid: rustix::process::Pid) -> Result<bool>

Then we traverse the the device and run findmnt -N <pid> --source <device> or so.

So the bootc install to-disk code is in a mount namespace, which results in the device mountpoints being hidden from findmnt?

For your suggestion, what process am I getting the pid from? Is that from the disk, since I'm feeding it into findmnt as the task id then?

Just trying to wrap my head around all this stuff, I appreciate the quick feedback a ton!

For your suggestion, what process am I getting the pid from?

Pid 1

So the bootc install to-disk code is in a mount namespace, which results in the device mountpoints being hidden from findmnt?

Yep. Try playing with this from a root shell:

$ unshare -m $ findmnt /tmp $ mount --bind /var/tmp /tmp $ findmnt /tmp $ findmnt -N 1 /tmp

Note the second one (looking in the root mountns) didn't see the bind. Press ctrl-d to exit the unshare bash shell which will also tear down the mount.

I think I have a better idea of it now, Joseph helped me walk through it yesterday too. My basic understanding is that bootc install to-disk can't see the same pids (and hence the mountpoints) that the podman container can see, so I need to bridge the gap and allow install to-disk to see the mountpoints podman can. Then I can run findmnt with that specified pid and check for the mountpoints off of it

I'll experiment with those commands you listed too, thank you

djach7 · 2024-10-03T18:22:05Z

This should be working now, I made changes to parse the findmnt output for the device and bail if found. @cgwalters for the findmnt command I hardcoded -N 1, is there any issue with that? I was attempting to pass it a pid (you can see that attempt in the wip commit if you're curious) but the pid being passed was unable to see the device I was attempting to mount to.

cgwalters · 2024-10-07T19:23:07Z

lib/src/blockdev.rs

+            .output()
+            .expect("Failed to execute findmnt");
+
+        let mounts = String::from_utf8(output.stdout).unwrap();


While findmnt giving non-utf8 output is a "shouldn't happen" case, it's equally easy for us to use ? here to propagate it as an error instead of aborting, so let's do that.

How about adding this code to mount.rs instead? We could aim to share code with run_findmnt...at least in terms of parsing JSON instead of text.

Just pushed up a refactor into mount.rs, it now shares some code with the other findmnt functions in there. Is that more in line with what you were thinking?

lib/src/blockdev.rs

cgwalters · 2024-10-07T19:27:05Z

Thanks for working on this BTW, I think it's getting close!

Signed-off-by: djach7 <djachimo@redhat.com>

cgwalters · 2024-10-17T21:16:08Z

lib/src/mount.rs

+}
+
+// Recursively check a given filesystem to see if it contains an already mounted source
+pub(crate) fn is_source_mounted(path: &String, mounted_fs: &Filesystem) -> Result<bool> {


This function looks infallible so I think it can just be -> bool

Prefer using &str vs &String for parameters; recent blog: https://steveklabnik.com/writing/when-should-i-use-string-vs-str/

Updated, will resolve when pushed

cgwalters · 2024-10-17T21:17:48Z

lib/src/mount.rs

+}
+
+// Check if a specified device contains an already mounted filesystem
+pub(crate) fn is_mounted_in_pid1_mountns(path: String) -> Result<bool> {


Let's make this take &str

Updated, will resolve when pushed

cgwalters · 2024-10-17T21:20:07Z

lib/src/mount.rs

+    for fs in o.filesystems.iter() {
+        if is_source_mounted(&path, fs)? {
+            return Ok(true);
+        }
+    }
+
+    Ok(false)


This is all fine but with the below change could be condensed to something like
Ok(o.filesystems.iter().any(|fs| is_source_mounted(path)))

Or:

let mounted = o.filesystems.iter().any(|fs| is_source_mounted(path)); Ok(mounted)

Updated, will resolve when pushed

cgwalters · 2024-10-17T21:21:10Z

lib/src/mount.rs

+    findmnt_filesystem(&["--source"], &(format!("UUID={uuid}")))
+}
+
+// Check if a specified device contains an already mounted filesystem


"already mounted filesystem in the root mount namespace" or so.

Updated, will resolve when pushed

cgwalters · 2024-10-17T21:25:37Z

lib/src/mount.rs

+
+// Recursively check a given filesystem to see if it contains an already mounted source
+pub(crate) fn is_source_mounted(path: &String, mounted_fs: &Filesystem) -> Result<bool> {
+    if mounted_fs.source.contains(path) {


A bit uncertain about source.contains(path) vs mounted_fs.source.as_str() == path here...I guess you're probably doing that as a way to handle e.g. /dev/vda3 being mounted, but we're trying to match vs a whole device like /dev/vda?

Just thinking through this...what is probably cleaner actually is to do a set-intersection; on the calling side loop through all children of the device.

I'm not sure I follow what you're saying, my understanding is that we want to see if /dev/vda (the device) is mounted or if any partition on it is mounted. That's what I wrote it for, so it will kickback the mounting error if any part of the device is mounted.

Are you saying that this should only fail if the device alone is mounted? As in it shouldn't be checking the partitions of the device?

That's the reasoning behind the source.contains(path), I can't be sure what number partition will be mounted but I can work off it having the same naming stem.

I think what I mean is doing the check like this:

diff --git a/lib/src/install/baseline.rs b/lib/src/install/baseline.rs index f7c1c35..b8eb8bd 100644 --- a/lib/src/install/baseline.rs +++ b/lib/src/install/baseline.rs @@ -165,7 +165,13 @@ pub(crate) fn install_create_rootfs( // Handle wiping any existing data if opts.wipe { let dev = &opts.device; + if crate::mount::is_mounted_in_pid1_mountns(device.path()) { + anyhow::bail!("Device {} is mounted", device.path()); + } for child in device.children.iter().flatten() { + if crate::mount::is_mounted_in_pid1_mountns(child.path()) { + anyhow::bail!("Device {} is mounted", child.path()); + } let child = child.path(); println!("Wiping {child}"); crate::blockdev::wipefs(Utf8Path::new(&child))?;

Or maybe cleaner actually, make is_mounted_in_pid1_mountns take a &Device and have it iterate over children.

openshift-ci bot added the do-not-merge/work-in-progress label Sep 25, 2024

cgwalters reviewed Sep 26, 2024

View reviewed changes

djach7 force-pushed the installToDisk branch 3 times, most recently from 738eb54 to eece03c Compare October 3, 2024 18:20

cgwalters requested changes Oct 7, 2024

View reviewed changes

Check for existing mounts when installing to disk

6117e2c

Signed-off-by: djach7 <djachimo@redhat.com>

djach7 force-pushed the installToDisk branch from eece03c to 6117e2c Compare October 17, 2024 19:05

github-actions bot added the area/install Issues related to `bootc install` label Oct 17, 2024

cgwalters reviewed Oct 17, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

install-to-disk: Check for mounts before install #805

install-to-disk: Check for mounts before install #805

djach7 commented Sep 25, 2024

cgwalters left a comment

cgwalters Sep 26, 2024

cgwalters Sep 26, 2024 •

edited

Loading

djach7 Sep 26, 2024

cgwalters Sep 26, 2024 •

edited

Loading

djach7 Sep 27, 2024

djach7 Sep 27, 2024

djach7 commented Oct 3, 2024

cgwalters Oct 7, 2024

cgwalters Oct 7, 2024

djach7 Oct 17, 2024

cgwalters commented Oct 7, 2024

cgwalters Oct 17, 2024

djach7 Oct 21, 2024

cgwalters Oct 17, 2024

djach7 Oct 21, 2024

cgwalters Oct 17, 2024

djach7 Oct 21, 2024

cgwalters Oct 17, 2024

djach7 Oct 21, 2024

cgwalters Oct 17, 2024

djach7 Oct 21, 2024

cgwalters Oct 21, 2024

cgwalters Oct 21, 2024

install-to-disk: Check for mounts before install #805

Are you sure you want to change the base?

install-to-disk: Check for mounts before install #805

Conversation

djach7 commented Sep 25, 2024

cgwalters left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cgwalters Sep 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cgwalters Sep 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

djach7 commented Oct 3, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cgwalters commented Oct 7, 2024

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

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

cgwalters Sep 26, 2024 •

edited

Loading

cgwalters Sep 26, 2024 •

edited

Loading