install: ommit mountspec kargs when --root-mount-spec is empty

[jbtrystram]

This is an attempt to address #1441

I don't really know this code base, so i am opening this to get some feedback.
In this state the kargs are effectivelty omitted from the grub BLS entry.

Here is how i tested it :

podman run --privileged --rm \
--pid=host \
-v ./rootfs:/target \
-v /dev:/dev \
-v /var/lib/containers:/var/lib/containers \
--security-opt label=type:unconfined_t \
oci-archive:/var/mnt/workdir/fcos-rawhide.ociarchive \
bootc install to-filesystem --generic-image \
--root-mount-spec= \
   --target-imgref ostree-remote-registry:fedora:quay.io/fedora/fedora-coreos:rawhide \
   --karg '$ignition_firstboot' --karg mitigations=auto,nosmt \
   --stateroot fedora-coreos \
   /target

Using coreos-assembler to get the build bootc binary inside the oci-archive, using the overrides/rootfs mechanism.
./rootfs is a disk image mounted through loopback with /boot and /boot/efi already mounted

[gemini-code-assist]

Code Review

Hi, thanks for working on this! This change to omit the root mount specification kernel arguments for generic images is a good improvement.

I've left a couple of comments on the implementation:

1. There's a critical issue in the kernel argument construction logic that likely causes the boot failure you observed. The boot= argument is unintentionally dropped when the root mount spec is empty.
2. The new test for the empty mount spec will panic. It should instead assert that an error is returned, which is the expected behavior for an invalid (empty) mount spec.

Addressing these should get the PR into a good state. Let me know if you have any questions!

P.S. There's a small typo in the PR title ('ommit' -> 'omit').

[gemini-code-assist]

Hi, thanks for this contribution! I see you're trying to omit the root mount-related kernel arguments when --root-mount-spec is empty.

There's a potential issue with the current implementation. The bootarg is only included in the else branch, which means it's omitted when root_info.mount_spec.is_empty(). The boot= kernel argument is important for features like FIPS compliance, and omitting it might be the reason the generated image fails to boot, as you mentioned in the PR description.

I'd suggest restructuring the logic to ensure bootarg and RW_KARG are always added, while the root-specific arguments are conditional. This also makes the logic a bit easier to follow.

Here's a suggested change:

Suggested change

	// If the root mount spec is empty, we omit the mounts kargs entirely.
	// https://github.com/bootc-dev/bootc/issues/1441
	let mut kargs = if root_info.mount_spec.is_empty() {
	Vec::new()
	} else {
	[rootarg]
	.into_iter()
	.chain(root_info.kargs)
	.chain(bootarg)
	.collect::<Vec<_>>()
	};
	kargs.push(RW_KARG.to_string());
	// If the root mount spec is empty, we omit the mounts kargs entirely.
	// https://github.com/bootc-dev/bootc/issues/1441
	let mut kargs: Vec<String> = if !root_info.mount_spec.is_empty() {
	// Add root-related kargs
	std::iter::once(rootarg).chain(root_info.kargs).collect()
	} else {
	Vec::new()
	};
	// Always add boot and rw kargs
	kargs.extend(bootarg);
	kargs.push(RW_KARG.to_string());

[cgwalters]

Yeah. Also not that it matters at all but we could still use iterators here.

Also minor but it'd be nice to preserve the existing kernel argument ordering, i.e. keep RW_KARG before bootarg just to minimize changes.

[cgwalters]

In this state the kargs are effectivelty omitted from the grub BLS entry but the image it produces does not boot : the console stays blank after grub.

The reason the console is blank is we don't inject any console= lines by default in the default Fedora-derivative bootc base images.

And yes with an omitted root= we need some ambient knowledge (e.g. DPS which isn't used by to-disk by default, though we probably should) - or as is used in CoreOS the root label found in the initramfs.

So basically I'd just verify this works for you with CoreOS and we can skip tests here (though we could support reverse-dependency testing, would be cool to support triggering a cosa build from here!); also please update the documentation for the argument to note the change.

[jbtrystram]

The reason the console is blank is we don't inject any console= lines by default in the default Fedora-derivative bootc base images.

Yes I found that out an edited my message, but you reviewed in the meantime :)

So basically I'd just verify this works for you with CoreOS and we can skip tests here

It looks like an fstab entry is still created for boot somewhere, I need to investigate more, but it's the same behavior I had when manually deleting the kargs manually.

also please update the documentation for the argument to note the change.

Yes for sure, I plan to, I just wanted to get some feedback.
Thanks

[cgwalters]

I think this is directionally fine as is, but it feels to me like what we want (in addition) is a mechanism where the target image can drive this (which strongly relates also to systemd-repart as there the target knowledge is embedded in the image).

That could manifest say as an option in the bootc install config, then we update the FCOS images to set it, and install to-disk would always DTRT with that image or derivatives instead of people needing to know to set it.

[cgwalters]

It looks like an fstab entry is still created for boot somewhere, I need to investigate more, but it's the same behavior I had when manually deleting the kargs manually.

We also need support for --boot-mount-spec= right?

But also, related to this is #1388

[jbtrystram]

@cgwalters I updated to keep the kargs order and make the boot mount spec argument conditonnal on --boot-mount-spec.
I looked into keeping the iterators to buils the kargs but with the two different conditions it got a bit hard to read so I went with the more easier to read (and write) approach :)

Also updated the man page and a nicer commit message.

An FCOS rawhide image built with this nicely boot :) I'll run it through the tests to see if I find anything weird