[GitHub] rafaelweingartner commented on a change in pull request #2636: Fix limitation on tag matching in 'migrateVolume' with disk offering replacement
rafaelweingartner commented on a change in pull request #2636: Fix limitation
on tag matching in 'migrateVolume' with disk offering replacement
URL: https://github.com/apache/cloudstack/pull/2636#discussion_r204023925
##
File path: server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java
##
@@ -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.
+ *
+ *
+ *
+ * #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, ",");
Review comment:
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 :)
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] rafaelweingartner commented on a change in pull request #2636: Fix limitation on tag matching in 'migrateVolume' with disk offering replacement
rafaelweingartner commented on a change in pull request #2636: Fix limitation
on tag matching in 'migrateVolume' with disk offering replacement
URL: https://github.com/apache/cloudstack/pull/2636#discussion_r204021694
##
File path: server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java
##
@@ -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.
+ *
+ *
+ *
+ * #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, ",");
Review comment:
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.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] rafaelweingartner commented on a change in pull request #2636: Fix limitation on tag matching in 'migrateVolume' with disk offering replacement
rafaelweingartner commented on a change in pull request #2636: Fix limitation
on tag matching in 'migrateVolume' with disk offering replacement
URL: https://github.com/apache/cloudstack/pull/2636#discussion_r204020791
##
File path: server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java
##
@@ -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.
+ *
+ *
+ *
+ * #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, ",");
Review comment:
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?
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] rafaelweingartner commented on a change in pull request #2636: Fix limitation on tag matching in 'migrateVolume' with disk offering replacement
rafaelweingartner commented on a change in pull request #2636: Fix limitation
on tag matching in 'migrateVolume' with disk offering replacement
URL: https://github.com/apache/cloudstack/pull/2636#discussion_r203688796
##
File path: server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java
##
@@ -2189,6 +2189,53 @@ 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.
+ *
+ *
+ *
+ * #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) {
Review comment:
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).
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] rafaelweingartner commented on a change in pull request #2636: Fix limitation on tag matching in 'migrateVolume' with disk offering replacement
rafaelweingartner commented on a change in pull request #2636: Fix limitation
on tag matching in 'migrateVolume' with disk offering replacement
URL: https://github.com/apache/cloudstack/pull/2636#discussion_r203665280
##
File path: server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java
##
@@ -2189,6 +2189,53 @@ 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.
+ *
+ *
+ *
+ * #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) {
Review comment:
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.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] rafaelweingartner commented on a change in pull request #2636: Fix limitation on tag matching in 'migrateVolume' with disk offering replacement
rafaelweingartner commented on a change in pull request #2636: Fix limitation
on tag matching in 'migrateVolume' with disk offering replacement
URL: https://github.com/apache/cloudstack/pull/2636#discussion_r203463594
##
File path: server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java
##
@@ -2189,6 +2189,53 @@ 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.
+ *
+ *
+ *
+ * #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) {
Review comment:
That sounds like a plan. I just did not understand what you mean by
`matchOk` . Could you explain it a little bit further?
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] rafaelweingartner commented on a change in pull request #2636: Fix limitation on tag matching in 'migrateVolume' with disk offering replacement
rafaelweingartner commented on a change in pull request #2636: Fix limitation
on tag matching in 'migrateVolume' with disk offering replacement
URL: https://github.com/apache/cloudstack/pull/2636#discussion_r203420800
##
File path: server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java
##
@@ -2189,6 +2189,53 @@ 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.
+ *
+ *
+ *
+ * #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) {
Review comment:
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?
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] rafaelweingartner commented on a change in pull request #2636: Fix limitation on tag matching in 'migrateVolume' with disk offering replacement
rafaelweingartner commented on a change in pull request #2636: Fix limitation
on tag matching in 'migrateVolume' with disk offering replacement
URL: https://github.com/apache/cloudstack/pull/2636#discussion_r203339499
##
File path: server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java
##
@@ -2189,6 +2189,53 @@ 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.
+ *
+ *
+ *
+ * #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) {
Review comment:
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.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] rafaelweingartner commented on a change in pull request #2636: Fix limitation on tag matching in 'migrateVolume' with disk offering replacement
rafaelweingartner commented on a change in pull request #2636: Fix limitation
on tag matching in 'migrateVolume' with disk offering replacement
URL: https://github.com/apache/cloudstack/pull/2636#discussion_r187596530
##
File path: server/src/test/java/com/cloud/storage/VolumeApiServiceImplTest.java
##
@@ -580,9 +580,86 @@ 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();
}
+
+@Test
+public void
doesTargetStorageSupportNewDiskOfferingTestDiskOfferingMoreTagsThanStorageTags()
{
+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
doesTargetStorageSupportNewDiskOfferingTestDiskOfferingTagsIsSubSetOfStorageTags()
{
Review comment:
Sure
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] rafaelweingartner commented on a change in pull request #2636: Fix limitation on tag matching in 'migrateVolume' with disk offering replacement
rafaelweingartner commented on a change in pull request #2636: Fix limitation
on tag matching in 'migrateVolume' with disk offering replacement
URL: https://github.com/apache/cloudstack/pull/2636#discussion_r187596399
##
File path: server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java
##
@@ -2142,6 +2142,54 @@ 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.
+ *
+ *
+ *
+ * #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();
+if (org.apache.commons.lang.StringUtils.isBlank(newDiskOfferingTags)) {
+return true;
+}
+String storagePoolTags = getStoragePoolTags(destPool);
+boolean isStorageTagsBlank =
org.apache.commons.lang.StringUtils.isBlank(storagePoolTags);
Review comment:
Done
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] rafaelweingartner commented on a change in pull request #2636: Fix limitation on tag matching in 'migrateVolume' with disk offering replacement
rafaelweingartner commented on a change in pull request #2636: Fix limitation
on tag matching in 'migrateVolume' with disk offering replacement
URL: https://github.com/apache/cloudstack/pull/2636#discussion_r187591238
##
File path: server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java
##
@@ -2142,6 +2142,55 @@ 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.
+ *
+ *
+ *
+ * #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();
+boolean isnewDiskOfferingTagsBlank =
org.apache.commons.lang.StringUtils.isBlank(newDiskOfferingTags);
+if (isnewDiskOfferingTagsBlank) {
+return true;
+}
+String storageTags = getStoragePoolTags(destPool);
Review comment:
Done
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] rafaelweingartner commented on a change in pull request #2636: Fix limitation on tag matching in 'migrateVolume' with disk offering replacement
rafaelweingartner commented on a change in pull request #2636: Fix limitation
on tag matching in 'migrateVolume' with disk offering replacement
URL: https://github.com/apache/cloudstack/pull/2636#discussion_r187590974
##
File path: server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java
##
@@ -2142,6 +2142,55 @@ 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.
+ *
+ *
+ *
+ * #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();
+boolean isnewDiskOfferingTagsBlank =
org.apache.commons.lang.StringUtils.isBlank(newDiskOfferingTags);
Review comment:
Sure, I can do that.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] rafaelweingartner commented on a change in pull request #2636: Fix limitation on tag matching in 'migrateVolume' with disk offering replacement
rafaelweingartner commented on a change in pull request #2636: Fix limitation
on tag matching in 'migrateVolume' with disk offering replacement
URL: https://github.com/apache/cloudstack/pull/2636#discussion_r187585842
##
File path: server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java
##
@@ -2142,6 +2142,55 @@ 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.
+ *
+ *
+ *
+ * #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) {
Review comment:
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.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] rafaelweingartner commented on a change in pull request #2636: Fix limitation on tag matching in 'migrateVolume' with disk offering replacement
rafaelweingartner commented on a change in pull request #2636: Fix limitation
on tag matching in 'migrateVolume' with disk offering replacement
URL: https://github.com/apache/cloudstack/pull/2636#discussion_r187585071
##
File path: server/src/test/java/com/cloud/storage/VolumeApiServiceImplTest.java
##
@@ -580,9 +580,86 @@ 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();
}
+
+@Test
+public void
doesTargetStorageSupportNewDiskOfferingTestDiskOfferingMoreTagsThanStorageTags()
{
Review comment:
Sure, this is something I do to control the unit tests. The pattern I
normally use is the following:
Test
Translating this case:
I am testing the method `doesTargetStorageSupportNewDiskOffering` with the
following condition:
* the disk offering will have more tags than the target storage
There is also another format I use:
`Test`
Happy day test. It can be the case when the method finishes successfully or
a method that does not have different execution flows.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
