create_disk: Add `bootupd-epoch`, hard require new bootupd by cgwalters · Pull Request #3631 · coreos/coreos-assembler

cgwalters · 2023-09-20T20:51:33Z

This hard requires

src/create_disk.sh

build.sh

src/create_disk.sh

dustymabe · 2023-09-20T21:02:25Z

Thanks for working on this

src/create_disk.sh

ravanelli

Thanks for adding it @cgwalters!
Overall LGTM, waiting to see #3631 (comment)

cgwalters · 2023-09-20T22:14:52Z

Now with a config knob, off by default. This definitely needs some careful review (whee shell script fragility) and testing (across all architectures really).

build.sh

dustymabe · 2023-09-21T02:11:08Z

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).

src/create_disk.sh

jlebon

We should probably sanity-check the epoch 1 path manually on aarch64 and ppc64le.

src/create_disk.sh

cgwalters · 2023-09-21T18:10:07Z

Tested now on x86_64, booting BIOS and UEFI in both epoch 0 and 1.

cgwalters · 2023-09-21T18:15:53Z

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)

cgwalters · 2023-09-21T18:28:26Z

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...)

jlebon · 2023-09-21T19:10:11Z

Interesting, CI just hit coreos/rpm-ostree#4565. So I guess #3619 didn't do the trick.

jlebon

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).

dustymabe · 2023-09-22T16:13:37Z

src/create_disk.sh

+    chroot_run /usr/bin/bootupctl backend install --src-root="${deploy_root}" "${bootupd_args[@]}" "${rootfs}"
+    case "${arch}" in
+        x86_64|aarch64)
+            inject_grub_uefi


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:

Suggested change

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.

dustymabe · 2023-09-22T16:15:35Z

src/create_disk.sh

+    # For background on bootupd, see https://github.com/coreos/fedora-coreos-tracker/issues/510
    # 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


if we do https://github.com/coreos/coreos-assembler/pull/3631/files#r1334584042 then we'd need to add a call here:

Suggested change

inject_grub_uefi

inject_grub_uefi

inject_grub_cfg

dustymabe · 2023-09-22T16:17:38Z

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.

cgwalters · 2023-09-22T16:50:47Z

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 · 2023-10-06T21:29:09Z

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 platform.cfg stuff.

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 · 2023-10-12T17:59:41Z

OK this now builds on coreos/bootupd#543 and coreos/ignition#1728
And I've reworked it to just completely delete the old code. We can land this once we've updated bootupd and ignition in fcos (ugh I guess all the way to stable?) and tagged into rhcos.

This hard requires - coreos/bootupd#543 - coreos/ignition#1728

openshift-ci · 2023-10-12T19:05:34Z

@cgwalters: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/rhcos	`ed7fce9`	link	true	`/test rhcos`

Full PR test history. Your PR dashboard.

Details

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.

openshift-merge-robot · 2023-10-22T23:23:12Z

PR needs rebase.

Details

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.

cgwalters · 2023-10-24T18:43:08Z

I guess one option is to basically close this PR and aim to try to cut over to osbuild in #3643 ?

dustymabe · 2023-10-24T19:10:16Z

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.

cgwalters · 2023-10-24T21:54:06Z

Right, it was useful to write this to sanity check the bootupd support.

