This is an automated email from the ASF dual-hosted git repository.
rafael pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/cloudstack.git
The following commit(s) were added to refs/heads/master by this push:
new 756a7e8 Fix limitation on tag matching in 'migrateVolume' with disk
offering replacement (#2636)
756a7e8 is described below
commit 756a7e89cbcec7a1aa98b3f470cbdf8181af75ad
Author: Rafael Weingärtner <[email protected]>
AuthorDate: Sat Jul 21 07:01:24 2018 -0300
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
---
.../java/com/cloud/storage/VolumeApiService.java | 35 +++++++-
.../com/cloud/storage/VolumeApiServiceImpl.java | 66 ++++++++++++++--
.../cloud/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 cf13cd6..ec20c33 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.
+ * <table border="1">
+ * <tr>
+ * <th>#</th><th>Disk offering tags</th><th>Storage
tags</th><th>Does the storage support the disk offering?</th>
+ * </tr>
+ * <body>
+ * <tr>
+ * <td>1</td><td>A,B</td><td>A</td><td>NO</td>
+ * </tr>
+ * <tr>
+ * <td>2</td><td>A,B,C</td><td>A,B,C,D,X</td><td>YES</td>
+ * </tr>
+ * <tr>
+ * <td>3</td><td>A,B,C</td><td>X,Y,Z</td><td>NO</td>
+ * </tr>
+ * <tr>
+ * <td>4</td><td>null</td><td>A,S,D</td><td>YES</td>
+ * </tr>
+ * <tr>
+ * <td>5</td><td>A</td><td>null</td><td>NO</td>
+ * </tr>
+ * <tr>
+ * <td>6</td><td>null</td><td>null</td><td>YES</td>
+ * </tr>
+ * </body>
+ * </table>
+ */
+ 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 9bff4a1..3fcf761 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.
* <ul>
- * <li>We check if the given volume is of ROOT type. We cannot change the
disk offering of a ROOT volume. Therefore, we thrown an {@link
InvalidParameterValueException}.
- * <li>We the disk is being migrated to shared storage and the new disk
offering is for local storage (or vice versa), we throw an {@link
InvalidParameterValueException}. Bear in mind that we are validating only the
new disk offering. If none is provided we can override the current disk
offering. This means, placing a volume with shared disk offering in local
storage and vice versa.
- * <li>We then proceed checking if the tags of the new disk offerings
match the tags of the target storage. If they do not match an {@link
InvalidParameterValueException} is thrown.
+ * <li>We check if the given volume is of ROOT type. We cannot change the
disk offering of a ROOT volume. Therefore, we thrown an {@link
InvalidParameterValueException};
+ * <li>We the disk is being migrated to shared storage and the new disk
offering is for local storage (or vice versa), we throw an {@link
InvalidParameterValueException}. Bear in mind that we are validating only the
new disk offering. If none is provided we can override the current disk
offering. This means, placing a volume with shared disk offering in local
storage and vice versa;
+ * <li>We then proceed checking the target storage pool supports the new
disk offering {@link #doesTargetStorageSupportNewDiskOffering(StoragePool,
DiskOfferingVO)}.
* </ul>
*
* 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());
@@ -2190,6 +2190,58 @@ public class VolumeApiServiceImpl extends ManagerBase
implements VolumeApiServic
}
/**
+ * 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.
+ *
+ * <table border="1">
+ * <tr>
+ * <th>#</th><th>Disk offering tags</th><th>Storage
tags</th><th>Does the storage support the disk offering?</th>
+ * </tr>
+ * <body>
+ * <tr>
+ * <td>1</td><td>A,B</td><td>A</td><td>NO</td>
+ * </tr>
+ * <tr>
+ * <td>2</td><td>A,B,C</td><td>A,B,C,D,X</td><td>YES</td>
+ * </tr>
+ * <tr>
+ * <td>3</td><td>A,B,C</td><td>X,Y,Z</td><td>NO</td>
+ * </tr>
+ * <tr>
+ * <td>4</td><td>null</td><td>A,S,D</td><td>YES</td>
+ * </tr>
+ * <tr>
+ * <td>5</td><td>A</td><td>null</td><td>NO</td>
+ * </tr>
+ * <tr>
+ * <td>6</td><td>null</td><td>null</td><td>YES</td>
+ * </tr>
+ * </body>
+ * </table>
+ */
+ 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.
*/
protected String getStoragePoolTags(StoragePool destPool) {
diff --git
a/server/src/test/java/com/cloud/storage/VolumeApiServiceImplTest.java
b/server/src/test/java/com/cloud/storage/VolumeApiServiceImplTest.java
index 8ae122d..890b1ea 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);
+ }
}
