[GitHub] mike-tutkowski commented on issue #2761: Add managed storage pool constraints to MigrateWithVolume API method

[GitBox]

mike-tutkowski commented on issue #2761: Add managed storage pool constraints 
to MigrateWithVolume API method
URL: https://github.com/apache/cloudstack/pull/2761#issuecomment-420036445
 
 
   @rafaelweingartner Or are you guys still investigating regression-test 
issues?


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] mike-tutkowski commented on issue #2761: Add managed storage pool constraints to MigrateWithVolume API method

[GitBox]

mike-tutkowski commented on issue #2761: Add managed storage pool constraints 
to MigrateWithVolume API method
URL: https://github.com/apache/cloudstack/pull/2761#issuecomment-420035890
 
 
   It looks like we have two LGTMs and tests passing within expectations. 
@rafaelweingartner Would you like to merge this PR?


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] mike-tutkowski commented on issue #2761: Add managed storage pool constraints to MigrateWithVolume API method

[GitBox]

mike-tutkowski commented on issue #2761: Add managed storage pool constraints 
to MigrateWithVolume API method
URL: https://github.com/apache/cloudstack/pull/2761#issuecomment-413934072
 
 
   @syed Would you be able to review this?


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] mike-tutkowski commented on issue #2761: Add managed storage pool constraints to MigrateWithVolume API method

[GitBox]

mike-tutkowski commented on issue #2761: Add managed storage pool constraints 
to MigrateWithVolume API method
URL: https://github.com/apache/cloudstack/pull/2761#issuecomment-412321877
 
 
   Yes, LGTM


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] mike-tutkowski commented on issue #2761: Add managed storage pool constraints to MigrateWithVolume API method

[GitBox]

mike-tutkowski commented on issue #2761: Add managed storage pool constraints 
to MigrateWithVolume API method
URL: https://github.com/apache/cloudstack/pull/2761#issuecomment-412125942
 
 
   @rafaelweingartner How are we doing here? It looks like the code is in a 
good state. Do we need to kick off more tests? Thanks!


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] mike-tutkowski commented on issue #2761: Add managed storage pool constraints to MigrateWithVolume API method

[GitBox]

mike-tutkowski commented on issue #2761: Add managed storage pool constraints 
to MigrateWithVolume API method
URL: https://github.com/apache/cloudstack/pull/2761#issuecomment-409977463
 
 
   Cool...I guess I don’t merge enough PRs to have noticed that option. :)
   
   On Aug 2, 2018, at 5:09 AM, Rafael Weingärtner 
mailto:notificati...@github.com>> wrote:
   
   
   @DaanHoogland that is what I was going to 
say to Mike. We can squash at the merge "moment".
   
   Can we wait more one week or so before merging? I would like to cover this 
code with unit tests. It is a very complicated and delicate one code, and it 
does not have unit-tests for it.
   
   —
   You are receiving this because you were mentioned.
   Reply to this email directly, view it on 
GitHub, 
or mute the 
thread.
   


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] mike-tutkowski commented on issue #2761: Add managed storage pool constraints to MigrateWithVolume API method

[GitBox]

mike-tutkowski commented on issue #2761: Add managed storage pool constraints 
to MigrateWithVolume API method
URL: https://github.com/apache/cloudstack/pull/2761#issuecomment-409806548
 
 
   Once we are good with the standard set of tests, I suggest squashing this 
work down to one commit before merging. Thanks!


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] mike-tutkowski commented on issue #2761: Add managed storage pool constraints to MigrateWithVolume API method

[GitBox]

mike-tutkowski commented on issue #2761: Add managed storage pool constraints 
to MigrateWithVolume API method
URL: https://github.com/apache/cloudstack/pull/2761#issuecomment-409806417
 
 
   The tests have passed:
   
   test_01_storage_migrate_root_and_data_disks 