(Incidentally bootc-dev/bootc#157 also landed)

dustymabe · 2024-05-09T14:51:41Z

going to close this out since we are moving away from using create_disk for our disk image creation.

cgwalters commented Sep 20, 2023

View reviewed changes

src/create_disk.sh Show resolved Hide resolved

dustymabe reviewed Sep 20, 2023

View reviewed changes

src/create_disk.sh Outdated Show resolved Hide resolved

dustymabe reviewed Sep 20, 2023

View reviewed changes

build.sh Outdated Show resolved Hide resolved

dustymabe reviewed Sep 20, 2023

View reviewed changes

src/create_disk.sh Outdated Show resolved Hide resolved

cgwalters force-pushed the enable-bootupd-bios branch from ddbd785 to f00f7ff Compare September 20, 2023 21:09

dustymabe reviewed Sep 20, 2023

View reviewed changes

src/create_disk.sh Outdated Show resolved Hide resolved

ravanelli previously approved these changes Sep 20, 2023

View reviewed changes

cgwalters dismissed ravanelli’s stale review via 2f2fbea September 20, 2023 21:32

cgwalters force-pushed the enable-bootupd-bios branch 2 times, most recently from 2f2fbea to 8a891ae Compare September 20, 2023 22:14

dustymabe reviewed Sep 21, 2023

View reviewed changes

build.sh Outdated Show resolved Hide resolved

HuijingHei reviewed Sep 21, 2023

View reviewed changes

src/create_disk.sh Outdated Show resolved Hide resolved

jlebon reviewed Sep 21, 2023

View reviewed changes

src/create_disk.sh Outdated Show resolved Hide resolved

src/create_disk.sh Outdated Show resolved Hide resolved

cgwalters force-pushed the enable-bootupd-bios branch 2 times, most recently from 1405ab9 to ab421a0 Compare September 21, 2023 17:54

cgwalters changed the title ~~create_disk: Use bootupd for BIOS too~~ create_disk: Add bootupd-epoch = 0|1 Sep 21, 2023

jlebon mentioned this pull request Sep 21, 2023

cmdlib: Drop unsafety for cache disk #3619

Merged

jlebon previously approved these changes Sep 21, 2023

View reviewed changes

dustymabe reviewed Sep 22, 2023

View reviewed changes

cgwalters mentioned this pull request Sep 29, 2023

backend: Make --generate-update-metadata require a sysroot of / coreos/bootupd#541

Merged

cgwalters dismissed jlebon’s stale review via 2ae4e50 October 6, 2023 21:26

cgwalters force-pushed the enable-bootupd-bios branch from ab421a0 to 2ae4e50 Compare October 6, 2023 21:26

cgwalters mentioned this pull request Oct 6, 2023

Add support for installing static grub configs coreos/bootupd#543

Merged

cgwalters force-pushed the enable-bootupd-bios branch from 2ae4e50 to ae5352a Compare October 12, 2023 17:58

cgwalters changed the title ~~create_disk: Add bootupd-epoch = 0|1~~ create_disk: Add bootupd-epoch, hard require new bootupd Oct 12, 2023

create_disk: Add bootupd-epoch, hard require new bootupd

ed7fce9

This hard requires - coreos/bootupd#543 - coreos/ignition#1728

cgwalters force-pushed the enable-bootupd-bios branch from ae5352a to ed7fce9 Compare October 12, 2023 18:34

openshift-merge-robot added the needs-rebase label Oct 22, 2023

dustymabe closed this May 9, 2024

Conversation

cgwalters commented Sep 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dustymabe commented Sep 20, 2023

Uh oh!

Uh oh!

ravanelli left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cgwalters commented Sep 20, 2023

Uh oh!

Uh oh!

dustymabe commented Sep 21, 2023

Uh oh!

Uh oh!

jlebon left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

cgwalters commented Sep 21, 2023

Uh oh!

cgwalters commented Sep 21, 2023

Uh oh!

cgwalters commented Sep 21, 2023

Uh oh!

jlebon commented Sep 21, 2023

Uh oh!

jlebon left a comment

Choose a reason for hiding this comment

Uh oh!

dustymabe Sep 22, 2023

Choose a reason for hiding this comment

Uh oh!

dustymabe Sep 22, 2023

Choose a reason for hiding this comment

Uh oh!

dustymabe commented Sep 22, 2023

Uh oh!

cgwalters commented Sep 22, 2023

Uh oh!

cgwalters commented Oct 6, 2023

Uh oh!

cgwalters commented Oct 12, 2023

Uh oh!

openshift-ci bot commented Oct 12, 2023

Uh oh!

openshift-merge-robot commented Oct 22, 2023

Uh oh!

cgwalters commented Oct 24, 2023

Uh oh!

dustymabe commented Oct 24, 2023

Uh oh!

cgwalters commented Oct 24, 2023

Uh oh!

dustymabe commented May 9, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

cgwalters commented Sep 20, 2023 •

edited

Loading

ravanelli left a comment •

edited

Loading