lib/bootloader: Write to PReP partition on ppc64le #667

sacsant · 2024-07-04T06:52:56Z

Bootloader code currently writes required data to base/parent
device (eg /dev/sda). This logic does not work for ppc64le
architecture as bootloader configuration has to be written
to PRePboot partition(typically first partition of disk).

This patch adds code to identify PowerPC-PReP-boot partition
(for ppc64le architecture) and writes bootloader data to it.
realpath command is used to identify PReP partition.

Incase of writing to qcow2 image, first partition of loopback
device is used as it points to PRePBoot. In order to identify
loopback device usage the patch introduces a boolean flag to
filesystem_impl() routine.

Signed-off-by: Sachin Sant [email protected]

sacsant · 2024-07-04T07:03:48Z

This is part of ppc64le enablement 578

cgwalters

Thanks for working on this!

cgwalters · 2024-07-06T14:18:59Z

lib/src/bootloader.rs

+        } else {
+            // While writing to a disk use realpath to extract
+            // PowerPC-PReP-boot partition
+            let result = Command::new("realpath")


I don't think this will do much different from the builtin https://doc.rust-lang.org/std/fs/fn.canonicalize.html

Thanks, this helps.

cgwalters · 2024-07-06T14:22:24Z

lib/src/bootloader.rs

@@ -12,15 +16,44 @@ pub(crate) fn install_via_bootupd(
    device: &Utf8Path,
    rootfs: &Utf8Path,
    configopts: &crate::install::InstallConfigOpts,
+    via_loopback: bool,


I think it'd be a bit simpler for us to inspect the target block device here and special case loopback, instead of passing down this boolean.

(But, not opposed to the bool either, it's OK)

cgwalters · 2024-07-06T14:23:20Z

lib/src/bootloader.rs

+            // PowerPC-PReP-boot partition
+            let result = Command::new("realpath")
+                .arg("-L")
+                .arg("/dev/disk/by-partlabel/PowerPC-PReP-boot")


This is a complex topic because in the fully general case, there might already be an existing partition with this label on the system. Think about a scenario where we are trying to create a raw disk image from an already booted host.

Now, we're effectively bypassing that here because we special case the loopback path.

Backing up to a higher level here, I've been trying to drive alignment between all the different install paths we have in the Fedora-derivative ecosystem. One of those is Fedora CoreOS and derivatives, see https://github.com/coreos/coreos-assembler/blob/c426a1c34a745762a9314872212e447b9fd26488/src/osbuild-manifests/coreos.osbuild.ppc64le.mpp.yaml#L343 on this topic.

That's basically the loopback install path (generic disk image generation).

Whereas this one should more align with Anaconda...which, I suspect probably needs similar logic with bootupd.

All of this perhaps argues that we should change bootupd to handle this instead?

Right, fair point.

Infact my first preference was lsblk command, not realpath. Since lsblk command takes a device as input we should be able to solve this issue of multiple devices (containing PReP boot) present on a system. For some unknown reason the command did not work as expected, although the same works outside of bootc.

$ lsblk --paths --noheadings -o NAME --filter 'PARTLABEL=="PowerPC-PReP-boot"' /dev/sdb
/dev/sdb1

I probably did not codify the logic correctly. --filter option wasn't being enforced correctly and as a result all the partitions on the device were listed in the output. I will revisit the logic again.

I think it may be simpler to use our internal wrapper for lsblk and then just traverse the children looking for ones with that label in Rust code instead of filtering at the lsblk level.

But I don't object either to adding support for filtering to our lsblk wrapper either.

Can you post (whether as a diff here or just force push to this PR) the code you were trying that wasn't working?

I have pushed the latest code.

cgwalters · 2024-07-06T14:25:30Z

lib/src/bootloader.rs

+                println!("{}", String::from_utf8_lossy(&result.stderr));
+                anyhow::bail!("realpath failed with {}", result.status);
+            }
+            prepboot_dir = String::from_utf8(result.stdout).unwrap();


We generally avoid use of unwrap() except for things we know "statically" can't/shouldn't fail. Non-UTF8 filesystem paths are generally in that "shouldn't happen" case but crashing the process is undesirable if we do happen to find one.

In this case it's basically equally easy to handle this as an error, just change .unwrap() to ?.

sacsant · 2024-07-08T05:18:32Z

All of this perhaps argues that we should change bootupd to handle this instead?

Will look at the bootupd code.

Is it okay then to close this PR?

cgwalters · 2024-07-08T12:05:53Z

Is it okay then to close this PR?

The fact the CoreOS pipeline is doing something similar to this today argues for doing the same thing here to start.

But I think both could likely be cleaned up by having all the logic in bootupd. And it would likely simplify a future change for bootc-image-builder to support ppc64le too.

So...up to you! I would lean slightly towards landing work here to start as I think we can condense this patch into something pretty simple.

sacsant · 2024-07-11T11:49:54Z

using lsblk I was able to simply the code by restricting the change only to bootloader.rs and also ensure it will work even if more that one device is present on the system. Unfortunately lsblk command that should help implement this logic is not working as expected.

// get PowerPC-PReP-boot device information
let output = Command::new("lsblk")
.args([
"--noheadings",
"--filter",
&format!(r#"'PARTLABEL=="PowerPC-PReP-boot"'"#),
"--output",
&format!("NAME"),
"--paths",
])
.arg(device)
.output()?;

--filter option doesn't seem to work and gives following o/p

Running bootupctl to install bootloader

bootupctl backend install --write-uuid --update-firmware --auto --device /dev/sdb
/dev/sdb1
/dev/sdb2 /run/bootc/mounts/rootfs
Installing for powerpc-ieee1275 platform.

The same command with --filter option works correctly when executed manually

Any idea what can be the reason for lsblk --filter command to not work as expected?

yoheiueda · 2024-07-11T15:30:30Z

It looks like the--filter option was introduced recently. util-linux/util-linux@3e0b4a8 Did you confirm that lsblk in the container has the option?

I encountered another version issue in my PR #665 (comment)

sacsant · 2024-07-12T12:26:26Z

It looks like the--filter option was introduced recently. util-linux/util-linux@3e0b4a8 Did you confirm that lsblk in the container has the option?

I encountered another version issue in my PR #665 (comment)

Thanks. Yes --filter option is available with lsblk. The version of lsblk inside the container is 2.40.1 (lsblk from util-linux 2.40.1 )
Not sure if this issue is due to the fact that --filter syntax mandates usage of both ' and ". Possibly the usage of these with .args() trips the code.

cgwalters · 2024-07-12T18:07:51Z

lib/src/bootloader.rs

+                "--noheadings",
+                "--paths",
+                "--filter",
+                &(format!(r#"'PARTLABEL=="PowerPC-PReP-boot"'"#)),


I think the problem may be here in that you're passing an extra ' that is needed only when invoking from shell script.

cgwalters · 2024-07-12T18:13:10Z

I've pushed an additional commit to this PR that I think will fix the problem; I have a ppc64le machine provisioning now to try to at least test this out myself. What do you think of this updated code? Can you review/test?

sacsant · 2024-07-13T05:29:06Z

I've pushed an additional commit to this PR that I think will fix the problem; I have a ppc64le machine provisioning now to try to at least test this out myself. What do you think of this updated code? Can you review/test?

Thank You. I tried this code, but unfortunately it still fails with

Deploying container image...Freed objects: 118 bytes
done
ERROR Installing to disk: Installing bootloader: Failed to find partition with label PowerPC-PReP-boot

Looking at the lsblk -J output, label string is null for all partitions. partlabel attribute contains the right string.

$ lsblk -J -O /dev/sdb | grep label
         "label": null,
         "partlabel": null,
               "label": null,
               "partlabel": "PowerPC-PReP-boot",
               "label": null,
               "partlabel": "root",

I extended the device structure to include partlabel as follows

--- a/lib/src/blockdev.rs
+++ b/lib/src/blockdev.rs
@@ -28,6 +28,7 @@ pub(crate) struct Device {
     pub(crate) serial: Option<String>,
     pub(crate) model: Option<String>,
     pub(crate) label: Option<String>,
+    pub(crate) partlabel: Option<String>,
     pub(crate) fstype: Option<String>,
     pub(crate) children: Option<Vec<Device>>,
     pub(crate) size: Option<String>,
@@ -56,7 +57,7 @@ pub(crate) fn wipefs(dev: &Utf8Path) -> Result<()> {
 
 fn list_impl(dev: Option<&Utf8Path>) -> Result<Vec<Device>> {
     let o = Command::new("lsblk")
-        .args(["-J", "-o", "NAME,SERIAL,MODEL,LABEL,FSTYPE,SIZE"])
+        .args(["-J", "-o", "NAME,SERIAL,MODEL,LABEL,PARTLABEL,FSTYPE,SIZE"])
         .args(dev)
         .output()?;
     if !o.status.success() {

and then replaced label with partlabel in bootloader code

-            .find(|dev| dev.label.as_deref() == Some(PREPBOOT_LABEL))
+            .find(|dev| dev.partlabel.as_deref() == Some(PREPBOOT_LABEL))

It still fails with
ERROR Installing to disk: Installing bootloader: Failed to find partition with label PowerPC-PReP-boot

Going back to your comment about using ' in the string (shell vs rust), it was spot-on. I removed the ' from the string and it worked. bootc install now works with both writing image to block device as well as writing to a qcow2 file.

cgwalters · 2024-07-13T17:37:47Z

OK a few things. First, I split out a PR to beef up our blockdev handling, see #680 (if you or @yoheiueda could glance at/review that'd be nice).

On this specific PR I've:

Proactively rebased this PR on the blockdev updates
Squashed my changes with yours
Fixed the label vs partlabel confusion

What do you think?

sacsant · 2024-07-14T15:12:26Z

OK a few things. First, I split out a PR to beef up our blockdev handling, see #680 (if you or @yoheiueda could glance at/review that'd be nice).

On this specific PR I've:

Proactively rebased this PR on the blockdev updates

Squashed my changes with yours

Fixed the label vs partlabel confusion

What do you think?

The first 3 patches looks good to me. With the code from commit 3be47ba included the bootupd specific changes to handle powerpc64 fails to compile.

error[E0507]: cannot move out of `device.children` which is behind a shared reference
  --> lib/src/bootloader.rs:19:16
   |
19 |           return device
   |  ________________^
20 | |             .children
   | |                     ^
   | |                     |
   | |_____________________help: consider calling `.as_ref()` or `.as_mut()` to borrow the type's contents
   |                       move occurs because `device.children` has type `std::option::Option<std::vec::Vec<blockdev::Device>>`, which does not implement the `Copy` trait
21 |               .unwrap_or_default()
   |                ------------------- `device.children` moved due to this method call
   |
note: `std::option::Option::<T>::unwrap_or_default` takes ownership of the receiver `self`, which moves `device.children`
  --> /builddir/build/BUILD/rustc-1.79.0-src/library/core/src/option.rs:1002:30
help: you could `clone` the value and consume it, if the `blockdev::Device: Clone` trait bound could be satisfied
   |
19 ~         return <std::option::Option<std::vec::Vec<blockdev::Device>> as Clone>::clone(&device
20 ~             .children)
   |

cgwalters · 2024-07-15T12:34:19Z

Sorry about that, I had compile tested the previous code by locally changing the ifdef. Anyways the compiler is right with:

help: consider calling .as_ref() or .as_mut() to borrow the type's contents

Pushed a fix.

sacsant · 2024-07-15T15:53:05Z

Sorry about that, I had compile tested the previous code by locally changing the ifdef. Anyways the compiler is right with:

help: consider calling .as_ref() or .as_mut() to borrow the type's contents

Pushed a fix.

Thank you for the fix. With this fix the code compiles correctly.
Unfortunately I have now run into a completely new issue. bootc install (either to disk or generate a qcow2) now fails with following error:

ERROR Installing to disk: Creating rootfs: Listing device /dev/sdb: invalid type: integer 2048, expected a string at line 136 column 29

With tracing enabled I get

TRACE Scanning directory '/usr/local/lib/bootc/install'    
TRACE Scanning directory '/etc/bootc/install'    
TRACE Scanning directory '/run/bootc/install'    
DEBUG Loaded install configuration
Wiping /dev/sdb
Wiping device /dev/sdb
DEBUG exec: "wipefs" "-a" "/dev/sdb"
/dev/sdb: 8 bytes were erased at offset 0x00001000 (gpt): 45 46 49 20 50 41 52 54
/dev/sdb: 8 bytes were erased at offset 0x18fffff000 (gpt): 45 46 49 20 50 41 52 54
/dev/sdb: 2 bytes were erased at offset 0x000001fe (PMBR): 55 aa
/dev/sdb: calling ioctl to re-read partition table: Success
Block setup: direct
       Size: 107374182400
     Serial: <unknown>
      Model: VDASD           
TRACE ExitStatus(unix_wait_status(0))
DEBUG Loaded SELinux policy: 4b68ea534ffcd253135e11fc5d62eb4b27d346966c4128f3a3a242ba37be6310
Initializing partitions
DEBUG exec: "sgdisk" "-Z" "/dev/sdb" "-U" "R" "-n" "1:0:+4M" "-c" "1:PowerPC-PReP-boot" "-t" "1:9E1A2D38-C612-4316-AA26-8B49521E5A8B" "-n" "2:0:0" "-c" "2:root" "-t" "2:0FC63DAF-8483-4772-8E79-3D69D8477DE4"
TRACE ExitStatus(unix_wait_status(0))
DEBUG Created partition table
TRACE starting
DEBUG argv0=Some("exe")
TRACE "udevadm" ["settle"]
ERROR Installing to disk: Creating rootfs: Listing device /dev/sdb: invalid type: integer `2048`, expected a string at line 136 column 29

cgwalters · 2024-07-15T17:24:17Z

ERROR Installing to disk: Creating rootfs: Listing device /dev/sdb: invalid type: integer 2048, expected a string at line 136 column 29

I think what happened here is basically I was working on #680 locally and ended up splitting it out from this PR and fixed bugs there, but I forgot to rebase this PR on the properly tested version. I've forced push an update to this PR (and I have a ppc64le system provisioning again...on Friday it took 4 hours to make one for some reason and I didn't get a chance to actually e2e test myself)

cgwalters · 2024-07-15T22:06:34Z

OK yeah, testing this e2e meaning one needs to build both bib and the bootc base images is clearly an ergonomic hit, but it's all kind of circular; once we land a few enablement patches like this I think we can turn it on in the build systems by default.

cgwalters · 2024-07-15T22:41:06Z

I tested this, and we still fail to find the partition. Digging in the reason is because for the stuff not in /sys like labels, lsblk basically picks up data that udev writes, and it does this via parsing the files in /run/udev/data. Adding a -v /run/udev:/run/udev to the install invocation fixes this...but of course adding a new requirement to the install invocation is going to introduce a lot of pain.

In the short term, we could run lsblk from the host...but, that opens us up to more skew. Probably the most sustainable path is what we were talking about in dynamic mounts.

cgwalters · 2024-07-15T22:45:19Z

Another viable path is to use sfdisk -J as I think jlebon suggested; it seems to always parse the disk directly, and doesn't use kernel cached data.

I may look at this tomorrow. (It's probably high time we split out a separate blockdev internal crate...)

sacsant · 2024-07-16T06:15:23Z

OK yeah, testing this e2e meaning one needs to build both bib and the bootc base images is clearly an ergonomic hit, but it's all kind of circular; once we land a few enablement patches like this I think we can turn it on in the build systems by default.

yes, I have already submitted ppc64le arch enablement patches to bootc-image-builder.

sacsant · 2024-07-16T06:19:38Z

I tested this, and we still fail to find the partition. Digging in the reason is because for the stuff not in /sys like labels, lsblk basically picks up data that udev writes, and it does this via parsing the files in /run/udev/data. Adding a -v /run/udev:/run/udev to the install invocation fixes this...but of course adding a new requirement to the install invocation is going to introduce a lot of pain.

Thanks. Adding -v /run/udev:/run/udev fixes the issue while writing the image to a disk. The same does not work while writing image to qcow2.

cgwalters · 2024-07-16T14:02:44Z

➡️ #688

cgwalters · 2024-07-16T20:27:49Z

OK now rebased 🏄 on top of #688 - I also changed things so we have a shared constant for the GUID, which makes more sense to use than a label by default.

cgwalters · 2024-07-16T21:15:36Z

@sacsant can you give this another test/review?

Bootloader code currently writes required data to base/parent device (eg /dev/sda). This logic does not work for ppc64le architecture as bootloader configuration has to be written to PRePboot partition(typically first partition of disk). This patch adds code to identify PowerPC-PReP-boot partition (for ppc64le architecture) using lsblk command and writes bootloader data to it. Co-authored-by: Colin Walters <[email protected]> Signed-off-by: Sachin Sant <[email protected]> Signed-off-by: Colin Walters <[email protected]>

sacsant · 2024-07-17T09:21:48Z

@sacsant can you give this another test/review?

Thanks. The changes look good except for using uuid for comparison. I have pushed updated code with following change

-            .find(|p| p.uuid.as_deref() == Some(PREPBOOT_GUID))
+            .find(|p| p.parttype.as_str() == PREPBOOT_GUID)

With this change install to-disk works correctly. Without this change the command will fail with

ERROR Installing to disk: Installing bootloader: Failed to find PReP partition with GUID 9E1A2D38-C612-4316-AA26-8B49521E5A8B

The above mentioned change is required as uuid field of sfdisk is dynamically generated and will keep changing each time a partition is created. IMO we should rely on type field which will always point to PREP_BOOT ID string

"partitions": [
         {
            "node": "/dev/sdb1",
            "start": 256,
            "size": 1024,
            "type": "9E1A2D38-C612-4316-AA26-8B49521E5A8B",  <<== static and always contains PReP-boot id
            "uuid": "A51FCE19-00FA-42C3-8ABB-305230D01592",  <<== dynamic
            "name": "PowerPC-PReP-boot"
         },{
            "node": "/dev/sdb2",

Let me know.

cgwalters · 2024-07-17T12:17:44Z

The changes look good except for using uuid for comparison.

Oops yes, my bad at getting the uuids mixed up. Thanks! Queued this one to merge!

lib: Release 0.15.1

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

sacsant mentioned this pull request Jul 4, 2024

bootc enablement for ppc64le #578

Closed

cgwalters reviewed Jul 6, 2024

View reviewed changes

sacsant force-pushed the powerpc64 branch from bd66f53 to 9103908 Compare July 12, 2024 17:07

cgwalters reviewed Jul 12, 2024

View reviewed changes

cgwalters mentioned this pull request Jul 13, 2024

blockdev+install updates #680

Merged

cgwalters force-pushed the powerpc64 branch from e762cf6 to 47e882d Compare July 13, 2024 17:35

cgwalters mentioned this pull request Jul 13, 2024

install: Use zipl to install bootloader on s390x #665

Merged

cgwalters force-pushed the powerpc64 branch from 47e882d to dc9d4cc Compare July 15, 2024 12:33

cgwalters force-pushed the powerpc64 branch from dc9d4cc to d81612c Compare July 15, 2024 17:22

cgwalters force-pushed the powerpc64 branch from d81612c to 8e236b8 Compare July 16, 2024 20:27

sacsant force-pushed the powerpc64 branch from 8e236b8 to 7f90550 Compare July 17, 2024 09:14

cgwalters approved these changes Jul 17, 2024

View reviewed changes

cgwalters enabled auto-merge July 17, 2024 12:17

cgwalters merged commit ffbce7d into containers:main Jul 17, 2024
25 of 27 checks passed

sacsant deleted the powerpc64 branch July 23, 2024 05:53

cgwalters pushed a commit to cgwalters/bootc that referenced this pull request Nov 6, 2024

Merge pull request containers#667 from cgwalters/release

f243445

lib: Release 0.15.1

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lib/bootloader: Write to PReP partition on ppc64le #667

lib/bootloader: Write to PReP partition on ppc64le #667

sacsant commented Jul 4, 2024

sacsant commented Jul 4, 2024

cgwalters left a comment

cgwalters Jul 6, 2024

sacsant Jul 8, 2024

cgwalters Jul 6, 2024

cgwalters Jul 6, 2024

sacsant Jul 8, 2024 •

edited

Loading

cgwalters Jul 12, 2024

sacsant Jul 12, 2024

cgwalters Jul 6, 2024

sacsant commented Jul 8, 2024 •

edited

Loading

cgwalters commented Jul 8, 2024

sacsant commented Jul 11, 2024

yoheiueda commented Jul 11, 2024

sacsant commented Jul 12, 2024

cgwalters Jul 12, 2024

cgwalters commented Jul 12, 2024

sacsant commented Jul 13, 2024 •

edited

Loading

cgwalters commented Jul 13, 2024

sacsant commented Jul 14, 2024

cgwalters commented Jul 15, 2024

sacsant commented Jul 15, 2024

cgwalters commented Jul 15, 2024

cgwalters commented Jul 15, 2024

cgwalters commented Jul 15, 2024

cgwalters commented Jul 15, 2024

sacsant commented Jul 16, 2024

sacsant commented Jul 16, 2024

cgwalters commented Jul 16, 2024

cgwalters commented Jul 16, 2024

cgwalters commented Jul 16, 2024

sacsant commented Jul 17, 2024

cgwalters commented Jul 17, 2024

lib/bootloader: Write to PReP partition on ppc64le #667

lib/bootloader: Write to PReP partition on ppc64le #667

Conversation

sacsant commented Jul 4, 2024

sacsant commented Jul 4, 2024

cgwalters left a 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

sacsant Jul 8, 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

sacsant commented Jul 8, 2024 • edited Loading

cgwalters commented Jul 8, 2024

sacsant commented Jul 11, 2024

yoheiueda commented Jul 11, 2024

sacsant commented Jul 12, 2024

Choose a reason for hiding this comment

cgwalters commented Jul 12, 2024

sacsant commented Jul 13, 2024 • edited Loading

cgwalters commented Jul 13, 2024

sacsant commented Jul 14, 2024

cgwalters commented Jul 15, 2024

sacsant commented Jul 15, 2024

cgwalters commented Jul 15, 2024

cgwalters commented Jul 15, 2024

cgwalters commented Jul 15, 2024

cgwalters commented Jul 15, 2024

sacsant commented Jul 16, 2024

sacsant commented Jul 16, 2024

cgwalters commented Jul 16, 2024

cgwalters commented Jul 16, 2024

cgwalters commented Jul 16, 2024

sacsant commented Jul 17, 2024

cgwalters commented Jul 17, 2024

sacsant Jul 8, 2024 •

edited

Loading

sacsant commented Jul 8, 2024 •

edited

Loading

sacsant commented Jul 13, 2024 •

edited

Loading