METAL-1750: baremetal: use q35 machine type with EFI firmware for bootstrap VM#10349
METAL-1750: baremetal: use q35 machine type with EFI firmware for bootstrap VM#10349elfosardo wants to merge 1 commit intoopenshift:mainfrom
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@elfosardo: This pull request references METAL-1750 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
|
/retest |
1 similar comment
|
/retest |
elfosardo
force-pushed
the
q35-efi-metal-support
branch
from
53a7f2e to
2399c31
Compare
|
/lgtm |
|
Looks good but conflicts with #9814 which should go in first. Sorry for the delay. /approve |
|
/hold |
openshift-ci
bot
added
the
do-not-merge/hold
|
/unhold |
openshift-merge-robot
added
the
needs-rebase
openshift-ci
bot
removed
the
do-not-merge/hold
elfosardo
force-pushed
the
q35-efi-metal-support
branch
from
2399c31 to
11bdb2d
Compare
openshift-merge-robot
removed
the
needs-rebase
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughCentralizes architecture-specific domain configuration into a new configureDomainArch() helper and adds unit tests; integrates arch-based firmware/machine/graphics rules (x86_64 → q35 + EFI; aarch64 → EFI; aarch64, s390x, ppc64* → graphics removed). Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.5.0)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Comment |
openshift-ci
bot
added
the
approved
|
@elfosardo: This pull request references METAL-1750 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/infrastructure/baremetal/bootstrap.go (1)
61-64:⚠️ Potential issue | 🟡 MinorTest expectation mismatch for Memory field.
The implementation sets
Memory.Value = 6withUnit = "GiB", but the test (TestNewDomainlines 18-19) expectsValue = 6144withUnit = "MiB". The test will fail.Either update the test to match:
- assert.Equal(t, uint(6144), dom.Memory.Value) - assert.Equal(t, "MiB", dom.Memory.Unit) + assert.Equal(t, uint(6), dom.Memory.Value) + assert.Equal(t, "GiB", dom.Memory.Unit)Or if 6144 MiB is intentional, update the implementation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/infrastructure/baremetal/bootstrap.go` around lines 61 - 64, The Memory block in the domain spec currently sets Memory.Value = 6 with Unit = "GiB", but tests expect 6144 MiB; update the libvirtxml.DomainMemory initialization (the Memory: &libvirtxml.DomainMemory{...} entry) to use Value = 6144 and Unit = "MiB" so the constructed domain matches TestNewDomain expectations (alternatively, if you prefer GiB, update the test to expect 6 GiB—pick one and make both implementation and tests consistent).
🧹 Nitpick comments (1)
pkg/infrastructure/baremetal/bootstrap_test.go (1)
27-63: Consider adding a test case for "ppc64" architecture.The implementation uses
strings.HasPrefix(arch, "ppc64")which handles bothppc64andppc64le. Onlyppc64leis tested. Consider adding appc64test case for completeness:{ name: "ppc64 removes graphics", arch: "ppc64", expectMachine: "", expectFirmware: "", expectGraphics: false, },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/infrastructure/baremetal/bootstrap_test.go` around lines 27 - 63, Add a new test case to TestConfigureDomainArch's tests slice for the "ppc64" architecture (similar to the existing "ppc64le" case) so the behavior driven by strings.HasPrefix(arch, "ppc64") is covered; specifically add a struct entry with name "ppc64 removes graphics", arch "ppc64", expectMachine "", expectFirmware "", and expectGraphics false to validate that ppc64 also removes graphics and does not set machine or firmware.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/infrastructure/baremetal/bootstrap_test.go`:
- Around line 18-19: The test asserts memory as 6144 MiB but newDomain sets
dom.Memory.Value = 6 and dom.Memory.Unit = "GiB"; update the assertions to match
newDomain by asserting dom.Memory.Value equals uint(6) and dom.Memory.Unit
equals "GiB" (refer to dom.Memory.Value and dom.Memory.Unit in the test and the
newDomain implementation).
---
Outside diff comments:
In `@pkg/infrastructure/baremetal/bootstrap.go`:
- Around line 61-64: The Memory block in the domain spec currently sets
Memory.Value = 6 with Unit = "GiB", but tests expect 6144 MiB; update the
libvirtxml.DomainMemory initialization (the Memory:
&libvirtxml.DomainMemory{...} entry) to use Value = 6144 and Unit = "MiB" so the
constructed domain matches TestNewDomain expectations (alternatively, if you
prefer GiB, update the test to expect 6 GiB—pick one and make both
implementation and tests consistent).
---
Nitpick comments:
In `@pkg/infrastructure/baremetal/bootstrap_test.go`:
- Around line 27-63: Add a new test case to TestConfigureDomainArch's tests
slice for the "ppc64" architecture (similar to the existing "ppc64le" case) so
the behavior driven by strings.HasPrefix(arch, "ppc64") is covered; specifically
add a struct entry with name "ppc64 removes graphics", arch "ppc64",
expectMachine "", expectFirmware "", and expectGraphics false to validate that
ppc64 also removes graphics and does not set machine or firmware.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ea8965f2-47c7-432a-9fd4-2e4aab3e5cdc
📒 Files selected for processing (2)
pkg/infrastructure/baremetal/bootstrap.gopkg/infrastructure/baremetal/bootstrap_test.go
elfosardo
force-pushed
the
q35-efi-metal-support
branch
from
11bdb2d to
a6e331e
Compare
|
@elfosardo: This pull request references METAL-1750 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/infrastructure/baremetal/bootstrap_test.go (1)
27-81: Well-structured table-driven tests.Good coverage of the architecture-specific behavior. Consider adding a test case for
ppc64(big-endian) to verify thestrings.HasPrefix(arch, "ppc64")logic handles both variants.Optional: Add ppc64 test case
{ name: "ppc64le removes graphics", arch: "ppc64le", expectMachine: "", expectFirmware: "", expectGraphics: false, }, + { + name: "ppc64 removes graphics", + arch: "ppc64", + expectMachine: "", + expectFirmware: "", + expectGraphics: false, + }, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/infrastructure/baremetal/bootstrap_test.go` around lines 27 - 81, Add a table-driven test case in TestConfigureDomainArch to exercise the strings.HasPrefix(arch, "ppc64") logic for the big-endian variant: add a test entry with name like "ppc64 big-endian handles ppc64 prefix", arch set to "ppc64", and the same expectations as ppc64le (expectMachine "", expectFirmware "", expectGraphics false); this ensures configureDomainArch and newDomain behavior for the "ppc64" prefix is validated alongside "ppc64le".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/infrastructure/baremetal/bootstrap_test.go`:
- Around line 27-81: Add a table-driven test case in TestConfigureDomainArch to
exercise the strings.HasPrefix(arch, "ppc64") logic for the big-endian variant:
add a test entry with name like "ppc64 big-endian handles ppc64 prefix", arch
set to "ppc64", and the same expectations as ppc64le (expectMachine "",
expectFirmware "", expectGraphics false); this ensures configureDomainArch and
newDomain behavior for the "ppc64" prefix is validated alongside "ppc64le".
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8352bd43-2f0b-4f3c-b764-1cb95ddd3470
📒 Files selected for processing (2)
pkg/infrastructure/baremetal/bootstrap.gopkg/infrastructure/baremetal/bootstrap_test.go
elfosardo
force-pushed
the
q35-efi-metal-support
branch
from
a6e331e to
aa7eda6
Compare
|
@elfosardo: This pull request references METAL-1750 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/infrastructure/baremetal/bootstrap.go (1)
360-362: Comment references SPICE but the domain uses VNC graphics.The comment on line 360 states "spice is not supported," but
newDomain()(lines 79-92) actually configures VNC graphics, not SPICE (the initial SPICE config at lines 32-38 is overwritten). If clearing graphics is correct for these architectures, consider updating the comment to reflect the actual graphics type being removed.📝 Suggested comment update
- // For aarch64, s390x, ppc64 and ppc64le spice is not supported + // For aarch64, s390x, ppc64 and ppc64le graphics console is not supported if arch == "aarch64" || arch == "s390x" || strings.HasPrefix(arch, "ppc64") { dom.Devices.Graphics = nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/infrastructure/baremetal/bootstrap.go` around lines 360 - 362, The comment saying "spice is not supported" is misleading because newDomain() ends up configuring VNC and the code clears dom.Devices.Graphics for certain architectures; update the comment to accurately reflect that VNC graphics (not SPICE) are being removed on those arches. Locate newDomain() and the branch that checks arch (uses variable arch and sets dom.Devices.Graphics = nil) and change the comment to something like "VNC graphics not supported on aarch64, s390x, ppc64 and ppc64le — clear graphics device" so it matches the actual behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/infrastructure/baremetal/bootstrap.go`:
- Around line 360-362: The comment saying "spice is not supported" is misleading
because newDomain() ends up configuring VNC and the code clears
dom.Devices.Graphics for certain architectures; update the comment to accurately
reflect that VNC graphics (not SPICE) are being removed on those arches. Locate
newDomain() and the branch that checks arch (uses variable arch and sets
dom.Devices.Graphics = nil) and change the comment to something like "VNC
graphics not supported on aarch64, s390x, ppc64 and ppc64le — clear graphics
device" so it matches the actual behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 82c28980-540b-4f58-ab8a-588c4d99754b
📒 Files selected for processing (2)
pkg/infrastructure/baremetal/bootstrap.gopkg/infrastructure/baremetal/bootstrap_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/infrastructure/baremetal/bootstrap_test.go
|
/retest |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dtantsur, zaneb The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/retest |
The bootstrap VM domain definition did not set a machine type, causing libvirt to default to the deprecated pc-i440fx-rhel7.6.0 chipset on CentOS Stream 9 hosts. This has been correlated with intermittent bootstrap VM startup failures in CI where the VM becomes unreachable after QEMU starts. Explicitly set the machine type to q35 with EFI firmware for x86_64, matching the pattern already used for aarch64 and aligning with how dev-scripts provisions master/worker VMs. The arch-specific domain configuration is extracted into a separate configureDomainArch() function with unit tests. Assisted-By: Claude 4.6 Opus High
elfosardo
force-pushed
the
q35-efi-metal-support
branch
from
aa7eda6 to
2b1d13b
Compare
|
@elfosardo: This pull request references METAL-1750 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
|
/lgtm |
|
/test e2e-metal-ipi-bm |
|
@elfosardo: The specified target(s) for The following commands are available to trigger optional jobs: Use DetailsIn response to this:
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. |
|
@elfosardo: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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. I understand the commands that are listed here. |
6 participants
The bootstrap VM domain definition did not set a machine type, causing libvirt to default to the deprecated pc-i440fx-rhel7.6.0 chipset on CentOS Stream 9 hosts. This has been correlated with intermittent bootstrap VM startup failures in CI where the VM becomes unreachable after QEMU starts.
Explicitly set the machine type to q35 with EFI firmware for x86_64, matching the pattern already used for aarch64 and aligning with how dev-scripts provisions master/worker VMs.
The arch-specific domain configuration is extracted into a separate configureDomainArch() function with unit tests.
Assisted-By: Claude 4.6 Opus High
Summary by CodeRabbit
Refactor
Tests