-
Notifications
You must be signed in to change notification settings - Fork 29
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
Rework grub2 default static config #841
base: main
Are you sure you want to change the base?
Conversation
Hi @champtar. Thanks for your PR. I'm waiting for a coreos member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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-sigs/prow repository. |
25c7a37
to
8710963
Compare
faddad9
to
74ff55c
Compare
Can someone with CI / FCOS knowledge chime in ? I think 10_blscfg and other configs.d are not copied in the |
@cgwalters any opinions on this PR ? |
Anyone ? |
@HuijingHei maybe ? |
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.
So sorry for the delay on review here! I really value your contributions overall. You've chosen to work on some thorny problems, which is good but for this particular code we're highly subject to regressions.
Overall though, I see no issues in this so far, and probably what we need to do is just try to land it and maybe put it in Rawhide and soak for a bit.
Actually after landing here in git main it will appear in the bootc base-images-dev and we have CI coverage of that.
/ok-to-test |
That all said it does look quite likely related that the upgrade test failed here https://jenkins-coreos-ci.apps.ocp.fedoraproject.org/job/bootupd/job/PR-841/6/artifact/tmp/console.txt |
See my previous comment #841 (comment), I bet the failing test only copy the new binary and not the new |
74ff55c
to
e422843
Compare
My ci fix is ignored, should I move it to another PR that we merge first, then rebase on top ? |
ci: also install grub config and systemd unit looks correct to me and would indeed make sense to aim to land as a distinct PR. |
#879 need to be merged first then I can rebase on top |
e422843
to
ca96bf5
Compare
LGTM, and I tried using bootc, and check the new created grub.cfg includes:
The steps are:
Use bootc install to install grub.cfg:
|
Need to fix CI first. |
In |
Ok I'm actually breaking ignition because 40_coreos-ignition.cfg is now after
|
Reordered the files a bit to see if it fixes, if yes I'll need to change ignition.cfg to 40_ignition.cfg |
Jenkins seems to be broken right now |
/ok-to-test |
PR for greenboot: fedora-iot/greenboot#214 |
On a legacy grub install we have:
Before this PR we have
After all the PRs we should have:
So the ordering related to blscfg is preserved (can break ignition and uefi-firmware), user.cfg + timeout are way sooner but this match legacy grub install How do we want to go forward with this PR ? |
As no one is using |
I went ahead and opened a PR to rename to |
(draft until coreos/fedora-coreos-config#3406 lands) |
let's assume the rename to |
@dustymabe I would remove the last commit, so that would be:
|
at the top of each of the files should we mention the legacy version of GRUB config they correspond to? |
Now waiting for coreos/fedora-coreos-config#3407 |
Merged |
user.cfg, despite his name, is usually used to store GRUB2_PASSWORD variable: - grub2-set-password utility overwrite the whole file - security scanners look at the content of user.cfg https://github.com/ComplianceAsCode/content/blob/47fd3bcded59116ade8ea09eb396f363e37813d4/linux_os/guide/system/bootloader-grub2/uefi/grub2_uefi_password/oval/shared.xml Copy the content of the legacy /etc/grub.d/01_users as 01_users.cfg, and add 41_custom.cfg to source custom.cfg for people in need of custom configs. This gets us closer to classic grub2-mkconfig behaviour.
Both features were added 13 years ago. feature_menuentry_id was added in rhboot/grub2@d9bef9b feature_all_video_module was added in rhboot/grub2@22c7ce8
In classic install timeout setting is at the end of 00_header, being at the end of grub-static-pre.cfg is equivalent. This allow to overide the timeout setting using configs.d. While at it remove the feature_timeout_style check as the feature was added 12 years ago in rhboot/grub2@8f236c1
This allows to add menu entries after the BLS entries, for exemple 'UEFI Firmware Settings'.
This will make updating config easier as there will be no need to cleanup dropins files in /boot/grub2/.
23a3c83
to
07b984f
Compare
07b984f
to
741e9a5
Compare
Waiting for fedora-iot/greenboot#214, |
Use bootc to build image, and verify with the patch that grub password works. Let me know if other tests are needed.
|
Rework grub2 default static config
GRUB2_PASSWORD
inuser.cfg
(partially fix https://issues.redhat.com/browse/RHEL-78299)