(TestVMMigrationWithStorage.TestVMMigrationWithStorage) ... === TestName: 
test_01_storage_migrate_root_and_data_disks | Status : SUCCESS ===
   ok
   test_02_storage_migrate_root_and_data_disks 
(TestVMMigrationWithStorage.TestVMMigrationWithStorage) ... === TestName: 
test_02_storage_migrate_root_and_data_disks | Status : SUCCESS ===
   ok
   test_03_storage_migrate_root_and_data_disks_fail 
(TestVMMigrationWithStorage.TestVMMigrationWithStorage) ... === TestName: 
test_03_storage_migrate_root_and_data_disks_fail | Status : SUCCESS ===
   ok
   test_04_storage_migrate_root_disk_fails 
(TestVMMigrationWithStorage.TestVMMigrationWithStorage) ... === TestName: 
test_04_storage_migrate_root_disk_fails | Status : SUCCESS ===
   ok
   test_05_storage_migrate_data_disk_fails 
(TestVMMigrationWithStorage.TestVMMigrationWithStorage) ... === TestName: 
test_05_storage_migrate_data_disk_fails | Status : SUCCESS ===
   ok
   
   --
   Ran 5 tests in 2431.717s
   
   OK


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] mike-tutkowski commented on issue #2761: Add managed storage pool constraints to MigrateWithVolume API method

[GitBox]

mike-tutkowski commented on issue #2761: Add managed storage pool constraints 
to MigrateWithVolume API method
URL: https://github.com/apache/cloudstack/pull/2761#issuecomment-409767259
 
 
   I have to let this test suite run and come back to it later. Just an FYI, 
though: The test that failed before (and lead to this PR) has now passed. I'm 
running the entire relevant test suite, though, to see if anything else pops up.
   
   Now, this is not at all necessary, but what do you think about these 
suggested changes:
   
   
https://github.com/mike-tutkowski/cloudstack/commit/f12e9af1bb4ab384298ffdced8f30b22e484ab13
   
   They are not critical...just some changes I thought simplified the code a 
bit.


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] mike-tutkowski commented on issue #2761: Add managed storage pool constraints to MigrateWithVolume API method

[GitBox]

mike-tutkowski commented on issue #2761: Add managed storage pool constraints 
to MigrateWithVolume API method
URL: https://github.com/apache/cloudstack/pull/2761#issuecomment-409761409
 
 
   No problem. :) I think we are almost there now. I'm going to re-run my tests 
in a moment.


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] mike-tutkowski commented on issue #2761: Add managed storage pool constraints to MigrateWithVolume API method

[GitBox]

mike-tutkowski commented on issue #2761: Add managed storage pool constraints 
to MigrateWithVolume API method
URL: https://github.com/apache/cloudstack/pull/2761#issuecomment-409761023
 
 
   That's not the problem. The issue is iterating over 
volumeToStoragePoolObjectMap instead of  allVolumes.
   
   As an example, let's say the user didn't enter anything, then 
volumeToStoragePoolObjectMap is empty. This leads to an empty volumesNotMapped 
being passed back instead of one that contains all of the volumes of the VM.


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] mike-tutkowski commented on issue #2761: Add managed storage pool constraints to MigrateWithVolume API method

[GitBox]

mike-tutkowski commented on issue #2761: Add managed storage pool constraints 
to MigrateWithVolume API method
URL: https://github.com/apache/cloudstack/pull/2761#issuecomment-409757636
 
 
   This method doesn't work (example: assume volumeToStoragePoolObjectMap comes 
in empty):
   
   private List 
findVolumesThatWereNotMappedByTheUser(VirtualMachineProfile profile, 
Map volumeToStoragePoolObjectMap) {
   List allVolumes = 
_volsDao.findUsableVolumesForInstance(profile.getId());
   List volumesNotMapped = new ArrayList<>();
   for (Volume volume : volumeToStoragePoolObjectMap.keySet()) {
   if (!allVolumes.contains(volume)) {
   volumesNotMapped.add(volume);
   }
   }
   return volumesNotMapped;
   }
   
   I changed it to this:
   
   private List 
