Adapt vspehere bootimage tests to multi vcenter - #6317
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: sergiordlr 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
WalkthroughvSphere test helpers now resolve connection details from MachineSet-specific failure domains and workspace folders. Boot-image upload helpers and their call sites pass MachineSet context, while control-plane tests use nil where appropriate. ChangesMachineSet-aware vSphere uploads
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant getBackdatedBootImage
participant uploadBaseImageToCloud
participant GetVSphereConnectionInfoForMachineSet
participant UploadBaseImageToVsphere
getBackdatedBootImage->>uploadBaseImageToCloud: pass MachineSet and image data
uploadBaseImageToCloud->>GetVSphereConnectionInfoForMachineSet: resolve vSphere connection
GetVSphereConnectionInfoForMachineSet-->>uploadBaseImageToCloud: return connection info
uploadBaseImageToCloud->>UploadBaseImageToVsphere: pass connection info and folder
UploadBaseImageToVsphere-->>getBackdatedBootImage: upload result
Suggested reviewers: 🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✨ 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}
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 `@test/extended-priv/mco_scale.go`:
- Around line 691-698: Update the uploadBaseImageToCloud flow around
GetVSphereConnectionInfoForMachineSet to handle a nil ms before dereferencing
it: use the default GetVSphereConnectionInfo when ms is nil, and retain the
existing MachineSet-specific lookup and workspace folder extraction for non-nil
ms values.
🪄 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: d265aed6-2975-4ffd-aa5f-c1d0bcb64594
📒 Files selected for processing (6)
test/extended-priv/machineset.gotest/extended-priv/mco_bootimages.gotest/extended-priv/mco_bootimages_skew.gotest/extended-priv/mco_controlplanemachineset.gotest/extended-priv/mco_scale.gotest/extended-priv/util/vsphere.go
| vsInfo, err := GetVSphereConnectionInfoForMachineSet(ms) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| err = exutil.UploadBaseImageToVsphere(baseImageURL, baseImage, vsInfo) | ||
| folder, _ := ms.Get(`{.spec.template.spec.providerSpec.value.workspace.folder}`) | ||
|
|
||
| err = exutil.UploadBaseImageToVsphere(baseImageURL, baseImage, vsInfo, folder) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Prevent nil-pointer panic when ms is nil.
Currently, uploadBaseImageToCloud is called with ms = nil from the ControlPlaneMachineSet test suite. While VspherePlatform is skipped in that suite right now, this will crash with a nil-pointer dereference inside GetVSphereConnectionInfoForMachineSet if vSphere ever becomes a supported platform for CPMS updates.
Adding a nil guard and falling back to the default GetVSphereConnectionInfo ensures the helper remains robust against changes in upstream test definitions.
🛠️ Proposed fix to handle nil MachineSet
- vsInfo, err := GetVSphereConnectionInfoForMachineSet(ms)
- if err != nil {
- return err
- }
-
- folder, _ := ms.Get(`{.spec.template.spec.providerSpec.value.workspace.folder}`)
+ var vsInfo *exutil.VSphereConnectionInfo
+ var err error
+ var folder string
+
+ if ms != nil {
+ vsInfo, err = GetVSphereConnectionInfoForMachineSet(ms)
+ folder, _ = ms.Get(`{.spec.template.spec.providerSpec.value.workspace.folder}`)
+ } else {
+ vsInfo, err = exutil.GetVSphereConnectionInfo(oc.AsAdmin())
+ }
+
+ if err != nil {
+ return err
+ }📝 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.
| vsInfo, err := GetVSphereConnectionInfoForMachineSet(ms) | |
| if err != nil { | |
| return err | |
| } | |
| err = exutil.UploadBaseImageToVsphere(baseImageURL, baseImage, vsInfo) | |
| folder, _ := ms.Get(`{.spec.template.spec.providerSpec.value.workspace.folder}`) | |
| err = exutil.UploadBaseImageToVsphere(baseImageURL, baseImage, vsInfo, folder) | |
| var vsInfo *exutil.VSphereConnectionInfo | |
| var err error | |
| var folder string | |
| if ms != nil { | |
| vsInfo, err = GetVSphereConnectionInfoForMachineSet(ms) | |
| folder, _ = ms.Get(`{.spec.template.spec.providerSpec.value.workspace.folder}`) | |
| } else { | |
| vsInfo, err = exutil.GetVSphereConnectionInfo(oc.AsAdmin()) | |
| } | |
| if err != nil { | |
| return err | |
| } | |
| err = exutil.UploadBaseImageToVsphere(baseImageURL, baseImage, vsInfo, folder) |
🤖 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 `@test/extended-priv/mco_scale.go` around lines 691 - 698, Update the
uploadBaseImageToCloud flow around GetVSphereConnectionInfoForMachineSet to
handle a nil ms before dereferencing it: use the default
GetVSphereConnectionInfo when ms is nil, and retain the existing
MachineSet-specific lookup and workspace folder extraction for non-nil ms
values.
1 participant
- What I did
WIP
- How to verify it
Summary by CodeRabbit
New Features
Bug Fixes