Closed
Conversation
atd9876
force-pushed
the
604-provide-support-for-boot-from-virtual-media
branch
from
67253d1 to
520e64e
Compare
atd9876
changed the title
github-actions
Bot
added
the
documentation
atd9876
force-pushed
the
604-provide-support-for-boot-from-virtual-media
branch
from
956bb9e to
2fee327
Compare
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds VirtualMedia boot support across API, CRDs, BMC interfaces/implementations, controller logic, samples, and docs: new Changes
Sequence DiagramsequenceDiagram
participant Claim as ServerClaim
participant ClaimCtrl as Claim Controller
participant SBC as ServerBootConfiguration
participant BootOp as boot-operator
participant ServerCtrl as Server Controller
participant BMC as BMC Interface
participant OEM as OEM Redfish Impl
participant Redfish as Redfish Endpoint
Claim->>ClaimCtrl: create with spec.bootMethod = VirtualMedia
ClaimCtrl->>SBC: create/patch ServerBootConfiguration (bootMethod copied)
BootOp->>SBC: populate status.bootISOURL / status.configISOURL
ServerCtrl->>SBC: fetch referenced BootConfiguration
ServerCtrl->>BMC: GetVirtualMediaStatus(systemURI)
BMC->>OEM: delegate to OEM impl
OEM->>Redfish: query VirtualMedia
Redfish-->>OEM: return slots
OEM-->>BMC: virtual media list
BMC-->>ServerCtrl: status list
ServerCtrl->>BMC: EjectVirtualMedia(slot)
BMC->>OEM: eject
OEM->>Redfish: POST/PATCH eject
Redfish-->>OEM: success
ServerCtrl->>BMC: MountVirtualMedia(systemURI, bootISOURL, slot)
BMC->>OEM: mount with Image URL
OEM->>Redfish: POST/PATCH insert
Redfish-->>OEM: success
ServerCtrl->>BMC: SetVirtualMediaBootOnce(systemURI)
BMC->>Redfish: set one-time CD/DVD boot
Redfish-->>BMC: success
BMC-->>ServerCtrl: boot configured
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bmc/bmc.go`:
- Around line 154-155: The method signature for GetVirtualMediaStatus references
redfish.VirtualMedia but the package import is missing; add an import for
"github.com/stmcginnis/gofish/redfish" to the imports in bmc.go so the type
redfish.VirtualMedia used by GetVirtualMediaStatus(ctx context.Context,
systemURI string) ([]*redfish.VirtualMedia, error) is resolved and the file
compiles.
In `@bmc/redfish_dell.go`:
- Around line 478-488: The MountVirtualMedia function (and the similar methods
around lines 515-525 and 546-556) currently ignores the systemURI parameter and
always uses systems[0]; update it to locate the System from
r.client.Service.Systems() that matches the provided systemURI (e.g., by
comparing system.ODataID or ID to the systemURI) and use that System to build
vmURI; if no matching system is found, return an explicit error mentioning the
requested systemURI. Ensure the same change is applied to the equivalent
Unmount/Eject virtual media functions so they act on the requested system rather
than the first discovered system.
In `@bmc/redfish_hpe.go`:
- Around line 177-188: The current MountVirtualMedia (and the sibling methods
EjectVirtualMedia and GetVirtualMediaStatus) always pick managers[0] and ignore
the systemURI param; change each method to locate the correct manager by
matching the requested systemURI against the manager's managed-systems link
(e.g., inspect manager.Links.ManagedSystems/Members' ODataIDs or any
Manager->Systems relation) and use that manager's ODataID when building vmURI;
if no manager references the given systemURI return a clear error, and if
multiple managers reference the same systemURI return an ambiguity error rather
than defaulting to managers[0]; update MountVirtualMedia, EjectVirtualMedia, and
GetVirtualMediaStatus accordingly.
In `@bmc/redfish_lenovo.go`:
- Around line 200-210: The methods MountVirtualMedia, EjectVirtualMedia, and
GetVirtualMediaStatus currently ignore the passed systemURI and always use
managers[0]; update each to locate the correct manager for the requested system
by iterating the managers slice returned from r.client.Service.Managers(),
finding the manager whose ManagerForServers/ManagerForServerCollection (or the
field that lists managed system ODataIDs) contains the provided systemURI, then
use that manager's ODataID to build vmURI (instead of managers[0].ODataID); if
no matching manager is found return a clear error. Ensure the lookup logic is
reused or extracted into a helper (e.g., findManagerForSystem(ctx, systemURI)
used by MountVirtualMedia, EjectVirtualMedia, GetVirtualMediaStatus) so all
three functions resolve managers correctly.
In `@bmc/redfish.go`:
- Around line 1077-1139: Change the receiver from *RedfishBMC to *RedfishBaseBMC
for SetVirtualMediaBootOnce, MountVirtualMedia, EjectVirtualMedia, and
GetVirtualMediaStatus; replace redfish.GetComputerSystem and redfish.*
types/constants with the schemas package symbols (e.g. schemas.Boot,
schemas.VirtualMedia and the OnceBootSourceOverrideEnabled /
UEFIBootSourceOverrideMode / CdBootSourceOverrideTarget constants from
github.com/stmcginnis/gofish/schemas); use the existing
r.getSystemFromUri(systemURI) to obtain the system instead of
redfish.GetComputerSystem; and make the SetBoot call consistent by passing a
pointer (system.SetBoot(&setBoot)). Ensure the schemas import is present and
referenced as schemas.
In `@internal/controller/server_controller.go`:
- Around line 936-944: The code silently falls back to the default slot mapping
when bmcClient.GetSystemInfo(ctx, systemURI) returns an error, which can cause
wrong mounts for HPE systems; change the logic in the function containing
bmcClient.GetSystemInfo so that on err != nil you do not continue with a default
mapping but instead return or propagate a descriptive error (including the
original err) to the caller; keep the HPE detection branch for when systemInfo
is present (checking systemInfo.Manufacturer == "HPE" || "HP"), and ensure
bootISOSlot and configISOSlot are only set after a successful GetSystemInfo or
when an explicit, safe fallback policy is chosen by the caller.
- Around line 433-447: The code assumes VirtualMedia.ID is a reusable slot
identifier and passes it directly from GetVirtualMediaStatus into
EjectVirtualMedia (in the loop using bmcClient.GetVirtualMediaStatus /
bmcClient.EjectVirtualMedia), which breaks for OEMs that return vendor-prefixed
IDs (e.g., Lenovo's "EXT1"); change the call flow to normalize or translate
returned media.ID before calling EjectVirtualMedia (or update each OEM bmcClient
implementation to accept and handle vendor-native IDs). Implement a small helper
(e.g., normalizeVirtualMediaID(id string) string) and use it where media.ID is
forwarded to EjectVirtualMedia in both cleanup loops so the vendor-specific
prefixes are stripped or mapped consistently before making the eject call.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 443b1c84-42ee-4902-b71d-c7f07487d6be
⛔ Files ignored due to path filters (3)
dist/chart/templates/crd/metal.ironcore.dev_serverbootconfigurations.yamlis excluded by!**/dist/**dist/chart/templates/crd/metal.ironcore.dev_serverclaims.yamlis excluded by!**/dist/**dist/chart/templates/crd/metal.ironcore.dev_servermaintenances.yamlis excluded by!**/dist/**
📒 Files selected for processing (16)
api/v1alpha1/serverbootconfiguration_types.goapi/v1alpha1/serverclaim_types.gobmc/bmc.gobmc/redfish.gobmc/redfish_dell.gobmc/redfish_hpe.gobmc/redfish_lenovo.goconfig/crd/bases/metal.ironcore.dev_serverbootconfigurations.yamlconfig/crd/bases/metal.ironcore.dev_serverclaims.yamlconfig/crd/bases/metal.ironcore.dev_servermaintenances.yamlconfig/samples/metal_v1alpha1_serverbootconfiguration_virtualmedia.yamlconfig/samples/metal_v1alpha1_serverclaim.yamlconfig/samples/metal_v1alpha1_serverclaim_virtualmedia.yamldocs/api-reference/api.mdinternal/controller/server_controller.gointernal/controller/serverclaim_controller.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
docs/api-reference/api.md (1)
1493-1494: Document the image format onServerClaimSpec.imagetoo.
ServerClaimis the user-facing input, andinternal/controller/serverclaim_controller.gocopies this field straight intoServerBootConfiguration.Spec.Image. Right now only the boot-config section explains thatVirtualMediaexpects a bootable ISO layer, so the claim docs still read like the old PXE-only contract.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/api-reference/api.md` around lines 1493 - 1494, Add the image format documentation to the ServerClaim user-facing docs: update the ServerClaimSpec.image field description to state that the value is the boot image identifier/url and when bootMethod is "VirtualMedia" it must be a bootable ISO layer (e.g., an ISO image accessible by the controller), matching how internal/controller/serverclaim_controller.go copies ServerClaim.Spec.Image into ServerBootConfiguration.Spec.Image; also mention any PXE/backwards-compat behavior (PXE expects network-bootable images and may ignore ISO images) so users know which format to supply for each BootMethod.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/controller/server_controller.go`:
- Around line 543-559: The current code calls bootServer whenever
server.Status.PowerState == ServerOffPowerState which stages virtual media on
every reconcile; change this to only prepare boot media when a power-on is
actually requested by gating the call: check the desired power
(server.Spec.Power or equivalent field) and only call r.bootServer(ctx,
bmcClient, server, config) when desired == ServerOnPowerState and current status
== ServerOffPowerState (i.e., a transition to power-on), or alternatively set
and persist a durable flag (e.g., server.Status.BootMediaStaged or an
annotation) after staging and skip staging if that flag is already true; update
the code paths that set/read that flag (and update status via r.Status().Update)
so requeues are idempotent and bootServer is not invoked on every reconcile.
- Around line 459-474: The virtual-media cleanup block currently runs only when
server.Spec.BMCRef != nil; update the guard to run for inline BMC as well by
checking if either server.Spec.BMCRef != nil || server.Spec.BMC != nil (and
ensure server.Spec.SystemURI is still non-empty), so the existing calls to
bmcClient.GetVirtualMediaStatus and bmcClient.EjectVirtualMedia (the loop that
inspects media.Inserted and uses media.ID) execute for inline BMC configurations
too; keep the same logging and error handling but broaden the if condition to
include server.Spec.BMC.
---
Nitpick comments:
In `@docs/api-reference/api.md`:
- Around line 1493-1494: Add the image format documentation to the ServerClaim
user-facing docs: update the ServerClaimSpec.image field description to state
that the value is the boot image identifier/url and when bootMethod is
"VirtualMedia" it must be a bootable ISO layer (e.g., an ISO image accessible by
the controller), matching how internal/controller/serverclaim_controller.go
copies ServerClaim.Spec.Image into ServerBootConfiguration.Spec.Image; also
mention any PXE/backwards-compat behavior (PXE expects network-bootable images
and may ignore ISO images) so users know which format to supply for each
BootMethod.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8c07c57b-1804-4d5d-8a18-a55484cd23d8
📒 Files selected for processing (5)
bmc/redfish.gobmc/redfish_hpe.godocs/api-reference/api.mdinternal/controller/server_controller.gointernal/controller/serverclaim_controller.go
🚧 Files skipped from review as they are similar to previous changes (1)
- bmc/redfish.go
Contributor
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)
internal/controller/server_controller.go (1)
547-550:⚠️ Potential issue | 🟠 MajorWait for
BootISOURLbefore treating a VirtualMedia config as ready.The Reserved-state gate still only waits for
Status.State == Ready, butvirtualMediaBootServer()now hard-fails whenconfig.Status.BootISOURLis empty. If the boot-config controller publishesReadybefore the ISO URL, this path flips from “waiting” into an error loop. Please fold the VirtualMedia prerequisites into the readiness check before callingbootServer.Also applies to: 570-571, 968-970
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controller/server_controller.go` around lines 547 - 550, The readiness check currently returns true when Status.State == Ready but virtualMediaBootServer/bootServer will fail if the associated VirtualMedia config lacks config.Status.BootISOURL; update r.serverBootConfigurationIsReady to also verify virtual media prerequisites (specifically that the VirtualMedia config exists and config.Status.BootISOURL is non-empty) and only return ready when both state and BootISOURL are present, then replace the existing pre-boot checks at the spots that call virtualMediaBootServer/bootServer (the same call sites referenced around virtualMediaBootServer usage) so they rely on the enhanced serverBootConfigurationIsReady result before invoking bootServer.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/controller/server_controller.go`:
- Around line 352-358: The read of ServerBootConfiguration via r.Get can return
a stale cached object after applyBootConfigurationAndIgnitionForDiscovery() just
created/patched it; instead reuse the ServerBootConfiguration object returned
from applyBootConfigurationAndIgnitionForDiscovery() (or perform the read with
r.Client or r.APIReader to bypass the cache) so you don't rely on the
controller-runtime cache snapshot; update the code around
applyBootConfigurationAndIgnitionForDiscovery(), remove the immediate r.Get
call, and use the returned config (or call r.APIReader.Get) when reading the
BootConfiguration fields (ServerBootConfiguration, BootMethod, etc.).
---
Outside diff comments:
In `@internal/controller/server_controller.go`:
- Around line 547-550: The readiness check currently returns true when
Status.State == Ready but virtualMediaBootServer/bootServer will fail if the
associated VirtualMedia config lacks config.Status.BootISOURL; update
r.serverBootConfigurationIsReady to also verify virtual media prerequisites
(specifically that the VirtualMedia config exists and config.Status.BootISOURL
is non-empty) and only return ready when both state and BootISOURL are present,
then replace the existing pre-boot checks at the spots that call
virtualMediaBootServer/bootServer (the same call sites referenced around
virtualMediaBootServer usage) so they rely on the enhanced
serverBootConfigurationIsReady result before invoking bootServer.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 49134d33-2e84-4a7d-8c4e-067de4e797ea
📒 Files selected for processing (4)
bmc/redfish_dell.gobmc/redfish_hpe.gobmc/redfish_lenovo.gointernal/controller/server_controller.go
🚧 Files skipped from review as they are similar to previous changes (2)
- bmc/redfish_hpe.go
- bmc/redfish_lenovo.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
internal/controller/server_controller.go (2)
926-930: Consider aligning log levels with the rest of the file.These log statements use
log.Info()while the rest of the file consistently useslog.V(1).Info()for similar informational messages. Whilelog.Info()is valid, aligning with the existing pattern would improve consistency.♻️ Optional: Align log levels with file conventions
switch config.Spec.BootMethod { case metalv1alpha1.BootMethodVirtualMedia: - log.Info("Booting server via virtual media") + log.V(1).Info("Booting server via virtual media") return r.virtualMediaBootServer(ctx, bmcClient, server, config) default: // BootMethodPXE or empty - log.Info("Booting server via PXE") + log.V(1).Info("Booting server via PXE") return r.pxeBootServer(ctx, bmcClient, server) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controller/server_controller.go` around lines 926 - 930, The two log statements use log.Info() but the file consistently uses log.V(1).Info() for similar messages; update the calls before invoking virtualMediaBootServer and pxeBootServer so they use log.V(1).Info("Booting server via virtual media") and log.V(1).Info("Booting server via PXE") respectively to match the existing logging verbosity pattern in this file (symbols: virtualMediaBootServer, pxeBootServer).
991-999: Eject failures are logged but don't fail the boot operation.If ejecting existing media fails (e.g., due to a transient BMC issue), the function continues and attempts to mount new media. This is a reasonable design choice since the mount operation will fail if ejection was truly necessary. However, consider whether a retry or more explicit handling would be beneficial for reliability.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controller/server_controller.go` around lines 991 - 999, The loop that calls bmcClient.EjectVirtualMedia for each media (using normalizeVirtualMediaSlotID to compute normalizedSlotID) currently logs failures via log.V(1).Info but continues; change this to attempt a small retry/backoff (e.g., 3 tries with incremental delay) inside the loop when bmcClient.EjectVirtualMedia(ctx, systemURI, normalizedSlotID) returns an error, and log each attempt including attempt number and error; if all retries fail, return or propagate a clear error from the enclosing function (instead of silently continuing) so the boot operation can fail fast or be handled upstream; ensure you update any callers to handle the propagated error if necessary.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/controller/server_controller.go`:
- Around line 1002-1006: Fix the typo in the comment describing Dell/Lenovo slot
semantics: change the line that reads "Dell/Lenovo: Slot 1 = CD, Slot 2 = CD" to
"Dell/Lenovo: Slot 1 = CD, Slot 2 = USB" so the comment matches the intended
mapping (bootISOSlot / configISOSlot) and the HPE note; update the surrounding
comment block near the bootISOSlot and configISOSlot variables accordingly.
---
Nitpick comments:
In `@internal/controller/server_controller.go`:
- Around line 926-930: The two log statements use log.Info() but the file
consistently uses log.V(1).Info() for similar messages; update the calls before
invoking virtualMediaBootServer and pxeBootServer so they use
log.V(1).Info("Booting server via virtual media") and log.V(1).Info("Booting
server via PXE") respectively to match the existing logging verbosity pattern in
this file (symbols: virtualMediaBootServer, pxeBootServer).
- Around line 991-999: The loop that calls bmcClient.EjectVirtualMedia for each
media (using normalizeVirtualMediaSlotID to compute normalizedSlotID) currently
logs failures via log.V(1).Info but continues; change this to attempt a small
retry/backoff (e.g., 3 tries with incremental delay) inside the loop when
bmcClient.EjectVirtualMedia(ctx, systemURI, normalizedSlotID) returns an error,
and log each attempt including attempt number and error; if all retries fail,
return or propagate a clear error from the enclosing function (instead of
silently continuing) so the boot operation can fail fast or be handled upstream;
ensure you update any callers to handle the propagated error if necessary.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d67c9d5f-abaf-43a2-8a1b-d5f15262e720
📒 Files selected for processing (1)
internal/controller/server_controller.go
Author
|
Closing this PR until there is a solution to extend the boot contract where I can hang the boot method selection for boot from VirtualMedia |
Member
|
/ref #807 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Proposed Changes
API Extension: Added BootMethod enum field (PXE/VirtualMedia) to ServerClaim and ServerBootConfiguration
OEM VirtualMedia Implementation: Implemented Redfish VirtualMedia operations (MountVirtualMedia, EjectVirtualMedia, GetVirtualMediaStatus, SetVirtualMediaBootOnce) in [oem] for Dell, HPE, and Lenovo using manufacturer-specific API endpoints
Manufacturer-Specific Slot Mapping: Added automatic manufacturer detection in server controller to handle different VirtualMedia slot semantics:
HPE/HP: Boot ISO → slot 2 (CD), Config ISO → slot 1 (USB)
Dell/Lenovo: Boot ISO → slot 1, Config ISO → slot 2
VirtualMedia Boot Orchestration: Implemented virtualMediaBootServer() controller function that ejects existing media, mounts both ISOs to appropriate slots, and sets boot override to VirtualMedia
Fixes #
Summary by CodeRabbit
New Features
Documentation