You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cloudstack.apache.org by dmytro-shevchenko <gi...@git.apache.org> on 2015/12/12 16:16:22 UTC

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

GitHub user dmytro-shevchenko opened a pull request:

    https://github.com/apache/cloudstack/pull/1230

    CLOUDSTACK-8302: Removing snapshots on RBD

    Snapshot removing implemented if primary datastore is RBD
    https://issues.apache.org/jira/browse/CLOUDSTACK-8302

You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/SafeSwissCloud/cloudstack CLOUDSTACK-8302

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/cloudstack/pull/1230.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

    This closes #1230
    
----
commit 32df243422d5985c8631686a0b2a29835f804079
Author: Amin <am...@gmail.com>
Date:   2015-11-30T13:39:50Z

    Merge pull request #1 from apache/master
    
    Volume snapshot lifecycle patch on RBD as primary storage

commit c4d749f3b41090c47d98431aef67fa769642c798
Author: Dmytro Shevchenko <ds...@gmail.com>
Date:   2015-12-10T15:07:58Z

    Merge from apache/master

commit a47b9300e414039734017fde0a41d162b54891f2
Author: Dmytro Shevchenko <ds...@gmail.com>
Date:   2015-12-12T14:44:09Z

    https://issues.apache.org/jira/browse/CLOUDSTACK-8302
    Snapshot removing implemented on RBD

----


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

Posted by voloshanenko <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

Posted by asfgit <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

Posted by kiwiflyer <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

