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

[asfgit]

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 is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[swill]

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 your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[rhtyd]

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 feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[kiwiflyer]

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 cabf9a9e-7ac3-44b5-a3bb-02b820bed376
SNAPID NAMESIZE 
70 6efbba74-372c-4bd7-95bb-e539524e7209 5120 MB 
71 de6acd1b-ccac-4d3f-80aa-b40b05b86a63 5120 MB 

both snapshots are deleted
rbd snap ls -p rbdnjcloudhost cabf9a9e-7ac3-44b5-a3bb-02b820bed376
(no results as expected)

Works as expected. Functional tests 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 project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[swill]

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 not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[kiwiflyer]

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 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 is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[wido]

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 project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[dmytro-shevchenko]

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' table all snapshots from one volume was linked 
between each other as Parent->Child using field 'parent_snapshot_id'. If you 
removing one of previous snapshot and wait for 'storage.cleanup.interval' 
period,  it lead to NullPointerException when you creating new snapshot, 
because Cloudstack trying to build all this snapshot relations before. Before 
this patch this field was always set to '0' (no parent). From Cloudstack point 
of view all snapshots on Ceph not connected (Ceph care about this on his own 
level). 
So, in file 
engine/storage/snapshot/src/org/apache/cloudstack/storage/snapshot/XenserverSnapshotStrategy.java:
 
I moved this block:
`SnapshotDataStoreVO snapshotDataStoreVO = 
snapshotStoreDao.findByStoreSnapshot(primaryStore.getRole(), 
primaryStore.getId(), snapshot.getId());
if (snapshotDataStoreVO != null) {
snapshotDataStoreVO.setParentSnapshotId(0L);
snapshotStoreDao.update(snapshotDataStoreVO.getId(), 
snapshotDataStoreVO);
}`
from the condition: ...primaryStore).getPoolType() != StoragePoolType.RBD
and it will be executed in any way, as previously. Please review this part 
of code, if this is good solution.


---
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 is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[kiwiflyer]

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 rebase as there is a conflict?

Thanks!


---
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 is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[wido]

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 your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[mike-tutkowski]

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 name?

If not, I can submit a PR for that on 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 project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[mike-tutkowski]

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 your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[kiwiflyer]

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 XenserverSnapshotStrategy look to be 
valid for KVM.


