OCPBUGS-90560: resolve boot disk image at reconcile time - #181
OCPBUGS-90560: resolve boot disk image at reconcile time#181nader-ziada wants to merge 3 commits into
Conversation
When a Machine's boot disk has no image specified, resolve it dynamically by reading the coreos-bootimages ConfigMap from the MCO namespace and parsing the stream metadata for the correct architecture-specific RHCOS image. Falls back to hardcoded defaults when the ConfigMap is unavailable. Architecture is determined via the GCP MachineTypes API, with prefix-based detection as a secondary fallback. Signed-off-by: Nader Ziada <nziada@redhat.com>
openshift-ci-robot
added
jira/severity-important
|
@nader-ziada: This pull request references Jira Issue OCPBUGS-90560, 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. |
|
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:
WalkthroughThe GCP machine actuator resolves empty boot disk images from machine architecture and CoreOS boot image metadata. Machine creation applies this resolution before UEFI checks. Tests cover stream selection, architecture mapping, fallbacks, and image formatting. ChangesGCP boot image resolution
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant MachineReconciler
participant GCPMachineTypes
participant OSImageStream
participant BootImagesConfigMap
MachineReconciler->>GCPMachineTypes: Resolve machine architecture
MachineReconciler->>OSImageStream: Read spec.defaultStream
OSImageStream-->>MachineReconciler: Return active stream
MachineReconciler->>BootImagesConfigMap: Read stream metadata
BootImagesConfigMap-->>MachineReconciler: Return architecture-specific GCP image
Suggested reviewers: 🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
hello @RadekManak, this pr is following your suggestion in https://reports.radekmanak.com/687c2c45bd0b495ab03b1ea295d6d276 another PR will follow for the RBAC in |
![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
🧹 Nitpick comments (1)
pkg/cloud/gcp/actuators/machine/reconciler_test.go (1)
391-424: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the exact create-path image source.
Both cases can pass if the resolver returns an arbitrary RHCOS image, so they do not protect the fallback-versus-ConfigMap behavior introduced here.
pkg/cloud/gcp/actuators/machine/reconciler_test.go#L391-L424: assertSourceImage == defaultGCPBootImageX86.pkg/cloud/gcp/actuators/machine/reconciler_test.go#L901-L942: injecttestBootImagesConfigMap()into the fake client and assert the exact stream-derived GCP image reference, not merely a string containingrhcos.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/cloud/gcp/actuators/machine/reconciler_test.go` around lines 391 - 424, Strengthen the create-path assertions in pkg/cloud/gcp/actuators/machine/reconciler_test.go:391-424 by requiring the boot disk SourceImage to equal defaultGCPBootImageX86. In pkg/cloud/gcp/actuators/machine/reconciler_test.go:901-942, inject testBootImagesConfigMap() into the fake client and assert SourceImage equals the exact stream-derived GCP image reference instead of only checking for “rhcos”.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/cloud/gcp/actuators/machine/boot_image.go`:
- Around line 52-81: The resolveImageFromConfigMap method must validate
archData.Images.Gcp.Project and Name before calling gcpImageReference; return
the existing missing-image error when either value is blank so fallback behavior
is preserved. Add a regression test covering blank Project and Name metadata.
---
Nitpick comments:
In `@pkg/cloud/gcp/actuators/machine/reconciler_test.go`:
- Around line 391-424: Strengthen the create-path assertions in
pkg/cloud/gcp/actuators/machine/reconciler_test.go:391-424 by requiring the boot
disk SourceImage to equal defaultGCPBootImageX86. In
pkg/cloud/gcp/actuators/machine/reconciler_test.go:901-942, inject
testBootImagesConfigMap() into the fake client and assert SourceImage equals the
exact stream-derived GCP image reference instead of only checking for “rhcos”.
🪄 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: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: d5492457-a96d-43a0-85a7-2c48dc9e34b5
⛔ Files ignored due to path filters (7)
go.sumis excluded by!**/*.sumvendor/github.com/coreos/stream-metadata-go/LICENSEis excluded by!**/vendor/**,!vendor/**vendor/github.com/coreos/stream-metadata-go/stream/artifact_utils.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/coreos/stream-metadata-go/stream/rhcos/rhcos.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/coreos/stream-metadata-go/stream/stream.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/coreos/stream-metadata-go/stream/stream_utils.gois excluded by!**/vendor/**,!vendor/**vendor/modules.txtis excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (5)
go.modpkg/cloud/gcp/actuators/machine/boot_image.gopkg/cloud/gcp/actuators/machine/boot_image_test.gopkg/cloud/gcp/actuators/machine/reconciler.gopkg/cloud/gcp/actuators/machine/reconciler_test.go
| return "", fmt.Errorf("failed to get coreos-bootimages ConfigMap: %w", err) | ||
| } | ||
|
|
||
| streamData, ok := cm.Data["stream"] |
There was a problem hiding this comment.
I think this generally looks fine, but the structure of the configmap has changed in 4.22/5.0 to account for rhel-9 and rhel-10 streams. There is a new key in this CM called streams which has both set of bootimages. The old stream key used here only references rhel-9 images and is planned to be deprecated at some point. You can also take a look at the structure of the configmap from a CI run(grep for the configmap).
I'm assuming this is not getting backported, so we should make sure that we are using correct bootimages from the new streams key and remove any dependance on the stream key. You can examine the OSImageStream object in the MCO namespace to determine what stream is in use by the customer(default is rhel-10 for 5.0 installs)
There was a problem hiding this comment.
@djoshy thanks for reviewing, made a change to use streams key from coreos-bootimages ConfigMap
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/cloud/gcp/actuators/machine/boot_image_test.go`:
- Around line 77-84: Handle fixture-construction errors in
pkg/cloud/gcp/actuators/machine/boot_image_test.go:77-84 by updating
testStreamsJSON to return or propagate the json.Marshal error instead of
discarding it, and in pkg/cloud/gcp/actuators/machine/boot_image_test.go:110-116
update testOSImageStream to check unstructured.SetNestedField, failing setup
with the original error when it fails.
In `@pkg/cloud/gcp/actuators/machine/boot_image.go`:
- Line 127: Update the warning in the OSImageStream fallback handling near the
machine boot-image lookup to avoid interpolating the raw Kubernetes client
error. Use a stable, non-sensitive failure reason or omit the error value while
retaining the fallback stream name and behavior.
🪄 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: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 21217808-a5b5-44f6-9d0d-4c2e5a400984
📒 Files selected for processing (3)
pkg/cloud/gcp/actuators/machine/boot_image.gopkg/cloud/gcp/actuators/machine/boot_image_test.gopkg/cloud/gcp/actuators/machine/reconciler.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/cloud/gcp/actuators/machine/reconciler.go
The coreos-bootimages ConfigMap now contains a streams key with per-stream boot image data (rhel-9, rhel-10) for OCP 5.0. Read the OSImageStream cluster singleton to determine the active stream and resolve the correct boot image from the streams key. Falls back to the deprecated stream key for backwards compatibility during upgrades. Signed-off-by: Nader Ziada <nziada@redhat.com>
c0ddd5c to
7d53743
Compare
|
/retest |
|
/assign @RadekManak |
|
/retest |
|
@damdo the regression job history looks awful. @nader-ziada if you have any bandwith can you dig into it? hopefully won't take too long 😓 |
|
@nader-ziada Additionally, (human review) |
|
/test regression-clusterinfra-gcp-ipi-mapi |
|
[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 |
|
@RadekManak can you please take a look
|
will look into it |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/manager/main.go`:
- Around line 140-142: The default namespace scope must remain limited to
watchNamespace; remove openshift-machine-config-operator from DefaultNamespaces
and configure the coreos-bootimages lookup in the boot image resolver with a
per-object cache scope or uncached API reader, without broadening Machine or
MachineSet informer watches.
🪄 Autofix
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: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: f921c037-4b0a-470c-add6-4c9fe604c132
📒 Files selected for processing (5)
cmd/manager/main.gopkg/cloud/gcp/actuators/machine/boot_image.gopkg/cloud/gcp/actuators/machine/reconciler.gopkg/cloud/gcp/actuators/util/gcp_machine_architecture.gopkg/cloud/gcp/actuators/util/gcp_machine_architecture_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/cloud/gcp/actuators/machine/reconciler.go
- pkg/cloud/gcp/actuators/machine/boot_image.go
96816bf to
5183295
Compare
- Add openshift-machine-config-operator to cache DefaultNamespaces so the cache-backed client can read the coreos-bootimages ConfigMap cross-namespace (P0 fix) - Upgrade fallback log lines from V(3) to Warningf for operator visibility when image resolution fails - Update fallback images from RHCOS 4.14 to 4.18 (418.94.202602022246-0) - Add a4x prefix to ARM64 machine type detection Signed-off-by: Nader Ziada <nziada@redhat.com>
5183295 to
efc3d2f
Compare
|
@nader-ziada: The following test 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
When a Machine's boot disk has no image specified, resolve it dynamically by reading the coreos-bootimages ConfigMap from the MCO namespace and parsing the stream metadata for the correct architecture-specific RHCOS image. Falls back to hardcoded defaults when the ConfigMap is unavailable.
Architecture is determined via the GCP MachineTypes API, with prefix-based detection as a secondary fallback.
Summary by CodeRabbit
New Features
Bug Fixes