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

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> symlinks

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> boot semantics semantics

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!

cgwalters · 2025-02-04T16:10:21Z

src/libostree/ostree-sysroot-deploy.c

+        }
+
+      /* Remove trailing newline symbol if there is any */
+      type[strcspn (type, "\n")] = 0;


Let's use either https://docs.gtk.org/glib/func.strchomp.html or just `if (g_str_has_prefix (type, "type1"))

cgwalters · 2025-02-04T16:13:10Z

src/libostree/ostree-sysroot-deploy.c

@@ -2444,13 +2551,57 @@ write_deployments_bootswap (OstreeSysroot *self, GPtrArray *new_deployments,
        return glnx_prefix_error (error, "Bootloader write config");
    }

-  if (!prepare_new_bootloader_link (self, self->bootversion, new_bootversion, cancellable, error))
+  /* Handle when boot/loader is a link (normal deployment) and as a normal directory (e.g. EFI/vfat)


Super minor but I tend to avoid the word "normal" for things like this; how about
/* Handle when boot/loader is a symlink (original ostree model) or is a directory (e.g. EFI/vfat) for recommended systemd type1 BLS */

cgwalters · 2025-02-04T19:09:51Z

src/libostree/ostree-sysroot.c

+          gsize len;
+          g_autofree char *version = glnx_file_get_contents_utf8_at (
+              self->sysroot_fd, "boot/loader/ostree_bootversion", &len, cancellable, error);
+          if (version == NULL)


First, at a mechanical level this leaves error set but returns successfully, which will trigger an assertion failure later if something else tries to set error.

If we want to ignore the error, then pass NULL instead of error above.

However, I'm uncertain about ignoring errors here. I think we just want to special case ENOENT (i.e. nonexistent) right?

Which we can do by combining ot_openat_ignore_enoent and glnx_fd_readall_utf8 on it.

cgwalters · 2025-02-04T19:10:48Z

src/libostree/ostree-sysroot.c

+          /* Loader is a directory, check version by reading ostree_bootversion */
+          gsize len;
+          g_autofree char *version = glnx_file_get_contents_utf8_at (
+              self->sysroot_fd, "boot/loader/ostree_bootversion", &len, cancellable, error);


Can we make this a #define at least and add an explanatory comment for it?

Hmm...do we even need this ostree_bootversion file at all with the directory swap model? I am trying to think what would break if we just always said the boot version is zero.

I haven't thought deeply yet but...it seems to me the "boot version" is effectively just an implementation detail of the symlink-swap approach because we needed two different names.

cgwalters · 2025-02-04T19:15:40Z

src/libostree/ostree-sysroot-deploy.c

+  if (errno == ENOENT)
+    {
+      /* When there is no loader, check if the fs supports symlink or not */
+      if (TEMP_FAILURE_RETRY (symlinkat (".", self->sysroot_fd, "boot/boot")) < 0)


Minor but can we factor out a helper function like create_bootself_link() or so that's shared with the version in prepare_new_bootloader_link()?

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

ricardosalveti and others added 3 commits January 21, 2025 15:01

tests: directories instead of symbolic links in boot part

8e47955

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

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

igoropaniuk commented Dec 19, 2024

openshift-ci bot commented Dec 19, 2024

igoropaniuk commented Dec 19, 2024

cgwalters commented Dec 19, 2024

cgwalters commented Dec 19, 2024

igoropaniuk commented Jan 7, 2025 •

edited

Loading

igoropaniuk commented Jan 8, 2025

igoropaniuk commented Jan 21, 2025 •

edited

Loading

igoropaniuk commented Feb 1, 2025

cgwalters left a comment

cgwalters Feb 4, 2025

cgwalters Feb 4, 2025

cgwalters Feb 4, 2025

cgwalters Feb 4, 2025

cgwalters Feb 4, 2025

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

Conversation

igoropaniuk commented Dec 19, 2024

openshift-ci bot commented Dec 19, 2024

igoropaniuk commented Dec 19, 2024

cgwalters commented Dec 19, 2024

cgwalters commented Dec 19, 2024

igoropaniuk commented Jan 7, 2025 • edited Loading

igoropaniuk commented Jan 8, 2025

igoropaniuk commented Jan 21, 2025 • edited Loading

igoropaniuk commented Feb 1, 2025

cgwalters left a comment

Choose a reason for hiding this comment

cgwalters Feb 4, 2025

Choose a reason for hiding this comment

cgwalters Feb 4, 2025

Choose a reason for hiding this comment

cgwalters Feb 4, 2025

Choose a reason for hiding this comment

cgwalters Feb 4, 2025

Choose a reason for hiding this comment

cgwalters Feb 4, 2025

Choose a reason for hiding this comment

igoropaniuk commented Jan 7, 2025 •

edited

Loading

igoropaniuk commented Jan 21, 2025 •

edited

Loading