sysroot: Support /boot on / for syslinux and uboot #1404

wmanley · 2018-01-10T11:36:45Z

This is #215 rebased.

We've been using this patch in production for over a year now. One caveat is that we set OSTREE_SYSROOT_DEBUG=boot-is-not-mount during ostree admin deploy. IIRC we do this so we can deploy onto systems that aren't currently using ostree to using ostree. It would fail otherwise because of the if (self->booted_deployment) check.

As requested by @cgwalters in #1401 (comment)

TODO:

Test the new implementation for real
Consider interaction with Support mounting /sysroot (and /boot) read-only #1767
Fix deploy with --sysroot

wmanley · 2018-01-10T11:47:58Z

Arguably it would be more general to replace have_boot_partition with something like boot_prefix. This could be derived from /proc/self/mountinfo.

Examples:

Here boot_prefix is /boot:

16 14 179:1 /boot /boot rw,relatime shared:3 - ext4 /dev/root rw,data=ordered

And here boot_prefix is /:

78 25 8:1 / /boot rw,relatime shared:28 - ext4 /dev/sda1 rw,discard,data=ordered

but if /boot is not listed in column 5 fall back to looking for /. In this instance boot_prefix would also be /boot:

78 25 8:1 / / rw,relatime shared:28 - ext4 /dev/sda1 rw,discard,data=ordered

A more flowery example. boot_prefix is /real-root/boot:

78 25 8:1 /real-root / rw,relatime shared:28 - ext4 /dev/sda1 rw,discard,data=ordered

So you'd use $5 to find the appropriate entry and then $4 points to where it is relative to the actual root of the filesystem. You might not find an entry with $5 == "/boot" in which case you'd fall back to $5 == "/" and appending boot to $4 to get boot_prefix.

vtolstov · 2018-01-10T14:21:19Z

I'm looking forward to it =))

cgwalters · 2018-01-10T14:38:55Z

Arguably it would be more general to replace have_boot_partition with something like boot_prefix. This could be derived from /proc/self/mountinfo.

We're already using libmount which parses all of this right? But yeah I think you're right we need to do that so that #1265 can be extended to the nobootpart case.

Just a sanity check here (though the reply might be better in the other issue) - would your systems be OK if libostree started adding a read-only bind mount over /boot in the nobootpart case?

cgwalters · 2018-01-11T14:51:18Z

While we have some basic unit testing for this I'd be a lot more comfortable if we did more "real world" testing using qemu etc.; just to clarify I don't expect you to write that for this patch and it's not a new issue.

So the updated patch looks fine (I pushed a tiny fixup to update the code style), but did you get a chance to look at this:

#215 (comment)

In your setup is that /boot bind mount kicking in or no?

cgwalters · 2018-01-12T19:37:29Z

One caveat is that we set OSTREE_SYSROOT_DEBUG=boot-is-not-mount during ostree admin deploy.

Hmm...if that's really necessary that'd break Anaconda for the rpm-ostree case. Or is this only in the nobootpart case?

I'm debating this...I'd like to get your changes in but I'd like some soak time for this one, and there are a lot of other changes already queued that I'd like to get out. A consequence a bit of the "single release train" model.

wmanley · 2018-01-15T15:54:04Z

We're already using libmount which parses all of this right?

Yeah. I looked into it and it seems that mnt_table_parse_mtab parses /proc/self/mountinfo. It would look something like:

libmnt_table * tb = mnt_new_table();
mnt_table_parse_mtab(tb, NULL);
libmnt_fs *fs = mnt_table_find_target(tb, "/boot", MNT_ITER_FORWARD);
const char *boot_path = mnt_fs_get_bindsrc (fs);
mnt_free_table(tb);

cgwalters · 2018-01-16T18:51:43Z

@wmanley
I'm a bit torn on this; we can merge as is - particularly if you're using it today. The fact that we don't yet support it in grub2 will be problematic for our current testing, but we can fix that down the line.

To repeat though, I am honestly still confused by #215 (comment) - do you have the /boot bind mount kicking in or not?

Anyways so to rephrase; if you want to merge as is that's OK by me, or we can try to polish more of the design and fix issues like the one you mention in your comment

Hmm and actually I just realized the "built in" grub generator does have this logic:

    # Default to /boot if OSTREE_BOOT_PARTITION is not set and /boot is on the same device than ostree/repo
    if [ -z ${OSTREE_BOOT_PARTITION+x} ] && [ -d /boot/ostree ] && [ -d /ostree/repo ] && [ $(stat -c '%d' /boot/ostree) -eq $(stat -c '%d' /ostree/repo) ]; then
        boot_prefix="/boot"
    else
        boot_prefix="${OSTREE_BOOT_PARTITION}"
    fi

