-
Notifications
You must be signed in to change notification settings - Fork 165
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
[9.4-stable] Add Support for Persistent OVMF Settings in Pillar #4269
Conversation
cd33c2b
to
1dcff8b
Compare
1dcff8b
to
8a9d13f
Compare
ddead4c
to
cf803d2
Compare
cf803d2
to
ec1945c
Compare
May God be with us, and may the semicolons fall exactly where they should! |
func getFmlCustomResolution(status *types.DomainStatus, globalConfig *types.ConfigItemValueMap) (string, error) { | ||
fmlResolutions := status.FmlCustomResolution | ||
// if not set in the domain status, try to get it from the global config | ||
if fmlResolutions == types.FmlResolutionUnset { |
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.
@shjala, are you sure about this line? I the master branch I had to add a comparison with "" here. For some reason, it was a case...
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.
the status.FmlCustomResolution is always set from the domainmanager, either to a custom resolution that was read from the "cloud-config" or if nothing present in the "cloud-config" is set to Unset.
But now that you mentioned I think I should add a single line in case there is no "cloud-config" at all, but that shouldn't prevent us from testing, still can use the global config or "cloud-config".
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.
Yesterday, without this extra check for "", I got a situation when file resolution was set to "", and it resulted in the wrong filename and crash on app start.
I HOPE IT WILL NOT BE THE CASE NOW, AFTER HOURS OF WAITING FOR THE HECKING WIN APP TO BE DOWNLOADED. Sorry =D
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 tested it on 9.4 multiple times, both cloud-config and global config where working fine.
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.
you can check for yourself logread -f | grep FmlResolution
.
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.
My deployment process has yet to reach the point where this code is called...
Manual test failed =( Will have to retest with a smaller app ( |
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.
Approving to see the Eden tests results.
Didn't we updated the UEFI hash? build is failing on Eden. |
1cbfdbf
to
4b43aa3
Compare
We should revert the changes in the UEFI package and hence - not update the hash. I remove the 2 corresponding commits from the master PR (updating UEFI, updating hsashes) |
4b43aa3
to
d0478ce
Compare
These commits should be dropped: |
d0478ce
to
ce346d5
Compare
I'm taking care of the PR to sync it with the master version. |
awesome, thanks! |
This commit copies OVMF_VARS.fd into the pillar container by adding it to /usr/lib/xen/boot/ovmf_vars.bin. It is important that the file is available in the pillar container because Pillar will create per-domain copies of it stored in /persist, which are then accessible to the xen-tools container. This sets the groundwork for enabling virtual machines to save and retain UEFI settings across reboots by using per-domain NVRAM files. Signed-off-by: Nikolay Martyanov <[email protected]>
Introduce support for managing per-domain OVMF_VARS.fd files, which are essential for maintaining persistent UEFI settings for FML guests. It adds functionality to prepare and clean up individual OVMF settings files stored in the persist directory, ensuring that each virtual machine has its own dedicated NVRAM file. The VM configuration structures are updated to reference the bootloader settings file, enabling the creation of unique UEFI variable stores for each domain. Signed-off-by: Nikolay Martyanov <[email protected]>
Switch to using separate OVMF_CODE.fd and OVMF_VARS.fd files for FML x86 modes instead of a combined .bin file. This ensures that settings are stored correctly and maintains consistent naming conventions. These changes do not affect containers, ARM or Xen. To support ARM the OVMF build should produce separate files. Currently it produces QEMU_EFI that incorporates both code and variable sections. Signed-off-by: Nikolay Martyanov <[email protected]>
Add a new global config value app.fml.resolution to set custom resolution for FML apps. This is a string value in the format of "widthxheight". This value can be set device-wide as a global config value or set in a per-app/vm setting by defining it as top-level variable in the cloud-config. Signed-off-by: Shahriyar Jalayeri <[email protected]>
Implemented logic to select predefined OVMF_VARS.fd files for specific screen resolutions. Added pre-saved OVMF settings for 800x600, 1024x768, 1280x800, and 1920x1080 resolutions, ensuring clean boot entries. Signed-off-by: Nikolay Martyanov <[email protected]>
Adapt the existing tests to account for the addition of the OVMF settings file for FML guests, ensuring the expected output includes the BootLoader. It also fixes the ARM tests by ensuring the firmware line is present in the expected configuration, aligning the tests with the updated behavior for UEFI boot on both AMD64 and ARM architectures. Signed-off-by: Nikolay Martyanov <[email protected]>
ce346d5
to
ba0e367
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.
Approving to run Eden
In the FIRMWARE doc, add a new section explaining what OVMF is, when it's used in EVE, the key OVMF files, how settings are managed, and future automation plans. Signed-off-by: Nikolay Martyanov <[email protected]>
Eden fails to start QEMU with EVE (not Application!) with this error:
That's strange... I remember we had this problem previously and I hope it does not related to the changes. I see the following fix by @milan-zededa in the Eden repo that fixes similar issue: So I would either ignore the Eden tests failure or ask @milan-zededa and @uncleDecart to help with the Eden tests fix. |
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.
Rerun workflows
I'll close this PR and create a new one (into the "-stable" branch) to trigger the necessary workflows. |
Backport of #4261.