[GitHub] cloudstack pull request: CLOUDSTACK-8302: Removing snapshots on RB...

Github user asfgit closed the pull request at: https://github.com/apache/cloudstack/pull/1230 --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature

[GitHub] cloudstack pull request: CLOUDSTACK-8302: Removing snapshots on RB...

Github user swill commented on the pull request: https://github.com/apache/cloudstack/pull/1230#issuecomment-216226380 @kiwiflyer thank you. I will merge this today... --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If

[GitHub] cloudstack pull request: CLOUDSTACK-8302: Removing snapshots on RB...

Github user rhtyd commented on the pull request: https://github.com/apache/cloudstack/pull/1230#issuecomment-216208373 tag:mergeready --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this

[GitHub] cloudstack pull request: CLOUDSTACK-8302: Removing snapshots on RB...

Github user kiwiflyer commented on the pull request: https://github.com/apache/cloudstack/pull/1230#issuecomment-215242253 Manual testing against a 4.8 hardware lab with Ceph Primary Storage. 2 volume snapshots triggered: rbd snap ls -p rbdnjcloudhost

[GitHub] cloudstack pull request: CLOUDSTACK-8302: Removing snapshots on RB...

Github user swill commented on the pull request: https://github.com/apache/cloudstack/pull/1230#issuecomment-215122706 @kiwiflyer Thank you sir. :) --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does

[GitHub] cloudstack pull request: CLOUDSTACK-8302: Removing snapshots on RB...

Github user kiwiflyer commented on the pull request: https://github.com/apache/cloudstack/pull/1230#issuecomment-215118770 I'm building this now and I'll give it a final manual test. Then we'll do a CI run against a hardware lab. --- If your project is set up for it, you can reply

[GitHub] cloudstack pull request: CLOUDSTACK-8302: Removing snapshots on RB...

Github user wido commented on the pull request: https://github.com/apache/cloudstack/pull/1230#issuecomment-214856246 With these changes I think we are good? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your

[GitHub] cloudstack pull request: CLOUDSTACK-8302: Removing snapshots on RB...

Github user dmytro-shevchenko commented on the pull request: https://github.com/apache/cloudstack/pull/1230#issuecomment-214497874 Rebase with Master done, pom.xml file updated. Also I perform a small modification in code, during testing I found one issue: in 'snapshot_store_ref'

[GitHub] cloudstack pull request: CLOUDSTACK-8302: Removing snapshots on RB...

Github user kiwiflyer commented on the pull request: https://github.com/apache/cloudstack/pull/1230#issuecomment-214414936 @dmytro-shevchenko We have 2 LGTMs on this and it just needs a CI run. Could you make the change @DaanHoogland requested above regarding the pom.xml and also

[GitHub] cloudstack pull request: CLOUDSTACK-8302: Removing snapshots on RB...

Github user wido commented on the pull request: https://github.com/apache/cloudstack/pull/1230#issuecomment-208486444 Based on the tests I see above LGTM. A new PR afterwards to rename seems good. --- If your project is set up for it, you can reply to this email and have

[GitHub] cloudstack pull request: CLOUDSTACK-8302: Removing snapshots on RB...

Github user mike-tutkowski commented on the pull request: https://github.com/apache/cloudstack/pull/1230#issuecomment-208484921 I'm not sure of the history for that particular file, but I believe it would be better named DefaultSnapshotStrategy. Does anyone else have a better

[GitHub] cloudstack pull request: CLOUDSTACK-8302: Removing snapshots on RB...

Github user mike-tutkowski commented on the pull request: https://github.com/apache/cloudstack/pull/1230#issuecomment-208484491 @wido Yes, the name is no longer valid, but it should work just fine for KVM. --- If your project is set up for it, you can reply to this email and have

[GitHub] cloudstack pull request: CLOUDSTACK-8302: Removing snapshots on RB...

Github user kiwiflyer commented on the pull request: https://github.com/apache/cloudstack/pull/1230#issuecomment-208439532 LGTM based on hardware lab testing of feature on 4.8 branch with PR applied. Wido, As far as I can tell, the changes to

[GitHub] cloudstack pull request: CLOUDSTACK-8302: Removing snapshots on RB...

Github user kiwiflyer commented on the pull request: https://github.com/apache/cloudstack/pull/1230#issuecomment-208438657 Lab testing of feature: Cloudstack RBD snapshot job: 2016-04-11 11:26:22,955 DEBUG [c.c.a.t.Request] (Work-Job-Executor-1:ctx-5ac9d6c8