So it's just the current non-builtin grub2 backend that doesn't.

wmanley · 2018-01-16T19:40:46Z

To repeat though, I am honestly still confused by #215 (comment) - do you have the /boot bind mount kicking in or not?

Yes, and that's probably why we set OSTREE_SYSROOT_DEBUG=boot-is-not-mount

Anyways so to rephrase; if you want to merge as is that's OK by me, or we can try to polish more of the design and fix issues like the one you mention in your comment

I think it shouldn't be merged as is.

wmanley · 2018-01-16T22:18:05Z

I've made the change we discussed, but I haven't tested it for real yet.

wmanley · 2018-01-17T15:09:13Z

I can confirm that this works for us on real hardware, and we no-longer have to set OSTREE_SYSROOT_DEBUG=boot-is-not-mount to make it work. Please review.

wmanley · 2018-01-17T15:29:14Z

Hmm compile error on c7-primary:

In file included from /usr/lib64/glib-2.0/include/glibconfig.h:9:0,
                 from /usr/include/glib-2.0/glib/gtypes.h:32,
                 from /usr/include/glib-2.0/glib/galloca.h:32,
                 from /usr/include/glib-2.0/glib.h:30,
                 from /usr/include/glib-2.0/gobject/gbinding.h:28,
                 from /usr/include/glib-2.0/glib-object.h:23,
                 from /usr/include/glib-2.0/gio/gioenums.h:28,
                 from /usr/include/glib-2.0/gio/giotypes.h:28,
                 from /usr/include/glib-2.0/gio/gio.h:26,
                 from /usr/include/gio-unix-2.0/gio/gunixinputstream.h:24,
                 from src/libostree/ostree-sysroot-deploy.c:22:
src/libostree/ostree-sysroot-deploy.c: In function 'glib_autoptr_cleanup_libmnt_fs':
src/libostree/ostree-sysroot-deploy.c:2043:43: error: 'mnt_unref_fs' undeclared (first use in this function)
 G_DEFINE_AUTOPTR_CLEANUP_FUNC (libmnt_fs, mnt_unref_fs);
                                           ^
/usr/include/glib-2.0/glib/gmacros.h:444:88: note: in definition of macro 'G_DEFINE_AUTOPTR_CLEANUP_FUNC'
   static inline void _GLIB_AUTOPTR_FUNC_NAME(TypeName) (TypeName **_ptr) { if (*_ptr) (func) (*_ptr); }         \
                                                                                        ^
src/libostree/ostree-sysroot-deploy.c:2043:43: note: each undeclared identifier is reported only once for each function it appears in
 G_DEFINE_AUTOPTR_CLEANUP_FUNC (libmnt_fs, mnt_unref_fs);
                                           ^
/usr/include/glib-2.0/glib/gmacros.h:444:88: note: in definition of macro 'G_DEFINE_AUTOPTR_CLEANUP_FUNC'
   static inline void _GLIB_AUTOPTR_FUNC_NAME(TypeName) (TypeName **_ptr) { if (*_ptr) (func) (*_ptr); }         \
                                                                                        ^

wmanley · 2018-01-17T15:33:42Z

Perhaps that's what

ostree/src/libostree/ostree-sysroot-deploy.c

Lines 1997 to 2003 in 720e2ec

    
           #ifdef HAVE_MNT_UNREF_CACHE 
        
             mnt_unref_table (tb); 
        
             mnt_unref_cache (cache); 
        
           #else 
        
             mnt_free_table (tb); 
        
             mnt_free_cache (cache); 
        
           #endif

is all about?

Perhaps, perhaps not. See cee57a0

cgwalters · 2018-01-17T15:39:31Z

src/libostree/ostree-sysroot-deploy.c

@@ -2079,7 +2079,10 @@ boot_find_path_on_disk(char** path_out, GError **error)
  *path_out = g_steal_pointer(&path);
  return TRUE;
 #else /* !HAVE_LIBMOUNT */
