bootc-fstab-edit: 2 small improvements #1113

champtar · 2025-02-14T03:19:39Z

fixup_etc_fstab: comment / when using defaults

If we 'upgrade' to an image without composefs or if we runtime
disable composefs, remounting ro will break the system.
If option field is `defaults` lets comment the line instead.

bootc-fstab-edit: improve service dependencies

Looking at the target definitions:
- multi-user.target Requires basic.target
- basic.target Requires sysinit.target
- sysinit.target Wants local-fs.target
- local-fs.target is After local-fs-pre.target

local-fs-pre.target is wanted by some services, but it's indirect,
so it's cleaner to be WantedBy local-fs.target and Wants local-fs-pre.target.

While at it add Conflicts=shutdown.target.

lib/src/generator.rs

lib/src/deploy.rs

cgwalters · 2025-02-14T13:50:00Z

lib/src/deploy.rs

            return Ok(false);
        }

        writeln!(w, "# {}", crate::generator::BOOTC_EDITED_STAMP)?;

+        // If options == `defaults`, comment the whole line


If we 'upgrade' to an image without composefs or if we runtime disable composefs, remounting ro will break the system.
If option field is defaults lets comment the line instead.

OK so just to make sure we're on the same page:

The goal here is to support rollbacks to a pre-composefs state (or as you say handle runtime disabling of composefs (but IMO "why?" is the first question there...I would really like to move to having bootc hard require it))

This patch will only help in the case where the mount options are defaults

Is that right? (Mainly the second part)

If so...yes, I'm fine with this, we can merge it.