[GitHub] cloudstack pull request: CLOUDSTACK-8302: Removing snapshots on RB...

Github user wido commented on the pull request: https://github.com/apache/cloudstack/pull/1230#issuecomment-208196999 @mike-tutkowski @kiwiflyer So, it is just the class which has a incorrect name? The code is good? --- If your project is set up for it, you can reply to this email

[GitHub] cloudstack pull request: CLOUDSTACK-8302: Removing snapshots on RB...

Github user swill commented on a diff in the pull request: https://github.com/apache/cloudstack/pull/1230#discussion_r59020704 --- Diff: engine/storage/snapshot/pom.xml --- @@ -37,6 +37,11 @@ test-jar test + +

[GitHub] cloudstack pull request: CLOUDSTACK-8302: Removing snapshots on RB...

Github user mike-tutkowski commented on the pull request: https://github.com/apache/cloudstack/pull/1230#issuecomment-200958562 Yes, @kiwiflyer, the naming XenserverSnapshotStrategy appears to be old and no longer accurate. I believe it should be renamed to something like

[GitHub] cloudstack pull request: CLOUDSTACK-8302: Removing snapshots on RB...

Github user andrijapanic commented on the pull request: https://github.com/apache/cloudstack/pull/1230#issuecomment-200350271 I believe this is the reason Dmytro edit it, since it handles KVM stuff also... On 23 March 2016 at 14:40, Simon Weller

[GitHub] cloudstack pull request: CLOUDSTACK-8302: Removing snapshots on RB...

Github user kiwiflyer commented on the pull request: https://github.com/apache/cloudstack/pull/1230#issuecomment-200347989 @DaanHoogland So I've been digging into this a bit. I could be very wrong here, but it seems that XenserverSnapshotStrategy is a very inaccurate

[GitHub] cloudstack pull request: CLOUDSTACK-8302: Removing snapshots on RB...

Github user DaanHoogland commented on the pull request: https://github.com/apache/cloudstack/pull/1230#issuecomment-192325294 @dmytro-shevchenko you are changing XenserverSnapshotStrategy from the storage engine and KVMStorageProcessor from the kvm hypervisor plugin. It seems that

[GitHub] cloudstack pull request: CLOUDSTACK-8302: Removing snapshots on RB...

Github user DaanHoogland commented on a diff in the pull request: https://github.com/apache/cloudstack/pull/1230#discussion_r55043098 --- Diff: engine/storage/snapshot/pom.xml --- @@ -37,6 +37,11 @@ test-jar test + +

[GitHub] cloudstack pull request: CLOUDSTACK-8302: Removing snapshots on RB...

Github user dmytro-shevchenko commented on the pull request: https://github.com/apache/cloudstack/pull/1230#issuecomment-178890524 Done, rebased with 4.9xx master. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your

[GitHub] cloudstack pull request: CLOUDSTACK-8302: Removing snapshots on RB...

Github user bhaisaab commented on the pull request: https://github.com/apache/cloudstack/pull/1230#issuecomment-175687764 @dmytro-shevchenko can you rebase against latest master (with 4.9xx as the version) etc. --- If your project is set up for it, you can reply to this email and

[GitHub] cloudstack pull request: CLOUDSTACK-8302: Removing snapshots on RB...

Github user remibergsma commented on the pull request: https://github.com/apache/cloudstack/pull/1230#issuecomment-169295971 Really wondering why you need to alter XenserverSnapshotStrategy.java for something to work on KVM, like @wido asked. Please give an answer on that one,

[GitHub] cloudstack pull request: CLOUDSTACK-8302: Removing snapshots on RB...

Github user davidamorimfaria commented on the pull request: https://github.com/apache/cloudstack/pull/1230#issuecomment-169104666 LGTM, but haven't had the chance to test it yet. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as

[GitHub] cloudstack pull request: CLOUDSTACK-8302: Removing snapshots on RB...

Github user wido commented on the pull request: https://github.com/apache/cloudstack/pull/1230#issuecomment-168988457 Any other LGTMs? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this

[GitHub] cloudstack pull request: CLOUDSTACK-8302: Removing snapshots on RB...

Github user wido commented on the pull request: https://github.com/apache/cloudstack/pull/1230#issuecomment-167965031 Tests seem good according to @borisroman Just looking into why the XenServer strategy had to be used. That should be all --- If your project is set up for it, you

