From 756a7e89cbcec7a1aa98b3f470cbdf8181af75ad Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rafael=20Weing=C3=A4rtner?= Date: Sat, 21 Jul 2018 07:01:24 -0300 Subject: [PATCH] Fix limitation on tag matching in 'migrateVolume' with disk offering replacement (#2636) * Fix limitation on tag matching in 'migrateVolume' with disk offering replacement When the feature to enable disk offering replacement during volume migration was created, we were forcing the tags of the new disk offering to exact the same as the tags of the target storage poll. However, that is not how ACS manages volumes allocation. This change modifies this validation to make it consistent with volume allocation. * Address Nitin's suggestions * Apply Daan's suggestion regarding "doesTargetStorageSupportDiskOffering" method * fix problem --- .../com/cloud/storage/VolumeApiService.java | 35 ++++++- .../cloud/storage/VolumeApiServiceImpl.java | 66 +++++++++++-- .../storage/VolumeApiServiceImplTest.java | 92 ++++++++++++++++++- 3 files changed, 184 insertions(+), 9 deletions(-) diff --git a/api/src/main/java/com/cloud/storage/VolumeApiService.java b/api/src/main/java/com/cloud/storage/VolumeApiService.java index cf13cd671a9..ec20c33e17e 100644 --- a/api/src/main/java/com/cloud/storage/VolumeApiService.java +++ b/api/src/main/java/com/cloud/storage/VolumeApiService.java @@ -102,4 +102,37 @@ public interface VolumeApiService { void updateDisplay(Volume volume, Boolean displayVolume); Snapshot allocSnapshotForVm(Long vmId, Long volumeId, String snapshotName) throws ResourceAllocationException; -} + + /** + * Checks if the target storage supports the disk offering. + * This validation is consistent with the mechanism used to select a storage pool to deploy a volume when a virtual machine is deployed or when a data disk is allocated. + * + * The scenarios when this method returns true or false is presented in the following table. + * + * + * + * + * + * + * + * + * + * + * + * + * + * + * + * + * + * + * + * + * + * + * + * + *
#Disk offering tagsStorage tagsDoes the storage support the disk offering?
1A,BANO
2A,B,CA,B,C,D,XYES
3A,B,CX,Y,ZNO
4nullA,S,DYES
5AnullNO
6nullnullYES
+ */ + boolean doesTargetStorageSupportDiskOffering(StoragePool destPool, String diskOfferingTags); +} \ No newline at end of file diff --git a/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java b/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java index 9bff4a17295..3fcf7618ade 100644 --- a/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java +++ b/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java @@ -19,6 +19,7 @@ package com.cloud.storage; import java.net.MalformedURLException; import java.net.URL; import java.util.ArrayList; +import java.util.Arrays; import java.util.Date; import java.util.HashMap; import java.util.List; @@ -2161,9 +2162,9 @@ public class VolumeApiServiceImpl extends ManagerBase implements VolumeApiServic * Performs the validations required for replacing the disk offering while migrating the volume of storage. If no new disk offering is provided, we do not execute any validation. * If a disk offering is informed, we then proceed with the following checks. * * * If all of the above validations pass, we check if the size of the new disk offering is different from the volume. If it is, we log a warning message. @@ -2175,10 +2176,9 @@ public class VolumeApiServiceImpl extends ManagerBase implements VolumeApiServic if ((destPool.isShared() && newDiskOffering.getUseLocalStorage()) || destPool.isLocal() && newDiskOffering.isShared()) { throw new InvalidParameterValueException("You cannot move the volume to a shared storage and assing a disk offering for local storage and vice versa."); } - String storageTags = getStoragePoolTags(destPool); - if (!StringUtils.areTagsEqual(storageTags, newDiskOffering.getTags())) { - throw new InvalidParameterValueException(String.format("Target Storage [id=%s] tags [%s] does not match new disk offering [id=%s] tags [%s].", destPool.getUuid(), storageTags, - newDiskOffering.getUuid(), newDiskOffering.getTags())); + if (!doesTargetStorageSupportNewDiskOffering(destPool, newDiskOffering)) { + throw new InvalidParameterValueException(String.format("Target Storage [id=%s] tags [%s] does not match new disk offering [id=%s] tags [%s].", destPool.getUuid(), + getStoragePoolTags(destPool), newDiskOffering.getUuid(), newDiskOffering.getTags())); } if (volume.getSize() != newDiskOffering.getDiskSize()) { DiskOfferingVO oldDiskOffering = this._diskOfferingDao.findById(volume.getDiskOfferingId()); @@ -2189,6 +2189,58 @@ public class VolumeApiServiceImpl extends ManagerBase implements VolumeApiServic s_logger.info(String.format("Changing disk offering to [uuid=%s] while migrating volume [uuid=%s, name=%s].", newDiskOffering.getUuid(), volume.getUuid(), volume.getName())); } + /** + * Checks if the target storage supports the new disk offering. + * This validation is consistent with the mechanism used to select a storage pool to deploy a volume when a virtual machine is deployed or when a new data disk is allocated. + * + * The scenarios when this method returns true or false is presented in the following table. + * + * + * + * + * + * + * + * + * + * + * + * + * + * + * + * + * + * + * + * + * + * + * + * + * + *
#Disk offering tagsStorage tagsDoes the storage support the disk offering?
1A,BANO
2A,B,CA,B,C,D,XYES
3A,B,CX,Y,ZNO
4nullA,S,DYES
5AnullNO
6nullnullYES
+ */ + protected boolean doesTargetStorageSupportNewDiskOffering(StoragePool destPool, DiskOfferingVO newDiskOffering) { + String newDiskOfferingTags = newDiskOffering.getTags(); + return doesTargetStorageSupportDiskOffering(destPool, newDiskOfferingTags); + } + + @Override + public boolean doesTargetStorageSupportDiskOffering(StoragePool destPool, String diskOfferingTags) { + if (org.apache.commons.lang.StringUtils.isBlank(diskOfferingTags)) { + return true; + } + String storagePoolTags = getStoragePoolTags(destPool); + if (org.apache.commons.lang.StringUtils.isBlank(storagePoolTags)) { + return false; + } + String[] storageTagsAsStringArray = org.apache.commons.lang.StringUtils.split(storagePoolTags, ","); + String[] newDiskOfferingTagsAsStringArray = org.apache.commons.lang.StringUtils.split(diskOfferingTags, ","); + + return CollectionUtils.isSubCollection(Arrays.asList(newDiskOfferingTagsAsStringArray), Arrays.asList(storageTagsAsStringArray)); + } + /** * Retrieves the storage pool tags as a {@link String}. If the storage pool does not have tags we return a null value. */ diff --git a/server/src/test/java/com/cloud/storage/VolumeApiServiceImplTest.java b/server/src/test/java/com/cloud/storage/VolumeApiServiceImplTest.java index 8ae122d4c46..890b1eab526 100644 --- a/server/src/test/java/com/cloud/storage/VolumeApiServiceImplTest.java +++ b/server/src/test/java/com/cloud/storage/VolumeApiServiceImplTest.java @@ -613,7 +613,6 @@ public class VolumeApiServiceImplTest { inOrder.verify(storagePoolMock).isLocal(); inOrder.verify(newDiskOfferingMock, times(0)).isShared(); inOrder.verify(volumeApiServiceImpl).getStoragePoolTags(storagePoolMock); - inOrder.verify(newDiskOfferingMock).getTags(); inOrder.verify(volumeVoMock).getSize(); inOrder.verify(newDiskOfferingMock).getDiskSize(); @@ -996,4 +995,95 @@ public class VolumeApiServiceImplTest { volumeApiServiceImpl.deleteVolume(volumeMockId, accountMock); } + + @Test + public void doesTargetStorageSupportDiskOfferingTestDiskOfferingMoreTagsThanStorageTags() { + DiskOfferingVO diskOfferingVoMock = Mockito.mock(DiskOfferingVO.class); + Mockito.doReturn("A,B,C").when(diskOfferingVoMock).getTags(); + + StoragePool storagePoolMock = Mockito.mock(StoragePool.class); + Mockito.doReturn("A").when(volumeApiServiceImpl).getStoragePoolTags(storagePoolMock); + + boolean result = volumeApiServiceImpl.doesTargetStorageSupportNewDiskOffering(storagePoolMock, diskOfferingVoMock); + + Assert.assertFalse(result); + } + + @Test + public void doesTargetStorageSupportDiskOfferingTestDiskOfferingTagsIsSubSetOfStorageTags() { + DiskOfferingVO diskOfferingVoMock = Mockito.mock(DiskOfferingVO.class); + Mockito.doReturn("A,B,C").when(diskOfferingVoMock).getTags(); + + StoragePool storagePoolMock = Mockito.mock(StoragePool.class); + Mockito.doReturn("A,B,C,D,X,Y").when(volumeApiServiceImpl).getStoragePoolTags(storagePoolMock); + + boolean result = volumeApiServiceImpl.doesTargetStorageSupportNewDiskOffering(storagePoolMock, diskOfferingVoMock); + + Assert.assertTrue(result); + } + + @Test + public void doesTargetStorageSupportDiskOfferingTestDiskOfferingTagsEmptyAndStorageTagsNotEmpty() { + DiskOfferingVO diskOfferingVoMock = Mockito.mock(DiskOfferingVO.class); + Mockito.doReturn("").when(diskOfferingVoMock).getTags(); + + StoragePool storagePoolMock = Mockito.mock(StoragePool.class); + Mockito.doReturn("A,B,C,D,X,Y").when(volumeApiServiceImpl).getStoragePoolTags(storagePoolMock); + + boolean result = volumeApiServiceImpl.doesTargetStorageSupportNewDiskOffering(storagePoolMock, diskOfferingVoMock); + + Assert.assertTrue(result); + } + + @Test + public void doesTargetStorageSupportDiskOfferingTestDiskOfferingTagsNotEmptyAndStorageTagsEmpty() { + DiskOfferingVO diskOfferingVoMock = Mockito.mock(DiskOfferingVO.class); + Mockito.doReturn("A").when(diskOfferingVoMock).getTags(); + + StoragePool storagePoolMock = Mockito.mock(StoragePool.class); + Mockito.doReturn("").when(volumeApiServiceImpl).getStoragePoolTags(storagePoolMock); + + boolean result = volumeApiServiceImpl.doesTargetStorageSupportNewDiskOffering(storagePoolMock, diskOfferingVoMock); + + Assert.assertFalse(result); + } + + @Test + public void doesTargetStorageSupportDiskOfferingTestDiskOfferingTagsEmptyAndStorageTagsEmpty() { + DiskOfferingVO diskOfferingVoMock = Mockito.mock(DiskOfferingVO.class); + Mockito.doReturn("").when(diskOfferingVoMock).getTags(); + + StoragePool storagePoolMock = Mockito.mock(StoragePool.class); + Mockito.doReturn("").when(volumeApiServiceImpl).getStoragePoolTags(storagePoolMock); + + boolean result = volumeApiServiceImpl.doesTargetStorageSupportNewDiskOffering(storagePoolMock, diskOfferingVoMock); + + Assert.assertTrue(result); + } + + @Test + public void doesTargetStorageSupportDiskOfferingTestDiskOfferingTagsDifferentFromdStorageTags() { + DiskOfferingVO diskOfferingVoMock = Mockito.mock(DiskOfferingVO.class); + Mockito.doReturn("A,B").when(diskOfferingVoMock).getTags(); + + StoragePool storagePoolMock = Mockito.mock(StoragePool.class); + Mockito.doReturn("C,D").when(volumeApiServiceImpl).getStoragePoolTags(storagePoolMock); + + boolean result = volumeApiServiceImpl.doesTargetStorageSupportNewDiskOffering(storagePoolMock, diskOfferingVoMock); + + Assert.assertFalse(result); + } + + @Test + public void doesTargetStorageSupportDiskOfferingTestDiskOfferingTagsEqualsStorageTags() { + DiskOfferingVO diskOfferingVoMock = Mockito.mock(DiskOfferingVO.class); + Mockito.doReturn("A").when(diskOfferingVoMock).getTags(); + + StoragePool storagePoolMock = Mockito.mock(StoragePool.class); + Mockito.doReturn("A").when(volumeApiServiceImpl).getStoragePoolTags(storagePoolMock); + + boolean result = volumeApiServiceImpl.doesTargetStorageSupportNewDiskOffering(storagePoolMock, diskOfferingVoMock); + + Assert.assertTrue(result); + } }