[merged] Introducing ostree-grub-generator #228

gatispaeglis · 2016-03-29T17:21:37Z

ostree-grub-generator can be used to generate
an easy customizable grub.cfg file. Compile time
decision ostree-grub-generator vs grub2-mkconfig
can be overwritten with the OSTREE_GRUB2_EXEC
envvar - useful for auto tests and OS installers.

Why this alternative approach:

The current approach is less flexible than using a
custom 'ostree-grub-generator' script. Each system can
adjust this script for its needs, instead of using the
hardcoded values from ostree-bootloader-grub2.c.
Too much overhead on embedded to generate grub.cfg
via /etc/grub.d/ configuration files. It is still
possible to do so, even with this patch applied.
No need to install grub2 package on a target device.
The grub2-mkconfig code path has other issues:
https://bugzilla.gnome.org/show_bug.cgi?id=761180

Task: https://bugzilla.gnome.org/show_bug.cgi?id=762220

cgwalters · 2016-03-30T13:02:57Z

One thing that's worth noting here as well is that Peter Jones wrote this: http://pkgs.fedoraproject.org/cgit/rpms/grub2.git/tree/0029-blscfg-add-blscfg-module-to-parse-Boot-Loader-Specif.patch?id=dcaf8f68832f12983a03aa57275627c7c7167021

Which fits in a bit better with the original intention of the BLS...I forget why we're not using it in Fedora Atomic right now. It might be as prosaic as it not being enabled by default? That'd likely be easy to fix. Possibly a larger issue is the uncertain state of the BLS (https://www.redhat.com/archives/anaconda-devel-list/2014-July/msg00002.html etc).

Anyways, this approach is also quite simple. GRUB2 is critical path for me so I'll need to take some time to test this. Doing so now.

cgwalters · 2016-03-30T13:58:45Z

src/boot/grub2/ostree-grub-generator

+{
+    # Prepend "boot/" prefix when boot/ directory is on the same partition as usr/,
+    # if boot/ directory is on a separate partition, set boot_prefix=""
+    boot_prefix="/boot"


See #215 - I wonder if we could have OSTree pass this down via environment variable? Though that only works on-target. Perhaps anyone doing a cross system like OpenEmbedded could have that set the OSTREE_BOOT_PARTITION environment variable there?

gatispaeglis · 2016-03-30T14:13:28Z

Having to set 1 extra envvar when building image not-on-target device sounds reasonable and maintainable to me.
You are thinking to introduce OSTREE_BOOT_PARTITION in #215?

rh-atomic-bot · 2016-03-30T16:54:40Z

Can one of the admins verify this patch?
I understand the following comments:

bot, add author to whitelist
bot, test pull request
bot, test pull request once

jlebon · 2016-03-30T17:10:46Z

bot, test pull request

gatispaeglis · 2016-03-30T18:10:58Z

One thing that's worth noting here as well is that Peter Jones wrote [1]

I was not aware of this plugin. I can see how it could be handy on Desktop and if other operating systems / boot loaders actually followed the Boot Loader Specification. And as you already mentioned, state of BLS seems to be unclear.

But if we compare ostree-grub-generator vs GRUB BLS plugin [1] the advantage for ostree-grub-generator is:

I guess GSubprocess is not limited to executing shell scripts only. Which means that ostree-grub-generator can be a compiled executable implemented in C, C++ (thus, can borrow the logic from [1] without the need for chroot-ing, grub dependencies and other complexities mentioned earlier) or a Python script for example. Whatever works better for a target environment. I guess the biggest advantage here is the flexibility.

[1] http://pkgs.fedoraproject.org/cgit/rpms/grub2.git/tree/0029-blscfg-add-blscfg-module-to-parse-Boot-Loader-Specif.patch?id=dcaf8f68832f12983a03aa57275627c7c7167021

cgwalters · 2016-03-30T18:35:29Z

src/boot/grub2/ostree-grub-generator

+    while read -r line
+    do
+        record=$(echo ${line} | cut -f 1 -d ' ')
+        value=$(echo ${line} | cut -s -f2- -d ' ')


This is a clear enough script, but it's tempting to move the logic into C, basically what we're doing in the syslinux/u-boot ones. We don't have to do that now though, and shell script is convenient for customization.

At some point though I do want to revisit #228 (comment) - I didn't quite understand you reply there. The GRUB2 builtin BLS loader wouldn't have any on-target dependencies either, but it would need some changes to be possibly more flexible/configurable?

What I meant here is that this logic could be implemented entirely in ostree. Instead of working on making a GRUB2 builtin BLS loader more configurable, there could be a C binary (configurable) in ostree project that does what normally would be the job of "GRUB2 builtin BLS loader".

Then we could reuse the same code path what is currently used by ostree-grub-generator. Where ostree instead of executing ostree-grub-generator shell script would execute the above mentioned C binary. The benefit would be that the code is better localized. Probably there are some benefits at developing this upstream (in grub2 repo) too.

Hopefully this clarifies it better.

cgwalters · 2016-03-30T19:09:28Z

You are thinking to introduce OSTREE_BOOT_PARTITION in #215?

Yeah.

cgwalters · 2016-03-30T19:32:20Z

src/boot/grub2/ostree-grub-generator

+cat >> ${new_grub2_cfg} <<EOF
+serial --unit=0 --speed=115200 --word=8 --parity=no --stop=1
+default=boot
+timeout=10


This is the sort of thing where we should probably have a config file at least for any general purpose use. It's basically what /etc/grub.d/00_header is right?

I guess this is where things are a bit nicer with the state we have with syslinux where the ostree-syslinux code edits just the entries in there, otherwise admins can edit it by hand. But that's only really possible because the syslinux format is easy to parse, which is definitely not true of grub2 😦

It's basically what /etc/grub.d/00_header is right

Yeah, looks like it. This definitely can be improved further by Atomic project or whoever else decides that they need it first :)

I guess this is where things are a bit nicer with the state we have with syslinux where the ostree-syslinux code edits just the entries in there, otherwise admins can edit it by hand.

I want to achieve similar funcionality in the u-boot backend by this patch:
5ee4977
Where admin can edit /uEnv.txt, deploy the edited version. Where during the deploy process OSTree prepends its env (loader/uEnv.txt) on top of the /uEnv.txt.
I am planing to revisit that change soon and add an auto tests that demonstrates the use case.

cgwalters · 2016-03-30T19:34:05Z

At a high level I'm inclined to take this patch with just some small cleanups - we don't need to rearchitect things to have a config file etc. initially...anyone who wants a general purpose bootloader can just use grub2-mkconfig, and we can keep this code path targeted for embedded.

gatispaeglis · 2016-03-30T19:43:01Z

Cool, I will clean up the remaining parts tomorrow then :)

