You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cloudstack.apache.org by ustcweizhou <gi...@git.apache.org> on 2015/08/24 11:11:45 UTC
[GitHub] cloudstack pull request: CLOUDSTACK-5863: revert volume snapshot f...
GitHub user ustcweizhou opened a pull request:
https://github.com/apache/cloudstack/pull/732
CLOUDSTACK-5863: revert volume snapshot for KVM/QCOW2
Guys, can you review it? things need to be discussed:
(1) this supports KVM/QCOW2 only. Anyone want to implement for other Hypervisor/format ?
(2) The original data volume (on primary storage) will be removed.
(3) The script uses the default timeout in libvirtComputingResource. Do we need to add one in global configuration (like copy.volume.wait or backup.snapshot.wait, create.volume.from.snapshot.wait)
(4) In scripts/storage/qcow2/managesnapshot.sh, I use "qemu-img convert -f qcow2 -O qcow2" to copy the snapshot from secondary to primary (hence there is no base image file), instead of "cp -f", this is because convert is faster than cp in my testing.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/ustcweizhou/cloudstack revert-volume-snapshot-master
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/cloudstack/pull/732.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 #732
----
commit 92344c006dfa8abf4ce81e2a25839cbb762d1b17
Author: Wei Zhou <w....@tech.leaseweb.com>
Date: 2015-08-24T09:01:50Z
CLOUDSTACK-5863: revert volume snapshot for KVM/QCOW2
----
---
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-5863: revert volume snapshot f...
Posted by wido <gi...@git.apache.org>.
Github user wido commented on the pull request:
https://github.com/apache/cloudstack/pull/732#issuecomment-134179676
LGTM to me.
We should however stay as far away as possible from invoking all kinds of scripts.
Implementing this for RBD is also a lot easier since you laid some groundwork. The java RBD bindings should be able to do this.
---
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-5863: revert volume snapshot f...
Posted by ustcweizhou <gi...@git.apache.org>.
Github user ustcweizhou commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/732#discussion_r37963222
--- Diff: api/src/org/apache/cloudstack/api/command/user/snapshot/RevertSnapshotCmd.java ---
@@ -31,25 +31,36 @@
import org.apache.cloudstack.api.response.SnapshotResponse;
import org.apache.cloudstack.api.response.SuccessResponse;
import org.apache.cloudstack.context.CallContext;
+import org.apache.log4j.Logger;
import com.cloud.event.EventTypes;
import com.cloud.storage.Snapshot;
import com.cloud.user.Account;
-@APICommand(name = "revertSnapshot", description = "revert a volume snapshot.", responseObject = SnapshotResponse.class, entityType = {Snapshot.class},
+@APICommand(name = "revertSnapshot", description = "revert a volume snapshot.", responseObject = SuccessResponse.class, entityType = {Snapshot.class},
--- End diff --
I will change it to SnapshotResponse for backward compatibility.
---
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-5863: revert volume snapshot f...
Posted by ustcweizhou <gi...@git.apache.org>.
Github user ustcweizhou commented on the pull request:
https://github.com/apache/cloudstack/pull/732#issuecomment-135661166
@bhaisaab I updated this PR . The reponses is changed to SnapshotResponse for backward compatibility.
@karuturi I will add some unit tests for snapshot (create, delete,revert) in another PR.
---
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-5863: revert volume snapshot f...
Posted by karuturi <gi...@git.apache.org>.
Github user karuturi commented on the pull request:
https://github.com/apache/cloudstack/pull/732#issuecomment-134820562
Can you add some tests please?
---
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-5863: revert volume snapshot f...
Posted by karuturi <gi...@git.apache.org>.
Github user karuturi commented on the pull request:
https://github.com/apache/cloudstack/pull/732#issuecomment-138426187
@ustcweizhou this caused a new coverity issue. Can you check?
```
411 SnapshotInfo snapshotOnPrimaryStore = _snapshotFactory.getSnapshot(snapshot.getId(), DataStoreRole.Primary);
>>> CID 1323172: Null pointer dereferences (NULL_RETURNS)
>>> Calling a method on null object "snapshotOnPrimaryStore".
412 PrimaryDataStore store = (PrimaryDataStore)snapshotOnPrimaryStore.getDataStore();
```
---
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-5863: revert volume snapshot f...
Posted by bhaisaab <gi...@git.apache.org>.
Github user bhaisaab commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/732#discussion_r37854506
--- Diff: api/src/org/apache/cloudstack/api/command/user/snapshot/RevertSnapshotCmd.java ---
@@ -91,7 +102,6 @@ public void execute() {
boolean result = _snapshotService.revertSnapshot(getId());
if (result) {
SuccessResponse response = new SuccessResponse(getCommandName());
- response.setResponseName(getCommandName());
--- End diff --
this might cause API response issues.
---
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-5863: revert volume snapshot f...
Posted by ustcweizhou <gi...@git.apache.org>.
Github user ustcweizhou commented on the pull request:
https://github.com/apache/cloudstack/pull/732#issuecomment-136705849
any other comment about this PR ?
If no, I will merge it into 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-5863: revert volume snapshot f...
Posted by bhaisaab <gi...@git.apache.org>.
Github user bhaisaab commented on the pull request:
https://github.com/apache/cloudstack/pull/732#issuecomment-134558876
In general LGTM, please see if you could fix the response class and object of the API (pl see the comments).
---
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-5863: revert volume snapshot f...
Posted by milamberspace <gi...@git.apache.org>.
Github user milamberspace commented on the pull request:
https://github.com/apache/cloudstack/pull/732#issuecomment-134553590
LGTM to me too.
Tested on a master test deployment (Ubuntu 14.04 / KVM / NFS)
Success with a manual snapshot from cloudmonkey and web ui
Success on bad condition (error message if the VM is running)
---
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-5863: revert volume snapshot f...
Posted by ustcweizhou <gi...@git.apache.org>.
Github user ustcweizhou closed the pull request at:
https://github.com/apache/cloudstack/pull/732
---
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-5863: revert volume snapshot f...
Posted by wido <gi...@git.apache.org>.
Github user wido commented on the pull request:
https://github.com/apache/cloudstack/pull/732#issuecomment-136708467
No, seems good. We can merge this.
---
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-5863: revert volume snapshot f...
Posted by bhaisaab <gi...@git.apache.org>.
Github user bhaisaab commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/732#discussion_r37854560
--- Diff: api/src/org/apache/cloudstack/api/command/user/snapshot/RevertSnapshotCmd.java ---
@@ -31,25 +31,36 @@
import org.apache.cloudstack.api.response.SnapshotResponse;
import org.apache.cloudstack.api.response.SuccessResponse;
import org.apache.cloudstack.context.CallContext;
+import org.apache.log4j.Logger;
import com.cloud.event.EventTypes;
import com.cloud.storage.Snapshot;
import com.cloud.user.Account;
-@APICommand(name = "revertSnapshot", description = "revert a volume snapshot.", responseObject = SnapshotResponse.class, entityType = {Snapshot.class},
+@APICommand(name = "revertSnapshot", description = "revert a volume snapshot.", responseObject = SuccessResponse.class, entityType = {Snapshot.class},
--- End diff --
I think for API backward compatibility, please revert this to send SnapshotResponse as the responseobject, unless this really needs to be fixed (the older implementation got it wrong). Please comment.
---
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-5863: revert volume snapshot f...
Posted by wido <gi...@git.apache.org>.
Github user wido commented on the pull request:
https://github.com/apache/cloudstack/pull/732#issuecomment-135667378
LGTM
---
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.
---