OCPBUGS-90560: Resolve GCP boot image from worker machines#1512
OCPBUGS-90560: Resolve GCP boot image from worker machines#1512nader-ziada wants to merge 2 commits into
Conversation
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. |
|
[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 |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: ⛔ Files ignored due to path filters (8)
📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (3)
WalkthroughAdds shared webhook defaulter configuration containing platform status, cluster ID, and boot image stream data. Machine and MachineSet GCP defaulting selects architecture-specific images from that stream and falls back to the existing static image. Startup wiring and tests are updated for configuration injection. ChangesGCP boot image configuration and Machine defaulting
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant WebhookStartup
participant ResolveDefaulterConfig
participant BootImagesConfigMap
participant MachineDefaulter
participant MachineSetDefaulter
WebhookStartup->>ResolveDefaulterConfig: resolve shared defaulter configuration
ResolveDefaulterConfig->>BootImagesConfigMap: load coreos-bootimages stream data
BootImagesConfigMap-->>ResolveDefaulterConfig: return stream payload
ResolveDefaulterConfig-->>WebhookStartup: return DefaulterConfig
WebhookStartup->>MachineDefaulter: construct with DefaulterConfig
WebhookStartup->>MachineSetDefaulter: construct with DefaulterConfig
Suggested reviewers: 🚥 Pre-merge checks | ✅ 13 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (13 passed)
✨ 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.12.2)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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/webhooks/machine_webhook_test.go`:
- Line 6805: The TestResolveGCPBootImage test cases have hardcoded expected and
mismatch image values that are specific to x86-64 architecture, but the
resolveGCPBootImage function filters results by the arch parameter. Make the
test architecture-aware by parameterizing the expected image assertions based on
the architecture being tested (such as by deriving expected images from the arch
field in each test case) so that assertions pass for both x86-64 and ARM64
architectures. This should be applied to all the test cases mentioned at the
specified line ranges.
- Line 6827: In the machine_webhook_test.go file, the json.Marshal calls at
lines 6827, 6857, 6887, 6911, and 6941 are currently ignoring errors by using
blank identifier (_). Instead of discarding the error, capture it in a variable
and check for non-nil errors. When json.Marshal fails, the test should fail
immediately using the testing.T helper (commonly using t.Fatalf or t.Errorf) to
prevent invalid ProviderSpec payloads from being silently used in the test
fixtures. This ensures test reliability and follows the project guideline of
never ignoring error returns in Go files.
In `@pkg/webhooks/machine_webhook.go`:
- Around line 354-366: The function resolveGCPBootImage currently treats all
List API failures as "no image found" by returning an empty string, which masks
RBAC and API outages. Modify resolveGCPBootImage to return both a string and an
error instead of just a string. When the c.List() call fails in the error
handling block, return the actual error along with an empty string rather than
silently returning an empty string. Reserve the empty string return for
successful list operations that find no matching worker machines. Additionally,
replace context.Background() with a context that supports cancellation and
timeouts, such as one derived from a timeout or cancellation mechanism
appropriate to your codebase standards.
- Around line 378-379: The loop iterating over providerSpec.Disks dereferences
each disk element without checking if it is nil, which will cause a panic if the
slice contains nil elements. Add a nil guard check inside the for loop that
iterates over providerSpec.Disks to ensure each disk is not nil before accessing
its Boot and Image fields.
- Around line 378-380: In the disk iteration loop where you check disk.Boot and
disk.Image, replace the strings.Contains call with strings.HasSuffix to ensure
the architecture suffix matches exactly at the end of the image path rather than
anywhere within it. This prevents accidentally selecting the wrong image variant
if the suffix text appears elsewhere in the image path.
🪄 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: 4245399b-3c94-4d86-a8ab-a45017c1c659
📒 Files selected for processing (5)
cmd/machineset/main.gopkg/webhooks/machine_webhook.gopkg/webhooks/machine_webhook_test.gopkg/webhooks/machineset_webhook.gopkg/webhooks/machineset_webhook_test.go
Instead of hardcoding a stale RHCOS 4.14 boot disk image for GCP defaults, resolve the image dynamically from existing worker machines at webhook startup. Falls back to the hardcoded default when no workers are found. Filters by architecture (x86-64/aarch64) to avoid selecting the wrong image on mixed-arch clusters. Introduces ResolveDefaulterConfig() so both Machine and MachineSet defaulters share a single client and boot image resolution call.
fef4618 to
8389bf4
Compare
|
/retest |
|
/pipeline auto |
|
/test ? |
|
/test e2e-gcp-ovn |
|
/retest |
|
@theobarberbany all the tests passed, is there other specific tests to kick off? |
RadekManak
left a comment
RadekManak
left a comment
There was a problem hiding this comment.
This is quite an old code. And there has been work in this area that we could leverage. (4.19+ GA). I think we can reuse the coreos-bootimages configmap for the images used in defaulting. This is already used to update MachineSet Images on AWS and GCP. Other platforms I am unsure of the status but generic solution where we can add platforms later on would be great.
---AI
The MCO's golden ConfigMap coreos-bootimages in openshift-machine-config-operator is the canonical source of truth for boot images. Key properties:
- Updated by the CVO during upgrades, so it always matches the release payload
- Multi-platform: Contains images for GCP, AWS, Azure, vSphere — organized by architecture and platform
- Structured data: The stream key contains JSON structured as .architectures..images..
For GCP specifically, the MCO BIC already reads it like this:go
newBootImage := fmt.Sprintf("projects/%s/global/images/%s",
streamData.Architectures[arch].Images.Gcp.Project,
streamData.Architectures[arch].Images.Gcp.Name)
If the webhook defaulter read from this ConfigMap instead of using a hardcoded value, you would:
- Eliminate the staleness problem entirely — the default always matches the current release
- Get a solution that generalizes to all platforms (AWS AMIs per region, Azure URNs, etc.)
- Align the webhook with the same source of truth the MCO already uses
- Avoid the indirection of "look at what workers happen to have" which the current PR does
--AI
|
|
||
| func resolveGCPBootImage(c client.Client) string { | ||
| archSuffix := "x86-64" | ||
| if arch == ARM64 { |
There was a problem hiding this comment.
arch is the webhook pod arch not the machine you are defaulting.
| archSuffix = "aarch64" | ||
| } | ||
|
|
||
| machineList := &machinev1beta1.MachineList{} |
There was a problem hiding this comment.
MachineSets images are being updated by MCO's Boot Image Controller. So they are better source if we stick to this approach.
|
|
||
| machineList := &machinev1beta1.MachineList{} | ||
| if err := c.List(context.Background(), machineList, | ||
| client.InNamespace(defaultSecretNamespace), |
There was a problem hiding this comment.
defaultSecretNamespace?
| dnsDisconnected bool | ||
| client client.Client | ||
| featureGates featuregate.MutableFeatureGate | ||
| gcpBootImage string |
There was a problem hiding this comment.
Having one platform image here is a code smell. This is generic struct. With platform field added on onto it which is useless on all platforms but GCP. Either make it generic or another solution entirely.
|
@nader-ziada: This pull request references Jira Issue OCPBUGS-90560, which is invalid:
Comment 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. |
|
hello @RadekManak, tried to follow a similar approach to MCO. would appreciate another review |
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 `@go.mod`:
- Line 119: Remove the stale // indirect marker from the
github.com/coreos/stream-metadata-go dependency in go.mod, making it a direct
dependency while preserving its existing version.
🪄 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: 1d96524d-ecda-4d6f-89f1-23970646cc54
⛔ Files ignored due to path filters (8)
go.sumis excluded by!**/*.sumvendor/github.com/coreos/stream-metadata-go/LICENSEis excluded by!**/vendor/**,!vendor/**vendor/github.com/coreos/stream-metadata-go/arch/arch.gois 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/webhooks/machine_webhook.gopkg/webhooks/machine_webhook_test.gopkg/webhooks/machineset_webhook.gopkg/webhooks/machineset_webhook_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/webhooks/machineset_webhook_test.go
Replace worker-machine-based boot image resolution with reading the coreos-bootimages ConfigMap from openshift-machine-config-operator, the canonical source updated by CVO on upgrades. - Store *stream.Stream in admissionConfig instead of platform-specific gcpBootImage string, making the approach generic across platforms - Detect target machine architecture via annotation with pod arch fallback, matching MCO's Boot Image Controller pattern - Fall back to hardcoded defaults if ConfigMap is unavailable Signed-off-by: Nader Ziada <nziada@redhat.com>
41d5f0b to
d6c686f
Compare
|
@nader-ziada: 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. |
4 participants
Instead of hardcoding a stale RHCOS 4.14 boot disk image for GCP defaults, resolve the image dynamically from existing worker machines at webhook startup. Falls back to the hardcoded default when no workers are found. Filters by architecture (x86-64/aarch64) to avoid selecting the wrong image on mixed-arch clusters.
Introduces ResolveDefaulterConfig() so both Machine and MachineSet defaulters share a single client and boot image resolution call.
Summary by CodeRabbit