---
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 is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[kiwiflyer]

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 job-121/job-122 ctx-e3c6a9f8) 
(logid:af23718c) Seq 1-1541075497490841731: Received:  { Ans: , MgmtId: 
52239507206, via: 1(njcloudhost.dev.ena.net), Ver: v1, Flags: 10, { 
CreateObjectAnswer } }
2016-04-11 11:26:23,025 DEBUG [o.a.c.s.m.AncientDataMotionStrategy] 
(Work-Job-Executor-1:ctx-5ac9d6c8 job-121/job-122 ctx-e3c6a9f8) 
(logid:af23718c) copyAsync inspecting src type SNAPSHOT copyAsync inspecting 
dest type SNAPSHOT
2016-04-11 11:26:23,056 DEBUG [c.c.h.o.r.Ovm3HypervisorGuru] 
(Work-Job-Executor-1:ctx-5ac9d6c8 job-121/job-122 ctx-e3c6a9f8) 
(logid:af23718c) getCommandHostDelegation: class 
org.apache.cloudstack.storage.command.CopyCommand
2016-04-11 11:26:23,056 DEBUG [c.c.h.XenServerGuru] 
(Work-Job-Executor-1:ctx-5ac9d6c8 job-121/job-122 ctx-e3c6a9f8) 
(logid:af23718c) getCommandHostDelegation: class 
org.apache.cloudstack.storage.command.CopyCommand
2016-04-11 11:26:23,058 DEBUG [c.c.a.t.Request] 
(Work-Job-Executor-1:ctx-5ac9d6c8 job-121/job-122 ctx-e3c6a9f8) 
(logid:af23718c) Seq 1-1541075497490841732: Sending  { Cmd , MgmtId: 
52239507206, via: 1(njcloudhost.dev.ena.net), Ver: v1, Flags: 100111, 
[{"org.apache.cloudstack.storage.command.CopyCommand":{"srcTO":{"org.apache.cloudstack.storage.to.SnapshotObjectTO":{"path":"rbdnjcloudhost/c656809e-ecec-47a0-875d-af297fb77fe3/938125ff-da6d-4355-b83a-e3aa8d941807","volume":{"uuid":"c656809e-ecec-47a0-875d-af297fb77fe3","volumeType":"ROOT","dataStore":{"org.apache.cloudstack.storage.to.PrimaryDataStoreTO":{"uuid":"f0dbafaa-52a3-3077-bc53-d7ad3a5ac132","id":4,"poolType":"RBD","host":"192.168.100.100","path":"rbdnjcloudhost","port":6789,"url":"RBD://192.168.100.100/rbdnjcloudhost/?ROLE=Primary=f0dbafaa-52a3-3077-bc53-d7ad3a5ac132"}},"name":"ROOT-8","size":8589934592,"path":"c656809e-ecec-47a0-875d-af297fb77fe3","volumeId":8,"vmName":"i-4-8-VM","accountId":4,"format":"RAW","pr
 
ovisioningType":"THIN","id":8,"deviceId":0,"hypervisorType":"KVM"},"dataStore":{"org.apache.cloudstack.storage.to.PrimaryDataStoreTO":{"uuid":"f0dbafaa-52a3-3077-bc53-d7ad3a5ac132","id":4,"poolType":"RBD","host":"192.168.100.100","path":"rbdnjcloudhost","port":6789,"url":"RBD://192.168.100.100/rbdnjcloudhost/?ROLE=Primary=f0dbafaa-52a3-3077-bc53-d7ad3a5ac132"}},"vmName":"i-4-8-VM","name":"test","hypervisorType":"KVM","id":1,"quiescevm":false,"physicalSize":0}},"destTO":{"org.apache.cloudstack.storage.to.SnapshotObjectTO":{"path":"snapshots/4/8","volume":{"uuid":"c656809e-ecec-47a0-875d-af297fb77fe3","volumeType":"ROOT","dataStore":{"org.apache.cloudstack.storage.to.PrimaryDataStoreTO":{"uuid":"f0dbafaa-52a3-3077-bc53-d7ad3a5ac132","id":4,"poolType":"RBD","host":"192.168.100.100","path":"rbdnjcloudhost","port":6789,"url":"RBD://192.168.100.100/rbdnjcloudhost/?ROLE=Primary=f0dbafaa-52a3-3077-bc53-d7ad3a5ac132"}},"name":"ROOT-8","size":8589934592,"path":"c656809e-ec
 
ec-47a0-875d-af297fb77fe3","volumeId":8,"vmName":"i-4-8-VM","accountId":4,"format":"RAW","provisioningType":"THIN","id":8,"deviceId":0,"hypervisorType":"KVM"},"dataStore":{"com.cloud.agent.api.to.NfsTO":{"_url":"nfs://10.103.0.42/secondary","_role":"Image"}},"vmName":"i-4-8-VM","name":"test","hypervisorType":"KVM","id":1,"quiescevm":false,"physicalSize":0}},"executeInSequence":true,"options":{"fullSnapshot":"true"},"options2":{},"wait":21600}}]
 }


2016-04-11 11:27:41,178 DEBUG [o.a.c.f.j.i.AsyncJobManagerImpl] 
(API-Job-Executor-1:ctx-80f166f9 job-121) (logid:af23718c) Done executing 
org.apache.cloudstack.api.command.user.snapshot.CreateSnapshotCmd for job-121
2016-04-11 11:27:41,178 INFO  [o.a.c.f.j.i.AsyncJobMonitor] 
(API-Job-Executor-1:ctx-80f166f9 job-121) (logid:af23718c) Remove job-121 from 
job monitoring

Ceph Log show snapshot has been created:

rbd snap ls -p rbdnjcloudhost c656809e-ecec-47a0-875d-af297fb77fe3
SNAPID NAMESIZE 
12 938125ff-da6d-4355-b83a-e3aa8d941807 8192 MB 


Cloudstack delete RBD snashot job:

2016-04-11 11:34:03,679 DEBUG [c.c.a.ApiServlet] 
(catalina-exec-5:ctx-954c6978) (logid:3f05f35f) ===START===  10.16.0.38 -- GET  
command=deleteSnapshot=36e2eebe-8268-4c36-b21c-b2e57cb62974=json&_=1460392443709
2016-04-11 11:34:03,730 DEBUG [o.a.c.f.j.i.AsyncJobManagerImpl] 
(catalina-exec-5:ctx-954c6978 ctx-02ab09fe) (logid:3f05f35f) submit async 
job-123, details: AsyncJobVO {id:123, userId: 2, accountId: 2, instanceType: 
Snapshot, instanceId: 1, cmd: 
org.apache.cloudstack.api.command.user.snapshot.DeleteSnapshotCmd, cmdInfo: 
{"id":"36e2eebe-8268-4c36-b21c-b2e57cb62974","response":"json","ctxDetails":"{\"interface
 

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

[wido]

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 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 is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[swill]

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
   
+  
+  org.apache.cloudstack
+  cloud-engine-storage-volume
+  4.9.0-SNAPSHOT
--- End diff --

@dmytro-shevchenko please address this for forward compatibility.  Thanks...


---
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 is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[mike-tutkowski]

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 
DefaultSnapshotStrategy.


---
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 is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[andrijapanic]

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  wrote:

> @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 description of
> what this class is actually doing. It seems to be very general and based 
on
> some investigation being used for KVM as well. This might be an old
> artifact of the original feature being Xen specific, until others
> re-purposed it for other hypervisors.
>
> Mike @mike-tutkowski  mentioned
> something to this effect very recently in this PR: #1441
> 
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly or view it on GitHub
> 
>



-- 

Andrija Panić



---
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 is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[kiwiflyer]

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 description of what 
this class is actually doing. It seems to be very general and based on some 
investigation being used for KVM as well. This might be an old artifact of the 
original feature being Xen specific, until others re-purposed it for other 
hypervisors.

Mike @mike-tutkowski mentioned something to this effect very recently in 
this PR: https://github.com/apache/cloudstack/pull/1441


---
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 is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[DaanHoogland]

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 you are solving this for two hypervisors half. Can you explain what you 
encountered in the code to come up with this solution?


---
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 is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[DaanHoogland]

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
   
+  
+  org.apache.cloudstack
+  cloud-engine-storage-volume
+  4.9.0-SNAPSHOT
--- End diff --

this shopuld read ${project.version} instead of 4.9.0-SNAPSHOT


---
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 is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[dmytro-shevchenko]

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 project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[bhaisaab]

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 have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[remibergsma]

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, 
thanks!

Also, could someone run the integration tests and post the results?


---
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 is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[davidamorimfaria]

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 well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[wido]

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 feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[wido]

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 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 is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[wido]

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 appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[dmytro-shevchenko]

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 well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[wido]

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 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 is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[wido]

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 createVolumeFromSnapshot(final 
CopyCommand cmd) {
 
 @Override
 public Answer deleteSnapshot(final DeleteCommand cmd) {
-return new Answer(cmd);
+try {
+SnapshotObjectTO snapshotTO = (SnapshotObjectTO) cmd.getData();
+PrimaryDataStoreTO primaryStore = (PrimaryDataStoreTO) 
snapshotTO.getDataStore();
+VolumeObjectTO volume = snapshotTO.getVolume();
+String snapshotFullPath = snapshotTO.getPath();
+String snapshotName = 
snapshotFullPath.substring(snapshotFullPath.lastIndexOf("/") + 1);
+KVMStoragePool primaryPool = 
storagePoolMgr.getStoragePool(primaryStore.getPoolType(), 
primaryStore.getUuid());
+KVMPhysicalDisk disk = 
storagePoolMgr.getPhysicalDisk(primaryStore.getPoolType(), 
primaryStore.getUuid(), volume.getPath());
+if (primaryPool.getType() == StoragePoolType.RBD) {
+Rados r = new Rados(primaryPool.getAuthUserName());
+r.confSet("mon_host", primaryPool.getSourceHost() + ":" + 
primaryPool.getSourcePort());
+r.confSet("key", primaryPool.getAuthSecret());
+r.confSet("client_mount_timeout", "30");
+r.connect();
+s_logger.debug("Succesfully connected to Ceph cluster at " 
+ r.confGet("mon_host"));
+IoCTX io = r.ioCtxCreate(primaryPool.getSourceDir());
+Rbd rbd = new Rbd(io);
+RbdImage image = rbd.open(disk.getName());
+try {
+s_logger.info("Attempting to remove RBD snapshot " + 
disk.getName() + "@" + snapshotName);
+if (image.snapIsProtected(snapshotName)) {
+s_logger.debug("Unprotecting RBD snapshot " + 
snapshotFullPath);
+image.snapUnprotect(snapshotName);
+}
+image.snapRemove(snapshotName);
+s_logger.info("Snapshot " + snapshotFullPath + " 
successfully removed from " +
+primaryPool.getType().toString() + "  pool.");
+} finally {
+rbd.close(image);
+r.ioCtxDestroy(io);
+}
+} else {
+s_logger.warn("Operation not implemented for storage pool 
type of " + primaryPool.getType().toString());
+throw new InternalErrorException("Operation not 
implemented for storage pool type of " + primaryPool.getType().toString());
+}
+return new Answer(cmd, true, "Snapshot removed successfully.");
+} catch (Exception e) {
+s_logger.error(e.getMessage());
+return new Answer(cmd, false, "Failed to remove snapshot!");
--- End diff --

Same here. Tell which snapshot couldn't be removed


---
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 is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[wido]

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 createVolumeFromSnapshot(final 
CopyCommand cmd) {
 
 @Override
 public Answer deleteSnapshot(final DeleteCommand cmd) {
-return new Answer(cmd);
+try {
+SnapshotObjectTO snapshotTO = (SnapshotObjectTO) cmd.getData();
+PrimaryDataStoreTO primaryStore = (PrimaryDataStoreTO) 
snapshotTO.getDataStore();
+VolumeObjectTO volume = snapshotTO.getVolume();
+String snapshotFullPath = snapshotTO.getPath();
+String snapshotName = 
snapshotFullPath.substring(snapshotFullPath.lastIndexOf("/") + 1);
+KVMStoragePool primaryPool = 
storagePoolMgr.getStoragePool(primaryStore.getPoolType(), 
primaryStore.getUuid());
+KVMPhysicalDisk disk = 
storagePoolMgr.getPhysicalDisk(primaryStore.getPoolType(), 
primaryStore.getUuid(), volume.getPath());
+if (primaryPool.getType() == StoragePoolType.RBD) {
+Rados r = new Rados(primaryPool.getAuthUserName());
+r.confSet("mon_host", primaryPool.getSourceHost() + ":" + 
primaryPool.getSourcePort());
+r.confSet("key", primaryPool.getAuthSecret());
+r.confSet("client_mount_timeout", "30");
+r.connect();
+s_logger.debug("Succesfully connected to Ceph cluster at " 
+ r.confGet("mon_host"));
+IoCTX io = r.ioCtxCreate(primaryPool.getSourceDir());
+Rbd rbd = new Rbd(io);
+RbdImage image = rbd.open(disk.getName());
+try {
+s_logger.info("Attempting to remove RBD snapshot " + 
disk.getName() + "@" + snapshotName);
+if (image.snapIsProtected(snapshotName)) {
+s_logger.debug("Unprotecting RBD snapshot " + 
snapshotFullPath);
+image.snapUnprotect(snapshotName);
+}
+image.snapRemove(snapshotName);
+s_logger.info("Snapshot " + snapshotFullPath + " 
successfully removed from " +
+primaryPool.getType().toString() + "  pool.");
+} finally {
+rbd.close(image);
+r.ioCtxDestroy(io);
+}
+} else {
+s_logger.warn("Operation not implemented for storage pool 
type of " + primaryPool.getType().toString());
+throw new InternalErrorException("Operation not 
implemented for storage pool type of " + primaryPool.getType().toString());
+}
+return new Answer(cmd, true, "Snapshot removed successfully.");
--- End diff --

I would add the path of the snapshot here: pool/image@snap. Logs should be 
easy to consume


---
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 is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[remibergsma]

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 as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[dmytro-shevchenko]

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 createVolumeFromSnapshot(final 
CopyCommand cmd) {
 
 @Override
 public Answer deleteSnapshot(final DeleteCommand cmd) {
-return new Answer(cmd);
+try {
+SnapshotObjectTO snapshotTO = (SnapshotObjectTO) cmd.getData();
+PrimaryDataStoreTO primaryStore = (PrimaryDataStoreTO) 
snapshotTO.getDataStore();
+VolumeObjectTO volume = snapshotTO.getVolume();
+String snapshotFullPath = snapshotTO.getPath();
+String snapshotName = 
snapshotFullPath.substring(snapshotFullPath.lastIndexOf("/") + 1);
+KVMStoragePool primaryPool = 
storagePoolMgr.getStoragePool(primaryStore.getPoolType(), 
primaryStore.getUuid());
+KVMPhysicalDisk disk = 
storagePoolMgr.getPhysicalDisk(primaryStore.getPoolType(), 
primaryStore.getUuid(), volume.getPath());
+if (primaryPool.getType() == StoragePoolType.RBD) {
+Rados r = new Rados(primaryPool.getAuthUserName());
+r.confSet("mon_host", primaryPool.getSourceHost() + ":" + 
primaryPool.getSourcePort());
+r.confSet("key", primaryPool.getAuthSecret());
+r.confSet("client_mount_timeout", "30");
+r.connect();
+s_logger.debug("Succesfully connected to Ceph cluster at " 
+ r.confGet("mon_host"));
+IoCTX io = r.ioCtxCreate(primaryPool.getSourceDir());
+Rbd rbd = new Rbd(io);
+RbdImage image = rbd.open(disk.getName());
+try {
+s_logger.info("Attempting to remove RBD snapshot " + 
disk.getName() + "@" + snapshotName);
+if (image.snapIsProtected(snapshotName)) {
+s_logger.debug("Unprotecting snapshot " + 
snapshotFullPath);
+image.snapUnprotect(snapshotName);
+}
+image.snapRemove(snapshotName);
+s_logger.info("Snapshot " + snapshotFullPath + " 
successfully removed.");
+} finally {
+rbd.close(image);
+r.ioCtxDestroy(io);
+}
+} else {
+s_logger.warn("Operation not implemented!");
+throw new InternalErrorException("Operation not 
implemented!");
--- End diff --

Storage pool type added to messages.


---
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 is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[andrijapanic]

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 well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[wido]

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 on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[wido]

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 createVolumeFromSnapshot(final 
CopyCommand cmd) {
 
 @Override
 public Answer deleteSnapshot(final DeleteCommand cmd) {
-return new Answer(cmd);
+try {
+SnapshotObjectTO snapshotTO = (SnapshotObjectTO) cmd.getData();
+PrimaryDataStoreTO primaryStore = (PrimaryDataStoreTO) 
snapshotTO.getDataStore();
+VolumeObjectTO volume = snapshotTO.getVolume();
+String snapshotFullPath = snapshotTO.getPath();
+String snapshotName = 
snapshotFullPath.substring(snapshotFullPath.lastIndexOf("/") + 1);
+KVMStoragePool primaryPool = 
storagePoolMgr.getStoragePool(primaryStore.getPoolType(), 
primaryStore.getUuid());
+KVMPhysicalDisk disk = 
storagePoolMgr.getPhysicalDisk(primaryStore.getPoolType(), 
primaryStore.getUuid(), volume.getPath());
+if (primaryPool.getType() == StoragePoolType.RBD) {
+Rados r = new Rados(primaryPool.getAuthUserName());
+r.confSet("mon_host", primaryPool.getSourceHost() + ":" + 
primaryPool.getSourcePort());
+r.confSet("key", primaryPool.getAuthSecret());
+r.confSet("client_mount_timeout", "30");
+r.connect();
+s_logger.debug("Succesfully connected to Ceph cluster at " 
+ r.confGet("mon_host"));
+IoCTX io = r.ioCtxCreate(primaryPool.getSourceDir());
+Rbd rbd = new Rbd(io);
+RbdImage image = rbd.open(disk.getName());
+try {
+s_logger.info("Attempting to remove RBD snapshot " + 
disk.getName() + "@" + snapshotName);
+if (image.snapIsProtected(snapshotName)) {
+s_logger.debug("Unprotecting snapshot " + 
snapshotFullPath);
+image.snapUnprotect(snapshotName);
+}
+image.snapRemove(snapshotName);
+s_logger.info("Snapshot " + snapshotFullPath + " 
successfully removed.");
+} finally {
+rbd.close(image);
+r.ioCtxDestroy(io);
+}
+} else {
+s_logger.warn("Operation not implemented!");
+throw new InternalErrorException("Operation not 
implemented!");
--- End diff --

Can we also add the type of storage pool in this message? Now this line 
doesn't say much. snapshotTO and primaryStore should contain all the 
information you need


---
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 is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[voloshanenko]

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 createVolumeFromSnapshot(final 
CopyCommand cmd) {
 
 @Override
 public Answer deleteSnapshot(final DeleteCommand cmd) {
-return new Answer(cmd);
+try {
+SnapshotObjectTO snapshotTO = (SnapshotObjectTO) cmd.getData();
+PrimaryDataStoreTO primaryStore = (PrimaryDataStoreTO) 
snapshotTO.getDataStore();
+VolumeObjectTO volume = snapshotTO.getVolume();
+String snapshotFullPath = snapshotTO.getPath();
+String snapshotName = 
snapshotFullPath.substring(snapshotFullPath.lastIndexOf("/") + 1);
+KVMStoragePool primaryPool = 
storagePoolMgr.getStoragePool(primaryStore.getPoolType(), 
primaryStore.getUuid());
+KVMPhysicalDisk disk = 
storagePoolMgr.getPhysicalDisk(primaryStore.getPoolType(), 
primaryStore.getUuid(), volume.getPath());
+if (primaryPool.getType() == StoragePoolType.RBD) {
+Rados r = new Rados(primaryPool.getAuthUserName());
+r.confSet("mon_host", primaryPool.getSourceHost() + ":" + 
primaryPool.getSourcePort());
+r.confSet("key", primaryPool.getAuthSecret());
+r.confSet("client_mount_timeout", "30");
+r.connect();
+s_logger.debug("Succesfully connected to Ceph cluster at " 
+ r.confGet("mon_host"));
+IoCTX io = r.ioCtxCreate(primaryPool.getSourceDir());
+Rbd rbd = new Rbd(io);
+RbdImage image = rbd.open(disk.getName());
+try {
+s_logger.info("Attempting to remove RBD snapshot " + 
disk.getName() + "@" + snapshotName);
+if (image.snapIsProtected(snapshotName)) {
+s_logger.debug("Unprotecting snapshot " + 
snapshotFullPath);
+image.snapUnprotect(snapshotName);
+}
+image.snapRemove(snapshotName);
+s_logger.info("Snapshot " + snapshotFullPath + " 
successfully removed.");
+} finally {
+rbd.close(image);
+r.ioCtxDestroy(io);
+}
+} else {
+s_logger.warn("Operation not implemented!");
+throw new InternalErrorException("Operation not 
implemented!");
--- End diff --

Wido, what do your mean? Improve s_logger.info messages? And include 
shared/dedicated message for pool type? Because if you mean RBD pool type - 
it's already included into message " .. remove RBD sn..."


---
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 is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[wido]

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 createVolumeFromSnapshot(final 
CopyCommand cmd) {
 
 @Override
 public Answer deleteSnapshot(final DeleteCommand cmd) {
-return new Answer(cmd);
+try {
+SnapshotObjectTO snapshotTO = (SnapshotObjectTO) cmd.getData();
+PrimaryDataStoreTO primaryStore = (PrimaryDataStoreTO) 
snapshotTO.getDataStore();
+VolumeObjectTO volume = snapshotTO.getVolume();
+String snapshotFullPath = snapshotTO.getPath();
+String snapshotName = 
snapshotFullPath.substring(snapshotFullPath.lastIndexOf("/") + 1);
+KVMStoragePool primaryPool = 
storagePoolMgr.getStoragePool(primaryStore.getPoolType(), 
primaryStore.getUuid());
+KVMPhysicalDisk disk = 
storagePoolMgr.getPhysicalDisk(primaryStore.getPoolType(), 
primaryStore.getUuid(), volume.getPath());
+if (primaryPool.getType() == StoragePoolType.RBD) {
+Rados r = new Rados(primaryPool.getAuthUserName());
+r.confSet("mon_host", primaryPool.getSourceHost() + ":" + 
primaryPool.getSourcePort());
+r.confSet("key", primaryPool.getAuthSecret());
+r.confSet("client_mount_timeout", "30");
+r.connect();
+s_logger.debug("Succesfully connected to Ceph cluster at " 
+ r.confGet("mon_host"));
+IoCTX io = r.ioCtxCreate(primaryPool.getSourceDir());
+Rbd rbd = new Rbd(io);
+RbdImage image = rbd.open(disk.getName());
+try {
+s_logger.info("Attempting to remove RBD snapshot " + 
disk.getName() + "@" + snapshotName);
+if (image.snapIsProtected(snapshotName)) {
+s_logger.debug("Unprotecting snapshot " + 
snapshotFullPath);
+image.snapUnprotect(snapshotName);
+}
+image.snapRemove(snapshotName);
+s_logger.info("Snapshot " + snapshotFullPath + " 
successfully removed.");
+} finally {
+rbd.close(image);
+r.ioCtxDestroy(io);
+}
+} else {
+s_logger.warn("Operation not implemented!");
+throw new InternalErrorException("Operation not 
implemented!");
--- End diff --

For example:

"Operation not supported for storage pool type of NFS"


---
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 is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[voloshanenko]

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 GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---