OCPBUGS-92792: vsphere - fix CNS volume destroy to aggregate errors#10658
OCPBUGS-92792: vsphere - fix CNS volume destroy to aggregate errors#10658jcpowermac wants to merge 1 commit into
Conversation
openshift-ci-robot
added
jira/severity-important
|
@jcpowermac: This pull request references Jira Issue OCPBUGS-92792, 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. |
jcpowermac
changed the title
|
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)
📝 WalkthroughWalkthroughCNS volume lookup now keeps successful matches while aggregating per-volume query errors. Deletion continues across all vCenter clients, aggregates failures, and is covered for deletion failures and partial query results. ChangesvSphere CNS volume deletion resilience
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ 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 Comment |
|
/jira refresh |
openshift-ci-robot
added
the
jira/valid-bug
|
@jcpowermac: This pull request references Jira Issue OCPBUGS-92792, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
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. |
openshift-ci-robot
removed
the
jira/invalid-bug
|
@jcpowermac: This pull request references Jira Issue OCPBUGS-92792, 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.
🧹 Nitpick comments (1)
pkg/destroy/vsphere/vsphere_test.go (1)
727-812: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueLGTM! The three cases map cleanly onto the new behaviors: continue-after-delete-failure, all-clients-processed-after-first-fails, and partial-results-still-reported. Mock expectations (
Times) precisely pin the continuation semantics.Optional: the new cases assert only
assert.Error, whereas the existing ones useassert.Regexpto pin the source message. Matching the failing volume's error text would tighten regression detection, but it's not blocking.🤖 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/destroy/vsphere/vsphere_test.go` around lines 727 - 812, The new deleteCnsVolumes test cases only assert that an error occurred, but they should also verify the propagated error text like the existing assertions do. Update the three t.Run blocks in deleteCnsVolumes tests to assert on the specific failing message/source using assert.Regexp or an equivalent message match, so regressions in error reporting from deleteCnsVolumes/GetCnsVolumes/DeleteCnsVolumes are caught. Use the existing test names and mock expectations in deleteCnsVolumes as the anchor points.
🤖 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.
Nitpick comments:
In `@pkg/destroy/vsphere/vsphere_test.go`:
- Around line 727-812: The new deleteCnsVolumes test cases only assert that an
error occurred, but they should also verify the propagated error text like the
existing assertions do. Update the three t.Run blocks in deleteCnsVolumes tests
to assert on the specific failing message/source using assert.Regexp or an
equivalent message match, so regressions in error reporting from
deleteCnsVolumes/GetCnsVolumes/DeleteCnsVolumes are caught. Use the existing
test names and mock expectations in deleteCnsVolumes as the anchor points.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: a9f450eb-b6c0-44ac-beb5-a30f7833fb1e
📒 Files selected for processing (3)
pkg/destroy/vsphere/client.gopkg/destroy/vsphere/vsphere.gopkg/destroy/vsphere/vsphere_test.go
|
/assign @AnnaZivkovic @vr4manta |
deleteCnsVolumes returned on the first error, which silently skipped remaining volumes on that vCenter and all volumes on subsequent vCenters. GetCnsVolumes had the same early-return on per-volume query failures, discarding already-queried results. Align both functions with the error aggregation pattern used by deleteVirtualMachines: collect errors, attempt all items, and return utilerrors.NewAggregate. Also log deleted/found counts at Info level so partial cleanup is visible without --log-level=debug. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
jcpowermac
force-pushed
the
OCPBUGS-92792/vsphere-cns-destroy-error-handling
branch
from
8e38d64 to
0223f4f
Compare
|
/cherry-pick release-4.22 |
|
@jcpowermac: once the present PR merges, I will cherry-pick it on top of 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 kubernetes-sigs/prow repository. |
|
/lgtm |
|
/verify by @jcpowermac via ci and unit tests |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: vr4manta 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
|
/verified by @jcpowermac via ci and unit tests |
openshift-ci-robot
added
the
verified
|
@jcpowermac: 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. |
|
@jcpowermac: 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. |
5 participants
deleteCnsVolumes returned on the first error, which silently skipped remaining volumes on that vCenter and all volumes on subsequent vCenters. GetCnsVolumes had the same early-return on per-volume query failures, discarding already-queried results.
Align both functions with the error aggregation pattern used by deleteVirtualMachines: collect errors, attempt all items, and return utilerrors.NewAggregate. Also log deleted/found counts at Info level so partial cleanup is visible without --log-level=debug.
Summary by CodeRabbit