Skip to content

Fixes #39310 - Normalize firmware type for image based provisioning#10982

Merged
sbernhard merged 1 commit into
theforeman:developfrom
stejskalleos:ls/39310
May 21, 2026
Merged

Fixes #39310 - Normalize firmware type for image based provisioning#10982
sbernhard merged 1 commit into
theforeman:developfrom
stejskalleos:ls/39310

Conversation

@stejskalleos
Copy link
Copy Markdown
Contributor

@stejskalleos stejskalleos commented May 11, 2026

Fix suggestion from the community post: https://community.theforeman.org/t/46182

Co-Authored-By: @orelops95

evgeni
evgeni previously approved these changes May 15, 2026
Copy link
Copy Markdown
Member

@evgeni evgeni left a comment

Choose a reason for hiding this comment

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

This makes sense given the problem description, but a bit sad that we can't repro this in house.

"annotation" => args[:annotation],
"virtual_tpm" => args[:virtual_tpm],
"firmware" => args[:firmware],
"firmware" => normalize_firmware_type(args[:firmware]),
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Wait, args comes from parse_args and that does:

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

and that already calls normalize_firmware_type:
if firmware_type.present?
firmware = resolve_automatic_firmware(firmware, firmware_type)
attrs[:firmware] = normalize_firmware_type(firmware)
else

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Okay, reading https://community.theforeman.org/t/vmware-image-provisioning-fails-with-invalidvmconfig-invalidargument-after-upgrade-to-3-18-1-firmware-not-normalized-for-image-based-clone/46182 explains that this normalization only happens for PXE-style deployment, not for image based provisioning.

So why aren't we fixing it in the CR model for all CRs, instead of only for VMware?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

So why aren't we fixing it in the CR model for all CRs, instead of only for VMware?

I'll be honest, I was so focused on VMware that I have not thought of other resources.
IMO it makes sense to do the fix for all CRs.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

irb(main):001:0> h = Host::Managed.find_by(name: 'evgeni-01.example.com')
=> #<Host::Managed id: 187, name: "evgeni-01.example.com", last_compile: nil, last_report: nil, updated_at: "2026-05-15 10:23:21.822889000 +0000", created_at: "2026-05-15 10:23:21.822889000 +0000", root_pass: nil, architecture...
irb(main):002:0> h.pxe_loader
=> "PXELinux BIOS"
irb(main):003:0> h.firmware_type
=> :bios
irb(main):009:0> h.vm_compute_attributes[:firmware]
=> "efi"

this is… confusing (that's a libvirt deployment, not vmware, but the code should be the same, right)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

actually, the vmware clone method in https://github.com/theforeman/foreman/blob/develop/app/models/compute_resources/foreman/model/vmware.rb#L595 does re-set the attributes because of reasons - I don't know.

This method is only used in the very last step when the host is really created on the vmware. The "parse_args" method in https://github.com/theforeman/foreman/blob/develop/app/models/compute_resources/foreman/model/vmware.rb#L470 is called e.g. if you access the form or during validation (save).

Copy link
Copy Markdown
Contributor

@sbernhard sbernhard May 18, 2026

Choose a reason for hiding this comment

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

So why aren't we fixing it in the CR model for all CRs, instead of only for VMware?

This would be great - but I'm unsure if this will work. It would be easier, to add this normalize_firmware_type method for libvirt / vmware (and maybe others) to the create_vm method if it's image based deployment.

@evgeni evgeni dismissed their stale review May 15, 2026 09:51

doubts

@stejskalleos stejskalleos requested a review from a team as a code owner May 19, 2026 14:06
@github-actions github-actions Bot added the UI label May 19, 2026
@stejskalleos stejskalleos changed the title Fixes #39310 - VMware - Normalize firmware type in clone args Fixes #39310 - Normalize firmware type for image based provisioning May 19, 2026
@stejskalleos
Copy link
Copy Markdown
Contributor Author

With an OS without assigned PXE templates & VCenter v7, I was able to replicate the issue.
Updated fix is at the compute resource level, not just for VMware.

Tested VCenter 7 host creation and cloning, and both workflows work.
The fog/fog-vsphere#317 is not needed anymore.

@evgeni
Copy link
Copy Markdown
Member

evgeni commented May 19, 2026

you force-pushed the original (vmware only) version back, or is it github behaving like github again?

@stejskalleos
Copy link
Copy Markdown
Contributor Author

What the hell ...

@stejskalleos
Copy link
Copy Markdown
Contributor Author

stejskalleos commented May 20, 2026

you force-pushed the original (vmware only) version back, or is it github behaving like github again?

That's what you get when you force-push changes a minute before leaving. Pr updated

Comment thread app/models/compute_resource.rb Outdated
Comment thread test/models/compute_resource_test.rb
Comment thread app/models/compute_resource.rb
@sbernhard
Copy link
Copy Markdown
Contributor

I picked the change and did some quick tests on the UI. I was not able to reproduce the original issue. Nor I was able to reproduce the original issue from #10550 (comment)
So, looks promising.

I did not yet test it with hammer or API.

Copy link
Copy Markdown
Member

@evgeni evgeni left a comment

Choose a reason for hiding this comment

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

I patched an instance where I originally reproduced the issue and all 4 firmware types (auto, bios, uefi, uefi-sb) work fine with an OS that has no pxe loaders.

@evgeni evgeni requested a review from sbernhard May 20, 2026 14:43
@stejskalleos
Copy link
Copy Markdown
Contributor Author

I did not yet test it with hammer or API.

I created hosts with Hammer and cloned hosts in UI, so that should be covered.

Copy link
Copy Markdown

@shubhamsg199 shubhamsg199 left a comment

Choose a reason for hiding this comment

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

Ack.
Tested with this PR and I couldn't reproduce the issue.
Tested with all firmware -> Automatic, BIOS, UEFI, UEFI Secure Boot and all worked without any issue.

@sbernhard
Copy link
Copy Markdown
Contributor

Tested with API. Normalization works. Thank you very much @stejskalleos

@sbernhard sbernhard merged commit dc528f4 into theforeman:develop May 21, 2026
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants