GutoVeronezi commented on code in PR #6581:
URL: https://github.com/apache/cloudstack/pull/6581#discussion_r966215397
##########
server/src/test/java/com/cloud/storage/VolumeApiServiceImplTest.java:
##########
@@ -1106,6 +1108,65 @@ public void
doesTargetStorageSupportDiskOfferingTestDiskOfferingTagsEqualsStorag
Assert.assertTrue(result);
}
+ @Test
+ public void
validateIfVMHaveBackupsTestExceptionWhenTryToDetachVolumeFromVMWhichBackupOffering()
{
Review Comment:
```suggestion
public void
validateIfVmHaveBackupsTestExceptionWhenTryToDetachVolumeFromVMWhichBackupOffering()
{
```
##########
server/src/test/java/com/cloud/storage/VolumeApiServiceImplTest.java:
##########
@@ -1106,6 +1108,65 @@ public void
doesTargetStorageSupportDiskOfferingTestDiskOfferingTagsEqualsStorag
Assert.assertTrue(result);
}
+ @Test
+ public void
validateIfVMHaveBackupsTestExceptionWhenTryToDetachVolumeFromVMWhichBackupOffering()
{
+ try {
+ UserVmVO vm = Mockito.mock(UserVmVO.class);
+ when(vm.getBackupOfferingId()).thenReturn(1l);
+ volumeApiServiceImpl.validateIfVmHasBackups(vm, false);
+ } catch (Exception e) {
+ Assert.assertEquals("Unable to detach volume, cannot detach volume
from a VM that has backups. First remove the VM from the backup offering or set
the global configuration 'backup.enable.attach.detach.of.volumes' to true.",
e.getMessage());
+ }
+ }
+
+ @Test
+ public void
validateIfVMHaveBackupsTestExceptionWhenTryToAttachVolumeFromVMWhichBackupOffering()
{
Review Comment:
```suggestion
public void
validateIfVmHaveBackupsTestExceptionWhenTryToAttachVolumeFromVMWhichBackupOffering()
{
```
##########
server/src/test/java/com/cloud/storage/VolumeApiServiceImplTest.java:
##########
@@ -1106,6 +1108,65 @@ public void
doesTargetStorageSupportDiskOfferingTestDiskOfferingTagsEqualsStorag
Assert.assertTrue(result);
}
+ @Test
+ public void
validateIfVMHaveBackupsTestExceptionWhenTryToDetachVolumeFromVMWhichBackupOffering()
{
+ try {
+ UserVmVO vm = Mockito.mock(UserVmVO.class);
+ when(vm.getBackupOfferingId()).thenReturn(1l);
+ volumeApiServiceImpl.validateIfVmHasBackups(vm, false);
+ } catch (Exception e) {
+ Assert.assertEquals("Unable to detach volume, cannot detach volume
from a VM that has backups. First remove the VM from the backup offering or set
the global configuration 'backup.enable.attach.detach.of.volumes' to true.",
e.getMessage());
+ }
+ }
+
+ @Test
+ public void
validateIfVMHaveBackupsTestExceptionWhenTryToAttachVolumeFromVMWhichBackupOffering()
{
+ try {
+ UserVmVO vm = Mockito.mock(UserVmVO.class);
+ when(vm.getBackupOfferingId()).thenReturn(1l);
+ volumeApiServiceImpl.validateIfVmHasBackups(vm, true);
+ } catch (Exception e) {
+ Assert.assertEquals("Unable to attach volume, please specify a VM
that does not have any backups or set the global configuration
'backup.enable.attach.detach.of.volumes' to true.", e.getMessage());
+ }
+ }
+
+ @Test
+ public void
validateIfVMHaveBackupsTestSucessWhenVMDontHaveBackupOffering() {
Review Comment:
```suggestion
public void
validateIfVmHaveBackupsTestSucessWhenVMDontHaveBackupOffering() {
```
##########
server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java:
##########
@@ -2397,6 +2398,35 @@ private void checkDeviceId(Long deviceId, VolumeInfo
volumeToAttach, UserVmVO vm
return volumeToAttach;
}
+ protected void validateIfVmHasBackups(UserVmVO vm, boolean attach) {
+ if ((vm.getBackupOfferingId() == null ||
CollectionUtils.isEmpty(vm.getBackupVolumeList())) ||
BooleanUtils.isTrue(BackupManager.BackupEnableAttachDetachVolumes.value())) {
+ return;
+ }
+ String errorMsg = "Unable to detach volume, cannot detach volume from
a VM that has backups. First remove the VM from the backup offering or "
+ + "set the global configuration
'backup.enable.attach.detach.of.volumes' to true.";
+ if (attach)
+ errorMsg = "Unable to attach volume, please specify a VM that does
not have any backups or set the global configuration "
+ + "'backup.enable.attach.detach.of.volumes' to true.";
Review Comment:
We can use `BackupManager.BackupEnableAttachDetachVolumes.key()` instead of
writing the config name again.
##########
server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java:
##########
@@ -2397,6 +2398,35 @@ private void checkDeviceId(Long deviceId, VolumeInfo
volumeToAttach, UserVmVO vm
return volumeToAttach;
}
+ protected void validateIfVmHasBackups(UserVmVO vm, boolean attach) {
+ if ((vm.getBackupOfferingId() == null ||
CollectionUtils.isEmpty(vm.getBackupVolumeList())) ||
BooleanUtils.isTrue(BackupManager.BackupEnableAttachDetachVolumes.value())) {
+ return;
+ }
+ String errorMsg = "Unable to detach volume, cannot detach volume from
a VM that has backups. First remove the VM from the backup offering or "
+ + "set the global configuration
'backup.enable.attach.detach.of.volumes' to true.";
+ if (attach)
+ errorMsg = "Unable to attach volume, please specify a VM that does
not have any backups or set the global configuration "
+ + "'backup.enable.attach.detach.of.volumes' to true.";
Review Comment:
```suggestion
if (attach) {
errorMsg = "Unable to attach volume, please specify a VM that
does not have any backups or set the global configuration "
+ "'backup.enable.attach.detach.of.volumes' to true.";
}
```
##########
plugins/hypervisors/vmware/src/main/java/com/cloud/hypervisor/guru/VMwareGuru.java:
##########
@@ -919,11 +942,25 @@ private Map<VirtualDisk, VolumeVO> getDisksMapping(Backup
backup, List<VirtualDi
if (vm == null) {
throw new CloudRuntimeException("Failed to find the volumes
details from the VM backup");
}
+
List<Backup.VolumeInfo> backedUpVolumes = vm.getBackupVolumeList();
Map<String, Boolean> usedVols = new HashMap<>();
Map<VirtualDisk, VolumeVO> map = new HashMap<>();
for (Backup.VolumeInfo backedUpVol : backedUpVolumes) {
+ VolumeVO volumeExtra =
_volumeDao.findByUuid(backedUpVol.getUuid());
+ if (volumeExtra != null) {
+ s_logger.debug(String.format("Marking volume [id: %s] of vm
[id: %s, name: %s] as removed for the backup process.", backedUpVol.getUuid(),
vm.getUuid(), vm.getInstanceName()));
Review Comment:
We could use `ReflectionToStringBuilderUtils` here or the object's
`toString()`.
##########
plugins/hypervisors/vmware/src/main/java/com/cloud/hypervisor/guru/VMwareGuru.java:
##########
@@ -736,12 +740,14 @@ protected VolumeVO updateVolume(VirtualDisk disk,
Map<VirtualDisk, VolumeVO> dis
volume.setPath(volumeName);
volume.setPoolId(poolId);
VirtualMachineDiskInfo diskInfo = getDiskInfo(vmToImport, poolId,
volumeName);
- volume.setChainInfo(new Gson().toJson(diskInfo));
+ volume.setChainInfo(GSON.toJson(diskInfo));
volume.setInstanceId(vm.getId());
volume.setState(Volume.State.Ready);
volume.setAttached(new Date());
_volumeDao.update(volume.getId(), volume);
if (volume.getRemoved() != null) {
+ s_logger.debug(String.format("Marking volume [uuid: %s] of
restored VM [id: %s, name: %s] as non removed.", volume.getUuid(),
+ vm.getUuid(), vm.getInstanceName()));
Review Comment:
We could use `ReflectionToStringBuilderUtils` here or the object's
`toString()`.
--
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]
