CLOUDSTACK-8302: Removing snapshots on RBD #1230
Conversation
|
Thanks for implementing the removal code. Could you squash the commits and add a descriptive commit message? |
|
Commits are squashed and message updated. |
|
@dmytro-shevchenko Thank you, I'll give them a test run! |
|
Guys, i see that build hangs... Can you please re run them? |
|
Anyone has any more feedback ? Should we merge? |
|
I'll see if I can run tests on it today with @borisroman on our dev setup |
| } | ||
| } else { | ||
| s_logger.warn("Operation not implemented!"); | ||
| throw new InternalErrorException("Operation not implemented!"); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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..."
There was a problem hiding this comment.
For example:
"Operation not supported for storage pool type of NFS"
There was a problem hiding this comment.
Storage pool type added to messages.
|
@andrijapanic Merge without testing and LGTMs? Really? |
| 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."); |
There was a problem hiding this comment.
I would add the path of the snapshot here: pool/image@snap. Logs should be easy to consume
|
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. |
|
Full snapshot path added into log messages. |
|
ping @borisroman. I would like to see this one go in. Code-wise it looks good. |
|
Tests seem good according to @borisroman Just looking into why the XenServer strategy had to be used. That should be all |
|
Any other LGTMs? |
|
LGTM, but haven't had the chance to test it yet. |
|
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? |
|
@dmytro-shevchenko can you rebase against latest master (with 4.9xx as the version) etc. |
|
Done, rebased with 4.9xx master. |
| <dependency> | ||
| <groupId>org.apache.cloudstack</groupId> | ||
| <artifactId>cloud-engine-storage-volume</artifactId> | ||
| <version>4.9.0-SNAPSHOT</version> |
There was a problem hiding this comment.
this should read ${project.version} instead of 4.9.0-SNAPSHOT
There was a problem hiding this comment.
@dmytro-shevchenko please address this for forward compatibility. Thanks...
|
@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? |
|
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 |
|
I believe this is the reason Dmytro edit it, since it handles KVM stuff On 23 March 2016 at 14:40, Simon Weller notifications@github.com wrote:
Andrija Panić |
|
Yes, @kiwiflyer, the naming XenserverSnapshotStrategy appears to be old and no longer accurate. I believe it should be renamed to something like DefaultSnapshotStrategy. |
|
@mike-tutkowski @kiwiflyer So, it is just the class which has a incorrect name? The code is good? |
|
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: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 Ceph Log show snapshot has been created: rbd snap ls -p rbdnjcloudhost c656809e-ecec-47a0-875d-af297fb77fe3 Cloudstack delete RBD snashot job: 2016-04-11 11:34:03,679 DEBUG c.c.a.ApiServlet (logid:3f05f35f) ===START=== 10.16.0.38 -- GET command=deleteSnapshot&id=36e2eebe-8268-4c36-b21c-b2e57cb62974&response=json&=1460392443709 Ceph Log show snapshot has been deleted: rbd snap ls -p rbdnjcloudhost c656809e-ecec-47a0-875d-af297fb77fe3 |
|
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. |
|
@wido Yes, the name is no longer valid, but it should work just fine for KVM. |
|
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. |
|
Based on the tests I see above LGTM. A new PR afterwards to rename seems good. |
|
@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! |
Snapshot removing implemented on RBD. 1. On management side: when created new shanpshot we checking if our primary storage is RBD, then do not remove record from cloud.snapshot_store_ref with link to Ceph image via 'install_path' field. 2. On management side: when removing snapshot, also send command to agent 'DeleteCommand'. 3. On agent side: method implemented 'public Answer deleteSnapshot(final DeleteCommand cmd)'
|
Rebase with Master done, pom.xml file updated. |
|
With these changes I think we are good? |
|
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. |
|
@kiwiflyer Thank you sir. :) |
|
Manual testing against a 4.8 hardware lab with Ceph Primary Storage. 2 volume snapshots triggered: both snapshots are deleted Works as expected. Functional tests are good. |
|
tag:mergeready |
|
@kiwiflyer thank you. I will merge this today... |
CLOUDSTACK-8302: Removing snapshots on RBDSnapshot removing implemented if primary datastore is RBD https://issues.apache.org/jira/browse/CLOUDSTACK-8302 * pr/1230: CLOUDSTACK-8302 - Cleanup snapshot on KVM with RBD Snapshot removing implemented on RBD. 1. On management side: when created new shanpshot we checking if our primary storage is RBD, then do not remove record from cloud.snapshot_store_ref with link to Ceph image via 'install_path' field. 2. On management side: when removing snapshot, also send command to agent 'DeleteCommand'. 3. On agent side: method implemented 'public Answer deleteSnapshot(final DeleteCommand cmd)' Signed-off-by: Will Stevens <williamstevens@gmail.com>
….mail@gmail.com> Cleanup snapshot on KVM with RBD Snapshot removing implemented on RBD. 1. On management side: when created new shanpshot we checking if our primary storage is RBD, then do not remove record from cloud.snapshot_store_ref with link to Ceph image via 'install_path' field. 2. On management side: when removing snapshot, also send command to agent 'DeleteCommand'. 3. On agent side: method implemented 'public Answer deleteSnapshot(final DeleteCommand cmd)'
Snapshot removing implemented if primary datastore is RBD
https://issues.apache.org/jira/browse/CLOUDSTACK-8302