[GitHub] cloudstack pull request: CLOUDSTACK-5863: revert volume snapshot f...
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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: CLOUDSTACK-5863: revert volume snapshot f...
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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: CLOUDSTACK-5863: revert volume snapshot f...
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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: CLOUDSTACK-5863: revert volume snapshot f...
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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: CLOUDSTACK-5863: revert volume snapshot f...
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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: CLOUDSTACK-5863: revert volume snapshot f...
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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: CLOUDSTACK-5863: revert volume snapshot f...
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-5863: revert volume snapshot f...
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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: CLOUDSTACK-5863: revert volume snapshot f...
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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: CLOUDSTACK-5863: revert volume snapshot f...
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-5863: revert volume snapshot f...
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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: CLOUDSTACK-5863: revert volume snapshot f...
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-5863: revert volume snapshot f...
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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[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.z...@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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
