Skip to content

CLOUDSTACK-5863: revert volume snapshot for KVM/QCOW2#732

Closed
ustcweizhou wants to merge 2 commits into
apache:masterfrom
ustcweizhou:revert-volume-snapshot-master
Closed

CLOUDSTACK-5863: revert volume snapshot for KVM/QCOW2#732
ustcweizhou wants to merge 2 commits into
apache:masterfrom
ustcweizhou:revert-volume-snapshot-master

Conversation

@ustcweizhou
Copy link
Copy Markdown
Contributor

@ustcweizhou ustcweizhou commented Aug 24, 2015

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.

@asfbot
Copy link
Copy Markdown

asfbot commented Aug 24, 2015

cloudstack-pull-rats #376 SUCCESS
This pull request looks good

@asfbot
Copy link
Copy Markdown

asfbot commented Aug 24, 2015

cloudstack-pull-analysis #309 SUCCESS
This pull request looks good

@wido
Copy link
Copy Markdown
Contributor

wido commented Aug 24, 2015

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.

@milamberspace
Copy link
Copy Markdown
Contributor

milamberspace commented Aug 25, 2015

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)

yadvr
yadvr reviewed Aug 25, 2015
View reviewed changes
Comment thread api/src/org/apache/cloudstack/api/command/user/snapshot/RevertSnapshotCmd.java Outdated
Copy link
Copy Markdown
Member

@yadvr yadvr Aug 25, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this might cause API response issues.

@yadvr
Copy link
Copy Markdown
Member

yadvr commented Aug 25, 2015

In general LGTM, please see if you could fix the response class and object of the API (pl see the comments).

@karuturi
Copy link
Copy Markdown
Member

karuturi commented Aug 26, 2015

Can you add some tests please?

@asfbot
Copy link
Copy Markdown

asfbot commented Aug 28, 2015

cloudstack-pull-rats #432 SUCCESS
This pull request looks good

@ustcweizhou
Copy link
Copy Markdown
Contributor Author

ustcweizhou commented Aug 28, 2015

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

@wido
Copy link
Copy Markdown
Contributor

wido commented Aug 28, 2015

LGTM

@asfbot
Copy link
Copy Markdown

asfbot commented Aug 28, 2015

cloudstack-pull-analysis #365 FAILURE
Looks like there's a problem with this pull request

@ustcweizhou
Copy link
Copy Markdown
Contributor Author

ustcweizhou commented Sep 1, 2015

any other comment about this PR ?
If no, I will merge it into master.

@wido
Copy link
Copy Markdown
Contributor

wido commented Sep 1, 2015

No, seems good. We can merge this.

@ustcweizhou ustcweizhou closed this Sep 1, 2015
yadvr pushed a commit to shapeblue/cloudstack that referenced this pull request Sep 1, 2015
@ustcweizhou
Merge pull request apache#732 from ustcweizhou/revert-volume-snapshot…
c0a0aec 
…-master

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.

* pr/732:
  CLOUDSTACK-5863: revert volume snapshot for KVM/QCOW2

Signed-off-by: Wei Zhou <w.zhou@tech.leaseweb.com>
@karuturi
Copy link
Copy Markdown
Member

karuturi commented Sep 8, 2015

@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();

@weizhouapache weizhouapache mentioned this pull request Nov 22, 2020
Closed
yadvr pushed a commit that referenced this pull request Jan 20, 2021
@davidjumani @yadvr
Fixing form not closing on submit (#732)
bed8fab 
Signed-off-by: Rohit Yadav <rohit.yadav@shapeblue.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@ustcweizhou @asfbot @wido @milamberspace @yadvr @karuturi