✨ Make bootMACAddress optional for virtual media with inspection enabled #2793

donpenney · 2025-11-17T19:22:57Z

Virtual media BMC drivers (redfish-virtualmedia, idrac-virtualmedia,
ilo5-virtualmedia) can boot from virtual media and discover the MAC
address during hardware inspection. Therefore, bootMACAddress is optional
when inspection is enabled but still required when inspection is disabled
(since there's no other way to discover the MAC address).

This change updates the virtual media driver implementations to return
false from their NeedsMAC() methods, and adds validation logic to require
bootMACAddress when virtual media is used with inspection disabled.

Drivers that require PXE boot (like libvirt, ipmi) continue to require
bootMACAddress in all cases.

Changes:

pkg/hardwareutils/bmc/redfish_virtualmedia.go: NeedsMAC() returns false
pkg/hardwareutils/bmc/idrac_virtualmedia.go: NeedsMAC() returns false
internal/webhooks/metal3.io/v1alpha1/baremetalhost_validation.go:
Add logic to require bootMACAddress for virtual media when inspection
is disabled (via InspectionMode field or inspect.metal3.io annotation)
pkg/provisioner/ironic/ironic.go: Skip MAC-based port queries when
bootMACAddress is empty to prevent false MAC conflicts

Test Coverage:

internal/webhooks/metal3.io/v1alpha1/baremetalhost_validation_test.go:
- bootMACAddress not required for virtual media with inspection enabled
- bootMACAddress required for virtual media with inspection disabled
- bootMACAddress valid when provided for virtual media with inspection disabled
pkg/hardwareutils/bmc/access_test.go:
Update virtual media test expectations (needsMac: false)

Assisted-By: Claude [email protected]

Checklist:

Documentation has been updated, if necessary.
Unit tests have been added, if necessary.
E2E tests have been added, if necessary.
Integration tests have been added, if necessary.

metal3-io-bot · 2025-11-17T19:23:04Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign dtantsur for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

metal3-io-bot · 2025-11-17T19:23:09Z

Hi @donpenney. Thanks for your PR.

I'm waiting for a github.com member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

dtantsur · 2025-11-18T12:31:12Z

I'm not sure I understand the link between PreprovisioningNetworkDataName and BootMacAddress. Honestly, I suggest just dropping the requirement on BootMacAddress for virtual media based drivers.

donpenney · 2025-11-18T13:38:03Z

I'm not sure I understand the link between PreprovisioningNetworkDataName and BootMacAddress. Honestly, I suggest just dropping the requirement on BootMacAddress for virtual media based drivers.

Thanks @dtantsur . I was aiming to be a bit surgical here to minimize behavoural changes. My thinking was that if the network config has been provided in an nmstate secret as part of the initial configuration, then the BootMacAddress was unnecessary for the inspection stage. I assumed it would always be the case for inspection with virtual media, but hadn't thought about using that as the determining factor, so I'll go that route instead.

dtantsur · 2025-11-18T13:54:11Z

For background: since quite some time, the Ironic inspection implementation for Redfish is actually hybrid: it starts with getting MAC addresses from Redfish, then reboots into the agent ramdisk.

Now that I said that, I think we should still require BootMacAddress if inspection is disabled on the BMH @donpenney.

Virtual media BMC drivers (redfish-virtualmedia, idrac-virtualmedia, ilo5-virtualmedia) can boot from virtual media and discover the MAC address during hardware inspection. Therefore, bootMACAddress is optional when inspection is enabled but still required when inspection is disabled (since there's no other way to discover the MAC address). This change updates the virtual media driver implementations to return false from their NeedsMAC() methods, and adds validation logic to require bootMACAddress when virtual media is used with inspection disabled. Drivers that require PXE boot (like libvirt, ipmi) continue to require bootMACAddress in all cases. Changes: - pkg/hardwareutils/bmc/redfish_virtualmedia.go: NeedsMAC() returns false - pkg/hardwareutils/bmc/idrac_virtualmedia.go: NeedsMAC() returns false - internal/webhooks/metal3.io/v1alpha1/baremetalhost_validation.go: Add logic to require bootMACAddress for virtual media when inspection is disabled (via InspectionMode field or inspect.metal3.io annotation) - pkg/provisioner/ironic/ironic.go: Skip MAC-based port queries when bootMACAddress is empty to prevent false MAC conflicts Test Coverage: - internal/webhooks/metal3.io/v1alpha1/baremetalhost_validation_test.go: * bootMACAddress not required for virtual media with inspection enabled * bootMACAddress required for virtual media with inspection disabled * bootMACAddress valid when provided for virtual media with inspection disabled - pkg/hardwareutils/bmc/access_test.go: Update virtual media test expectations (needsMac: false) Assisted-By: Claude <[email protected]> Signed-off-by: Don Penney <[email protected]>

dtantsur · 2025-11-19T15:38:17Z

internal/webhooks/metal3.io/v1alpha1/baremetalhost_validation.go

 	}

-	if oldObj.Spec.BootMACAddress != "" && !strings.EqualFold(newObj.Spec.BootMACAddress, oldObj.Spec.BootMACAddress) {
+	if oldObj.Spec.BootMACAddress != "" && newObj.Spec.BootMACAddress != oldObj.Spec.BootMACAddress {


What's the reason you're reverting the EqualFold part? I think it was here on purpose so that we're not case sensitive.

dtantsur · 2025-11-19T15:39:06Z

internal/webhooks/metal3.io/v1alpha1/baremetalhost_validation.go


-func validateBMCAccess(s metal3api.BareMetalHostSpec, bmcAccess bmc.AccessDetails) []error {
+// isInspectionDisabled checks if inspection is disabled via annotation or spec field.
+func isInspectionDisabled(host *metal3api.BareMetalHost) bool {


There is already host.InspectionDisabled

Thanks! I'll get these fixed up and retested in my lab, then update the PR

dtantsur · 2025-11-19T15:40:00Z

/ok-to-test

Changes: 1. Restore case-insensitive MAC comparison using strings.EqualFold() - The EqualFold check was removed inadvertently but is necessary for case-insensitive bootMACAddress validation 2. Use existing host.InspectionDisabled() method - Remove duplicate isInspectionDisabled() function - Replace custom implementation with the existing BareMetalHost method - The host.InspectionDisabled() method provides identical functionality Assisted-by: Claude (AI Assistant) Signed-off-by: Don Penney <[email protected]>

elfosardo · 2025-11-20T10:25:14Z

/lgtm

metal3-io-bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 17, 2025

metal3-io-bot requested review from honza and zaneb November 17, 2025 19:23

metal3-io-bot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Nov 17, 2025

metal3-io-bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Nov 17, 2025

donpenney force-pushed the allow-unset-boot-mac branch from dcd4003 to da22bd4 Compare November 18, 2025 22:02

donpenney changed the title ~~✨ Allow BMH registration without bootMACAddress when preprovisioningNet…~~ ✨ Make bootMACAddress optional for virtual media with inspection enabled Nov 18, 2025

donpenney marked this pull request as ready for review November 19, 2025 14:51

metal3-io-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 19, 2025

metal3-io-bot requested review from dtantsur and lentzi90 November 19, 2025 14:51

donpenney mentioned this pull request Nov 19, 2025

Add support for deferred boot MAC population from interface labels openshift-kni/oran-o2ims#1427

Merged

dtantsur reviewed Nov 19, 2025

View reviewed changes

metal3-io-bot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Nov 19, 2025

metal3-io-bot added the lgtm Indicates that a PR is ready to be merged. label Nov 20, 2025

✨ Make bootMACAddress optional for virtual media with inspection enabled #2793

Are you sure you want to change the base?

✨ Make bootMACAddress optional for virtual media with inspection enabled #2793

Uh oh!

Conversation

donpenney commented Nov 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

metal3-io-bot commented Nov 17, 2025

Uh oh!

metal3-io-bot commented Nov 17, 2025

Uh oh!

dtantsur commented Nov 18, 2025

Uh oh!

donpenney commented Nov 18, 2025

Uh oh!

dtantsur commented Nov 18, 2025

Uh oh!

dtantsur Nov 19, 2025

Choose a reason for hiding this comment

Uh oh!

donpenney Nov 19, 2025

Choose a reason for hiding this comment

Uh oh!

dtantsur Nov 19, 2025

Choose a reason for hiding this comment

Uh oh!

donpenney Nov 19, 2025

Choose a reason for hiding this comment

Uh oh!

donpenney Nov 19, 2025

Choose a reason for hiding this comment

Uh oh!

dtantsur commented Nov 19, 2025

Uh oh!

elfosardo commented Nov 20, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

donpenney commented Nov 17, 2025 •

edited

Loading