-
Notifications
You must be signed in to change notification settings - Fork 29
adopt: add tag to install the static GRUB config from tree #945
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Skipping CI for Draft Pull Request. |
aac4857
to
ad92037
Compare
555700c
to
a5a7636
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this! An initial review pass here
src/bootupd.rs
Outdated
const BACKUP: &str = "grub.cfg.backup"; | ||
let grubconfig = grub_config_dir.join(GRUBCONFIG); | ||
|
||
#[cfg(any(target_arch = "x86_64", target_arch = "powerpc64"))] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nonblocking and not a new issue but we should probably centralize this into a #[cfg(grub_arch)]
or so (I think we'd need to do that in a build.rs
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Like #[cfg(grub_arch="bios")]
to match #[cfg(any(target_arch = "x86_64", target_arch = "powerpc64"))]
,
and #[cfg(grub_arch="efi")]
to match #[cfg(any( target_arch = "x86_64", target_arch = "aarch64", target_arch = "riscv64" ))]
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah something like that, anyways it's fine as is!
src/bootupd.rs
Outdated
if !grubconfig.exists() { | ||
// Not existing, maybe UEFI | ||
println!("Not found '{}'", grubconfig.display()); | ||
if cfg!(target_arch = "powerpc64") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That said is powerpc64 actually relevant here? It looks like https://mirror.openshift.com/pub/openshift-v4/ppc64le/dependencies/rhcos/ only has a version starting from 4.3...was that not using static configs? (Ugh it's painful to inspect from the outside because that's using the old LUKS setup)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I check 4.3 on ppc, it is already using bootloader=none
, maybe better to remove the condition?
Makefile
Outdated
@@ -37,6 +37,7 @@ install-grub-static: | |||
.PHONY: install-systemd-unit | |||
install-systemd-unit: | |||
install -m 644 -D -t "${DESTDIR}$(PREFIX)/lib/systemd/system/" systemd/bootloader-update.service | |||
install -m 644 -D -t "${DESTDIR}$(PREFIX)/lib/systemd/system/" systemd/bootloader-migrate.service |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It'd be nice to only enable this service for cases that need it which is basically RHCOS right?
I think a lot of cases now are using install-systemd-unit
so this will have a large blast radius as is.
Now in theory of course the
ExecCondition=/bin/sh -c '[ "$(ostree config --repo=/sysroot/ostree/repo get sysroot.bootloader 2>/dev/null)" != "none" ]'
will mean this new service just starts and then does nothing on each boot for those systems, but...it's probably worth turning this around and only actually installing the service there inside openshift/os or so?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And didn't we end up writing something similar to this for the atomic desktops? cc @travier
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We added a drop-in for the existing unit for the Atomic Desktops: https://pagure.io/workstation-ostree-config/blob/main/f/bootupd.yaml#_17
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is very much a "per-variant" thing here as to how to do this, so we should probably not install this by default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(I am jetlagged right now and may be missing something obvious but) how is that different from what's being done here? Is it about fetching the static config from the new version in the target root?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, unless I'm missing something, the line we're commenting on here is just installing the unit, not enabling it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, by default the unit is disabled. We should make it enabled on RHCOS to let it running on each boot. I did testing with starting the service manually after boot.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm. This unit is not just enabling static configs, it's also doing the adopt-and-update
flow i.e. updating the bootloader as well (which makes sense!).
Wouldn't it be simpler to structure this then as a drop-in for the bootloader-update.service
unit? I think we'd trigger it on the firstboot path in OCP or so?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't it be simpler to structure this then as a drop-in for the
bootloader-update.service
unit?
In this case, bootloader-update.service
will run update
, and we only need to enable static config for installed component, right? This is a little different from adopt-and-update --with-static-config
that only enable static config for adoptable component. This would make with-static-config
independently without update or adopt like the current migrate
. Or also make update --with-static-config
work, that means enable static for installed component. WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I'm not sure about structuring this as a drop-in for bootloader-update.service
. But I think it's fine to carry the unit definition in OCP if for whatever reason we don't want it to live in bootupd. That probably makes sense anyway since the conditions under which you want to do this migration can be quite specific (and I guess is why e.g. no systemd unit was added as part of the Atomic Desktop migration work either).
da7bbfe
to
5f29489
Compare
5f29489
to
c347741
Compare
96242b0
to
b1063ac
Compare
Update to |
So, combining the two threads here. How about:
|
b1063ac
to
1c8fde0
Compare
SGTM, does this mean we will add the script in openshift/os 4.19 branch? |
I guess so, but meh... I'm also fine to just keep it in rhel-coreos-config. That would mean we wouldn't be able to remove it until 4.22 instead of 4.21, but that's fine. It probably make testing the final fix easier. |
That should be I think "only if |
OK to me, do you mean add the generator in rhel-coreos-config for main branch (that is 4.20), then backport to 4.19?
Maybe add check in generator to skip s390x. |
03e32a4
to
6b37831
Compare
Could you help to review coreos/rhel-coreos-config#23 ? Thank you! |
The rhel-coreos-config repo does not branch per OCP release. |
6b37831
to
c054196
Compare
7a91078
to
a1297fa
Compare
@jlebon I update the logic that will migrate to static grub config if |
If the EFI System Partition (ESP) is unmounted, GRUB config installation can fail silently or with little useful information. This change adds better logging to help diagnose such failures by clearly reporting when the ESP is not mounted.
Add `bootupctl adopt-and-update --with-static-config` to migrate RHCOS system to use static GRUB config, for example 4.1/4.2 born RHCOS nodes. Fixes: https://issues.redhat.com/browse/OCPBUGS-52485
a1297fa
to
6818742
Compare
Add
bootupctl adopt-and-update --with-static-config
to migrateRHCOS system to use static GRUB config, for example 4.1/4.2 born
RHCOS nodes.
Fixes: https://issues.redhat.com/browse/OCPBUGS-52485