gatispaeglis · 2016-03-31T11:57:17Z

tests/libtest.sh

@@ -282,10 +296,13 @@ EOF

    case $bootmode in
        "syslinux")
-	    setup_os_boot_syslinux
+        setup_os_boot_syslinux
            ;;


Does OSTree repository has policy regarding spaces vs tabs? When I see tabs around lines that I am touching I feel like I have to convert everything to spaces :)

C code is GNU style with no tabs, but we don't have a policy for shell yet. I think it's mostly just been whatever Emacs does by default which is 4 spaces?

I'd say no tabs, and try match what exists, and maybe we'll revisit consistent indentation for shell later?

gatispaeglis · 2016-03-31T12:23:19Z

Ok, if I am not missing anything, then this patch can be considered complete. Unless you find something more than needs to be fixed up.

cgwalters · 2016-03-31T18:03:08Z

bot, retest this please

cgwalters · 2016-03-31T18:56:41Z

@cgwalters-bot try

cgwalters-bot · 2016-03-31T18:56:46Z

⌛ Trying commit 7c308a9 with merge 410a0fc...

ostree-grub-generator can be used to generate an easy customizable grub.cfg file. Compile time decision ostree-grub-generator vs grub2-mkconfig can be overwritten with the OSTREE_GRUB2_EXEC envvar - useful for auto tests and OS installers. Why this alternative approach: 1) The current approach is less flexible than using a custom 'ostree-grub-generator' script. Each system can adjust this script for its needs, instead of using the hardcoded values from ostree-bootloader-grub2.c. 2) Too much overhead on embedded to generate grub.cfg via /etc/grub.d/ configuration files. It is still possible to do so, even with this patch applied. No need to install grub2 package on a target device. 3) The grub2-mkconfig code path has other issues: https://bugzilla.gnome.org/show_bug.cgi?id=761180 Task: https://bugzilla.gnome.org/show_bug.cgi?id=762220 Closes: #228 Approved by: <try>

cgwalters · 2016-03-31T18:57:29Z

I think the PR tester has been failing since you based on a commit before we got it all working. Can you rebase again? But let's see what happens with a "try" merge.

cgwalters-bot · 2016-03-31T18:58:28Z

☀️ Test successful - travis
State: approved= try=True

jlebon · 2016-03-31T19:46:53Z

