Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bootc-fstab-edit: 2 small improvements #1113

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
+35 −7
Open

bootc-fstab-edit: 2 small improvements #1113

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1343980
fixup_etc_fstab: comment `/` when using defaults
champtar Feb 14, 2025
fcf131b
bootc-fstab-edit: improve service dependencies
champtar Feb 14, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 32 additions & 6 deletions lib/src/deploy.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -849,11 +849,19 @@ pub(crate) fn fixup_etc_fstab(root: &Dir) -> Result<()> {
}
// If options already contains `ro`, nothing to do
if options.split(',').any(|s| s == "ro") {
println!("/etc/fstab `/` already contains `ro`, nothing to do");
return Ok(false);
}

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

// If options == `defaults`, comment the whole line
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Author

@champtar champtar Feb 14, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Copy link
Collaborator

@cgwalters cgwalters Feb 14, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

@champtar champtar Feb 14, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

@champtar champtar Feb 14, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 options == "defaults" {
writeln!(w, "# {line}")?;
println!("Updated /etc/fstab to comment `/` line");
return Ok(true);
}

// SAFETY: we unpacked the options before.
// This adds `ro` to the option list
assert!(!options.is_empty()); // Split wouldn't have turned this up if it was empty
Expand All @@ -871,6 +879,7 @@ pub(crate) fn fixup_etc_fstab(root: &Dir) -> Result<()> {
}
// And add the trailing newline
writeln!(w)?;
println!("Updated /etc/fstab to add `ro` for `/`");
Ok(true)
}

Expand All @@ -886,7 +895,6 @@ pub(crate) fn fixup_etc_fstab(root: &Dir) -> Result<()> {
})
.context("Replacing /etc/fstab")?;

println!("Updated /etc/fstab to add `ro` for `/`");
Ok(())
}

Expand Down Expand Up @@ -934,7 +942,7 @@ mod tests {
}

#[test]
fn test_fixup_etc_fstab_default() -> Result<()> {
fn test_fixup_etc_fstab_no_root() -> Result<()> {
let tempdir = cap_std_ext::cap_tempfile::tempdir(cap_std::ambient_authority())?;
let default = "UUID=f7436547-20ac-43cb-aa2f-eac9632183f6 /boot auto ro 0 0\n";
tempdir.create_dir_all("etc")?;
Expand All @@ -945,7 +953,7 @@ mod tests {
}

#[test]
fn test_fixup_etc_fstab_multi() -> Result<()> {
fn test_fixup_etc_fstab_no_root_multi() -> Result<()> {
let tempdir = cap_std_ext::cap_tempfile::tempdir(cap_std::ambient_authority())?;
let default = "UUID=f7436547-20ac-43cb-aa2f-eac9632183f6 /boot auto ro 0 0\n\
UUID=6907-17CA /boot/efi vfat umask=0077,shortname=winnt 0 2\n";
Expand All @@ -957,7 +965,7 @@ UUID=6907-17CA /boot/efi vfat umask=0077,shortname=win
}

#[test]
fn test_fixup_etc_fstab_ro() -> Result<()> {
fn test_fixup_etc_fstab_root_ro() -> Result<()> {
let tempdir = cap_std_ext::cap_tempfile::tempdir(cap_std::ambient_authority())?;
let default = "UUID=f7436547-20ac-43cb-aa2f-eac9632183f6 /boot auto ro 0 0\n\
UUID=1eef9f42-40e3-4bd8-ae20-e9f2325f8b52 / xfs ro 0 0\n\
Expand All @@ -970,15 +978,33 @@ UUID=6907-17CA /boot/efi vfat umask=0077,shortname=win
}

#[test]
fn test_fixup_etc_fstab_rw() -> Result<()> {
fn test_fixup_etc_fstab_root_defaults() -> Result<()> {
let tempdir = cap_std_ext::cap_tempfile::tempdir(cap_std::ambient_authority())?;
// This case uses `defaults`
let default = "UUID=f7436547-20ac-43cb-aa2f-eac9632183f6 /boot auto ro 0 0\n\
UUID=1eef9f42-40e3-4bd8-ae20-e9f2325f8b52 / xfs defaults 0 0\n\
UUID=6907-17CA /boot/efi vfat umask=0077,shortname=winnt 0 2\n";
let modified = "UUID=f7436547-20ac-43cb-aa2f-eac9632183f6 /boot auto ro 0 0\n\
# Updated by bootc-fstab-edit.service\n\
UUID=1eef9f42-40e3-4bd8-ae20-e9f2325f8b52 / xfs defaults,ro 0 0\n\
#UUID=1eef9f42-40e3-4bd8-ae20-e9f2325f8b52 / xfs defaults 0 0\n\
UUID=6907-17CA /boot/efi vfat umask=0077,shortname=winnt 0 2\n";
tempdir.create_dir_all("etc")?;
tempdir.atomic_write("etc/fstab", default)?;
fixup_etc_fstab(&tempdir).unwrap();
assert_eq!(tempdir.read_to_string("etc/fstab")?, modified);
Ok(())
}

#[test]
fn test_fixup_etc_fstab_root_non_defaults() -> Result<()> {
let tempdir = cap_std_ext::cap_tempfile::tempdir(cap_std::ambient_authority())?;
// This case uses `defaults`
let default = "UUID=f7436547-20ac-43cb-aa2f-eac9632183f6 /boot auto ro 0 0\n\
UUID=1eef9f42-40e3-4bd8-ae20-e9f2325f8b52 / xfs defaults,x-systemd.device-timeout=0 0 0\n\
UUID=6907-17CA /boot/efi vfat umask=0077,shortname=winnt 0 2\n";
let modified = "UUID=f7436547-20ac-43cb-aa2f-eac9632183f6 /boot auto ro 0 0\n\
# Updated by bootc-fstab-edit.service\n\
UUID=1eef9f42-40e3-4bd8-ae20-e9f2325f8b52 / xfs defaults,x-systemd.device-timeout=0,ro 0 0\n\
UUID=6907-17CA /boot/efi vfat umask=0077,shortname=winnt 0 2\n";
tempdir.create_dir_all("etc")?;
tempdir.atomic_write("etc/fstab", default)?;
Expand Down
4 changes: 3 additions & 1 deletion lib/src/generator.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,16 +72,18 @@ fn generate_fstab_editor(unit_dir: &Dir) -> Result<()> {
EDIT_UNIT,
"[Unit]\n\
DefaultDependencies=no\n\
Conflicts=shutdown.target\n\
After=systemd-fsck-root.service\n\
Before=local-fs-pre.target local-fs.target shutdown.target systemd-remount-fs.service\n\
Wants=local-fs-pre.target\n\
\n\
[Service]\n\
Type=oneshot\n\
RemainAfterExit=yes\n\
ExecStart=bootc internals fixup-etc-fstab\n\
",
)?;
let target = "local-fs-pre.target.wants";
let target = "local-fs.target.wants";
unit_dir.create_dir_all(target)?;
unit_dir.symlink(&format!("../{EDIT_UNIT}"), &format!("{target}/{EDIT_UNIT}"))?;
Ok(())
Expand Down