But the more I think about this the more I feel a stronger fix is for us to change the generator to actually hard switch to rootflags in the kargs too. That would fix the non-defaults mount option case (which is an important one because it's common to have root mount arguments for e.g. btrfs).

If we 'upgrade' to an image without composefs or if we runtime disable composefs, remounting ro will break the system.
If option field is defaults lets comment the line instead.

OK so just to make sure we're on the same page:

* The goal here is to support rollbacks to a pre-composefs state (or as you say handle runtime disabling of composefs (but IMO "why?" is the first question there...I would really like to move to having bootc hard require it)) * This patch will only help in the case where the mount options are `defaults`

Is that right? (Mainly the second part)

correct, defaults already handles most non btrfs use cases I guess (my personal focus is RHEL/Alma, not Fedora)

the why is because rollback is a must for how we use rpm-ostree today, if a bug is discovered in our appliance, support team needs to be able to bisect when the bug appeared, so "upgrading" to old version is a must. As for runtime disabling of composefs not sure it make sense indeed.

If so...yes, I'm fine with this, we can merge it.

But the more I think about this the more I feel a stronger fix is for us to change the generator to actually hard switch to rootflags in the kargs too. That would fix the non-defaults mount option case (which is an important one because it's common to have root mount arguments for e.g. btrfs).

So something like:

rootflags is present -> comment the / line

rootflags is absent -> set rootflags to the current option (for the current deployment only) and comment the / line ?

Do we have the option to edit kargs inplace just for the current deployment ?

Do we have the option to edit kargs inplace just for the current deployment ?

Yes, definitely. There's an intentionally-obscure command ostree admin instutil set-kargs because part of the philosophy here is that deployments should be "immutable" mostly, and if you change kargs you want a rollback.

But it's just using a public API https://github.com/ostreedev/ostree/blob/82b660b12dd886439912cc38d1f6316f32ec8599/src/ostree/ot-admin-instutil-builtin-set-kargs.c#L109

So to flesh this out then, I think in the non-defaults case, we would generate a systemd unit that does e.g.

ExecStart=bootc internals convert-to-rootflags and in that convert-to-rootflags code we'd do the karg mutations.

IMO we are abusing generators here, there is nothing dynamic, if we want a check before running we can use ExecCondition=, but we could just always run and say there is nothing to do, so in the end we run bootc only once.

@cgwalters what do you think of

[Unit] DefaultDependencies=no Conflicts=shutdown.target After=systemd-fsck-root.service Before=local-fs-pre.target local-fs.target shutdown.target systemd-remount-fs.service Wants=local-fs-pre.target WantsMountsFor=/boot /boot/efi MountFlags=slave ConditionKernelCommandLine=ostree ConditionKernelCommandLine=!rootflags ConditionPathIsReadWrite=!/ ConditionPathIsReadWrite=/etc ConditionPathExists=/etc/fstab [Service] Type=oneshot RemainAfterExit=yes ExecStart=bootc internals fixup-etc-fstab

Then in fixup-etc-fstab we always set rootflags if missing (set to defaults if there is no / line), and always comment the / line.
With all the conditions we don't need a generator anymore.

IMO we are abusing generators here, there is nothing dynamic, if we want a check before running we can use ExecCondition=, but we could just always run and say there is nothing to do, so in the end we run bootc only once.

In the end my goal in using generators for this is mostly cosmetic - it means we don't display the "Starting bootc-fstab fixup" log message to the console (or the journal) if we don't have anything to do.

In a quick test, it looks like even with a unit that does ExecCondition=/bin/sh -c 'exit 1' we do still log the starting message to the journal and the console.

With all the conditions we don't need a generator anymore.

I am not opposed if you want to do that, but I am not excited by it either.

If we 'upgrade' to an image without composefs or if we runtime disable composefs, remounting ro will break the system. If option field is `defaults` lets comment the line instead. Signed-off-by: Etienne Champetier <[email protected]>

Looking at the target definitions: - multi-user.target Requires basic.target - basic.target Requires sysinit.target - sysinit.target Wants local-fs.target - local-fs.target is After local-fs-pre.target local-fs-pre.target is wanted by some services, but it's indirect, so it's cleaner to be WantedBy local-fs.target and Wants local-fs-pre.target. While at it add Conflicts=shutdown.target. Signed-off-by: Etienne Champetier <[email protected]>

cgwalters · 2025-02-14T15:17:43Z

That said this one also intersects with #640 (I probably mentioned this before)

If we had that, then we could easily do this before rebooting into the new target OS, and not have to worry about mutating the system live with a generator.

champtar · 2025-02-14T18:39:33Z

That said this one also intersects with #640 (I probably mentioned this before)

If we had that, then we could easily do this before rebooting into the new target OS, and not have to worry about mutating the system live with a generator.

agreed, but until we have #640 for some time, we have the ratcheting issue again.

cgwalters · 2025-03-07T17:55:26Z

I was thinking about this more and actually there's another obvious choice between:

pre-reboot
boot

We can move the bootc generator and fstab-editing logic to the initramfs instead.

This would require us to ship a dracut module etc. and we should probably try to create a separate small low-dependency binary for our code that runs in the initramfs. So some prep work to do. But it'd be way more elegant than the generator, and also set us up for doing more logic in the initramfs in the future. Maybe we take over some of what ostree-prepare-root.service is doing for example.

cgwalters reviewed Feb 14, 2025

View reviewed changes

lib/src/generator.rs Show resolved Hide resolved

cgwalters reviewed Feb 14, 2025

View reviewed changes

champtar added 2 commits February 14, 2025 09:56

fixup_etc_fstab: comment / when using defaults

1343980

If we 'upgrade' to an image without composefs or if we runtime disable composefs, remounting ro will break the system. If option field is `defaults` lets comment the line instead. Signed-off-by: Etienne Champetier <[email protected]>

champtar force-pushed the fstab-comment-line branch from 996e26d to fcf131b Compare February 14, 2025 14:57

champtar mentioned this pull request Feb 14, 2025

Add simple service to comment / in fstab ostreedev/ostree#3372

Open

This was referenced Feb 28, 2025

fstab cleanup of / does not work #971

Open

systemd-remount-fs.service fails on f42 disk images with composefs enabled fedora-iot/iot-distro#81

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bootc-fstab-edit: 2 small improvements #1113

bootc-fstab-edit: 2 small improvements #1113

champtar commented Feb 14, 2025

cgwalters Feb 14, 2025

champtar Feb 14, 2025 •

edited

Loading

cgwalters Feb 14, 2025 •

edited

Loading

cgwalters Feb 14, 2025

champtar Feb 14, 2025 •

edited

Loading

champtar Feb 14, 2025 •

edited

Loading

cgwalters Feb 20, 2025

cgwalters commented Feb 14, 2025

champtar commented Feb 14, 2025

cgwalters commented Mar 7, 2025 •

edited

Loading

bootc-fstab-edit: 2 small improvements #1113

Are you sure you want to change the base?

bootc-fstab-edit: 2 small improvements #1113

Conversation

champtar commented Feb 14, 2025

cgwalters Feb 14, 2025

Choose a reason for hiding this comment

champtar Feb 14, 2025 • edited Loading

Choose a reason for hiding this comment

cgwalters Feb 14, 2025 • edited Loading

Choose a reason for hiding this comment

cgwalters Feb 14, 2025

Choose a reason for hiding this comment

champtar Feb 14, 2025 • edited Loading

Choose a reason for hiding this comment

champtar Feb 14, 2025 • edited Loading

Choose a reason for hiding this comment

cgwalters Feb 20, 2025

Choose a reason for hiding this comment

cgwalters commented Feb 14, 2025

champtar commented Feb 14, 2025

cgwalters commented Mar 7, 2025 • edited Loading

champtar Feb 14, 2025 •

edited

Loading

cgwalters Feb 14, 2025 •

edited

Loading

champtar Feb 14, 2025 •

edited

Loading

champtar Feb 14, 2025 •

edited

Loading

cgwalters commented Mar 7, 2025 •

edited

Loading