OS Streams with image-service using rhcos version - #10742
Draft
giladravid16 wants to merge 1 commit into
Draft
Conversation
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
openshift-ci
Bot
added
size/XL
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: giladravid16 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 |
openshift-ci
Bot
added
the
approved
WalkthroughThe change adds OS stream selection to cluster and InfraEnv APIs, CRDs, models, image metadata, and installer configuration. Image lookup now supports explicit, default, and legacy streams. Controllers, inventory, boot artifacts, and admission validation propagate and validate the selected stream. ChangesOS stream contracts and schemas
Stream-aware image resolution
Resource propagation and artifact generation
InfraEnv admission validation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant InfraEnvController
participant Inventory
participant OSImages
participant BootArtifacts
Client->>InfraEnvController: submit InfraEnv with OSStream
InfraEnvController->>Inventory: resolve and persist OSStream
Inventory->>OSImages: request image by version, architecture, and stream
OSImages-->>Inventory: return selected OS image
Inventory->>BootArtifacts: generate URLs from image version
BootArtifacts-->>Client: return boot artifact data
🚥 Pre-merge checks | ✅ 12 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (12 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. 🔧 ast-grep (0.45.0)internal/bminventory/inventory_test.goast-grep timed out on this file 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: 3
🧹 Nitpick comments (1)
internal/imageservice/url_test.go (1)
176-194: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd test coverage for the error paths of
OsImageVersion.The new tests cover the "prefers
Version" and "falls back toOpenshiftVersion" cases.OsImageVersionalso returns an error for a nilosImageand for anosImagewith bothVersionandOpenshiftVersionempty. Add tests for these two error paths.♻️ Suggested additional test cases
It("errors when osImage is nil", func() { _, err := OsImageVersion(nil) Expect(err).Should(HaveOccurred()) }) It("errors when both Version and OpenshiftVersion are empty", func() { _, err := OsImageVersion(&models.OsImage{}) Expect(err).Should(HaveOccurred()) })🤖 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 `@internal/imageservice/url_test.go` around lines 176 - 194, Add two error-path tests to the OsImageVersion Describe block: verify OsImageVersion(nil) returns an error, and verify an empty models.OsImage with both Version and OpenshiftVersion unset also returns an error.
🤖 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 `@internal/bminventory/inventory.go`:
- Line 2676: Update the cluster update flow around validateUpdateCluster and
updateClusterData to validate ClusterUpdateParams.OsStream against an available
OS image before optionalParam persists it to os_stream. Reject invalid streams
and preserve the existing write path for valid values.
In `@internal/controller/controllers/infraenv_controller.go`:
- Around line 203-205: Update
internal/controller/controllers/infraenv_controller.go lines 203-205 in
updateInfraEnv to pass cluster to getOSStream, compare the resolved stream
including ClusterRef fallback with internalInfraEnv.OsStream, and send the
update whenever they differ, including an empty result. Update
internal/bminventory/inventory.go lines 5762-5765 so a non-nil
InfraEnvUpdateParams.OsStream is persisted even when it is empty.
In `@internal/versions/osimages.go`:
- Around line 128-132: Update the comparator in the patchMatches sorting logic
to handle nil version pointers and propagate or explicitly handle every error
returned by version.NewVersion. Avoid dereferencing malformed OpenshiftVersion
values or calling GreaterThan with nil; preserve deterministic sorting while
ensuring a single invalid catalog entry cannot panic the OS image lookup.
---
Nitpick comments:
In `@internal/imageservice/url_test.go`:
- Around line 176-194: Add two error-path tests to the OsImageVersion Describe
block: verify OsImageVersion(nil) returns an error, and verify an empty
models.OsImage with both Version and OpenshiftVersion unset also returns an
error.
🪄 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: 0cfd5ad8-8cbd-49fb-a727-a1be48e39674
⛔ Files ignored due to path filters (25)
api/v1beta1/zz_generated.deepcopy.gois excluded by!**/zz_generated*api/vendor/github.com/openshift/assisted-service/models/cluster.gois excluded by!**/vendor/**api/vendor/github.com/openshift/assisted-service/models/cluster_create_params.gois excluded by!**/vendor/**api/vendor/github.com/openshift/assisted-service/models/infra_env.gois excluded by!**/vendor/**api/vendor/github.com/openshift/assisted-service/models/infra_env_create_params.gois excluded by!**/vendor/**api/vendor/github.com/openshift/assisted-service/models/infra_env_update_params.gois excluded by!**/vendor/**api/vendor/github.com/openshift/assisted-service/models/os_image.gois excluded by!**/vendor/**api/vendor/github.com/openshift/assisted-service/models/v2_cluster_update_params.gois excluded by!**/vendor/**client/vendor/github.com/openshift/assisted-service/models/cluster.gois excluded by!**/vendor/**client/vendor/github.com/openshift/assisted-service/models/cluster_create_params.gois excluded by!**/vendor/**client/vendor/github.com/openshift/assisted-service/models/infra_env.gois excluded by!**/vendor/**client/vendor/github.com/openshift/assisted-service/models/infra_env_create_params.gois excluded by!**/vendor/**client/vendor/github.com/openshift/assisted-service/models/infra_env_update_params.gois excluded by!**/vendor/**client/vendor/github.com/openshift/assisted-service/models/os_image.gois excluded by!**/vendor/**client/vendor/github.com/openshift/assisted-service/models/v2_cluster_update_params.gois excluded by!**/vendor/**vendor/github.com/openshift/assisted-service/api/hiveextension/v1beta1/agentclusterinstall_types.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/assisted-service/api/v1beta1/agentserviceconfig_types.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/assisted-service/api/v1beta1/infraenv_types.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/assisted-service/models/cluster.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/assisted-service/models/cluster_create_params.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/assisted-service/models/infra_env.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/assisted-service/models/infra_env_create_params.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/assisted-service/models/infra_env_update_params.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/assisted-service/models/os_image.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/assisted-service/models/v2_cluster_update_params.gois excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (42)
api/hiveextension/v1beta1/agentclusterinstall_types.goapi/v1beta1/agentserviceconfig_types.goapi/v1beta1/infraenv_types.gocmd/agentbasedinstaller/register.goconfig/crd/bases/agent-install.openshift.io_agentserviceconfigs.yamlconfig/crd/bases/agent-install.openshift.io_hypershiftagentserviceconfigs.yamlconfig/crd/bases/agent-install.openshift.io_infraenvs.yamlconfig/crd/bases/extensions.hive.openshift.io_agentclusterinstalls.yamlconfig/crd/resources.yamldeploy/olm-catalog/manifests/agent-install.openshift.io_agentserviceconfigs.yamldeploy/olm-catalog/manifests/agent-install.openshift.io_hypershiftagentserviceconfigs.yamldeploy/olm-catalog/manifests/agent-install.openshift.io_infraenvs.yamldeploy/olm-catalog/manifests/extensions.hive.openshift.io_agentclusterinstalls.yamlinternal/bminventory/inventory.gointernal/bminventory/inventory_test.gointernal/bminventory/inventory_v2_handlers.gointernal/controller/controllers/agentserviceconfig_controller.gointernal/controller/controllers/clusterdeployments_controller.gointernal/controller/controllers/infraenv_controller.gointernal/controller/controllers/infraenv_controller_test.gointernal/host/hostcommands/download_boot_artifacts_cmd.gointernal/host/hostcommands/download_boot_artifacts_cmd_test.gointernal/host/hostcommands/instruction_manager_test.gointernal/imageservice/url_test.gointernal/imageservice/urls.gointernal/installcfg/builder/builder.gointernal/installcfg/builder/builder_test.gointernal/versions/api.gointernal/versions/mock_osimages.gointernal/versions/osimages.gointernal/versions/osimages_test.gomodels/cluster.gomodels/cluster_create_params.gomodels/infra_env.gomodels/infra_env_create_params.gomodels/infra_env_update_params.gomodels/os_image.gomodels/v2_cluster_update_params.gopkg/webhooks/agentinstall/v1beta1/infraenv_admission_hook.gopkg/webhooks/agentinstall/v1beta1/infraenv_admission_hook_test.gorestapi/embedded_spec.goswagger.yaml
| optionalParam(params.ClusterUpdateParams.NoProxy, "no_proxy", updates) | ||
| optionalParam(params.ClusterUpdateParams.SSHPublicKey, "ssh_public_key", updates) | ||
| optionalParam(params.ClusterUpdateParams.Hyperthreading, "hyperthreading", updates) | ||
| optionalParam(params.ClusterUpdateParams.OsStream, "os_stream", updates) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -A160 -B10 'func \(b \*bareMetalInventory\) validateUpdateCluster' internal/bminventory
rg -n -A6 -B6 'OsStream|GetOsImage' internal/bminventory/inventory_test.goRepository: openshift/assisted-service
Length of output: 43286
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- cluster update validation and OsStream references ---'
rg -n -A220 -B20 'func \(b \*bareMetalInventory\) validateAndUpdateClusterParams|OsStream|os_stream|GetOsImage' internal/bminventory/inventory.go internal/bminventory/*.go
printf '%s\n' '--- all repository references to cluster OS stream validation ---'
rg -n -i 'os.?stream|validate.*stream|stream.*image|GetOsImage' --glob '*.go' | head -n 300Repository: openshift/assisted-service
Length of output: 50383
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- relevant definitions in inventory.go ---'
rg -n 'func \(b \*bareMetalInventory\) validateAndUpdateClusterParams|func \(b \*bareMetalInventory\) updateClusterData|OsStream|os_stream|GetOsImage' internal/bminventory/inventory.go
printf '%s\n' '--- validation helper implementations ---'
rg -n -A120 -B10 'func \(b \*bareMetalInventory\) validateAndUpdateClusterParams' internal/bminventory/inventory.go
rg -n -A180 -B10 'func \(b \*bareMetalInventory\) updateClusterData' internal/bminventory/inventory.goRepository: openshift/assisted-service
Length of output: 17549
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- production OsStream references outside inventory.go ---'
rg -n -i 'os.?stream|stream.*os|os.*stream' --glob '*.go' --glob '!**/*_test.go' --glob '!internal/bminventory/inventory.go' | head -n 200
printf '%s\n' '--- update request model and generated validation references ---'
rg -n -i 'type .*ClusterUpdateParams|ClusterUpdateParams struct|OsStream' --glob '*.go' --glob '*.yaml' --glob '*.yml' --glob '*.json' | head -n 300
printf '%s\n' '--- nearby cluster registration and InfraEnv validation for comparison ---'
sed -n '580,620p' internal/bminventory/inventory.go
sed -n '5648,5682p' internal/bminventory/inventory.goRepository: openshift/assisted-service
Length of output: 375
Validate OsStream before persisting it. validateUpdateCluster does not validate OsStream, but updateClusterData writes it directly to os_stream. Reject streams that do not match an available OS image before this write.
🤖 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 `@internal/bminventory/inventory.go` at line 2676, Update the cluster update
flow around validateUpdateCluster and updateClusterData to validate
ClusterUpdateParams.OsStream against an available OS image before optionalParam
persists it to os_stream. Reject invalid streams and preserve the existing write
path for valid values.
Comment on lines
+203
to
+205
| if infraEnv.Spec.OSStream != "" && infraEnv.Spec.OSStream != internalInfraEnv.OsStream { | ||
| updateParams.InfraEnvUpdateParams.OsStream = &infraEnv.Spec.OSStream | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Propagate empty and inherited OS stream updates.
The controller and backend both treat an empty OS stream as a no-op. A standalone InfraEnv cannot revert from rhel-9 to the default stream. An InfraEnv with ClusterRef also remains pinned to its old stream when the referenced cluster stream changes.
internal/controller/controllers/infraenv_controller.go#L203-L205: resolve the effective stream withgetOSStream, including the ClusterRef fallback, and send an update whenever it differs frominternalInfraEnv.OsStream. PassclusterintoupdateInfraEnvto do this.internal/bminventory/inventory.go#L5762-L5765: whenparams.InfraEnvUpdateParams.OsStream != nil, persist its value even when the value is"".
📍 Affects 2 files
internal/controller/controllers/infraenv_controller.go#L203-L205(this comment)internal/bminventory/inventory.go#L5762-L5765
🤖 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 `@internal/controller/controllers/infraenv_controller.go` around lines 203 -
205, Update internal/controller/controllers/infraenv_controller.go lines 203-205
in updateInfraEnv to pass cluster to getOSStream, compare the resolved stream
including ClusterRef fallback with internalInfraEnv.OsStream, and send the
update whenever they differ, including an empty result. Update
internal/bminventory/inventory.go lines 5762-5765 so a non-nil
InfraEnvUpdateParams.OsStream is persisted even when it is empty.
Comment on lines
+128
to
+132
| sort.Slice(patchMatches, func(i, j int) bool { | ||
| v1, _ := version.NewVersion(*patchMatches[i].OpenshiftVersion) | ||
| v2, _ := version.NewVersion(*patchMatches[j].OpenshiftVersion) | ||
| return v1.GreaterThan(v2) | ||
| }) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Guard against nil *version.Version before calling GreaterThan.
version.NewVersion errors are discarded with _. If parsing fails for either patchMatches[i].OpenshiftVersion or patchMatches[j].OpenshiftVersion, v1 or v2 is nil, and v1.GreaterThan(v2) can panic inside sort.Slice. This crashes the OS image lookup on a single malformed catalog entry.
As per path instructions, Go files must "Never ignore error returns."
🐛 Proposed fix to handle parse errors safely
sort.Slice(patchMatches, func(i, j int) bool {
- v1, _ := version.NewVersion(*patchMatches[i].OpenshiftVersion)
- v2, _ := version.NewVersion(*patchMatches[j].OpenshiftVersion)
- return v1.GreaterThan(v2)
+ v1, err1 := version.NewVersion(*patchMatches[i].OpenshiftVersion)
+ v2, err2 := version.NewVersion(*patchMatches[j].OpenshiftVersion)
+ if err1 != nil || err2 != nil {
+ return false
+ }
+ return v1.GreaterThan(v2)
})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| sort.Slice(patchMatches, func(i, j int) bool { | |
| v1, _ := version.NewVersion(*patchMatches[i].OpenshiftVersion) | |
| v2, _ := version.NewVersion(*patchMatches[j].OpenshiftVersion) | |
| return v1.GreaterThan(v2) | |
| }) | |
| sort.Slice(patchMatches, func(i, j int) bool { | |
| v1, err1 := version.NewVersion(*patchMatches[i].OpenshiftVersion) | |
| v2, err2 := version.NewVersion(*patchMatches[j].OpenshiftVersion) | |
| if err1 != nil || err2 != nil { | |
| return false | |
| } | |
| return v1.GreaterThan(v2) | |
| }) |
🤖 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 `@internal/versions/osimages.go` around lines 128 - 132, Update the comparator
in the patchMatches sorting logic to handle nil version pointers and propagate
or explicitly handle every error returned by version.NewVersion. Avoid
dereferencing malformed OpenshiftVersion values or calling GreaterThan with nil;
preserve deterministic sorting while ensuring a single invalid catalog entry
cannot panic the OS image lookup.
Source: Path instructions
openshift-ci
Bot
added
the
needs-rebase
|
PR needs rebase. 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. |
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
1 participant
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.
List all the issues related to this PR
What environments does this code impact?
How was this code tested?
Checklist
docs, README, etc)Reviewers Checklist
Summary by CodeRabbit