-
Notifications
You must be signed in to change notification settings - Fork 175
Allow to setup the environment blob for grub #2932
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
2d0d3af to
20b1716
Compare
|
Do we have a staging image build to observe this with? |
I will do this today, stay tuned |
95eafdd to
0be45c9
Compare
|
I have to refactor several parts of the code in order to implement this better. |
0be45c9 to
d045467
Compare
|
@Conan-Kudo I updated the Staging build with the current code of this PR and also pointed the integration test to it. See the integration test build here: As you can see I used and it prints the setting as expected. However I'm unsure about the real behavior of the bootloader with this setting applied. The integration test also sets up the console to serial. Therefore if I run the integration test in qemu I can see the grub loader and the menu shown on the serial console. Should that I would like to build something that actually shows an effect. Thanks |
|
setting |
Hmm, doesn't behave like that for the built TW integration test. Can you do me a favor and do the following You will see the menu shown on the serial console. Boot into the system and call From my perspective I have done it correctly but it has no impact as you described. Thanks |
|
This feature doesn't exist in Tumbleweed yet. |
|
The test needs to be on Rawhide. |
ok that is an important information :) ... more fun tomorrow |
8f26905 to
f84c80a
Compare
|
f84c80a to
c368458
Compare
|
@Conan-Kudo this is now working. I have enabled integration tests for TW and rawhide. As you know rawhide doesn't build due to the libboost thing (not related to this PR). I want this to be merged not before the rawhide test can build again. As such setting to blocked |
|
On TW the test setup with Thanks |
|
@rdoxenham Thanks :) |
91741b7 to
f6a6656
Compare
|
@Conan-Kudo rawhide tests are building now again. Thanks for the quick turnaround. See the integration test here I enabled When I boot the integration test I still see the grub menu on the serial console. After boot I checked the env settings Please check the result as you see fit. |
|
I don't see any logging of the step for setting it and execution of the command or any output from it. |
osc rbl Virtualization:Appliances:Images:Testing_x86:rawhide test-image-live-disk images x86_64 -M Virtual This integration test builds multiple profiles. I have set the grub env for the |
|
Oh we need a test case with |
Well we can certainly add that but it should not make any difference. Point is this integration sets the value, it's there in the grubenv and that should be enough to test this feature. I also showed the log entry which allows to double check that things are happening. As such I'm seeking for some feedback and review if it does what it is expected to do. I'm a bit tired of getting more and more requests when I'm actually wanted to get some feedback on the current state. If it does not what it is expected to do on a simple layout image it will not help to add it to a complex layout image. As such can someone first share feedback if the current integration test: If there is a yes to both of these questions I'm willing to extend it to more integration tests Thanks |
|
The reason is that we need to check that the command actually works and returns the right result in that case. Because if it doesn't, then we're doing some other setup wrong. In principle, the code looks fine, but we need to deliberately check for different behavior when Btrfs is used. |
The layout of the disk is really immaterial. It has to work in any case.
I answered it myself. There are more settings that must play well together to make I updated the integration test and now the image behaves as expected. No menu shown Now we can apply the same settings to a btrfs integration test |
edb872a to
527de47
Compare
@Conan-Kudo done and tested, works for me. Please check The Virtual_btrfs profile build |
|
@Conan-Kudo this one is imho also ready for merge |
|
I'm doing a local verification and will merge after that. |
|
As I wrote in the matrix chat I believe this is not the way how you should check for env functionality. Your hexdump shows the environment blob of the grub boot code in the first 512 byte (MBR). That is not what grub loads via The grubenv blob from the correct boot volume is loaded by this action and also effective in all my tests for the image. Meaning the behavior with regards to the menu_auto_hide=1 is working as I would expect it. So unless your image boot process shows a menu I don't see a mistake. The env blob in the MBR is imho meaningless in this layout. In general the env blob in the MBR (if there is any) is the result of |
|
I'm not sure I understand what's going on here. The @WenhuaChang, @lsandov1: what is going wrong here? |
The BootLoaderInstallBase class was missing the default implementation for the set_disk_password API
Added new <environment> section to the existing <bootloadersettings> section which allows to specify environment variables for setting up an environment blob for the selected loader. With this commit we add support for grub by using grub2-editenv. Other loaders do not yet have an implementation or does not support environment blobs. Settings will be ignored for unsupported loaders. This Fixes #2922 Co-authored-by: Rhys Oxenham <rhys.oxenham@suse.com>
527de47 to
186c5bb
Compare

Added new section to the existing section which allows to specify environment variables for setting up an environment blob for the selected loader. With this commit we add support for grub by using grub2-editenv. Other loaders do not yet have an implementation or does not support environment blobs. Settings will be ignored for unsupported loaders.
This Fixes #2922