MULTIARCH-6014: PowerVS: Use PowerVS Stock Catalogue Images#10647
MULTIARCH-6014: PowerVS: Use PowerVS Stock Catalogue Images#10647AshwinHIBM wants to merge 1 commit into
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@AshwinHIBM: This pull request references MULTIARCH-6014 which is a valid jira issue. 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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughAdds ChangesPowerVS Boot Image Resolution
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error)
✅ Passed checks (14 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 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 |
|
[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 |
![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
🤖 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/asset/machines/powervs/images.go`:
- Around line 18-51: The GetBootImageFromWorkspace function currently hard-fails
when errors occur during PowerVS client creation (after NewClient call), PowerVS
session creation (after NewIBMPISession call), or image retrieval (after GetAll
call). Instead of returning errors at these three failure points, treat the
image lookup as best-effort and gracefully fall back to returning the default
fallback value of rhcos-{clusterID} whenever any of these operations fail. This
ensures that missing or invalid serviceInstanceGUID values and other lookup
failures do not cause the function to fail, allowing the system to continue with
the fallback boot image instead.
- Around line 15-72: The GetBootImageFromWorkspace function has multiple
decision branches that are not covered by unit tests, which violates the coding
guidelines requiring tests for all pkg/ code. Create a new test file
(images_test.go) in the same directory and write comprehensive unit tests that
cover all branches of the GetBootImageFromWorkspace function: successful
retrieval of an active image, fallback when no images exist, fallback when
images exist but none are active, error handling when the image client creation
fails, and error handling when GetAll fails to retrieve images. Use mocking or
test doubles for the PowerVS client and image client dependencies to isolate the
function logic from external dependencies.
- Around line 61-65: The boot image selection loop starting at line 61 with the
range over images.Images returns the first active image with a non-null name
without verifying it is a RHCOS image. This is too broad and can result in
incompatible images being selected in workspaces with multiple active images.
Add additional filtering logic to the condition checking image.State and
image.Name to also validate that the image is specifically a RHCOS image, either
by checking the image name contains expected RHCOS naming patterns or by
validating image metadata. Only return the image name when all conditions are
satisfied: active state, non-empty name, and confirmed RHCOS identity.
🪄 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 YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 27447613-bc0e-4e59-a97d-10b991f12419
📒 Files selected for processing (3)
pkg/asset/machines/powervs/images.gopkg/asset/machines/powervs/machinesets.gopkg/asset/machines/powervs/powervsmachines.go
| for _, image := range images.Images { | ||
| if image.State != nil && *image.State == "active" && image.Name != nil { | ||
| logrus.Infof("Selected PowerVS boot image from workspace: %s", *image.Name) | ||
| return *image.Name, nil | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Selection predicate is too broad for boot image resolution.
At Line 61-64, the code returns the first active image regardless of OS/image intent. In workspaces with multiple active images, this can pick a non-RHCOS image and break node boot compatibility. Filter explicitly for the expected RHCOS image naming/metadata and require non-empty names.
🤖 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/asset/machines/powervs/images.go` around lines 61 - 65, The boot image
selection loop starting at line 61 with the range over images.Images returns the
first active image with a non-null name without verifying it is a RHCOS image.
This is too broad and can result in incompatible images being selected in
workspaces with multiple active images. Add additional filtering logic to the
condition checking image.State and image.Name to also validate that the image is
specifically a RHCOS image, either by checking the image name contains expected
RHCOS naming patterns or by validating image metadata. Only return the image
name when all conditions are satisfied: active state, non-empty name, and
confirmed RHCOS identity.
AshwinHIBM
force-pushed
the
workspace-image
branch
from
b144e1d to
915b6da
Compare
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/asset/machines/powervs/powervsmachines.go`:
- Around line 42-51: The error handling for the GetBootImageFromWorkspace call
in the powervsmachines.go file diverges from the identical call in
machinesets.go. Currently, when GetBootImageFromWorkspace returns an error
(auth/network/session failures), the code falls back to a default image name
pattern and logs a warning. Instead, align this with the machinesets.go approach
by propagating the error up the call stack using a formatted error (e.g.,
fmt.Errorf wrapping the original error). Remove the error handling block that
catches the error and falls back to the rhcos-clusterID pattern, allowing real
lookup failures to be treated as fatal rather than masked by a potentially
non-existent default image name.
- Around line 123-127: The powervsmachines.go file lacks unit test coverage
entirely, violating repository requirements that all code under pkg/ must have
tests. Create a new test file powervsmachines_test.go that includes test cases
covering both the happy path for the main functionality and the critical
fallback branch (lines 47-51) that handles errors when GetBootImageFromWorkspace
fails. Ensure the tests exercise all code paths including the error case where
the bootstrap image lookup returns an error, and verify that the fallback
behavior correctly uses the default image value when the primary lookup fails.
🪄 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 YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 745168e0-bda5-4415-a219-effe8d30890e
📒 Files selected for processing (3)
pkg/asset/machines/powervs/images.gopkg/asset/machines/powervs/machinesets.gopkg/asset/machines/powervs/powervsmachines.go
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/asset/machines/powervs/machinesets.go
- pkg/asset/machines/powervs/images.go
AshwinHIBM
force-pushed
the
workspace-image
branch
from
915b6da to
3883583
Compare
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/asset/machines/powervs/machinesets.go`:
- Around line 32-37: The GetBootImageFromWorkspace function returns the first
active image without verifying it is RHCOS-specific, which can cause wrong image
selection in workspaces with multiple active images. After the
GetBootImageFromWorkspace call succeeds, add validation to check that the
returned image is RHCOS-specific (such as verifying it matches the expected
RHCOS naming pattern or contains RHCOS identifiers). If the returned image does
not match RHCOS criteria, treat it as a failure and fall back to the default
image naming pattern, similar to the existing error handling. This ensures only
appropriate RHCOS images are selected before passing the image variable to the
provider function call.
- Line 32: The GetBootImageFromWorkspace function call uses context.TODO() which
leaves the IBM PowerVS API call unbounded, potentially causing manifest
generation to hang indefinitely if the API stalls. Replace context.TODO() with
context.WithTimeout() to establish a deadline for the API call, and ensure you
defer cancel() to clean up the context. Apply this same fix to both the
GetBootImageFromWorkspace call in machinesets.go at line 32 and any similar
calls in powervsmachines.go.
🪄 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 YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 7c907315-4a63-4fee-b4d0-d8ce03d85ecd
📒 Files selected for processing (3)
pkg/asset/machines/powervs/images.gopkg/asset/machines/powervs/machinesets.gopkg/asset/machines/powervs/powervsmachines.go
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/asset/machines/powervs/images.go
- pkg/asset/machines/powervs/powervsmachines.go
| image, err := GetBootImageFromWorkspace(context.TODO(), config.PowerVS.ServiceInstanceGUID, config.PowerVS.Zone, clusterID) | ||
| if err != nil { | ||
| // Fallback to default image naming pattern | ||
| image = fmt.Sprintf("rhcos-%s", clusterID) | ||
| logrus.Warnf("Failed to get boot image from PowerVS workspace, using default: %s (error: %v)", image, err) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Constrain workspace image selection before using it for MachineSets.
This path now trusts GetBootImageFromWorkspace(...), but that helper currently returns the first active image, not a verified RHCOS candidate. In a workspace with multiple active images, this can select the wrong boot image and break provisioning. Please enforce RHCOS-specific selection (or explicit reject/fallback) before passing image into provider(...).
🤖 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/asset/machines/powervs/machinesets.go` around lines 32 - 37, The
GetBootImageFromWorkspace function returns the first active image without
verifying it is RHCOS-specific, which can cause wrong image selection in
workspaces with multiple active images. After the GetBootImageFromWorkspace call
succeeds, add validation to check that the returned image is RHCOS-specific
(such as verifying it matches the expected RHCOS naming pattern or contains
RHCOS identifiers). If the returned image does not match RHCOS criteria, treat
it as a failure and fall back to the default image naming pattern, similar to
the existing error handling. This ensures only appropriate RHCOS images are
selected before passing the image variable to the provider function call.
3883583 to
b05cb3c
Compare
Signed-off-by: Ashwin Hendre <112116232+AshwinHIBM@users.noreply.github.com>
AshwinHIBM
force-pushed
the
workspace-image
branch
from
b05cb3c to
a6ab913
Compare
|
@AshwinHIBM: 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. |
2 participants
If a stock catalogue OS image is available in the Workspace, use it. If not, fall back to importing the image from public COS.
Why this change:
Importing images from COS is time consuming and introduces a failure mode that can be avoided by using an existing stock catalogue image.
Summary by CodeRabbit
rhcos-<clusterID>default and logs a warning.