findVolumesThatWereNotMappedByTheUser(VirtualMachineProfile profile, 
Map volumeToStoragePoolObjectMap) {
   List allVolumes = 
_volsDao.findUsableVolumesForInstance(profile.getId());
   List volumesNotMapped = new ArrayList<>();
   for (Volume volume : allVolumes) {
   if (!volumeToStoragePoolObjectMap.containsKey(volume)) {
   volumesNotMapped.add(volume);
   }
   }
   return volumesNotMapped;
   }
   
   I'm still testing.


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] mike-tutkowski commented on issue #2761: Add managed storage pool constraints to MigrateWithVolume API method

[GitBox]

mike-tutkowski commented on issue #2761: Add managed storage pool constraints 
to MigrateWithVolume API method
URL: https://github.com/apache/cloudstack/pull/2761#issuecomment-409712517
 
 
   @rafaelweingartner This looks good. Let me set up a cloud using this code 
and run the test that originally failed (the one that caused me to open a bug 
for this in the first place).


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] mike-tutkowski commented on issue #2761: Add managed storage pool constraints to MigrateWithVolume API method

[GitBox]

mike-tutkowski commented on issue #2761: Add managed storage pool constraints 
to MigrateWithVolume API method
URL: https://github.com/apache/cloudstack/pull/2761#issuecomment-409676248
 
 
   You are really close now, @rafaelweingartner. It looks like you missed the 
suggestions at the bottom of my previous diff. I put those suggestions here for 
you to look at: 
https://github.com/mike-tutkowski/cloudstack/commit/f22e03f1866fd011e080219645ef695e751b2c0e.
   
   Here's a copy/paste of part of my previous comment from above that relates 
to this diff:
   
   "The other area I changed was in the createStoragePoolMappingsForVolumes 
method. It does not need to call executeManagedStorageChecks. However, we do 
need to handle managed storage specially here. If the storage pool is managed, 
make sure the target host can see this storage pool."


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] mike-tutkowski commented on issue #2761: Add managed storage pool constraints to MigrateWithVolume API method

[GitBox]

mike-tutkowski commented on issue #2761: Add managed storage pool constraints 
to MigrateWithVolume API method
URL: https://github.com/apache/cloudstack/pull/2761#issuecomment-409060694
 
 
   Once we are good to go with the changes, I can run the applicable 
managed-storage test on this branch (the one that caught the issue in the first 
place).


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] mike-tutkowski commented on issue #2761: Add managed storage pool constraints to MigrateWithVolume API method

[GitBox]

mike-tutkowski commented on issue #2761: Add managed storage pool constraints 
to MigrateWithVolume API method
URL: https://github.com/apache/cloudstack/pull/2761#issuecomment-409053890
 
 
   Hi @rafaelweingartner Yes, just a couple, though. Here is a diff of my 
recommended final changes: 
https://github.com/mike-tutkowski/cloudstack/commit/48fb1ecaf70dc9eb364a58eabf51504926e9c5eb.
   
   To summarize them:
   
   The executeManagedStorageChecks method is only called from one place. It 
does not require a check to make sure the managed storage pool is at the zone 
level. Also, instead of executeManagedStorageChecks checking if the current 
pool's cluster ID is equal to that of the cluster ID of the target host, we 
just compare the IDs of the current and target pools (because we don't allow 
you to change the managed storage pool during this operation). I needed to 
change what’s passed in to the method to make this work. Comparing the cluster 
ID of the current pool to the cluster ID of the target host won’t work with 
zone-wide primary storage.
   
   The other area I changed was in the createStoragePoolMappingsForVolumes 
method. It does not need to call executeManagedStorageChecks. However, we do 
need to handle managed storage specially here. If the storage pool is managed, 
make sure the target host can see this storage pool.


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] mike-tutkowski commented on issue #2761: Add managed storage pool constraints to MigrateWithVolume API method

