Fixes #37834 - Set Firmware selection correct #10529
Conversation
| <%= field(f, :firmware, :disabled => !new_vm, :label => _('Firmware'), :label_help => _("Choose 'Automatic' to set the firmware based on the host's PXE Loader. If no PXE Loader is selected, it defaults to BIOS."), :label_size => "col-md-2") do | ||
| <% | ||
| firmware_type = compute_resource.firmware_type(f.object.firmware, f.object.secure_boot) | ||
| firmware_help_block = _("If Compute Profile is set to 'Automatic', the firmware selection below was already auto-selected based on the PXE Loader. Setting firmware to 'Automatic' will match the detected firmware based on the PXE Loader unless manually changed.") if new_vm && controller_name != 'compute_attributes' |
There was a problem hiding this comment.
@Lennonka text review please (and for other texts as well)
ekohl
left a comment
ekohl
left a comment
There was a problem hiding this comment.
We really should have written tests for this behavior.
| source = opts.delete(:source).to_s | ||
| unless source == 'compute_profile' |
There was a problem hiding this comment.
You only set the source in the form, but how will that play with the API? It feels to me we're breaking some layers here: the controller should ensure the model is passed correct data.
There was a problem hiding this comment.
Actually, the main problem is, that the same form is used for Compute Profiles and Create Host and, that the this magic process_firmware_attributes(args[:firmware], firmware_type) is done while
a) the form is created (Compute Profile form and Create Host form)
b) after the form was saved, the data is stored in the DB and the host is created on the compute resource
In my opinion, the process_firmware_attributes should only be done for Create Host and when saving the data in the compute resource.
I have not tested it right now, but when used with the API, the source should be nil and therefore the process_firmware_attributes should be used as usual and all data should be handled as before.
Update. tested with hammer:
[root@test-deploy-bernhard ~]# hammer compute-profile info --id 10
Id: 10
Name: xyz_api
Created at: 2025/04/28 12:12:47
Updated at: 2025/04/28 12:12:47
Compute attributes:
1) Id: 7
Name: 1 CPUs and 2048 MB memory
Compute Resource: vcenter.atix.local
VM attributes: {"firmware"=>"uefi_secure_boot", "cpus"=>"1", "volumes_attributes"=>{}, "interfaces_attributes"=>{}}
[root@test-deploy-bernhard ~]# hammer compute-profile values update --compute-profile-id 10 --compute-resource-id 1 --compute-attributes firmware=bios,cpus=1
Compute profile attributes updated.
[root@test-deploy-bernhard ~]# hammer compute-profile info --id 10
Id: 10
Name: xyz_api
Created at: 2025/04/28 12:12:47
Updated at: 2025/04/28 12:12:47
Compute attributes:
1) Id: 7
Name: 1 CPUs and 2048 MB memory
Compute Resource: vcenter.atix.local
VM attributes: {"firmware"=>"bios", "cpus"=>"1", "volumes_attributes"=>{}, "interfaces_attributes"=>{}}
[root@test-deploy-bernhard ~]# hammer compute-profile values update --compute-profile-id 10 --compute-resource-id 1 --compute-attributes firmware=automatic,cpus=1
Compute profile attributes updated.
[root@test-deploy-bernhard ~]# hammer compute-profile info --id 10
Id: 10
Name: xyz_api
Created at: 2025/04/28 12:12:47
Updated at: 2025/04/28 12:12:47
Compute attributes:
1) Id: 7
Name: 1 CPUs and 2048 MB memory
Compute Resource: vcenter.atix.local
VM attributes: {"firmware"=>"automatic", "cpus"=>"1", "volumes_attributes"=>{}, "interfaces_attributes"=>{}}
There was a problem hiding this comment.
In my opinion, the
process_firmware_attributesshould only be done for Create Host and when saving the data in the compute resource.
You're right: it should only be made concrete when the compute resource is touched.
Update. tested with hammer:
Is that tested with the patch?
Could you also test creation of the compute profile with firmware set to automatic? My expectation is that it'll store automatic and not process it.
There was a problem hiding this comment.
Yes, it is with the patch. Here some more tests with the patch:
with automatic:
[root@test-deploy-master foreman]# hammer compute-profile create --name cp_automatic
Compute profile created.
[root@test-deploy-master foreman]# hammer compute-profile values create --compute-profile-id 6 --compute-resource-id 1 --compute-attributes firmware=automatic,cpus=1
Compute profile attributes are set.
[root@test-deploy-master foreman]# hammer compute-profile info --id 6
Id: 6
Name: cp_automatic
Created at: 2025/04/30 19:40:34
Updated at: 2025/04/30 19:40:34
Compute attributes:
1) Id: 3
Name: 1 CPUs and 2048 MB memory
Compute Resource: vcenter.local
VM attributes: {"firmware"=>"automatic", "cpus"=>"1"}
with bios:
[root@test-deploy-master foreman]# hammer compute-profile create --name cp_bios
Compute profile created.
[root@test-deploy-master foreman]# hammer compute-profile values create --compute-profile-id 8 --compute-resource-id 1 --compute-attributes firmware=bios,cpus=1
Compute profile attributes are set.
[root@test-deploy-master foreman]# hammer compute-profile info --id 8
Id: 8
Name: cp_bios
Created at: 2025/04/30 19:41:32
Updated at: 2025/04/30 19:41:32
Compute attributes:
1) Id: 5
Name: 1 CPUs and 2048 MB memory
Compute Resource: vcenter.local
VM attributes: {"firmware"=>"bios", "cpus"=>"1"}
the same thing happens without this PR, too BUT:
in case you have created a compute profile with firmware "bios" - in the comput profile UI it will show "automatic". If you then, on purpose or by accident (maybe because of a change of the cpu count or volumes) and press "save" -> it will save "automatic" as new value.
Same on the create host UI -> it will show automatic. in all cases.
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
nofaralfasi
left a comment
nofaralfasi
left a comment
There was a problem hiding this comment.
I’ve noticed three issues:
-
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.
-
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).
-
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.
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.
In my opinion, the issues with compute profiles and host creation are deeply interconnected and require a broader refactor, including proper test coverage.
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.
Yes, because [2] is used to determine firmware instead of setting it always to "Automatic".
This is very interesting. I thought, it should pick the correct firmware with [2] but it doesn't.
It makes it visible to the user what is currently on going without hiding some unexpected issues like:
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 |
|
@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 these PRS (10321, 10324) and this PR (10529) we have at lease this issue: 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. |
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. |
stejskalleos
requested review from
stejskalleos
and removed request for
stejskalleos
|
Fixed by #10550 |
5 participants
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