Copilot commented on code in PR #13773:
URL: https://github.com/apache/cloudstack/pull/13773#discussion_r3701544720
##########
vmware-base/src/test/java/com/cloud/hypervisor/vmware/mo/VirtualMachineMOTest.java:
##########
@@ -135,4 +144,26 @@ public void
testGetVmxFormattedVirtualHardwareVersionTwoDigits() {
public void testGetVmxFormattedVirtualHardwareVersionInvalid() {
VirtualMachineMO.getVmxFormattedVirtualHardwareVersion(-1);
}
+
+ @Test
+ public void testAttachDiskSucceedsWhenAdapterTypeUpdateFails() throws
Exception {
+ VirtualMachineMO spyVmMo = spy(vmMo);
+ ManagedObjectReference morDs = mock(ManagedObjectReference.class);
+ ManagedObjectReference morTask = mock(ManagedObjectReference.class);
+ VimPortType service = mock(VimPortType.class);
+
+ doReturn(1).when(spyVmMo).getScsiDiskControllerKey(anyString());
+ doReturn(200).when(spyVmMo).getIDEDeviceControllerKey();
+ doReturn(0).when(spyVmMo).getNextDeviceNumber(anyInt());
+ doThrow(new Exception("HTTP 500 from vCenter datastore
browser")).when(spyVmMo).updateVmdkAdapter(anyString(), anyString());
+
+ when(mor.getValue()).thenReturn("vm-1");
+ when(context.getService()).thenReturn(service);
+ when(service.reconfigVMTask(eq(mor), any())).thenReturn(morTask);
+ when(client.waitForTask(morTask)).thenReturn(true);
+
+ spyVmMo.attachDisk(new String[]{"[ds] i-2-3-VM/data.vmdk"}, morDs,
"pvscsi", null, null);
+
+ verify(spyVmMo).updateVmdkAdapter(eq("[ds] i-2-3-VM/data.vmdk"),
eq("pvscsi"));
Review Comment:
This test currently only verifies that `updateVmdkAdapter` was invoked; it
doesn't assert that the disk attach path actually proceeded to the vSphere
reconfigure task. Adding verifications for `reconfigVMTask` (and optionally
`waitForTask`) makes the test better match the stated intent (“attachDisk
succeeds”).
##########
vmware-base/src/main/java/com/cloud/hypervisor/vmware/mo/VirtualMachineMO.java:
##########
@@ -1466,7 +1466,11 @@ public void attachDisk(String[] vmdkDatastorePathChain,
ManagedObjectReference m
VirtualDevice newDisk = VmwareHelper.prepareDiskDevice(this, null,
controllerKey, vmdkDatastorePathChain, morDs, unitNumber, 1, maxIops);
if (StringUtils.isNotBlank(diskController)) {
String vmdkFileName = vmdkDatastorePathChain[0];
- updateVmdkAdapter(vmdkFileName, diskController);
+ try {
+ updateVmdkAdapter(vmdkFileName, diskController);
+ } catch (Exception e) {
+ logger.warn("Unable to verify/update adapter type for VMDK
file " + vmdkFileName + ", proceeding with disk attach: " + e.getMessage(), e);
+ }
Review Comment:
The new catch-all `catch (Exception e)` makes adapter verification/update
effectively best-effort for *all* failures. This also swallows non-recoverable
problems like an invalid/unsupported adapter type (which `updateVmdkAdapter`
currently throws for), potentially letting the attach proceed in a broken state
and only fail later in a less clear way. Consider only ignoring the
datastore-browser read failures (e.g., `IOException`) and rethrowing other
exceptions.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