[GitBox]

mike-tutkowski commented on issue #2761: Add managed storage pool constraints 
to MigrateWithVolume API method
URL: https://github.com/apache/cloudstack/pull/2761#issuecomment-408563095
 
 
   OK, I'm back at my computer now. I'm looking at this bit of code in my 
sample code: 
https://github.com/mike-tutkowski/cloudstack/commit/bea1dad5b84bb80ba1f1297a69a1daea1c4ec736#diff-a19937c69234222505de0e6bcaad43b9R2370
 .
   
   That method is only invoked in one place and only checks the two things I 
mentioned (if the current pool is managed and, if so, is its ID equal to that 
of the target pool).
   
   I don't think that will be a problem for the use case you specified.


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] mike-tutkowski commented on issue #2761: Add managed storage pool constraints to MigrateWithVolume API method

[GitBox]

mike-tutkowski commented on issue #2761: Add managed storage pool constraints 
to MigrateWithVolume API method
URL: https://github.com/apache/cloudstack/pull/2761#issuecomment-408540260
 
 
   I’ll have to take a look when I’m back at my computer.
   
   From memory, though, I thought there was only two checks in that 
managed-storage method:
   
   If it’s not managed storage, return.
   
   If it is managed storage, the ID of the current and target storage pools 
must be the same.
   
   I’m not sure how that breaks the use case you mentioned. I can take a look 
later, though.
   
   On Jul 27, 2018, at 2:59 PM, Rafael Weingärtner 
mailto:notificati...@github.com>> wrote:
   
   
   Yes, the second link has only one method. The problem with the second 
suggestion is that it causes a bug as I mentioned. I mean, it will create a 
situation where it will not allow the migration of VMs with volumes in 
cluster-wide managed storage, even though it should be supported.
   
   Imagine a VM that has two volumes one placed in local storage and other in a 
