GNOME Bugzilla – Bug 706477
admin: Don't fail to deploy if there isn't a bootloader config
Last modified: 2013-08-22 09:53:31 UTC
Currently, when deploying an OSTree that does not contain a bootloader configuration it fails with the following message: "No known bootloader configuration detected" A bootloader configuration is not strictly necessary if the bootloader used is able to parse /boot/loader/entries on boot. So, failing to deploy seems to be a little harsh. It is better to just not write the bootloader configuration if a previous one was not found but still swap the bootversion.
Created attachment 252518 [details] [review] [PATCH 1/1] admin: Don't fail to deploy if there isn't a bootloader config
Review of attachment 252518 [details] [review]: Hmm, interesting. I'm going back and forth on this. Even gummiboot (written by Kay who also authored the bootloader spec), has a config file, albeit a rather generically named one: /boot/loader/loader.conf. Is there a real-world bootloader which parses /boot/loader/entries and *doesn't* have a config file? Could one exist given our previous discussion about the necessity of having external bootloader configuration to specify the default?
BTW, see this code in gnome-ostree which generates the disk images: https://git.gnome.org/browse/gnome-ostree/tree/src/js/libqa.js#n294 That tells ostree to use the syslinux backend.
(In reply to comment #2) > Review of attachment 252518 [details] [review]: > > Hmm, interesting. I'm going back and forth on this. Even gummiboot (written > by Kay who also authored the bootloader spec), has a config file, albeit a > rather generically named one: /boot/loader/loader.conf. > > Is there a real-world bootloader which parses /boot/loader/entries and > *doesn't* have a config file? Could one exist given our previous discussion > about the necessity of having external bootloader configuration to specify the > default? No that I can think of. Now the question is whether OSTree should fail or not to deploy because a /boot/loader/loader.conf is not found (which btw is not searched in ot_admin_query_bootloader now so it will fail even if is present). Since gummiboot and Grub (with the blscfg module) are able to parse the /boot/loader/entries then IMHO it shouldn't fail to deploy if the configuration is not present. Or we should add support for grub.cfg and loader.conf but then I wonder how should behave their OtBootloaderInterface *iface write_config function.
(In reply to comment #3) > BTW, see this code in gnome-ostree which generates the disk images: > > https://git.gnome.org/browse/gnome-ostree/tree/src/js/libqa.js#n294 > > That tells ostree to use the syslinux backend. Yes, I noticed it and use a similar code to tell OSTree to use a U-Boot backend: # Stub U-Boot configuration mkdir -p sysroot/boot/loader.0 ln -s loader.0 sysroot/boot/loader touch sysroot/boot/loader/uEnv.txt # And a compatibility symlink ln -s loader/uEnv.txt sysroot/boot/uEnv.txt but what I do wonder if this is really necessary for Grub and gummiboot...
(In reply to comment #4) > No that I can think of. Now the question is whether OSTree should fail or not > to deploy because a /boot/loader/loader.conf is not found (which btw is not > searched in ot_admin_query_bootloader now so it will fail even if is present). Right. Ok, gummiboot has this syntax for defaults: default 6a9857a393724b7a981ebb5b8495b9ea-* For ostree it'd need to look like this: default ostree-* And then we don't touch it assuming we write correct "version" fields into our loader entries....which we're not at the moment (we're writing bootserial, when it should be the index). I'll fix that. Hum. So reading the gummiboot code, it has an internal default config if no loader.conf exists: http://cgit.freedesktop.org/gummiboot/tree/src/setup/setup.c?id=5975f9a421518fe3bd3f2f60695bfc45b2034a98#n1236 Which won't quite work for us because we're not prefixing our entries with the machine-id (should we try to make sure each OS installed in ostree has the same /etc/machine-id ?). But theoretically it could just use "default *" and just kind of assume there's only one entry, or that there is a useful global ordering among the individual entries "version" fields. So...where does that leave your patch? I guess it makes sense to take on a theoretical level, but I'll need to keep the gnome-ostree code to write a stub syslinux config, and you'll need to do the same for the uEnv file for now, so we're not really saving any code. Maybe potentially we could have ostree admin deploy --bootloader=syslinux to force the choice of bootloader, and then ostree could synthesize default configurations if none exist?
(In reply to comment #6) > (In reply to comment #4) > > > No that I can think of. Now the question is whether OSTree should fail or not > > to deploy because a /boot/loader/loader.conf is not found (which btw is not > > searched in ot_admin_query_bootloader now so it will fail even if is present). > > Right. Ok, gummiboot has this syntax for defaults: > > default 6a9857a393724b7a981ebb5b8495b9ea-* > > For ostree it'd need to look like this: > > default ostree-* > > And then we don't touch it assuming we write correct "version" fields into our > loader entries....which we're not at the moment (we're writing bootserial, when > it should be the index). I'll fix that. > > Hum. So reading the gummiboot code, it has an internal default config if no > loader.conf exists: > > http://cgit.freedesktop.org/gummiboot/tree/src/setup/setup.c?id=5975f9a421518fe3bd3f2f60695bfc45b2034a98#n1236 > > Which won't quite work for us because we're not prefixing our entries with the > machine-id (should we try to make sure each OS installed in ostree has the same > /etc/machine-id ?). > > But theoretically it could just use "default *" and just kind of assume there's > only one entry, or that there is a useful global ordering among the individual > entries "version" fields. > > So...where does that leave your patch? I guess it makes sense to take on a > theoretical level, but I'll need to keep the gnome-ostree code to write a stub > syslinux config, and you'll need to do the same for the uEnv file for now, so > we're not really saving any code. > Well this use case is not only theoretical for me since I want to have two kind of images: one for ARM that is going to boot using U-Boot and one for x86 that is going to boot using Grub with the blscfg module to parse /boot/loader/entries. For the later case OSTree fails to deploy since I don't have neither a syslinux nor uboot config file. Since my grub.cfg will be really minimal and just used to set the default boot entry I thought that I can create one on the original archive-z2 repo and OSTree admin deploy shouldn't touch it again (or now about its existence). Conversely I can add support for Grub so OSTree stores my config under /boot/loader/grub.cfg and create a symlinks but I don't know if is worth it. Will be too much code to maintain a file that is going to be static anyways. Yes, I can create a syslinux.cfg file on /boot just to make OSTree admin deploy happy but I don't think is the right solution neither. > Maybe potentially we could have ostree admin deploy --bootloader=syslinux to > force the choice of bootloader, and then ostree could synthesize default > configurations if none exist? I like that option, right ot_admin_query_bootloader() just first look for a syslinux conf and then for an uboot conf. If we add support for more bootloader backends then we should document clearly the precedence that each backend has so users know what to expect. So forcing to choose a bootloader on deploy seems like a sensible solution to me.
(In reply to comment #7) > (In reply to comment #6) > > (In reply to comment #4) > > > > > No that I can think of. Now the question is whether OSTree should fail or not > > > to deploy because a /boot/loader/loader.conf is not found (which btw is not > > > searched in ot_admin_query_bootloader now so it will fail even if is present). > > > > Right. Ok, gummiboot has this syntax for defaults: > > > > default 6a9857a393724b7a981ebb5b8495b9ea-* > > > > For ostree it'd need to look like this: > > > > default ostree-* > > > > And then we don't touch it assuming we write correct "version" fields into our > > loader entries....which we're not at the moment (we're writing bootserial, when > > it should be the index). I'll fix that. > > > > Hum. So reading the gummiboot code, it has an internal default config if no > > loader.conf exists: > > > > http://cgit.freedesktop.org/gummiboot/tree/src/setup/setup.c?id=5975f9a421518fe3bd3f2f60695bfc45b2034a98#n1236 > > > > Which won't quite work for us because we're not prefixing our entries with the > > machine-id (should we try to make sure each OS installed in ostree has the same > > /etc/machine-id ?). > > > > But theoretically it could just use "default *" and just kind of assume there's > > only one entry, or that there is a useful global ordering among the individual > > entries "version" fields. > > > > So...where does that leave your patch? I guess it makes sense to take on a > > theoretical level, but I'll need to keep the gnome-ostree code to write a stub > > syslinux config, and you'll need to do the same for the uEnv file for now, so > > we're not really saving any code. > > > > Well this use case is not only theoretical for me since I want to have two kind > of images: one for ARM that is going to boot using U-Boot and one for x86 that > is going to boot using Grub with the blscfg module to parse > /boot/loader/entries. > > For the later case OSTree fails to deploy since I don't have neither a syslinux > nor uboot config file. Since my grub.cfg will be really minimal and just used > to set the default boot entry I thought that I can create one on the original > archive-z2 repo and OSTree admin deploy shouldn't touch it again (or now about > its existence). > > Conversely I can add support for Grub so OSTree stores my config under > /boot/loader/grub.cfg and create a symlinks but I don't know if is worth it. > Will be too much code to maintain a file that is going to be static anyways. > Hi Colin, I really appreciate your help and feedback. Just to make sure we are on the same page. I'm using this patch for Grub that implements parsing /boot/loader/entries: http://pkgs.fedoraproject.org/cgit/grub2.git/tree/0460-blscfg-add-blscfg-module-to-parse-Boot-Loader-Specif.patch And my current grub.cfg looks like this: insmod blscfg bls_import set default='0' That's why I wanted this patch so I can deploy on images that don't use neither syslinux nor uboot. How do you think is the best approach to solve this issue? As I said, I could add grub.cfg support to OSTree but its write_config function will just copy the static configuration from one bootversion to another. But yes, at least will check if a grub.cfg exists before (a stub created in the archive-z2 just like in the syslinux/uboot case). But I really think that we should allow OSTree to deploy images even in the absence of a known bootloader config. For example, an embedded vendor could have its own custom bootloader that supports parsing Boot Loader Spec snippets and wants to use OSTree on a product. That's why I added a message: "No bootloader configuration detected, the bootloader must support parsing /boot/loader/entries on boot" hoping that the user knows what is doing :-)
(In reply to comment #8) > > That's why I wanted this patch so I can deploy on images that don't use neither > syslinux nor uboot. Right, that does make sense. > As I said, I could add grub.cfg support to OSTree but its write_config function > will just copy the static configuration from one bootversion to another. But > yes, at least will check if a grub.cfg exists before (a stub created in the > archive-z2 just like in the syslinux/uboot case). Yeah, OK. > "No bootloader configuration detected, the bootloader must support parsing > /boot/loader/entries on boot" > > hoping that the user knows what is doing :-) Yeah, I guess we could hope someone developing a new buildsystem would notice this, but it seems more likely they'll just ignore it and then try to debug issues with their bootloader later. The warning seems phrased in an unnecessarily scary way; maybe just a line like: Detected bootloader: bootloaderspec and: Detected bootloader: syslinux ?
(In reply to comment #9) > (In reply to comment #8) > > > > That's why I wanted this patch so I can deploy on images that don't use neither > > syslinux nor uboot. > > Right, that does make sense. > > > As I said, I could add grub.cfg support to OSTree but its write_config function > > will just copy the static configuration from one bootversion to another. But > > yes, at least will check if a grub.cfg exists before (a stub created in the > > archive-z2 just like in the syslinux/uboot case). > > Yeah, OK. > > > "No bootloader configuration detected, the bootloader must support parsing > > /boot/loader/entries on boot" > > > > hoping that the user knows what is doing :-) > > Yeah, I guess we could hope someone developing a new buildsystem would notice > this, but it seems more likely they'll just ignore it and then try to debug > issues with their bootloader later. > Sorry, I'm a bit confused now. Did I convince you that this change could be needed or do you want me to implement the grub config support to avoid OSTree failing to deploy? (there are not mutually exclusive btw) > The warning seems phrased in an unnecessarily scary way; maybe just a line > like: > > Detected bootloader: bootloaderspec > > and: > > Detected bootloader: syslinux > > ? Ok, I'll post a patch to print this info in ot_admin_query_bootloader() but how should we phrase in the case were no bootloader config is found? Assuming you finally want to add that option of course. The scary message was intentional but I agree with you that probably most users will ignore it anyway...
Created attachment 252690 [details] [review] [PATCH v2 1/1] admin: Don't fail to deploy if there isn't a bootloader Attached a v2 of the patch. Changes since v1: - Remove the scary warning since ot_admin_query_bootloader() will already notify that no bootloader configuration was found.
Created attachment 252699 [details] [review] [PATCH v2 1/1] admin: Don't fail to deploy if there isn't a bootloader config
Review of attachment 252699 [details] [review]: Ok, this is fine.