-  return g_strdup("");
+  g_printerr ("warning: ostree compiled without libmount so we can't determine "


Hm. It feels like we should make libmount a hard dependency but also add a --disable-ostree-as-host build option or something so that the people who just want libostree for flatpak can disable it?

Or alternatively just make it a hard dependency across the board, I can't really think of a case where someone would have an issue shipping libmount.

wmanley · 2018-01-17T16:02:35Z

I've squashed and force-pushed because fixing the 'mnt_unref_fs' undeclared error was going to get really confusing with more fixups.

wmanley · 2018-01-17T16:33:59Z

Hmm, it seems that with each fix the builders get angrier and angrier. I'll need to do a bit more work on this to get them all passing. I'll retest on my device once the builders are happy.

vtolstov · 2018-03-12T22:55:46Z

any news? does it possible to complete this pr and get it merged?
p.s. sorry for noise =(

rh-atomic-bot · 2018-03-26T16:58:30Z

☔ The latest upstream changes (presumably 2648c96) made this pull request unmergeable. Please resolve the merge conflicts.

vtolstov · 2019-06-30T11:29:28Z

@wmanley can you resolve conflicts to get this merged?

clime · 2020-02-15T12:41:52Z

Any progress here? I would like to specifically see support for grub2 with respect to this issue.

In preparation for adding another function that will need these `AUTOPTR_CLEANUP_FUNC`s.

People making smaller operating systems, particularly things like cloud images, don't need to have /boot as a separate partition.

openshift-ci-robot · 2020-07-02T12:32:44Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: wmanley
To complete the pull request process, please assign jlebon
You can assign the PR to them by writing /assign @jlebon in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

wmanley · 2020-07-02T12:36:18Z

I've rebased on master. I've not implemented this for the new zipl bootloader as I don't know enough about it, but it will at least error for unsupported configurations. I'll be testing with syslinux bootloader shortly.

wmanley · 2020-07-02T14:04:53Z

src/libostree/ostree-sysroot-deploy.c

+    return glnx_throw (error, "Failed to parse /proc/self/mountinfo");
+
+  const char *suffix = NULL;
+  struct libmnt_fs *fs = mnt_table_find_target(tb, "/boot", MNT_ITER_BACKWARD);


TODO: Check for interactions with #1767. This might confuse /boot as bind-mount with /boot on own FS.

Also: This currently won't work for ostree admin deploy --sysroot=xxx. Sometimes this is useful for embedded development where I'm deploying to an SD card and then plugging it into the embedded hardware for boot.

I've got some ideas on how to fix this. I think I've implemented it in the past (in python), but I can't remember where. I'll have a look.

wmanley · 2020-07-06T11:14:09Z

I'm working through trying to get this working for all cases. Here are the ones I've enumerated:

Deploy to currently booted ostree system - This is the case when doing ostree admin upgrade and as such it's the common case.
Deploy to currently booted non-ostree system - We do this when ostreeifying systems with an existing OS installed.
Deploy to filesystem image or mounted device - This is a recent use-case for us. We deploy to an SD card attached to a dev laptop and then plug it in the real device to boot. This involves mounting the filesystem on the laptop and setting --sysroot when deploying.
Deploy to directory to be copied onto real system, perhaps using rsync or as an input to another provisioning tool - This would also use the --sysroot option. This isn't something I personally use, but it's what the ostree tests currently test. It seems like a valid use-case but I can't see how this could be auto-configured as there isn't necessarily any information available at deploy time about how the final system will be configured.

I'll try to write test cases for each of these situations.

Because the location of /boot can't be figured out automatically in the general case I'll add a configuration variable to allow it to be explicitly controlled. This can already be configured to some extent with environment variables, but I think it would be better to have this configuration attached to an ostree sysroot, rather than having to remember to set the env-vars for every deploy.

One thing that makes the --sysroot cases more tricky is that libmount doesn't have support for passing a single sysroot variable. There are various APIs that allow loading specific files, but they also consult environment variables and have fallbacks to hard-coded locations. I don't particularly fancy patching libmount for this use case at this time though.

wmanley · 2020-07-06T11:27:32Z

We're trying to guess where the bootloader will look for the contents of /boot. One method for figuring this out is to ask:

What is the boot device?
Where is /boot relative to the root of that device

This is what the current version of the patch does. This doesn't work for use case (4) above because we can't answer "what is the boot device" when doing a --sysroot deploy onto an unrelated filesystem. Unfortunately this is the case the current tests test.

cgwalters · 2020-08-18T23:43:59Z

Closing since #2149 merged.

wmanley mentioned this pull request Jan 10, 2018

Support for booting without initramfs (rebased) #1401

Closed

cgwalters mentioned this pull request Jan 10, 2018

sysroot: Support /boot on / for syslinux and uboot #215

Closed

wmanley mentioned this pull request Jan 12, 2018

Add support for devicetree files alongside the kernel and initramfs #1411

Closed

wmanley force-pushed the bootloader-no-boot-partition branch 4 times, most recently from 7d58a56 to c5f613f Compare January 17, 2018 15:05

cgwalters reviewed Jan 17, 2018

View reviewed changes

wmanley force-pushed the bootloader-no-boot-partition branch from dc14cb8 to d100878 Compare January 17, 2018 15:57

wmanley force-pushed the bootloader-no-boot-partition branch from d100878 to 0578449 Compare January 17, 2018 16:09

cgwalters mentioned this pull request Feb 11, 2018

support /boot in the root filesystem; i.e. no separate /boot #1452

Closed

palvarez89 mentioned this pull request Sep 5, 2019

Bootloader no boot partition #1914

Closed

openshift-ci-robot added the needs-rebase label Feb 15, 2020

martinezjavier mentioned this pull request Jun 16, 2020

after switch to bls system not boot coreos/rpm-ostree#2121

Open

wmanley and others added 2 commits July 2, 2020 12:55

Refactor: Port is_ro_mount to new style

6f0c6b6

In preparation for adding another function that will need these `AUTOPTR_CLEANUP_FUNC`s.

sysroot: Support /boot on / for syslinux and uboot

cceda15

People making smaller operating systems, particularly things like cloud images, don't need to have /boot as a separate partition.

wmanley force-pushed the bootloader-no-boot-partition branch from 0578449 to cceda15 Compare July 2, 2020 12:32

openshift-ci-robot removed the needs-rebase label Jul 2, 2020

wmanley commented Jul 2, 2020

View reviewed changes

wmanley mentioned this pull request Jul 8, 2020

sysroot: Support customising /boot on root for syslinux and u-boot #2145

Closed

wmanley mentioned this pull request Jul 15, 2020

sysroot: Support /boot on root or as seperate filesystem for syslinux and u-boot #2149

Merged

cgwalters closed this Aug 18, 2020

wmanley deleted the bootloader-no-boot-partition branch September 1, 2020 13:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sysroot: Support /boot on / for syslinux and uboot #1404

sysroot: Support /boot on / for syslinux and uboot #1404

wmanley commented Jan 10, 2018 •

edited

Loading

wmanley commented Jan 10, 2018

vtolstov commented Jan 10, 2018

cgwalters commented Jan 10, 2018 •

edited

Loading

cgwalters commented Jan 11, 2018

cgwalters commented Jan 12, 2018 •

edited

Loading

wmanley commented Jan 15, 2018

cgwalters commented Jan 16, 2018

wmanley commented Jan 16, 2018

wmanley commented Jan 16, 2018

wmanley commented Jan 17, 2018

wmanley commented Jan 17, 2018

wmanley commented Jan 17, 2018 •

edited

Loading

cgwalters Jan 17, 2018

wmanley commented Jan 17, 2018

wmanley commented Jan 17, 2018 •

edited

Loading

vtolstov commented Mar 12, 2018

rh-atomic-bot commented Mar 26, 2018

vtolstov commented Jun 30, 2019

clime commented Feb 15, 2020

openshift-ci-robot commented Jul 2, 2020

wmanley commented Jul 2, 2020

wmanley Jul 2, 2020

wmanley commented Jul 6, 2020

wmanley commented Jul 6, 2020

cgwalters commented Aug 18, 2020

sysroot: Support /boot on / for syslinux and uboot #1404

sysroot: Support /boot on / for syslinux and uboot #1404

Conversation

wmanley commented Jan 10, 2018 • edited Loading

wmanley commented Jan 10, 2018

vtolstov commented Jan 10, 2018

cgwalters commented Jan 10, 2018 • edited Loading

cgwalters commented Jan 11, 2018

cgwalters commented Jan 12, 2018 • edited Loading

wmanley commented Jan 15, 2018

cgwalters commented Jan 16, 2018

wmanley commented Jan 16, 2018

wmanley commented Jan 16, 2018

wmanley commented Jan 17, 2018

wmanley commented Jan 17, 2018

wmanley commented Jan 17, 2018 • edited Loading

cgwalters Jan 17, 2018

Choose a reason for hiding this comment

wmanley commented Jan 17, 2018

wmanley commented Jan 17, 2018 • edited Loading

vtolstov commented Mar 12, 2018

rh-atomic-bot commented Mar 26, 2018

vtolstov commented Jun 30, 2019

clime commented Feb 15, 2020

openshift-ci-robot commented Jul 2, 2020

wmanley commented Jul 2, 2020

wmanley Jul 2, 2020

Choose a reason for hiding this comment

wmanley commented Jul 6, 2020

wmanley commented Jul 6, 2020

cgwalters commented Aug 18, 2020

wmanley commented Jan 10, 2018 •

edited

Loading

cgwalters commented Jan 10, 2018 •

edited

Loading

cgwalters commented Jan 12, 2018 •

edited

Loading

wmanley commented Jan 17, 2018 •

edited

Loading

wmanley commented Jan 17, 2018 •

edited

Loading