[GitHub] cloudstack pull request: CLOUDSTACK-8302: Removing snapshots on RB...

Github user wido commented on the pull request: https://github.com/apache/cloudstack/pull/1230#issuecomment-167075932 ping @borisroman. I would like to see this one go in. Code-wise it looks good. --- If your project is set up for it, you can reply to this email and have your reply

[GitHub] cloudstack pull request: CLOUDSTACK-8302: Removing snapshots on RB...

Github user dmytro-shevchenko commented on the pull request: https://github.com/apache/cloudstack/pull/1230#issuecomment-166261539 Full snapshot path added into log messages. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as

[GitHub] cloudstack pull request: CLOUDSTACK-8302: Removing snapshots on RB...

Github user wido commented on the pull request: https://github.com/apache/cloudstack/pull/1230#issuecomment-166100502 Last code review is LGTM to me, but didn't have the time to test it yet. I *think* it works, but need to verify first. --- If your project is set up for it, you can

[GitHub] cloudstack pull request: CLOUDSTACK-8302: Removing snapshots on RB...

Github user wido commented on a diff in the pull request: https://github.com/apache/cloudstack/pull/1230#discussion_r48101631 --- Diff: plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/storage/KVMStorageProcessor.java --- @@ -1274,7 +1274,46 @@ public Answer

[GitHub] cloudstack pull request: CLOUDSTACK-8302: Removing snapshots on RB...

Github user wido commented on a diff in the pull request: https://github.com/apache/cloudstack/pull/1230#discussion_r48101628 --- Diff: plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/storage/KVMStorageProcessor.java --- @@ -1274,7 +1274,46 @@ public Answer

[GitHub] cloudstack pull request: CLOUDSTACK-8302: Removing snapshots on RB...

Github user remibergsma commented on the pull request: https://github.com/apache/cloudstack/pull/1230#issuecomment-166087210 @andrijapanic Merge without testing and LGTMs? Really? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub

[GitHub] cloudstack pull request: CLOUDSTACK-8302: Removing snapshots on RB...

Github user dmytro-shevchenko commented on a diff in the pull request: https://github.com/apache/cloudstack/pull/1230#discussion_r48097934 --- Diff: plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/storage/KVMStorageProcessor.java --- @@ -1274,7 +1274,45 @@ public Answer

[GitHub] cloudstack pull request: CLOUDSTACK-8302: Removing snapshots on RB...

Github user andrijapanic commented on the pull request: https://github.com/apache/cloudstack/pull/1230#issuecomment-165716739 Anyone has any more feedback ? Should we merge? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as

[GitHub] cloudstack pull request: CLOUDSTACK-8302: Removing snapshots on RB...

Github user wido commented on the pull request: https://github.com/apache/cloudstack/pull/1230#issuecomment-165717186 I'll see if I can run tests on it today with @borisroman on our dev setup --- If your project is set up for it, you can reply to this email and have your reply appear

[GitHub] cloudstack pull request: CLOUDSTACK-8302: Removing snapshots on RB...

Github user wido commented on a diff in the pull request: https://github.com/apache/cloudstack/pull/1230#discussion_r48005662 --- Diff: plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/storage/KVMStorageProcessor.java --- @@ -1274,7 +1274,45 @@ public Answer

[GitHub] cloudstack pull request: CLOUDSTACK-8302: Removing snapshots on RB...

Github user voloshanenko commented on a diff in the pull request: https://github.com/apache/cloudstack/pull/1230#discussion_r48025223 --- Diff: plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/storage/KVMStorageProcessor.java --- @@ -1274,7 +1274,45 @@ public Answer

[GitHub] cloudstack pull request: CLOUDSTACK-8302: Removing snapshots on RB...

Github user wido commented on a diff in the pull request: https://github.com/apache/cloudstack/pull/1230#discussion_r48026314 --- Diff: plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/storage/KVMStorageProcessor.java --- @@ -1274,7 +1274,45 @@ public Answer

[GitHub] cloudstack pull request: CLOUDSTACK-8302: Removing snapshots on RB...

Github user voloshanenko commented on the pull request: https://github.com/apache/cloudstack/pull/1230#issuecomment-165465027 Guys, i see that build hangs... Can you please re run them? --- If your project is set up for it, you can reply to this email and have your reply appear on