etan39
/
cloudstack
mirror of https://github.com/apache/cloudstack.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

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
This commit is contained in:
Rafael Weingärtner 2018-07-21 07:01:24 -03:00 committed by GitHub
parent a21ebb1ce8
commit 756a7e89cb
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
3 changed files with 184 additions and 9 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

35
api/src/main/java/com/cloud/storage/VolumeApiService.java
View File

@ -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);
}

66
server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java
View File

@ -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());
@ -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.
*
* <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.
*/

92
server/src/test/java/com/cloud/storage/VolumeApiServiceImplTest.java
View File

@ -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);
}
}