Skip to content

Fix limitation on tag matching in 'migrateVolume' with disk offering replacement#2636

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
rafaelweingartner merged 4 commits into from
Jul 21, 2018
Merged

Fix limitation on tag matching in 'migrateVolume' with disk offering replacement #2636

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8bbeb46
Fix limitation on tag matching in 'migrateVolume' with disk offering …
rafaelweingartner May 10, 2018
4f59b93
Address Nitin's suggestions
rafaelweingartner May 11, 2018
4c9a945
Apply Daan's suggestion regarding "doesTargetStorageSupportDiskOfferi…
rafaelweingartner Jul 19, 2018
48dddbf
fix problem
rafaelweingartner Jul 20, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 34 additions & 1 deletion api/src/main/java/com/cloud/storage/VolumeApiService.java
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -102,4 +102,37 @@ Snapshot takeSnapshot(Long volumeId, Long policyId, Long snapshotId, Account acc
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 changes: 59 additions & 7 deletions server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -19,6 +19,7 @@
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;
Expand DownExpand Up@@ -2161,9 +2162,9 @@ private DiskOfferingVO retrieveAndValidateNewDiskOffering(MigrateVolumeCmd cmd)
* 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.
Expand All@@ -2175,10 +2176,9 @@ protected void validateConditionsToReplaceDiskOfferingOfVolume(VolumeVO volume,
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());
Expand All@@ -2189,6 +2189,58 @@ protected void validateConditionsToReplaceDiskOfferingOfVolume(VolumeVO volume,
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) {

@DaanHooglandDaanHooglandMay 11, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i see the parameter appearing in the method name. why not isSupported(pool, offering);?

@rafaelweingartnerrafaelweingartnerMay 11, 2018

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is a good point. I normally try to be very descriptive with method names. When I see names such as isSupported, even though we see the parameters there, we have no clues what the method is checking. It could be that the method is checking if the storage has enough IOPs, or the method has enough space, or any other business logic. Maybe, a better name here would be doesTargetStorageSupportNewDiskOfferingTags.

Of course, here I have documented the method in detail. However, this should not be an excuse not to try to work out a good and descriptive method name.

@DaanHooglandDaanHooglandJul 17, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a protected method. When we want to let a hypervisor do the entire migration it would be convenient to call this method from elsewhere. Does it make sense to put it on the interface? I am thinking about the VirtualMachineManager or the HypervisorGurus.

@rafaelweingartnerrafaelweingartnerJul 18, 2018

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, I am not sure if we have some other places that can use this method. I would normally only set a method to public and declare it in an interface if I need really need it. I can declare it if you think it is important though.

P.S.: I find our interface usage sometimes quite silly with single implementations. A good example is using interface for POJOs. It does not provide benefits, the only thing it does is adding dozens more lines for every object.

@DaanHooglandDaanHooglandJul 18, 2018•
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, nice subject: another valid usage is mutual dependent services. Or dependency injection.
I this case my concern is that the implementation of tag matching is done in several places while it should be uniform and thus implemented centrally. doesTargetStorageASupportNewDiskOffering() is a specialisation of does_this_entities_tags_match_the_resource_it_is_going_to(entity, resource) which is applicable to volume/storage in case of new offering but also in case of migration, and is also applicable for vm-serviceoffering/host-cluster-pod etc. I cant' think of any others by head but these are surely there as well.

@rafaelweingartnerrafaelweingartnerJul 18, 2018

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This new method that I implemented is the consolidation of the logic of tag matching in ACS, which is not extracted in a specific method and spread around the code. That is one of the reasons why I “made the mistake” in one of the features I developed where the tag matching was being too strict (requiring all tags to match) and was not making sense to operation guys.

To answer your question, yes this method can be used in other places. I am not sure if simply declaring the method in the interface will help much though. The person that is willing to consolidate these execution flow could do both, declare this method in the interface, and then use it in all of those places that you mentioned.

What do you think?

@DaanHooglandDaanHooglandJul 18, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is a nice future plan to extract the logic to a tagging utility and then make this method use it and be given a more general name like

 doesTargetStorageSupportDiskOffering() // not no 'New' in there

and have it call the utility

 matchOk(Set<String> tags, List<String> supported);

plan?

@DaanHooglandDaanHooglandJul 19, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

matchOk is just a bad method-name i came up with. we need a set of methods that do the matching for us for
Set tags, List supported or
List<List> tags, List supported or
String tag(s), List supported etc

@rafaelweingartnerrafaelweingartnerJul 19, 2018

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, but in this case I think we should create methods on demand. I mean, we do not know what we need yet. Therefore, there is no reason to start coding methods that we do not even know if we will use them.

@DaanHooglandDaanHooglandJul 19, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, maybe postpone this discussion to a beer session ;)

@DaanHooglandDaanHooglandJul 19, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

of course I meant hackathon

@rafaelweingartnerrafaelweingartnerJul 19, 2018

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahaha.. I do not know why, but I would prefer a beer session...

I will do the change you asked later (making the method public in the interface).

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, ",");

@DaanHooglandDaanHooglandJul 20, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there is a StringUtils.csvTagsToList()

@rafaelweingartnerrafaelweingartnerJul 20, 2018

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you talking about com.cloud.utils.StringUtils.csvTagsToList(String)?
Didn't we talk about stop using that "utils" class, so we can then remove it in the future?

@DaanHooglandDaanHooglandJul 20, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we agreed to move all string operation in it so it we have a small interface plain to external libraries.

@rafaelweingartnerrafaelweingartnerJul 20, 2018

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did we?! That means we would create a layer over a library. I think we had a discussion, but I was under the impression that the idea was not to redo the wheel. Of course, my memory is not as good as it was before, so I am probably mistaken.

@DaanHooglandDaanHooglandJul 20, 2018•
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my opinion it is best to isolate all external libraries in wrappers so porting from version to version or from vendor to vendor becomes easier/more isolated. In fact the hypervisor guru and resource and data motion strategies are examples of that as well. We didn't make an on mail list decision of it but I thought I had convinced you of it.

@rafaelweingartnerrafaelweingartnerJul 20, 2018

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, but those "gurus"/plugins/drivers and so on are different "things".
I mean, they are not library, they are part of our code base to deliver our business. On the other hand, creating a layer over third party utils libraries such as StringUtils, NumberUtils, Hibernate, Spring and so on sound like an overkill.

We should discuss that we beers in Montreal :)

@DaanHooglandDaanHooglandJul 20, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one word: gson
and a lot of beers

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.
*/
Expand Down
92 changes: 91 additions & 1 deletion server/src/test/java/com/cloud/storage/VolumeApiServiceImplTest.java
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -613,7 +613,6 @@ public void validateConditionsToReplaceDiskOfferingOfVolumeTestEverythingWorking
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();
Expand DownExpand Up@@ -996,4 +995,95 @@ public void deleteVolumeTestVolumeStateReadyThrowingRuntimeException() throws In

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