Actually, the logs say INFO: using MERGE of PR, which means that it was testing it with the latest changes.

Searching for "FAIL:" shows that only one test failed:

Running test: ostree/test-admin-deploy-grub2.sh.test
Copying gpghome to /tmp/test-tmp-ostree_test-admin-deploy-grub2.sh.test-U2W4EY
1db2ee44eb4bd7eb63ec641c657f8378d593394d129e931f53267524c3ae3ee9
42ea980590b99edb1e98b416480a6cb4272709296096c4773709a4bf5485f4a0
3fb815e88cd96f2e6a85211f31d2995c2057d96ff41876626dfd1ec148d8a69c
1/32 objects
4/32 objects
7/32 objects
10/32 objects
13/32 objects
16/32 objects
19/32 objects
22/32 objects
25/32 objects
28/32 objects
31/32 objects
ostree/deploy/testos initialized as OSTree root
cp: cannot stat '/usr/libexec/installed-tests/ostree/../src/boot/grub2/ostree-grub-generator': No such file or directory
FAIL: ostree/test-admin-deploy-grub2.sh.test (Child process exited with code 1)

gatispaeglis · 2016-03-31T20:44:42Z

Right, I tested with "make check", then it succeeds. Probably need to install ostree-grub-generator in usr/libexec/installed-tests/ostree/ for it to pass.

cgwalters · 2016-04-02T16:56:01Z

The tests are passing here on my laptop (both make check and installed). Hm, I wonder though if this has to do with the fact the PR tester is running the installed tests as root? I'll look at that.

In the meantime reviewing this patch more, I found it made a lot more sense to me if the configure option was --with-builtin-grub-mkconfig, rather than "use the system" be a default yes. And the naming in the code IMO is clearer too.

If you agree can you cherry-pick gatispaeglis#1 into your PR branch? (Yep, I made a PR for your PR, it works 😄 )

In the big picture I am a bit sad that it's unlikely we can use this for Project Atomic yet, as I would really like to just ditch the grub2 binaries. But it'd need a config file and other bits.

I'm also a bit uncertain about the long term maintenance of carrying this special case, and am wondering if we should redirect people to extlinux. Was there a reason you chose grub2 over extlinux?

gatispaeglis · 2016-04-02T22:15:14Z

I fixed the grub2 test after the last comment from jlebon so everything should be fine now.

If you agree can you cherry-pick gatispaeglis#1 into your PR branch?

Patch seems fine to me, I can do the cherry-pick on monday.

and am wondering if we should redirect people to extlinux. Was there a reason you chose grub2 over extlinux?

Several reasons actually. I have been experimenting with Intel NUC board where grub2 (with UEFI firmware) is what Intel provides by default in http://git.yoctoproject.org/cgit/cgit.cgi/meta-intel so I didn't even consider other boot loader.
Other reason is that GRUB has better support for UEFI. UEFI support appears to be limited on syslinux according to https://wiki.archlinux.org/index.php/syslinux#UEFI_Systems . Particularly the following limitation:
"For Syslinux, the kernel and initramfs files need to be in the ESP, as Syslinux does not (currently) have the ability to access files outside its own partition (i.e. outside ESP in this case)." So syslinux + UEFI != ostree

ostree-grub-generator can be used to customize the generated grub.cfg file. Compile time decision ostree-grub-generator vs grub2-mkconfig can be overwritten with the OSTREE_GRUB2_EXEC envvar - useful for auto tests and OS installers. Why this alternative approach: 1) The current approach is less flexible than using a custom 'ostree-grub-generator' script. Each system can adjust this script for its needs, instead of using the hardcoded values from ostree-bootloader-grub2.c. 2) Too much overhead on embedded to generate grub.cfg via /etc/grub.d/ configuration files. It is still possible to do so, even with this patch applied. No need to install grub2 package on a target device. 3) The grub2-mkconfig code path has other issues: https://bugzilla.gnome.org/show_bug.cgi?id=761180 Task: https://bugzilla.gnome.org/show_bug.cgi?id=762220

cgwalters · 2016-04-04T14:19:14Z

Oh right, I think we talked about the UEFI issue. OK, let's go with this then. At some point though I do want to revisit #228 (comment) - I didn't quite understand you reply there. The GRUB2 builtin BLS loader wouldn't have any on-target dependencies either, but it would need some changes to be possibly more flexible/configurable?

@cgwalters-bot r+ 92c088e

cgwalters-bot · 2016-04-04T14:19:15Z

🙀 92c088e is not a valid commit SHA. Please try again with 29c088e.

cgwalters · 2016-04-04T14:19:32Z

