sysroot: Support for directories instead of symbolic links in boot part #3359

igoropaniuk · 2024-12-19T14:43:58Z

Allow manipulating and updating /boot/loader entries under a normal directory, as well as using symbolic links.

For directories this uses renameat2 to do atomic swap of the loader directory in the boot partition. It fallsback to non-atomic rename. This stays atomic on filesystems supporting links but also provide a non-atomic behavior when filesystem does not provide any atomic alternative.

/boot/loader as a normal directory is needed by systemd-boot support, and can be stored under the EFI ESP vfat partition.

Tests were duplicated for simplicity reasons.

Based on the original implementation done by Valentin David [1].

[1] #1967

openshift-ci · 2024-12-19T14:44:10Z

Hi @igoropaniuk. Thanks for your PR.

I'm waiting for a ostreedev member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

igoropaniuk · 2024-12-19T16:56:46Z

This is based on the initial work done in the #1967 PR (looks like it stalled)
I'll push adjusted tests soon to cover this change.

cgwalters · 2024-12-19T19:29:58Z

Thanks so much for picking this back up!! I am very interested in systemd-boot support. But this is just one part of the picture as far as BLS type 1 spec.

Note that that spec added the srel file which I think we should use to dispatch on.

IOW if we find that file, it is an error if /boot/loader is a symlink. We should encourage installers to create that to signal compatibility (after this patch lands).

