ostree admin deploy fails if there are no existing deployments when a bootloader is detected

[ajeddeloh]

Version
2018.10

Expected behavior:
ostree admin deploy sets up a deployment and bootloader entries even if it is the first deployment

Actual behavior:
ostree admin deploy fails if there are no deployments and the grub2 bootloader is detected

Steps to reproduce:

mkdir sysroot
ostree admin init-fs sysroot
ostree pull-local <ostree repo> --repo sysroot/ostree/repo
ostree admin os-init osname --sysroot sysroot

# assuming only 1 ref
ref=$(ostree refs --repo sysroot/ostree/repo)

# trick ostree into thinking there is a bootloader installed
touch sysroot/boot/grub2/grub.cfg

# fails
ostree admin deploy "$ref" --sysroot sysroot --os osname

gives:

OSTree:ERROR:src/libostree/ostree-bootloader-grub2.c:356:_ostree_bootloader_grub2_write_config: assertion failed (deployments->len > 0): (0 > 0)

It appears it gets stuck in a catch-22 situation writing the bootloader entry where it uses an existing deployment to run grub-mkconfig rather than the pending deployment. Perhaps it should use the pending one in the case there are no completed deployments?

[cgwalters]

OK, I reproduced this locally. One trick one can do here btw on an existing ostree-based system is (assuming that /var is on the same filesystem), do:
cd /ostree/deploy/$stateroot/deploy/var
and then for the pull-local:

mkdir sysroot
ostree admin init-fs sysroot
cd sysroot
ostree pull-local --repo=sysroot/repo /sysroot/ostree/repo

That way it's just hardlinks and not copies and makes the process nearly free.

I had to do a pull by checksum and not ref though since the deployed system has a remote (pull-local should handle this though).

Anyways so what worked for me is to:

$ ostree admin deploy 89bfa708d349a5856cc5cd3be441c07e1f96d0be2aa97e2b676f6004e7f6abed --sysroot $(pwd) --os fcos --karg-proc-cmdline
$ touch boot/grub2/grub.cfg
$ ostree admin instutil set-kargs  --sysroot $(pwd) --append=foo=bar

Which is what Anaconda is doing today.

But...yes I think this code is buggy in that it should be looking at the pending deployments which are provided for the root to use for grub2-mkconfig.

[rfairley]

Can reproduce the behaviour here with the following commands with the assertion failing after running ostree admin deploy:

mkdir sysroot && ostree admin init-fs sysroot
ostree pull-local --repo=sysroot/ostree/repo /sysroot/ostree/repo
ostree admin os-init osname --sysroot sysroot
ref=$(ostree refs --repo sysroot/ostree/repo) && mkdir -p sysroot/boot/grub2 && touch sysroot/boot/grub2/grub.cfg
ostree admin deploy "$ref" --sysroot sysroot --os osname

Instead of failing when ostree is trying to write a new bootloader, and zero deployments of the given sysroot have been made - should ostree instead run grub2-mkconfig from the deployment that is about to happen?

From my understanding, grub2-mkconfig would use configuration at e.g. sysroot/ostree/deploy/osname/deploy/59ef65f66e92bf88c20c857907347a566feb194465d45b0fa0be1c8552a8adce.0/etc/grub.d) and generate a file at sysroot/boot/grub2/grub.cfg.

Going to read/experiment a bit more. Does the above sound like the right idea?

[cgwalters]

I think my vote here is that if we're going to rework things, let's take the bigger step of not running grub2-mkconfig at all in ostree. For Fedora, that's https://fedoraproject.org/wiki/Changes/BootLoaderSpecByDefault

[dustymabe]

I definitely want to get there but I don't think it's clear how much work is involved for the BLS work. Since this particular issue is blocking other work maybe we can get a tactical fix for now and then work on the BLS part right after?

[rfairley]

Interesting re the BLS, +1 on working towards that.

I'll focus on getting something quick up for review. Can see how much work is involved after that for the BLS, and maybe create another issue for that?

[dustymabe]

I'll focus on getting something quick up for review. Can see how much work is involved after that for the BLS, and maybe create another issue for that?

+1 thanks @rfairley. As for another issue I think #717 might cover it? lean on @ajeddeloh @jlebon and @LorbusChris for the BLS fragment work as I think they've all dabbled in it briefly.

[rfairley]

In #1814 , the decision was to not change the grub2 bootloader backend and instead provide the option of not going through the code path. I think I should have left this issue open - as with the default setting (sysroot.bootloader="auto") the behaviour described here still happens.

The diff below (also linked here) was a start at solving this bug directly.

Going to re-open (though feel free to close if there are any objections).