managed storage. The `migrateVirtualMachineWithVolume' is called to migrate the 
VM between hosts of the same cluster. The migration should be supported as long 
as the target host has a local storage to receive one of the volumes, and the 
other one does not need to be migrated as it is in a cluster-wide managed 
storage and the target host is in the same cluster as the source host.
   
   —
   You are receiving this because you were mentioned.
   Reply to this email directly, view it on 
GitHub, 
or mute the 
thread.
   


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] mike-tutkowski commented on issue #2761: Add managed storage pool constraints to MigrateWithVolume API method

[GitBox]

mike-tutkowski commented on issue #2761: Add managed storage pool constraints 
to MigrateWithVolume API method
URL: https://github.com/apache/cloudstack/pull/2761#issuecomment-408535487
 
 
   I can check a little later when I am back at the computer.
   
   If you take a look at the link I sent you yesterday (or the day before) that 
has my suggested code, there is only one method that does any manage storage 
checks and it is only called in one place.
   
   On Jul 27, 2018, at 2:42 PM, Rafael Weingärtner 
mailto:notificati...@github.com>> wrote:
   
   
   @rafaelweingartner commented on this pull request.
   
   
   
   In 
engine/orchestration/src/main/java/com/cloud/vm/VirtualMachineManagerImpl.java:
   
   >  StoragePoolVO currentPool = 
_storagePoolDao.findById(volume.getPoolId());
   -if (ScopeType.HOST.equals(currentPool.getScope())) {
   -createVolumeToStoragePoolMappingIfNeeded(profile, 
targetHost, volumeToPoolObjectMap, volume, currentPool);
   +
   +
executeManagedStorageChecksWhenTargetStoragePoolNotProvided(targetHost, 
currentPool, volume);
   
   
   That is exactly what I understood before. What I do not understand is why we 
need those executeManagedStorageChecksWhenTargetStoragePoolNotProvided and 
executeManagedStorageChecksWhenTargetStoragePoolProvided instead of a single 
executeManagedStorageChecks.
   
   Can you check at the last commit I pushed? Why would we need to divide that 
executeManagedStorageChecks into two other methods?
   
   —
   You are receiving this because you were mentioned.
   Reply to this email directly, view it on 
GitHub, 
or mute the 
thread.
   


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] mike-tutkowski commented on issue #2761: Add managed storage pool constraints to MigrateWithVolume API method

[GitBox]

mike-tutkowski commented on issue #2761: Add managed storage pool constraints 
to MigrateWithVolume API method
URL: https://github.com/apache/cloudstack/pull/2761#issuecomment-408271570
 
 
   By the way, I didn't change the test for 
https://github.com/mike-tutkowski/cloudstack/commit/bea1dad5b84bb80ba1f1297a69a1daea1c4ec736,
 so it won't technically compile. I intented it more to give you an idea of 
where we might want to go with the code here.


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] mike-tutkowski commented on issue #2761: Add managed storage pool constraints to MigrateWithVolume API method

[GitBox]

mike-tutkowski commented on issue #2761: Add managed storage pool constraints 
to MigrateWithVolume API method
URL: https://github.com/apache/cloudstack/pull/2761#issuecomment-408270678
 
 
   @rafaelweingartner Let's say the user provided a (target) managed storage 
pool for the volume in question. If that's the case, then we just want to make 
sure that this managed storage pool is the same as the storage pool of the 
volume. We don't care if it's a zone-wide managed storage pool or a 
cluster-wide managed storage pool in this case (either is acceptable). We just 
want to make sure that it's the same storage pool. Those checks are captured in 
executeManagedStorageChecksWhenTargetStoragePoolProvided (which does not care 
about the zone-wide requirement).
   
   However, if you don't provide a storage pool for the volume in question and 
it is located on managed storage, then we - in case you are doing a migration 
of the VM from one cluster to another - need to only make sure the managed 
storage pool is zone wide. If it turns out that this managed storage pool is 
cluster wide, then the user needs to explicitly pass the volume/target storage 
pool in as a parameter to the API command when performing the migration of the 
VM (with its storage) from one cluster to another.
   
   At least this is how it used to work.
   
   One alternative is to keep a single managed-storage method for checking, but 
to drop the zone-wide requirement. In fact, I think we can get away with only 
calling that check method once then.
   
   Take a look at this proposal:
   
   
https://github.com/mike-tutkowski/cloudstack/commit/bea1dad5b84bb80ba1f1297a69a1daea1c4ec736


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] mike-tutkowski commented on issue #2761: Add managed storage pool constraints to MigrateWithVolume API method

[GitBox]

mike-tutkowski commented on issue #2761: Add managed storage pool constraints 
to MigrateWithVolume API method
URL: https://github.com/apache/cloudstack/pull/2761#issuecomment-408255118
 
 
   @rafaelweingartner I think we are very close. Take a look at what I did 
here: 
https://github.com/mike-tutkowski/cloudstack/commit/07279b41a3277becdc3b6c79f69f46559958f442
 .
   
   I made a few changes that I felt better reflected what the code used to do 
around checks for managed storage.
   
   To get it to compile, I made a couple changes and commented out some code in 
the JUnit test for now.


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] mike-tutkowski commented on issue #2761: Add managed storage pool constraints to MigrateWithVolume API method

[GitBox]

mike-tutkowski commented on issue #2761: Add managed storage pool constraints 
to MigrateWithVolume API method
URL: https://github.com/apache/cloudstack/pull/2761#issuecomment-407896078
 
 
   Technically, this PR is about two issues:
   
   1) Non-Managed Storage: Migrating a VM across compute clusters (at least 
supported in XenServer) was no longer possible. If, say, a virtual disk resides 
on shared storage in the source compute cluster, we must be able to copy this 
virtual disk to shared storage in the destination compute cluster.
   
   2) Managed Storage: There is currently a constraint with zone-wide managed 
storage. This was not being honored by the new code.


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