theforeman
Fixes #37834 - Set Firmware selection correct
Without this PR, its is currently really broken. Without this PR, it does always! show 'Automatic' - except while editing hosts. For new/edit of Compute Profile or in the Create Host page - always 'Automatic' is shown. A user would never know what is really configured. This is because of [1].
If you create a host later, 'Automatic' is always displayed, regardless of which firmware is selected in the Compute Profile.
This change makes sure, that the selected firmware on the Compute Profile is the one which was saved previously and represents the value from the database while editing a existing Compute Profile.
On the Create Host page, it's a little bit more difficult, because the firmware is automatically chosed by the Compute Profile firmware and the PXE Loader field. To solve this and make it understandable for the user, a corresponding help message is shown while creating a host.
[1] https://github.com/theforeman/foreman/pull/10321/files#diff-c88b347f9af46e109987241498e9db3c776cc76433ffe3ba6f58ae1d8a74b9adR11
- When creating a host on VMware or Libvirt without selecting a compute profile, the firmware defaults to BIOS instead of Automatic. This behavior is inconsistent with the Hammer/API, where the default is Automatic.
I have tested this again with the state of develop branch: the firmware is always Automatic (because of [1]). In the version before the secure boot changes, it was set to Automatic, too. The behavior with this PR, is indeed that BIOS is selected. The root cause is, that this method is used [2] . In the current develop version, this is hidden because its always set to Automatic (see [1])
- When creating a compute profile with the firmware set to Automatic, assigning that profile to a host results in the host’s firmware being set to BIOS, rather than inheriting the Automatic value (as mentioned in your initial PR comment).
Yes, because [2] is used to determine firmware instead of setting it always to "Automatic".
- When creating a compute profile with the firmware set to Automatic, and a Host Group with PXE Loader set to
Grub2 UEFI SecureBoot, assigning that profile to a host still results in the firmware being set to BIOS, instead of resolving to UEFI Secure Boot via the Automatic logic.
This is very interesting. I thought, it should pick the correct firmware with [2] but it doesn't.
So, currently, this PR only addresses the issue on the compute profile form and adds a comment to the host form, which actually increases complexity for users during host creation.
It makes it visible to the user what is currently on going without hiding some unexpected issues like:
- create a profile with firmware "UEFI" and CPU count 2 -> Save.
- edit the profile again and change the CPU count to 4 - and this is the only change you want to do -> save => afterwards, the compute profile has CPU count 4 and the firmware is saved with "Automatic" because the form does always show and use! 'Automatic'.
In my opinion, the issues with compute profiles and host creation are deeply interconnected and require a broader refactor, including proper test coverage.
Yes, this is something we can do but, the current behavior is a step backward and therefore I would suggest to improve the behaviour step by step.
The root cause of the overall issue is, that the process_firmware_attributes(args[:firmware], firmware_type) is used while creating the form and not only after saving the form and storing the value to the database. Unfortunately, I think, this is how this froms in foreman / rails works and I have no clue how to improve this.
[1] https://github.com/theforeman/foreman/pull/10321/files#diff-c88b347f9af46e109987241498e9db3c776cc76433ffe3ba6f58ae1d8a74b9adR11
[2] https://github.com/theforeman/foreman/pull/10321/files#diff-525a301145c328b1c4d2e6dd7668a9ce4b9f64edb7b2876a4cdebeb5ee2689f9R152
@sbernhard Could you clarify what functionality was working before and is now broken as a result of the changes introduced in these PRs? https://github.com/theforeman/foreman/pull/10321, https://github.com/theforeman/foreman/pull/10324
@sbernhard Could you clarify what functionality was working before and is now broken as a result of the changes introduced in these PRs? #10321, #10324
Sure: Without the two mentioned PRs (10321 and 10324) on the compute profile page you selected firmware "EFI" and saved this. If you visit this compute profile again, it shows the selected firmware "EFI". On the create hosts page, if you select a compute profile "EFI", the firmware is selected to "EFI".
Without these PRS (10321, 10324) and this PR (10529) we have at lease this issue: On the compute profile page and firmware set to "Automatic" you visit it again, and the page shows "BIOS". Same on the create host page you select the Automatic firmware compute profile and it shows "BIOS" as selected.
With 10321 and 10324, its always switchting to "Automatic" on the compute profile and on the create host page. For the user, it looks like its never possible to change to any other value than "Automatic".
The implementation before 10321,10324 had also some pretty bad issues, but at least it wasn't always just "Automatic". Like, in case you wantted to enforce BIOS or EFI on the compute profile, this wasn't changed automatically to "Automatic".
If we pick 10321, 10324 and 10529 we are, AFAIK, 100% correct on the compute profile page.
There is still the issue on on the create hosts page mentioned by you in your previous comment which I tried to address by the hint for the user on the create host page.
Given that this PR introduces new bugs, I recommend holding off on merging it for now. Ideally, we should first focus on finding a way to resolve the issues introduced in PRs #10321 and #10324. Unfortunately, I don’t see a straightforward fix at the moment.
Given that this PR introduces new bugs, I recommend holding off on merging it for now. Ideally, we should first focus on finding a way to resolve the issues introduced in PRs #10321 and #10324. Unfortunately, I don’t see a straightforward fix at the moment.
I would not recommend this. In my point of view, without this PR, its even more broken. Other may need to decide.
But, I had an idea to solve the last remaining things on the create host page. This would be possible to fix in another PR, too.