make updating the vmdk descriptor optional - #13773
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adjusts VMware volume attach behavior so that failures while verifying/updating a VMDK’s descriptor (adapter type) no longer prevent attachDisk from proceeding, addressing attach failures seen when vCenter datastore browser access returns HTTP 500 (issue #13249). It also adds a unit test to ensure attachDisk succeeds even when the adapter-type update step fails.
Changes:
- Make
updateVmdkAdapter(...)best-effort duringVirtualMachineMO.attachDisk(...)by catching and logging failures, then continuing the attach. - Add a unit test covering the “adapter update fails but attach continues” scenario.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| vmware-base/src/main/java/com/cloud/hypervisor/vmware/mo/VirtualMachineMO.java | Wraps updateVmdkAdapter in a try/catch so attach can proceed when descriptor update/verification fails. |
| vmware-base/src/test/java/com/cloud/hypervisor/vmware/mo/VirtualMachineMOTest.java | Adds a test validating attachDisk does not fail when updateVmdkAdapter throws. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## 4.20 #13773 +/- ##
=========================================
Coverage 16.26% 16.27%
- Complexity 13434 13439 +5
=========================================
Files 5667 5667
Lines 500731 500735 +4
Branches 60803 60803
=========================================
+ Hits 81455 81477 +22
+ Misses 410172 410135 -37
- Partials 9104 9123 +19
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (2)
vmware-base/src/main/java/com/cloud/hypervisor/vmware/mo/VirtualMachineMO.java:1474
- The PR title says making descriptor updates optional, but the code only treats
IOExceptionas non-fatal. IfupdateVmdkAdapter(...)can fail with non-IO exceptions (e.g., runtime parsing/format issues), disk attach would still fail. Either broaden the handled failure modes (if the intent is truly optional) or clarify scope in the PR description/title (e.g., 'optional on datastore browser I/O errors') so behavior matches the stated intent.
try {
updateVmdkAdapter(vmdkFileName, diskController);
} catch (IOException e) {
logger.warn("Unable to verify/update adapter type for VMDK file " + vmdkFileName + " due to a datastore browser I/O failure, proceeding with disk attach: " + e.getMessage(), e);
}
vmware-base/src/main/java/com/cloud/hypervisor/vmware/mo/VirtualMachineMO.java:1473
- This log line uses string concatenation and appends
e.getMessage()while also passingeas the throwable. Consider using parameterized logging (placeholders) and omite.getMessage()since the stack trace already contains it; this avoids unnecessary string building when warn logging is disabled and prevents duplicated error text.
logger.warn("Unable to verify/update adapter type for VMDK file " + vmdkFileName + " due to a datastore browser I/O failure, proceeding with disk attach: " + e.getMessage(), e);
|
@blueorangutan package |
|
@DaanHoogland a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 18747 |
Description
This PR...
Fixes: #13249
Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity
Screenshots (if appropriate):
How Has This Been Tested?
How did you try to break this feature and the system with this change?