Default to UEFI (non-SB) on x86_64

[cgwalters]

There's some momentum around e.g. UKI
https://github.com/uapi-group/specifications/blob/main/specs/unified_kernel_image.md

I think it's about time to flip our default to be UEFI instead of BIOS.

(But, we obviously still do need to care about bios...and
we should e.g. be running at least a subset of our tests
acrosss that too)

Also while we're here, make the implementation here have the default for firmware be the empty string "" instead of bios, which doesn't really apply on e.g. ppc64le/s390x.

[dustymabe]

This would need related changes in https://github.com/coreos/coreos-ci-lib to keep the status quo with what we have today. We'd also probably need to make that code stop relying on the cosa default at all because for some time we'll be using older cosa while using latest coreos-ci-lib.

[dustymabe]

This would need related changes in https://github.com/coreos/coreos-ci-lib to keep the status quo with what we have today.

Well maybe not. Does this affect the defaults for kola testiso too?

[cgwalters]

Well maybe not. Does this affect the defaults for kola testiso too?

Yes, looks like most of the testiso scenarios will flip over to being UEFI by default on x86_64. I'll test that.

[bgilbert]

I'm okay with the premise, subject to Dusty's concerns about ratcheting. The change itself is too complex: it makes an empty Firmware value have three different behaviors at different points in the code. One of those is redundant and another one is too implicit. Let's simplify this.

[bgilbert]

Conversely, if the QEMU code treats the empty string as uefi on platforms where UEFI is supported, the remaining part of this conditional serves no purpose.

[cgwalters]

So yes...you're right but this also serves as a marker that this functionality actually requires uefi.

For example, one thing I can imagine us doing in the future is auto-generating a test matrix that e.g. runs some tests in BIOS, some in UEFI.

Hmm, perhaps what we want is kola.QEMUOptions.RequireUEFI - and hence it could also run in uefi-secure (which is a 3rd option in this matrix, that arguably instead should actually be the default...).

[openshift-ci]

@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	ef61465	link	true	/test rhcos

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.

[bgilbert]

This still seems much more complex than it needs to be. I agree that qemu.go is lower-level than kola, and I'm okay moving the defaulting logic there. But we don't need RequireUEFI at all; the original if kola.QEMUOptions.Native4k && kola.QEMUOptions.Firmware == "bios" check in syncOptions is fit for purpose. If we need something like RequireUEFI in the future, we can always add it.

For the qemu.go code, nested switch statements make things too hard to follow. Let's have two blocks: 1) convert a "" value into whatever makes sense on the current architecture, and 2) a switch block over every possible selected firmware, which checks invariants (like compatibility with the current architecture) and then implements whatever needs to be done for that firmware.

[cgwalters]

But we don't need RequireUEFI at all; the original if kola.QEMUOptions.Native4k && kola.QEMUOptions.Firmware == "bios" check in syncOptions is fit for purpose. If we need something like RequireUEFI in the future, we can always add it.

My motivation here is that - perhaps we actually want to change the default in qemu to uefi-secure (I think that's still open for debate and I'm increasingly thinking we should, but it has more fallout) - then the assignment in syncOptions to force uefi would be (IMO incorrectly) overriding it.

OK, though we can fix this another way - by exporting the default value and using it here. Done.

For the qemu.go code, nested switch statements make things too hard to follow.

Yeah, totally agreed. I started to do as you suggested but digging further we can actually do things a lot better by having it be right in the structure initialization.

[cgwalters]

To be clear, I should have originally marked this as draft because it was a quick drive-by thing I was investigating while looking at UEFI things. I should have made more clear it was more of a RFC. We don't need to merge this right away.

But that said I think the PR is a lot nicer now, the main blocker left seems to be

FAIL: coreos.boot-mirror.luks (147.40s)

and what seems messy here is that ISTM we effectively need to "invert" the control here - instead of doing ExcludeFirmwares: []string{"uefi"}, we should probably Firmware: "bios" but then we also need to propagate that requirement into the "cluster" in a way similar to the handling of AdditionalDisks?

[bgilbert]

My motivation here is that - perhaps we actually want to change the default in qemu to uefi-secure (I think that's still open for debate and I'm increasingly thinking we should, but it has more fallout) - then the assignment in syncOptions to force uefi would be (IMO incorrectly) overriding it.

I was initially proposing keeping the assignments in syncOptions, but I'm no longer proposing that. I'm saying that, since bios will no longer be the default, syncOptions can safely fail if BIOS is specified alongside 4Kn, as the original code was already doing. We shouldn't transparently convert to UEFI in that case; that'll make it unclear what's actually being run. As a result, AFAICT we shouldn't need any added plumbing outside of Exec(); we can just let a default "" value propagate there, then convert that to an arch-specific default (which would never be ""), then have a switch statement that acts on the selected firmware.

[cgwalters]

One major downside of this right now is that every boot starts off with "Boot option restoration" and a 5 second timer...

[bgilbert]

Right, that'll be fixed by the fix for coreos/fedora-coreos-tracker#946. I'm hoping to have some time to work on that early next year.

[cgwalters]

Split out the cleanups here into #3200

[openshift-merge-robot]

@cgwalters: 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.

[nikita-dubrovskii]

This is still nice to have, but not a priority right now. We may (re)consider it during work on UKIs