This is a tangentially related topic but I'd like to ask you: Have you investigated https://github.com/containers/bootc/ ? That's where most of my development work today is going. I will continue to help maintain ostree (and bootc hard depends on it today) but medium term I do plan to slowly sever that dependency (as we head towards composefs in particular, xref https://github.com/containers/composefs-rs/ for some cool PoC work). And especially that would mean here that we may end up doing BLS and UKI support in Rust in bootc, not here.

cgwalters · 2024-12-19T19:31:37Z

@igoropaniuk Based on your contributions I'd like to offer you reviewer-level access to ostree. This would mean both the right and some responsibility to review/merge PRs made by others.

src/libostree/ostree-sysroot-deploy.c

igoropaniuk · 2025-01-07T17:59:45Z

@cgwalters

@igoropaniuk Based on your contributions I'd like to offer you reviewer-level access to ostree. This would mean both the right and some responsibility to review/merge PRs made by others.

Sounds great, I will be happy to help with reviews!

This is a tangentially related topic but I'd like to ask you: Have you investigated https://github.com/containers/bootc/ ? That's where most of my development work today is going. I will continue to help maintain ostree (and bootc hard depends on it today) but medium term I do plan to slowly sever that dependency (as we head towards composefs in particular, xref https://github.com/containers/composefs-rs/ for some cool PoC work). And especially that would mean here that we may end up doing BLS and UKI support in Rust in bootc, not here.

Actually I had, but our existing OTA solution is based on Uptane (Aktualizr fork as a client) + OSTree, so my primary focus is on this software stack for now.

igoropaniuk · 2025-01-08T17:31:26Z

@cgwalters I've added additional commit for entries.srel support, let me know if you prefer to have everything in one commit (will squash them if needed)

igoropaniuk · 2025-01-21T13:37:49Z

@cgwalters I've added additional tests to cover proposed changes.

Let me know if there is anything else for me to do in the scope of this change

igoropaniuk · 2025-02-01T19:59:13Z

@cgwalters gentle ping, let me know if there is anything else for me to do

cgwalters

So sorry for the delay here and thanks for working on this again!

src/libostree/ostree-sysroot-deploy.c

src/libostree/ostree-sysroot.c

src/libostree/ostree-sysroot-deploy.c

igoropaniuk · 2025-03-26T15:25:29Z

@cgwalters Thank you for your review. I have addressed all your recently posted comments. Please let me know if there's anything else I need to do.

Allow manipulating and updating /boot/loader entries under a normal directory, as well as using symbolic links. For directories this uses `renameat2` to do atomic swap of the loader directory in the boot partition. It fallsback to non-atomic rename. This stays atomic on filesystems supporting links but also provide a non-atomic behavior when filesystem does not provide any atomic alternative. /boot/loader as a normal directory is needed by systemd-boot support, and can be stored under the EFI ESP vfat partition. Based on the original implementation done by Valentin David [1]. [1] ostreedev#1967 Signed-off-by: Ricardo Salveti <ricardo@foundries.io> Signed-off-by: Jose Quaresma <jose.quaresma@foundries.io> Signed-off-by: Igor Opaniuk <igor.opaniuk@foundries.io>

Add tests for boot/loader directory. Signed-off-by: Igor Opaniuk <igor.opaniuk@foundries.io>

Add support for standard-conformance marker file loader/entries.srel. There might be implementations of boot loading infrastructure that are also using the /loader/entries/ directory, but install files that do not follow the [1] specification. In order to minimize confusion, a boot loader implementation may place the file /loader/entries.srel next to the /loader/entries/ directory containing the ASCII string type1 (followed by a UNIX newline). Tools that need to determine whether an existing directory implements the semantics described here may check for this file and contents: if it exists and contains the mentioned string, it shall assume a standards-compliant implementation is in place. If it exists but contains a different string it shall assume other semantics are implemented. If the file does not exist, no assumptions should be made. [1] https://uapi-group.org/specifications/specs/boot_loader_specification/#type-1-boot-loader-entry-keys Signed-off-by: Igor Opaniuk <igor.opaniuk@foundries.io>

cgwalters · 2025-04-06T18:30:09Z

src/libostree/ostree-sysroot-deploy.c

+    return FALSE;
+
+  if (force_type1_semantics && loader_link)
+    return glnx_throw_errno_prefix (error, "type1 semantics, but boot/loader is symlink");


This should just be glnx_throw, there's no relevant errno set here

cgwalters · 2025-04-06T18:33:16Z

src/libostree/ostree-sysroot-deploy.c

+  if (TEMP_FAILURE_RETRY (symlinkat (".", self->sysroot_fd, "boot/boot")) < 0)
+    {
+      if (errno == EPERM)
+        return FALSE;


Again need to adhere to GError conventions; this case returns FALSE but leaves error unset. Honestly I think it's simplest to have this helper function not use gerror i.e.

// Create the symlink boot/boot. Sets `errno` on failure. static int create_bootself_link (OstreeSysroot *self) { return TEMP_FAILURE_RETRY (symlinkat (".", self->sysroot_fd, "boot/boot"); }

Then the callers can just directly check errno as they see fit?

cgwalters · 2025-04-06T18:34:35Z

src/libostree/ostree-sysroot-deploy.c

-  if (TEMP_FAILURE_RETRY (symlinkat (".", sysroot->sysroot_fd, "boot/boot")) < 0)
-    if (errno != EEXIST)
-      return glnx_throw_errno_prefix (error, "symlinkat");
+  (void)create_bootself_link (sysroot, cancellable, error);


See above; let's keep checking errors here, with my above suggestion this just changes to

if (create_bootself_link (sysroot) < 0 && errno != EEXIST) return glnx_throw_errno_prefix (error, "Creating bootself link);

cgwalters · 2025-04-06T18:36:39Z

src/libostree/ostree-sysroot-deploy.c

+  if (TEMP_FAILURE_RETRY (symlinkat (".", sysroot->sysroot_fd, "boot/boot")) < 0)
+    if (errno != EPERM && errno != EEXIST)
+      return glnx_throw_errno_prefix (error, "symlinkat");


Then this one is:

if (create_bootself_link (sysroot) < 0 && !G_IN_SET(errno, EEXIST, EPERM)) return glnx_throw_errno_prefix (error, "Creating bootself link);

cgwalters · 2025-04-06T18:49:51Z

src/libostree/ostree-sysroot-deploy.c

+ * rename it into place (via renameat2 RENAME_EXCHANGE).
+ */
+static gboolean
+prepare_new_bootloader_dir (OstreeSysroot *sysroot, int current_bootversion, int new_bootversion,


I was thinking about this whole thing more and it's not clear to me we need the concept of a "bootversion" at all in the case where we're doing a directory swap, do we?

We could easily have

boot/loader == current version

boot/loader.tmp == staging/temp dir

That would have been harder to do with the symlink logic but I think it'd be quite a simplification to the current code here. Especially then we can stop writing the special ostree_bootversion file into the loader directory right?

That would have been harder to do with the symlink logic

The directory name could have been random, or a hash of the content, and for the cleanup you just need to readlink before you rename, but we can't change now

I think it'd be quite a simplification to the current code here. Especially then we can stop writing the special ostree_bootversion file into the loader directory right?

Agreed, no need for bootversion when using directory swap

cgwalters · 2025-04-06T21:26:50Z

src/libostree/ostree-sysroot-deploy.c

    return FALSE;
+  if (errno == ENOENT)
+    loader_link = create_bootself_link (self, cancellable, error);


Hmm. In an ideal world we wouldn't get here...it's a bit tempting to ask that for the case of a user deploying this new version of ostree targeting an ESP we ask them to pre-create the link.

Or...actually another tempting thing to do is check the filesystem type for boot/loader and unconditionally require this.

Anyways...for now I think what I'd ask is we continue to properly check errors here...

OK I thought more about this and came up with
#3405
to start.

Then a lot of this code can start checking sysroot->boot_is_vfat instead of the return value of symlinkat; WDYT?

#3405 merged! Let me know what you think!

cgwalters · 2025-07-15T22:23:54Z

I pushed a rebased version of this to https://github.com/cgwalters/ostree/pull/new/symlinks-directories-support - I think I resolved the conflicts OK, but there's definitely some stuff here I want to look at more closely.

dbnicholson · 2025-08-15T16:14:32Z

I'd like to bring my issues with #1967 here:

I've thought about this several times as I would really like to have this in Endless to support our systemd-boot systems. What always makes me anxious is trying to figure out how to handle compatibility with the vast majority of our systems that have symlink based deployments. There are 2 main issues I'm concerned with:

The semantics of the deployment are different if loader is a symlink to a loader.N directory versus loader itself being a directory. What do the swapped loader.N directories mean in pure directory mode? I think the answer is that they don't really match the semantics of the symlink deployment. It's essentially a different algorithm even though it's seemingly only a small change from the symlink approach.
What do you do with systems that already have symlink based deployments? Potentially you could write a migration to the directory scheme, but that means you can't roll back to a system with a libostree that doesn't understand the directory scheme.

So, to me this requires a couple additional pieces of implementation and policy.

The directory deployment scheme is treated separately from the symlink deployment scheme. Your system can be in one scheme or the other, but the semantics are different and shouldn't be mixed.
If your system is already using a symlink based deployment, it should stay that way. You can opt in to a migration to the directory scheme, but this means you may potentially lose roll back targets. At Endless we'd do that at some major version check point and probably have to delete some old deployments to ensure users didn't try to roll back to a system where they'd be screwed. Or maybe we'd never migrate anyone because the downsides are too great.
If you're on a new deployment (i.e., no existing /boot/loader or /boot/ostree), then the directory scheme is preferred.

Does this PR consider any of these points?

champtar · 2025-08-20T18:49:12Z

src/libostree/ostree-sysroot.c

+    entries_path = g_strdup ("boot/loader/entries");
+  else
+    entries_path = g_strdup_printf ("boot/loader.%d/entries", bootversion);
+


Are we ever reading anything else than the current config ? ie boot/loader/entries

champtar · 2025-08-20T19:03:09Z

src/libostree/ostree-sysroot-deploy.c

+    {
+      /* New target is currently under the old/current version */
+      g_autofree char *new_target = g_strdup_printf ("loader.%d", current_bootversion);
+      if (glnx_renameat2_exchange (sysroot->boot_fd, new_target, sysroot->boot_fd, "loader") != 0)


glnx_renameat2_exchange swap the 2 directories, so you need to delete loader.%d after fsfreeze_thaw_cycle
(glnx_renameat only leaves 1 file on success)

champtar · 2025-08-20T19:05:47Z

@igoropaniuk can you rebase on main and address the current comments ?

champtar · 2025-08-20T19:17:01Z

src/libostree/ostree-sysroot-deploy.c

+  if (!_ostree_sysroot_ensure_boot_fd (sysroot, error))
+    return FALSE;
+
+  if (!glnx_fstatat_allow_noent (sysroot->boot_fd, "loader/entries.srel", &stbuf,


We need to create / write entries.srel each time we create a new loader dir ? Or we already copy the content of the current directory first ?

systemd-boot also reads loader/loader.conf and loader/entries/*.conf https://www.freedesktop.org/software/systemd/man/latest/loader.conf.html
either we need to just create loader/entries.tmp then rename to loader/entries.tmp (so we keep the content of loader at least), or we need to copy the full loader dir and just cleanup loader/entries/ostree-*.conf only to keep everything else

champtar · 2025-08-20T19:21:55Z

Looking at the PR I would say

So, to me this requires a couple additional pieces of implementation and policy.

* The directory deployment scheme is treated separately from the symlink deployment scheme. Your system can be in one scheme or the other, but the semantics are different and shouldn't be mixed.

yes

* If your system is already using a symlink based deployment, it should stay that way. You can opt in to a migration to the directory scheme, but this means you may potentially lose roll back targets. At Endless we'd do that at some major version check point and probably have to delete some old deployments to ensure users didn't try to roll back to a system where they'd be screwed. Or maybe we'd never migrate anyone because the downsides are too great.

yes (entries.srel containing "type1\n")

* If you're on a new deployment (i.e., no existing /boot/loader or /boot/ostree), then the directory scheme is preferred.

No, and this prevent downgrade, so at first I would personally prefer to use directories only when required (/boot is vfat)

openshift-ci bot added the needs-ok-to-test label Dec 19, 2024

github-actions bot added the area/prepare-root Issue relates to ostree-prepare-root label Dec 19, 2024

igoropaniuk force-pushed the symlinks-directories-support branch 2 times, most recently from bb7a7cb to 5be249e Compare December 19, 2024 15:22

cgwalters self-assigned this Dec 19, 2024

cgwalters self-requested a review December 19, 2024 19:30

cgwalters reviewed Dec 19, 2024

View reviewed changes

src/libostree/ostree-sysroot-deploy.c Show resolved Hide resolved

cgwalters mentioned this pull request Dec 19, 2024

bin/admin-upgrade: add kexec support #3362

Merged

cgwalters mentioned this pull request Jan 2, 2025

grub2 vs grub name coreos/bootupd#810

Open

igoropaniuk force-pushed the symlinks-directories-support branch from 5be249e to 11f4940 Compare January 8, 2025 17:29

igoropaniuk requested a review from cgwalters January 8, 2025 17:31

igoropaniuk force-pushed the symlinks-directories-support branch from 11f4940 to 76ffcf8 Compare January 21, 2025 13:36

igoropaniuk changed the title ~~[RFC] sysroot: Support for directories instead of symbolic links in boot part~~ sysroot: Support for directories instead of symbolic links in boot part Jan 21, 2025

igoropaniuk force-pushed the symlinks-directories-support branch from 76ffcf8 to 04fd02b Compare January 21, 2025 14:05

cgwalters reviewed Feb 4, 2025

View reviewed changes

This was referenced Mar 14, 2025

swap_bootloader not crash resistant #3392

Closed

RFE: systemd-boot bootc-dev/bootc#92

Open

igoropaniuk force-pushed the symlinks-directories-support branch 5 times, most recently from 341f55a to c0788dc Compare March 26, 2025 15:09

igoropaniuk force-pushed the symlinks-directories-support branch 3 times, most recently from c929308 to f567c5e Compare March 26, 2025 15:22

igoropaniuk requested a review from cgwalters March 26, 2025 15:24

ricardosalveti and others added 3 commits March 26, 2025 16:27

tests: directories instead of symbolic links in boot part

a2a67c8

Add tests for boot/loader directory. Signed-off-by: Igor Opaniuk <igor.opaniuk@foundries.io>

igoropaniuk force-pushed the symlinks-directories-support branch from f567c5e to d950b3e Compare March 26, 2025 15:27

cgwalters reviewed Apr 6, 2025

View reviewed changes

cgwalters mentioned this pull request May 12, 2025

Allow passing absolute paths to initrd and vmlinuz containers/composefs-rs#121

Merged

This was referenced Jun 25, 2025

Suggested layout for sysroot for bootc integration containers/composefs-rs#38

Open

Demonstrate a debian or arch base image bootc-dev/bootc#865

Open

cgwalters added this to the v2025.4 milestone Jul 10, 2025

champtar reviewed Aug 20, 2025

View reviewed changes

champtar mentioned this pull request Aug 20, 2025

Use rename of directories instead of symbolic links in boot partition. #1967

Closed

cgwalters mentioned this pull request Sep 2, 2025

composefs-native/boot: Handle systemd-boot and grub bootc-dev/bootc#1571

Merged

sysroot: Support for directories instead of symbolic links in boot part #3359

Are you sure you want to change the base?

sysroot: Support for directories instead of symbolic links in boot part #3359

Uh oh!

Conversation

igoropaniuk commented Dec 19, 2024

Uh oh!

openshift-ci bot commented Dec 19, 2024

Uh oh!

igoropaniuk commented Dec 19, 2024

Uh oh!

cgwalters commented Dec 19, 2024

Uh oh!

cgwalters commented Dec 19, 2024

Uh oh!

Uh oh!

igoropaniuk commented Jan 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

igoropaniuk commented Jan 8, 2025

Uh oh!

igoropaniuk commented Jan 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

igoropaniuk commented Feb 1, 2025

Uh oh!

cgwalters left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

igoropaniuk commented Mar 26, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cgwalters commented Jul 15, 2025

Uh oh!

dbnicholson commented Aug 15, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

champtar commented Aug 20, 2025

Uh oh!

champtar Aug 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

champtar commented Aug 20, 2025

Uh oh!

Uh oh!

igoropaniuk commented Jan 7, 2025 •

edited

Loading

igoropaniuk commented Jan 21, 2025 •

edited

Loading

champtar Aug 20, 2025 •

edited

Loading