Copilot commented on code in PR #13178:
URL: https://github.com/apache/cloudstack/pull/13178#discussion_r3257574063
##########
server/src/test/java/com/cloud/vm/UserVmManagerImplTest.java:
##########
@@ -3927,6 +3924,137 @@ public void createVirtualMachineWithExistingSnapshot()
throws ResourceUnavailabl
userVmManagerImpl.createVirtualMachine(deployVMCmd);
}
+ /**
+ * Bug fix: when deploying a VM from a snapshot whose source volume
resided on
+ * local (HOST-scoped) storage, the zone-wide pool check must be skipped.
+ * The source volume's pool type is irrelevant – only the snapshot matters.
+ */
+ @Test
+ public void
createVirtualMachineWithSnapshotFromExpungedLocalStorageVolumeSucceeds()
+ throws ResourceUnavailableException,
InsufficientCapacityException, ResourceAllocationException {
Review Comment:
The test name/Javadoc says it covers a snapshot whose *source volume is on
HOST-scoped (local) storage*, but the setup never stubs
volumeInfo.getDataStore()/scope to return HOST. As written, this test
effectively exercises the same “null data store” path (Mockito default) as the
next test, so it doesn’t validate the intended regression. Please explicitly
mock the data store scope to HOST here (or rename/remove the test to avoid
misleading/duplicated coverage).
##########
server/src/test/java/com/cloud/vm/UserVmManagerImplTest.java:
##########
@@ -3927,6 +3924,137 @@ public void createVirtualMachineWithExistingSnapshot()
throws ResourceUnavailabl
userVmManagerImpl.createVirtualMachine(deployVMCmd);
}
+ /**
+ * Bug fix: when deploying a VM from a snapshot whose source volume
resided on
+ * local (HOST-scoped) storage, the zone-wide pool check must be skipped.
+ * The source volume's pool type is irrelevant – only the snapshot matters.
+ */
+ @Test
+ public void
createVirtualMachineWithSnapshotFromExpungedLocalStorageVolumeSucceeds()
+ throws ResourceUnavailableException,
InsufficientCapacityException, ResourceAllocationException {
+ DeployVMCmd deployVMCmd = new DeployVMCmd();
+ ReflectionTestUtils.setField(deployVMCmd, "zoneId", zoneId);
+ ReflectionTestUtils.setField(deployVMCmd, "serviceOfferingId",
serviceOfferingId);
+ ReflectionTestUtils.setField(deployVMCmd, "snapshotId", snashotId);
+ deployVMCmd._accountService = accountService;
+
+ when(accountService.finalyzeAccountId(nullable(String.class),
nullable(Long.class), nullable(Long.class), eq(true))).thenReturn(accountId);
+
when(accountService.getActiveAccountById(accountId)).thenReturn(account);
+ when(entityManager.findById(DataCenter.class,
zoneId)).thenReturn(_dcMock);
+ when(entityManager.findById(ServiceOffering.class,
serviceOfferingId)).thenReturn(serviceOffering);
+ when(snapshotDaoMock.findById(snashotId)).thenReturn(snapshotMock);
+ when(snapshotMock.getVolumeId()).thenReturn(volumeId);
+ when(volumeDataFactory.getVolume(volumeId)).thenReturn(volumeInfo);
+ when(volumeInfo.getTemplateId()).thenReturn(templateId);
+ when(volumeInfo.getInstanceId()).thenReturn(null);
+
when(entityManager.findByIdIncludingRemoved(VirtualMachineTemplate.class,
templateId)).thenReturn(templateMock);
+
when(templateMock.getTemplateType()).thenReturn(Storage.TemplateType.VNF);
+
when(templateMock.getHypervisorType()).thenReturn(Hypervisor.HypervisorType.KVM);
+ when(templateMock.isDeployAsIs()).thenReturn(false);
+ when(templateMock.getFormat()).thenReturn(Storage.ImageFormat.QCOW2);
+ when(templateMock.getUserDataId()).thenReturn(null);
+
Mockito.doNothing().when(vnfTemplateManager).validateVnfApplianceNics(any(),
nullable(List.class), any());
+ when(_dcMock.isLocalStorageEnabled()).thenReturn(true);
+
when(_dcMock.getNetworkType()).thenReturn(DataCenter.NetworkType.Basic);
+
Mockito.doReturn(userVmVoMock).when(userVmManagerImpl).createBasicSecurityGroupVirtualMachine(any(),
any(), any(), any(), any(), any(), any(),
+ any(), any(), any(), any(), any(), any(), any(), any(), any(),
any(), any(), any(), nullable(Boolean.class), any(), any(), any(),
+ any(), any(), any(), any(), eq(true), any(), any(), any());
+
+ // Must NOT throw "Deployment of virtual machine is supported only for
Zone-wide storage pools"
+ userVmManagerImpl.createVirtualMachine(deployVMCmd);
+ }
+
+ /**
+ * Bug fix: when deploying a VM from a snapshot whose source volume's
storage pool
+ * was expunged (data store is null), the zone-wide pool check must be
skipped.
+ */
+ @Test
+ public void
createVirtualMachineWithSnapshotFromVolumeWithNullDataStoreSucceeds()
+ throws ResourceUnavailableException,
InsufficientCapacityException, ResourceAllocationException {
+ DeployVMCmd deployVMCmd = new DeployVMCmd();
Review Comment:
These two snapshot-success tests are nearly identical; with the current
stubbing they both pass with a null data store and don’t differentiate
local(HOST)-scoped vs expunged/null-datastore scenarios. Consider consolidating
them or making one explicitly HOST-scoped and the other explicitly null
datastore to keep the intent and coverage clear.
##########
server/src/main/java/com/cloud/vm/UserVmManagerImpl.java:
##########
@@ -6413,7 +6413,13 @@ public UserVm createVirtualMachine(DeployVMCmd cmd)
throws InsufficientCapacityE
}
_accountMgr.checkAccess(caller, null, true, snapshot);
VolumeInfo volumeOfSnapshot = getVolume(snapshot.getVolumeId(),
templateId, true);
- templateId = volumeOfSnapshot.getTemplateId();
+ if (volumeOfSnapshot != null) {
+ templateId = volumeOfSnapshot.getTemplateId();
+ } else if (templateId == null) {
+ throw new InvalidParameterValueException(
+ "Could not determine template from snapshot id=" +
cmd.getSnapshotId() +
+ "; the source volume no longer exists. Please
specify a templateId.");
Review Comment:
The error message asks users to specify "templateId", but the public API
parameter is named "templateid" (ApiConstants.TEMPLATE_ID). To reduce confusion
for API users/UI, consider referencing the actual parameter name (e.g.,
"templateid") or wording it as "template id".
--
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]
