Skip to content

Fixes #37834 - Set Firmware selection correct #10529

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.

Sign up for GitHub

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

Closed
sbernhard wants to merge 1 commit into from
Closed

Fixes #37834 - Set Firmware selection correct #10529

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions app/models/compute_resources/foreman/model/libvirt.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,8 +148,11 @@ def new_vm(attr = { })
opts[:boot_order] = %w[hd]
opts[:boot_order].unshift 'network' unless attr[:image_id]

firmware_type = opts.delete(:firmware_type).to_s
opts.merge!(process_firmware_attributes(opts[:firmware], firmware_type))
source = opts.delete(:source).to_s
unless source == 'compute_profile'
Comment on lines +151 to +152
Copy link
Member

@ekohl ekohl Apr 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@sbernhard sbernhard Apr 28, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"=>{}}

Copy link
Member

@ekohl ekohl Apr 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my opinion, the process_firmware_attributes should 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.

Copy link
Contributor Author

@sbernhard sbernhard Apr 30, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

firmware_type = opts.delete(:firmware_type).to_s
opts.merge!(process_firmware_attributes(opts[:firmware], firmware_type))
end

vm = client.servers.new opts
vm.memory = opts[:memory] if opts[:memory]
Expand Down
9 changes: 7 additions & 2 deletions app/models/compute_resources/foreman/model/vmware.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -481,8 +481,13 @@ def parse_args(args)

args.except!(:hardware_version) if args[:hardware_version] == 'Default'

firmware_type = args.delete(:firmware_type).to_s
args.merge!(process_firmware_attributes(args[:firmware], firmware_type))
source = args.delete(:source).to_s

unless source == 'compute_profile'
firmware_type = args.delete(:firmware_type).to_s
args.merge!(process_firmware_attributes(args[:firmware], firmware_type))
end

args[:virtual_tpm] = validate_tpm_compatibility(args[:virtual_tpm], args[:firmware])

args.reject! { |k, v| v.nil? }
Expand Down
2 changes: 1 addition & 1 deletion app/views/compute_attributes/_form.html.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
<%= select_f f, :compute_profile_id, ComputeProfile.all, :id, :name, {}, :disabled => true, :label => _('Compute profile'), :label_size => "col-md-2" %>
<%= select_f f, :compute_resource_id, ComputeResource.all, :id, :to_label, {}, :disabled => true, :label => _('Compute resource'), :label_size => "col-md-2" %>

<%= fields_for 'compute_attribute[vm_attrs]', @set.compute_resource.new_vm(@set.vm_attrs) do |f2| %>
<%= fields_for 'compute_attribute[vm_attrs]', @set.compute_resource.new_vm(@set.vm_attrs.merge(:source => 'compute_profile')) do |f2| %>
<%= render :partial => "compute_form",
:locals => { :f => f2, :compute_resource => @set.compute_resource, :selected_cluster => @set.vm_attrs['cluster'] } %>
<% end %>
Expand Down
7 changes: 5 additions & 2 deletions app/views/compute_resources_vms/form/libvirt/_base.html.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,11 @@
<% checked = params[:host] && params[:host][:compute_attributes] && params[:host][:compute_attributes][:start] || '1' %>
<%= checkbox_f f, :start, { :checked => (checked == '1'), :help_inline => _("Power ON this machine"), :label => _('Start'), :label_size => "col-md-2"} if new_vm && controller_name != "compute_attributes" %>

<% firmware_type = new_vm ? 'automatic' : compute_resource.firmware_type(f.object.firmware, f.object.secure_boot) %>
<%= 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 firmware on the compute profile is set to 'Automatic', the firmware above has already been pre-selected based on the PXE loader. If you set firmware to 'Automatic', Foreman will base the firmware on the PXE loader unless you select it manually.") if new_vm && controller_name != 'compute_attributes'
%>
<%= field(f, :firmware, :disabled => !new_vm, :label => _('Firmware'), :help_block => firmware_help_block, :label_help => _("Choose 'Automatic' to set the firmware based on the PXE loader of the host. If no PXE loader is selected, firmware defaults to BIOS."), :label_size => "col-md-2") do
compute_resource.firmware_types.collect do |type, name|
radio_button_f f, :firmware, {:checked => (firmware_type == type), :disabled => !new_vm, :value => type, :text => _(name) }
end.join(' ').html_safe
Expand Down
7 changes: 5 additions & 2 deletions app/views/compute_resources_vms/form/vmware/_base.html.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,11 @@
</div>
<%= text_f f, :memory_mb, :class => "col-md-2", :label => _("Memory (MB)") %>

<% firmware_type = new_vm ? 'automatic' : compute_resource.firmware_type(f.object.firmware, f.object.secure_boot) %>
<%= field(f, :firmware, :label => _('Firmware'), :label_help => _("Choose 'Automatic' to set the firmware based on the 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 firmware on the compute profile is set to 'Automatic', the firmware above has already been pre-selected based on the PXE loader. If you set firmware to 'Automatic', Foreman will base the firmware on the PXE loader unless you select it manually.") if new_vm && controller_name != 'compute_attributes'
%>
<%= field(f, :firmware, :label => _('Firmware'), :help_block => firmware_help_block, :label_help => _("Choose 'Automatic' to set the firmware based on the PXE loader of the host. If no PXE loader is selected, firmware defaults to BIOS."), :label_size => "col-md-2") do
compute_resource.firmware_types.collect do |type, name|
radio_button_f f, :firmware, { :checked => (firmware_type == type), :disabled => !new_vm, :value => type, :text => _(name) }
end.join(' ').html_safe
Expand Down
Loading