Posted by kiwiflyer <gi...@git.apache.org>.
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&STOREUUID=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&STOREUUID=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&STOREUUID=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 NAME                                    SIZE 
        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&id=36e2eebe-8268-4c36-b21c-b2e57cb62974&response=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 com.cloud.storage.Snapshot\":\"36e2eebe-8268-4c36-b21c-b2e57cb62974\"}","cmdEventType":"SNAPSHOT.DELETE","ctxUserId":"2","httpmethod":"GET","_":"1460392443709","uuid":"36e2eebe-8268-4c36-b21c-b2e57cb62974","ctxAccountId":"2","ctxStartEventId":"284"}, cmdVersion: 0, status: IN_PROGRESS, processStatus: 0, resultCode: 0, result: null, initMsid: 52239507206, completeMsid: null, lastUpdated: null, lastPolled: null, created: null}
    2016-04-11 11:34:03,731 DEBUG [c.c.a.ApiServlet] (catalina-exec-5:ctx-954c6978 ctx-02ab09fe) (logid:3f05f35f) ===END===  10.16.0.38 -- GET  command=deleteSnapshot&id=36e2eebe-8268-4c36-b21c-b2e57cb62974&response=json&_=1460392443709
    2016-04-11 11:34:03,732 INFO  [o.a.c.f.j.i.AsyncJobMonitor] (API-Job-Executor-2:ctx-8c9478b9 job-123) (logid:5f0157a8) Add job-123 into job monitoring
    2016-04-11 11:34:03,734 DEBUG [o.a.c.f.j.i.AsyncJobManagerImpl] (API-Job-Executor-2:ctx-8c9478b9 job-123) (logid:b6ce6bc3) Executing 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 com.cloud.storage.Snapshot\":\"36e2eebe-8268-4c36-b21c-b2e57cb62974\"}","cmdEventType":"SNAPSHOT.DELETE","ctxUserId":"2","httpmethod":"GET","_":"1460392443709","uuid":"36e2eebe-8268-4c36-b21c-b2e57cb62974","ctxAccountId":"2","ctxStartEventId":"284"}, cmdVersion: 0, status: IN_PROGRESS, processStatus: 0, resultCode: 0, result: null, initMsid: 52239507206, completeMsid: null, lastUpdated: null, lastPolled: null, created: null}
    2016-04-11 11:34:03,758 DEBUG [o.a.c.s.s.XenserverSnapshotStrategy] (API-Job-Executor-2:ctx-8c9478b9 job-123 ctx-c230036e) (logid:b6ce6bc3) delete snapshot chain for snapshot: 1
    2016-04-11 11:34:03,759 DEBUG [o.a.c.s.s.XenserverSnapshotStrategy] (API-Job-Executor-2:ctx-8c9478b9 job-123 ctx-c230036e) (logid:b6ce6bc3) Snapshot: 1 doesn't have children, so it's ok to delete it and its parents
    2016-04-11 11:34:03,770 DEBUG [c.c.h.o.r.Ovm3HypervisorGuru] (API-Job-Executor-2:ctx-8c9478b9 job-123 ctx-c230036e) (logid:b6ce6bc3) getCommandHostDelegation: class org.apache.cloudstack.storage.command.DeleteCommand
    2016-04-11 11:34:03,770 DEBUG [c.c.h.XenServerGuru] (API-Job-Executor-2:ctx-8c9478b9 job-123 ctx-c230036e) (logid:b6ce6bc3) getCommandHostDelegation: class org.apache.cloudstack.storage.command.DeleteCommand
    2016-04-11 11:34:03,772 DEBUG [c.c.a.t.Request] (API-Job-Executor-2:ctx-8c9478b9 job-123 ctx-c230036e) (logid:b6ce6bc3) Seq 3-826973481575907354: Sending  { Cmd , MgmtId: 52239507206, via: 3(s-1-VM), Ver: v1, Flags: 100011, [{"org.apache.cloudstack.storage.command.DeleteCommand":{"data":{"org.apache.cloudstack.storage.to.SnapshotObjectTO":{"path":"snapshots/4/8/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&STOREUUID=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","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}},"wait":0}}] }
    2016-04-11 11:34:04,438 DEBUG [c.c.a.t.Request] (AgentManager-Handler-5:null) (logid:) Seq 3-826973481575907354: Processing:  { Ans: , MgmtId: 52239507206, via: 3, Ver: v1, Flags: 10, [{"com.cloud.agent.api.Answer":{"result":true,"wait":0}}] }
    2016-04-11 11:34:04,439 DEBUG [c.c.a.t.Request] (API-Job-Executor-2:ctx-8c9478b9 job-123 ctx-c230036e) (logid:b6ce6bc3) Seq 3-826973481575907354: Received:  { Ans: , MgmtId: 52239507206, via: 3(s-1-VM), Ver: v1, Flags: 10, { Answer } }
    2016-04-11 11:34:04,489 DEBUG [c.c.h.o.r.Ovm3HypervisorGuru] (API-Job-Executor-2:ctx-8c9478b9 job-123 ctx-c230036e) (logid:b6ce6bc3) getCommandHostDelegation: class org.apache.cloudstack.storage.command.DeleteCommand
    2016-04-11 11:34:04,489 DEBUG [c.c.h.XenServerGuru] (API-Job-Executor-2:ctx-8c9478b9 job-123 ctx-c230036e) (logid:b6ce6bc3) getCommandHostDelegation: class org.apache.cloudstack.storage.command.DeleteCommand
    2016-04-11 11:34:04,490 DEBUG [c.c.a.t.Request] (API-Job-Executor-2:ctx-8c9478b9 job-123 ctx-c230036e) (logid:b6ce6bc3) Seq 1-1541075497490841823: Sending  { Cmd , MgmtId: 52239507206, via: 1(njcloudhost.dev.ena.net), Ver: v1, Flags: 100011, [{"org.apache.cloudstack.storage.command.DeleteCommand":{"data":{"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&STOREUUID=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","provisioni
 ngType":"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&STOREUUID=f0dbafaa-52a3-3077-bc53-d7ad3a5ac132"}},"vmName":"i-4-8-VM","name":"test","hypervisorType":"KVM","id":1,"quiescevm":false,"physicalSize":0}},"wait":0}}] }
    2016-04-11 11:34:04,853 DEBUG [c.c.a.t.Request] (AgentManager-Handler-6:null) (logid:) Seq 1-1541075497490841823: Processing:  { Ans: , MgmtId: 52239507206, via: 1, Ver: v1, Flags: 10, [{"com.cloud.agent.api.Answer":{"result":true,"details":"Snapshot c656809e-ecec-47a0-875d-af297fb77fe3@938125ff-da6d-4355-b83a-e3aa8d941807 removed successfully.","wait":0}}] }
    2016-04-11 11:34:04,853 DEBUG [c.c.a.t.Request] (API-Job-Executor-2:ctx-8c9478b9 job-123 ctx-c230036e) (logid:b6ce6bc3) Seq 1-1541075497490841823: Received:  { Ans: , MgmtId: 52239507206, via: 1(njcloudhost.dev.ena.net), Ver: v1, Flags: 10, { Answer } }
    2016-04-11 11:34:04,894 DEBUG [o.a.c.f.j.i.AsyncJobManagerImpl] (API-Job-Executor-2:ctx-8c9478b9 job-123 ctx-c230036e) (logid:b6ce6bc3) Complete async job-123, jobStatus: SUCCEEDED, resultCode: 0, result: org.apache.cloudstack.api.response.SuccessResponse/null/{"success":true}
    2016-04-11 11:34:04,894 DEBUG [o.a.c.f.j.i.AsyncJobManagerImpl] (API-Job-Executor-2:ctx-8c9478b9 job-123 ctx-c230036e) (logid:b6ce6bc3) Publish async job-123 complete on message bus
    2016-04-11 11:34:04,895 DEBUG [o.a.c.f.j.i.AsyncJobManagerImpl] (API-Job-Executor-2:ctx-8c9478b9 job-123 ctx-c230036e) (logid:b6ce6bc3) Wake up jobs related to job-123
    2016-04-11 11:34:04,895 DEBUG [o.a.c.f.j.i.AsyncJobManagerImpl] (API-Job-Executor-2:ctx-8c9478b9 job-123 ctx-c230036e) (logid:b6ce6bc3) Update db status for job-123
    2016-04-11 11:34:04,895 DEBUG [o.a.c.f.j.i.AsyncJobManagerImpl] (API-Job-Executor-2:ctx-8c9478b9 job-123 ctx-c230036e) (logid:b6ce6bc3) Wake up jobs joined with job-123 and disjoin all subjobs created from job- 123
    2016-04-11 11:34:04,899 DEBUG [o.a.c.f.j.i.AsyncJobManagerImpl] (API-Job-Executor-2:ctx-8c9478b9 job-123) (logid:b6ce6bc3) Done executing org.apache.cloudstack.api.command.user.snapshot.DeleteSnapshotCmd for job-123
    2016-04-11 11:34:04,899 INFO  [o.a.c.f.j.i.AsyncJobMonitor] (API-Job-Executor-2:ctx-8c9478b9 job-123) (logid:b6ce6bc3) Remove job-123 from job monitoring
    
    
    Ceph Log show snapshot has been deleted:
    
    rbd snap ls -p rbdnjcloudhost c656809e-ecec-47a0-875d-af297fb77fe3
    <no 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

Posted by wido <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

Posted by mike-tutkowski <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

Posted by mike-tutkowski <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

Posted by DaanHoogland <gi...@git.apache.org>.
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 @@
               <type>test-jar</type>
               <scope>test</scope>
           </dependency>
    +      <dependency>
    +          <groupId>org.apache.cloudstack</groupId>
    +          <artifactId>cloud-engine-storage-volume</artifactId>
    +          <version>4.9.0-SNAPSHOT</version>
    --- 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

Posted by kiwiflyer <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

Posted by wido <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

Posted by davidamorimfaria <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

Posted by wido <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

Posted by wido <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

Posted by wido <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

Posted by dmytro-shevchenko <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

Posted by swill <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

Posted by wido <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

Posted by wido <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

Posted by dmytro-shevchenko <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

Posted by rhtyd <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

Posted by wido <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

Posted by borisroman <gi...@git.apache.org>.
Github user borisroman commented on the pull request:

    https://github.com/apache/cloudstack/pull/1230#issuecomment-164161973
  
    Hi @dmytro-shevchenko
    
    Thanks for implementing the removal code. Could you squash the commits and add a descriptive commit message?


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

Posted by voloshanenko <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

Posted by swill <gi...@git.apache.org>.
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 @@
               <type>test-jar</type>
               <scope>test</scope>
           </dependency>
    +      <dependency>
    +          <groupId>org.apache.cloudstack</groupId>
    +          <artifactId>cloud-engine-storage-volume</artifactId>
    +          <version>4.9.0-SNAPSHOT</version>
    --- 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

Posted by remibergsma <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

Posted by kiwiflyer <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

Posted by borisroman <gi...@git.apache.org>.
Github user borisroman commented on the pull request:

    https://github.com/apache/cloudstack/pull/1230#issuecomment-164464276
  
    @dmytro-shevchenko Thank you, I'll give them a test run!


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

Posted by kiwiflyer <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

Posted by andrijapanic <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

Posted by wido <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

Posted by andrijapanic <gi...@git.apache.org>.
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 <no...@github.com> wrote:
    
    > @DaanHoogland <https://github.com/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 <https://github.com/mike-tutkowski> mentioned
    > something to this effect very recently in this PR: #1441
    > <https://github.com/apache/cloudstack/pull/1441>
    >
    > —
    > You are receiving this because you were mentioned.
    > Reply to this email directly or view it on GitHub
    > <https://github.com/apache/cloudstack/pull/1230#issuecomment-200347989>
    >
    
    
    
    -- 
    
    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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

Posted by wido <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

Posted by mike-tutkowski <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

Posted by DaanHoogland <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

Posted by dmytro-shevchenko <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

Posted by kiwiflyer <gi...@git.apache.org>.
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 NAME                                    SIZE 
        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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

Posted by remibergsma <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

Posted by bhaisaab <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

Posted by wido <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

Posted by wido <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

Posted by dmytro-shevchenko <gi...@git.apache.org>.
Github user dmytro-shevchenko commented on the pull request:

    https://github.com/apache/cloudstack/pull/1230#issuecomment-164459723
  
    Commits are squashed and message updated.


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

Posted by dmytro-shevchenko <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

Posted by swill <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---