@cgwalters-bot r+ 29c088e

cgwalters-bot · 2016-04-04T14:19:38Z

⌛ Testing commit 29c088e with merge 4e81548...

cgwalters-bot · 2016-04-04T14:20:58Z

☀️ Test successful - travis
Approved by: cgwalters
Pushing 4e81548 to master...

cgwalters reviewed Mar 30, 2016
View reviewed changes

gatispaeglis force-pushed the ostree-grub-generator branch 2 times, most recently from fb99e66 to da5b15f Compare March 30, 2016 16:03

cgwalters reviewed Mar 30, 2016
View reviewed changes

gatispaeglis force-pushed the ostree-grub-generator branch 2 times, most recently from e5378f2 to 7c308a9 Compare March 31, 2016 11:54

gatispaeglis reviewed Mar 31, 2016
View reviewed changes

gatispaeglis force-pushed the ostree-grub-generator branch 3 times, most recently from aa0582e to 9abb020 Compare April 1, 2016 12:02

gatispaeglis force-pushed the ostree-grub-generator branch from 3b234ad to 29c088e Compare April 4, 2016 08:57

cgwalters-bot changed the title ~~Introducing ostree-grub-generator~~ [merged] Introducing ostree-grub-generator Apr 4, 2016

cgwalters-bot closed this in 4e81548 Apr 4, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[merged] Introducing ostree-grub-generator #228

[merged] Introducing ostree-grub-generator #228

gatispaeglis commented Mar 29, 2016

cgwalters commented Mar 30, 2016

cgwalters Mar 30, 2016

gatispaeglis commented Mar 30, 2016

rh-atomic-bot commented Mar 30, 2016

jlebon commented Mar 30, 2016

gatispaeglis commented Mar 30, 2016

cgwalters Mar 30, 2016

gatispaeglis Apr 8, 2016

cgwalters commented Mar 30, 2016

cgwalters Mar 30, 2016

gatispaeglis Mar 31, 2016

cgwalters commented Mar 30, 2016

gatispaeglis commented Mar 30, 2016

gatispaeglis Mar 31, 2016

cgwalters Mar 31, 2016

gatispaeglis commented Mar 31, 2016

cgwalters commented Mar 31, 2016

cgwalters commented Mar 31, 2016

cgwalters-bot commented Mar 31, 2016

cgwalters commented Mar 31, 2016

cgwalters-bot commented Mar 31, 2016

jlebon commented Mar 31, 2016

gatispaeglis commented Mar 31, 2016

cgwalters commented Apr 2, 2016

gatispaeglis commented Apr 2, 2016

cgwalters commented Apr 4, 2016

cgwalters-bot commented Apr 4, 2016

cgwalters commented Apr 4, 2016

cgwalters-bot commented Apr 4, 2016

cgwalters-bot commented Apr 4, 2016

[merged] Introducing ostree-grub-generator #228

[merged] Introducing ostree-grub-generator #228

Conversation

gatispaeglis commented Mar 29, 2016

cgwalters commented Mar 30, 2016

cgwalters Mar 30, 2016

Choose a reason for hiding this comment

gatispaeglis commented Mar 30, 2016

rh-atomic-bot commented Mar 30, 2016

jlebon commented Mar 30, 2016

gatispaeglis commented Mar 30, 2016

cgwalters Mar 30, 2016

Choose a reason for hiding this comment

gatispaeglis Apr 8, 2016

Choose a reason for hiding this comment

cgwalters commented Mar 30, 2016

cgwalters Mar 30, 2016

Choose a reason for hiding this comment

gatispaeglis Mar 31, 2016

Choose a reason for hiding this comment

cgwalters commented Mar 30, 2016

gatispaeglis commented Mar 30, 2016

gatispaeglis Mar 31, 2016

Choose a reason for hiding this comment

cgwalters Mar 31, 2016

Choose a reason for hiding this comment

gatispaeglis commented Mar 31, 2016

cgwalters commented Mar 31, 2016

cgwalters commented Mar 31, 2016

cgwalters-bot commented Mar 31, 2016

cgwalters commented Mar 31, 2016

cgwalters-bot commented Mar 31, 2016

jlebon commented Mar 31, 2016

gatispaeglis commented Mar 31, 2016

cgwalters commented Apr 2, 2016

gatispaeglis commented Apr 2, 2016

cgwalters commented Apr 4, 2016

cgwalters-bot commented Apr 4, 2016

cgwalters commented Apr 4, 2016

cgwalters-bot commented Apr 4, 2016

cgwalters-bot commented Apr 4, 2016