create_disk: Add bootupd-epoch, hard require new bootupd
#3631
Conversation
|
Thanks for working on this |
cgwalters
force-pushed
the
enable-bootupd-bios
branch
from
ddbd785
to
f00f7ff
Compare
There was a problem hiding this comment.
Thanks for adding it @cgwalters!
Overall LGTM, waiting to see #3631 (comment)
cgwalters
force-pushed
the
enable-bootupd-bios
branch
2 times, most recently
from
2f2fbea
to
8a891ae
Compare
|
Now with a config knob, off by default. This definitely needs some careful review (whee shell script fragility) and testing (across all architectures really). |
+1 - i'll take a closer look in the morning when I'm fresh. Once we get it close on code review we can do a COSA build in the pipeline and run it through the bump-lockfile job in the staging pipeline (tests against all architectures). |
cgwalters
force-pushed
the
enable-bootupd-bios
branch
2 times, most recently
from
1405ab9
to
ab421a0
Compare
cgwalters
changed the title
bootupd-epoch = 0|1
|
Tested now on x86_64, booting BIOS and UEFI in both epoch 0 and 1. |
|
Note that we're also discussing moving the static grub configs into a subpackage of bootupd, xref coreos/bootupd#536 If we did that, it'd be another epoch. (Which, if we agree to do it, maybe we should just make "epoch 1" be that) |
|
Renata added this on internal chat but replicating here: Once we do this we also want to update https://docs.fedoraproject.org/en-US/fedora-coreos/bootloader-updates/ (That said, in the context of Sagano this would actually be a Sagano doc, not a CoreOS doc...) |
|
Interesting, CI just hit coreos/rpm-ostree#4565. So I guess #3619 didn't do the trick. |
There was a problem hiding this comment.
LGTM! (Would still be good to do some multi-arch testing but it being opt in means we can easily test it in e.g. rawhide first before propagating.)
Note that we're also discussing moving the static grub configs into a subpackage of bootupd, xref coreos/bootupd#536
I'm not sure that makes sense. Posted in coreos/bootupd#536 (comment).
src/create_disk.sh
Outdated
| chroot_run /usr/bin/bootupctl backend install --src-root="${deploy_root}" "${bootupd_args[@]}" "${rootfs}" | ||
| case "${arch}" in | ||
| x86_64|aarch64) | ||
| inject_grub_uefi |
There was a problem hiding this comment.
optional: I think this would be slightly easier to follow if the inject_grub_cfg wasn't embedded in inject_grub_uefi but rather called at the same level:
| inject_grub_uefi | |
| inject_grub_uefi | |
| inject_grub_cfg |
i.e. it would make it clearer that x86_64 and aarch64 need BOTH inject_grub_uefi and inject_grub_cfg, but ppc64le only needs inject_grub_cfg.
src/create_disk.sh
Outdated
| # Unshare mount ns to work around https://github.com/coreos/bootupd/issues/367 | ||
| unshare -m /usr/bin/bootupctl backend install --src-root="${deploy_root}" "${rootfs}" | ||
| inject_grub_uefi |
There was a problem hiding this comment.
if we do https://github.com/coreos/coreos-assembler/pull/3631/files#r1334584042 then we'd need to add a call here:
| inject_grub_uefi | |
| inject_grub_uefi | |
| inject_grub_cfg |
|
Let me know what you think of the optional comments. Otherwise everything LGTM. I can do a multi-arch build of COSA and run this through the staging bump-lockfile job with both epoch 0 and 1 to see how the architectures respond to it if you like. |
|
Sure, SGTM (honestly feel free to force push cleanups here if you like). That said I think I'd actually want to wait and make "epoch 1" include the grub.cfg rework, so will release a new bootupd soon. |
As of right now all known callers of this pass `/`. More generally we now expect bootupd to run in a container inside its target root; never outside of it. xref coreos/coreos-assembler#3631
cgwalters
force-pushed
the
enable-bootupd-bios
branch
from
ab421a0
to
2ae4e50
Compare
|
See coreos/bootupd#543 (not totally proud of it, kind of hacky but...) anyways this is now updated to also take ownership of most of the grub config, what's left here is I wonder if this PR would be better off actually just fully deleting the old code? Landing now would make it easier to test locally, but only slightly. The patch here would be way less ugly if it was 70% deletions instead. |
cgwalters
force-pushed
the
enable-bootupd-bios
branch
from
2ae4e50
to
ae5352a
Compare
cgwalters
changed the title
bootupd-epoch = 0|1bootupd-epoch, hard require new bootupd
|
OK this now builds on coreos/bootupd#543 and coreos/ignition#1728 |
This hard requires - coreos/bootupd#543 - coreos/ignition#1728
cgwalters
force-pushed
the
enable-bootupd-bios
branch
from
ae5352a
to
ed7fce9
Compare
|
@cgwalters: The following test failed, say
Full PR test history. Your PR dashboard. 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/test-infra repository. I understand the commands that are listed here. |
|
PR needs rebase. 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/test-infra repository. |
|
I guess one option is to basically close this PR and aim to try to cut over to osbuild in #3643 ? |
|
I think this PR is still useful. I wanted to get back to it and run it through some tests on multi-arch in the staging pipeline. This has also been nice inspiration for @ravanelli who is working on a bootupd stage in osbuild. |
|
Right, it was useful to write this to sanity check the bootupd support. (Incidentally containers/bootc#157 also landed) |
|
going to close this out since we are moving away from using create_disk for our disk image creation. |
This hard requires