OCPBUGS-105426: OCPBUGS-105430: OCPBUGS-105431: OCPBUGS-105315: Fix duplicate template error and e2es - #6387
OCPBUGS-105426: OCPBUGS-105430: OCPBUGS-105431: OCPBUGS-105315: Fix duplicate template error and e2es#6387djoshy wants to merge 5 commits into
Conversation
openshift-ci-robot
added
jira/severity-moderate
|
@djoshy: This pull request references Jira Issue OCPBUGS-105426, 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. |
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
WalkthroughThe change scopes vSphere template lookup to workspace folders, validates ignition stubs before vSphere mutations, matches failure domains using datastore and resource-pool paths, and uses unique templates during backdated boot-image tests. ChangesvSphere workspace and boot-image handling
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 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)
Comment |
openshift-ci
Bot
added
the
approved
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@djoshy: This pull request references Jira Issue OCPBUGS-105426, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
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. |
|
@djoshy: This pull request references Jira Issue OCPBUGS-105426, which is valid. 3 validation(s) were run on this bug
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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
pkg/controller/bootimage/vsphere_helpers_test.go (1)
115-158: 🗄️ Data Integrity & Integration | 🔵 Trivial | 🏗️ Heavy liftAdd a regression test for the full resolver path.
These tests validate string joining only. They can pass while
resolveExistingTemplateVMstill uses a bare computed-name lookup or accepts an outside match. Add a test with the same template name in the workspace and a sibling folder. Assert that the workspace VM is selected and the sibling VM remains untouched.🤖 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/controller/bootimage/vsphere_helpers_test.go` around lines 115 - 158, Extend TestTemplateSearchPath with a regression test covering the full resolveExistingTemplateVM flow, creating same-named templates in the workspace and sibling folders. Assert that resolution selects the workspace VM and leaves the sibling VM untouched, rather than validating only templateSearchPath string joining.
🤖 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/controller/bootimage/vsphere_helpers.go`:
- Around line 338-344: Restrict managed VM lookups in computedName, rollback,
and findAllRequiredResources to providerSpec.Workspace.Folder; only use global
fallback to classify an external same-name VM. Before destroyVMIfPresent removes
mco-tmp-* or mco-old-* VMs, verify the VM belongs to the workspace folder.
Validate the rollback VM’s workspace ownership before renaming it to
computedName.
- Around line 340-344: Update the template lookup flow around
finder.VirtualMachine and isInFolder to fail closed when folderErr is non-nil.
Ensure a failed workspace-folder lookup cannot leave workspaceFolder nil and
allow arbitrary global VM matches to be classified as the MCO template; only
accept matches when the folder lookup succeeded and the VM is confirmed within
the configured folder.
- Around line 234-242: Update templateSearchPath to reject relative template
names containing parent-directory segments before calling path.Join, preventing
names such as ../customer-template from escaping the workspace; preserve
unscoped handling for empty folders and absolute inventory paths, and add a
regression test covering the rejected traversal case.
In `@test/extended-priv/machineset.go`:
- Line 770: Update the error return in the MachineSet failure-domain lookup to
avoid including raw vSphere inventory values such as wsServer, wsDataCenter,
wsDatastore, and wsResourcePool. Return a generic message identifying the lookup
failure and MachineSet via ms.GetName(), and move any detailed values to an
approved controlled diagnostic path if needed.
---
Nitpick comments:
In `@pkg/controller/bootimage/vsphere_helpers_test.go`:
- Around line 115-158: Extend TestTemplateSearchPath with a regression test
covering the full resolveExistingTemplateVM flow, creating same-named templates
in the workspace and sibling folders. Assert that resolution selects the
workspace VM and leaves the sibling VM untouched, rather than validating only
templateSearchPath string joining.
🪄 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: cdfd66b7-b829-4807-b640-36e5caae53b7
📒 Files selected for processing (4)
pkg/controller/bootimage/vsphere_helpers.gopkg/controller/bootimage/vsphere_helpers_test.gotest/extended-priv/machineset.gotest/extended-priv/mco_bootimages.go
|
/test e2e-vsphere-mco-bootimages e2e-vsphere-multi-vcenter-mco-bootimages |
1 similar comment
|
/test e2e-vsphere-mco-bootimages e2e-vsphere-multi-vcenter-mco-bootimages |
6e97e28 to
ec08842
Compare
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_bootimages.go`:
- Line 28: Update the backdatedImageRunID initialization and its other
occurrences to retain at least 64 bits of entropy, either by extending the
UUID-derived substring or using the full UUID, while preserving the existing
identifier format and usage.
🪄 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: fe17236d-e5dd-44f5-9041-606132e96515
📒 Files selected for processing (1)
test/extended-priv/mco_bootimages.go
|
/test e2e-vsphere-mco-bootimages e2e-vsphere-multi-vcenter-mco-bootimages |
|
@djoshy: 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. |
|
Last set of runs look good, I'm going to roll in the secret degrade fix from #6382 so it is easier to backport. |
|
I suspect the vsphere presubmits will fail due to lack of leases, so I asked claude to run the full set of previously-failing vSphere bootimage tests(from all 4 bugs) against a live cluster to validate this PR:
/verified by manual test executions |
openshift-ci-robot
added
the
verified
|
@djoshy: This PR has been marked as verified by 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. |
|
@djoshy: This pull request references Jira Issue OCPBUGS-105426, which is valid. 3 validation(s) were run on this bug
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. |
djoshy
changed the title
|
/test e2e-vsphere-mco-bootimages e2e-vsphere-multi-vcenter-mco-bootimages |
|
Scheduling tests matching the |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: djoshy, isabella-janssen 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 |
|
This e2e-vsphere-mco-bootimages run managed to get a lease and passed 🥳 Unfortunately, the others variants didn't get a lease and timed out. Rerunning them below, however I don't think we should block on them for merging this fix. /test e2e-vsphere-multi-vcenter-mco-bootimages e2e-vsphere-zones-mco-bootimages |
4 participants
What I did
How to verify
The newly added vSphere optional pre-submits should now begin to pass. Unfortunately, due to lack of lease space, these are timing out currently. I've manually verified the test cases against a vsphere cluster with this fix in #6387 (comment).
Summary by CodeRabbit