diff --git a/src/libostree/ostree-bootloader-grub2.c b/src/libostree/ostree-bootloader-grub2.c
index 57673c7d6..8a6221959 100644
--- a/src/libostree/ostree-bootloader-grub2.c
+++ b/src/libostree/ostree-bootloader-grub2.c
@@ -332,6 +332,7 @@ grub2_child_setup (gpointer user_data)
 static gboolean
 _ostree_bootloader_grub2_write_config (OstreeBootloader      *bootloader,
                                        int                    bootversion,
+                                       GPtrArray         *new_deployments,
                                        GCancellable          *cancellable,
                                        GError               **error)
 {
@@ -363,9 +364,8 @@ _ostree_bootloader_grub2_write_config (OstreeBootloader      *bootloader,
 
       deployments = ostree_sysroot_get_deployments (self->sysroot);
 
-      g_assert_cmpint (deployments->len, >, 0);
-
-      tool_deployment = deployments->pdata[0];
+      g_assert_cmpint (new_deployments->len, > , 0);
+      tool_deployment = new_deployments->pdata[0];
 
       /* Sadly we have to execute code to generate the bootloader configuration.
        * If we're in a booted deployment, we just don't chroot.
@@ -380,6 +380,8 @@ _ostree_bootloader_grub2_write_config (OstreeBootloader      *bootloader,
       grub2_mkconfig_chroot = g_file_get_path (tool_deployment_root);
     }
 
+  g_debug ("Using grub2-mkconfig chroot: %s\n", grub2_mkconfig_chroot);
+
   g_autoptr(GFile) new_config_path = NULL;
   g_autoptr(GFile) config_path_efi_dir = NULL;
   if (self->is_efi)
diff --git a/src/libostree/ostree-bootloader-syslinux.c b/src/libostree/ostree-bootloader-syslinux.c
index 8b09a861b..59e6a6aa9 100644
--- a/src/libostree/ostree-bootloader-syslinux.c
+++ b/src/libostree/ostree-bootloader-syslinux.c
@@ -108,10 +108,11 @@ append_config_from_loader_entries (OstreeBootloaderSyslinux  *self,
 }
 
 static gboolean
-_ostree_bootloader_syslinux_write_config (OstreeBootloader          *bootloader,
-                                          int                    bootversion,
-                                          GCancellable          *cancellable,
-                                          GError               **error)
+_ostree_bootloader_syslinux_write_config (OstreeBootloader  *bootloader,
+                                          int               bootversion,
+                                          GPtrArray         *new_deployments,
+                                          GCancellable      *cancellable,
+                                          GError           **error)
 {
   OstreeBootloaderSyslinux *self = OSTREE_BOOTLOADER_SYSLINUX (bootloader);
 
diff --git a/src/libostree/ostree-bootloader-uboot.c b/src/libostree/ostree-bootloader-uboot.c
index 531a37560..87a938067 100644
--- a/src/libostree/ostree-bootloader-uboot.c
+++ b/src/libostree/ostree-bootloader-uboot.c
@@ -158,10 +158,11 @@ create_config_from_boot_loader_entries (OstreeBootloaderUboot     *self,
 }
 
 static gboolean
-_ostree_bootloader_uboot_write_config (OstreeBootloader          *bootloader,
-                                  int                    bootversion,
-                                  GCancellable          *cancellable,
-                                  GError               **error)
+_ostree_bootloader_uboot_write_config (OstreeBootloader *bootloader,
+                                       int               bootversion,
+                                       GPtrArray        *new_deployments,
+                                       GCancellable     *cancellable,
+                                       GError          **error)
 {
   OstreeBootloaderUboot *self = OSTREE_BOOTLOADER_UBOOT (bootloader);
 
diff --git a/src/libostree/ostree-bootloader.c b/src/libostree/ostree-bootloader.c
index 87823f7ea..5b8125dcc 100644
--- a/src/libostree/ostree-bootloader.c
+++ b/src/libostree/ostree-bootloader.c
@@ -54,13 +54,15 @@ _ostree_bootloader_get_name (OstreeBootloader  *self)
 gboolean
 _ostree_bootloader_write_config (OstreeBootloader  *self,
                             int            bootversion,
+                            GPtrArray     *new_deployments,
                             GCancellable  *cancellable,
                             GError       **error)
 {
   g_return_val_if_fail (OSTREE_IS_BOOTLOADER (self), FALSE);
 
-  return OSTREE_BOOTLOADER_GET_IFACE (self)->write_config (self, bootversion, 
-                                                       cancellable, error);
+  return OSTREE_BOOTLOADER_GET_IFACE (self)->write_config (self, bootversion,
+                                                           new_deployments,
+                                                           cancellable, error);
 }
 
 gboolean
diff --git a/src/libostree/ostree-bootloader.h b/src/libostree/ostree-bootloader.h
index a4793b08c..5af2dcc8a 100644
--- a/src/libostree/ostree-bootloader.h
+++ b/src/libostree/ostree-bootloader.h
@@ -44,6 +44,7 @@ struct _OstreeBootloaderInterface
   const char *         (* get_name)               (OstreeBootloader  *self);
   gboolean             (* write_config)           (OstreeBootloader  *self,
                                                    int            bootversion,
+                                                   GPtrArray     *new_deployments,
                                                    GCancellable  *cancellable,
                                                    GError       **error);
   gboolean             (* is_atomic)              (OstreeBootloader  *self);
@@ -61,6 +62,7 @@ const char *_ostree_bootloader_get_name (OstreeBootloader  *self);
 
 gboolean _ostree_bootloader_write_config (OstreeBootloader  *self,
                                           int            bootversion,
+                                          GPtrArray     *new_deployments,
                                           GCancellable  *cancellable,
                                           GError       **error);
 
diff --git a/src/libostree/ostree-sysroot-deploy.c b/src/libostree/ostree-sysroot-deploy.c
index b16f65b33..51cf40885 100644
--- a/src/libostree/ostree-sysroot-deploy.c
+++ b/src/libostree/ostree-sysroot-deploy.c
@@ -2122,7 +2122,8 @@ write_deployments_bootswap (OstreeSysroot     *self,
   if (bootloader)
     {
       if (!_ostree_bootloader_write_config (bootloader, new_bootversion,
-                                            cancellable, error))
+                                            new_deployments, cancellable,
+                                            error))
         return glnx_prefix_error (error, "Bootloader write config");
     }

[dustymabe]

rh-atomic-bot closed this in d69214a 16 days ago

❤️