OCPBUGS-78935: Fix bootstrap VM on non-x86 platforms#10419
OCPBUGS-78935: Fix bootstrap VM on non-x86 platforms#10419zaneb wants to merge 1 commit intoopenshift:mainfrom
Conversation
On x86 the bus type for cdrom devices must be explicitly set to 'sata', because the default 'ide' is outdated (and not even supported on q35 machines). However, on other platforms, this is not supported and we must use 'scsi' as the bus type. This is consistent with the settings for cdrom devices used by virt-manager and OpenStack Nova.
openshift-ci-robot
added
jira/valid-reference
|
@zaneb: This pull request references Jira Issue OCPBUGS-78935, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. 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. |
WalkthroughModified libvirt domain bootstrap configuration to add a SCSI controller and adjust device bus assignments based on CPU architecture, defaulting live CD-ROM disks to SCSI except on x86_64 systems where SATA is retained. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes ✨ 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.11.3)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Comment Tip CodeRabbit can use TruffleHog to scan for secrets in your code with verification capabilities.Add a TruffleHog config file (e.g. trufflehog-config.yml, trufflehog.yml) to your project to customize detectors and scanning behavior. The tool runs only when a config file is present. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@zaneb: This pull request references Jira Issue OCPBUGS-78935, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
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.
🧹 Nitpick comments (1)
pkg/infrastructure/baremetal/bootstrap.go (1)
51-56: Scope the SCSI controller to the non-x86 path.The
x86_64branch below keeps the CD-ROM onsata, so addingvirtio-scsihere still changes the x86 device topology for no functional gain. Appending this controller only when the selected CD-ROM bus isscsikeeps the x86 XML closer to today’s behavior and avoids the controller/bus selection drifting apart later.Possible cleanup
func newDomain(name string) libvirtxml.Domain { domainDef := libvirtxml.Domain{ ... Devices: &libvirtxml.DomainDeviceList{ ... - Controllers: []libvirtxml.DomainController{ - { - Type: "scsi", - Model: "virtio-scsi", - }, - }, }, ... }if arch == "x86_64" { // x86 traditionally uses IDE or SATA (only the latter is supported // with the q35 machine type) for cdrom devices liveCD.Target.Bus = "sata" + } else { + bootstrapDom.Devices.Controllers = append(bootstrapDom.Devices.Controllers, libvirtxml.DomainController{ + Type: "scsi", + Model: "virtio-scsi", + }) }🤖 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 51 - 56, The code unconditionally appends a libvirtxml.DomainController with Type "scsi" / Model "virtio-scsi" in the Controllers slice, which alters x86_64 guest topology; change this so the controller is only added when the chosen CD-ROM bus is "scsi" (i.e., scope the append to the non‑x86 path or to the path where cdromBus == "scsi"). Locate where Controllers is populated (the Controllers: []libvirtxml.DomainController {...} block) and wrap or gate the append/entry so it only runs when the CD-ROM bus selection equals "scsi" (or when arch != "x86_64" if your code uses that branch), ensuring x86 XML remains unchanged otherwise.
🤖 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 51-56: The code unconditionally appends a
libvirtxml.DomainController with Type "scsi" / Model "virtio-scsi" in the
Controllers slice, which alters x86_64 guest topology; change this so the
controller is only added when the chosen CD-ROM bus is "scsi" (i.e., scope the
append to the non‑x86 path or to the path where cdromBus == "scsi"). Locate
where Controllers is populated (the Controllers: []libvirtxml.DomainController
{...} block) and wrap or gate the append/entry so it only runs when the CD-ROM
bus selection equals "scsi" (or when arch != "x86_64" if your code uses that
branch), ensuring x86 XML remains unchanged otherwise.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: af0e20fc-666a-4f7f-998d-e6d445631a9d
📒 Files selected for processing (1)
pkg/infrastructure/baremetal/bootstrap.go
|
/retest |
openshift-ci-robot
added
verified-later
verified
|
@sgoveas: This PR has been marked to be verified later by 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. |
|
@zaneb: 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. |
3 participants
On x86 the bus type for
cdromdevices must be explicitly set tosata, because the defaultideis outdated (and not even supported onq35machines). However, on other platforms, this is not supported and we must usescsias the bus type.This is consistent with the settings for
cdromdevices used by virt-manager and OpenStack Nova.