FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

CLOUDSTACK-5863: revert volume snapshot for KVM/QCOW2 by ustcweizhou · Pull Request #732 · apache/cloudstack · GitHub

  1. apache
  2. cloudstack

Repository navigation

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

Closed
ustcweizhou wants to merge 2 commits into
apache:masterfrom
ustcweizhou:revert-volume-snapshot-master
ConversationCommits (2)ChecksFiles changed
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 commented Aug 24, 2015

Copy link
Copy Markdown
Contributor

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 commented Aug 24, 2015

Copy link
Copy Markdown

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

asfbot commented Aug 24, 2015

Copy link
Copy Markdown

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

wido commented Aug 24, 2015

Copy link
Copy Markdown
Contributor

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 commented Aug 25, 2015

Copy link
Copy Markdown
Contributor

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 reviewed Aug 25, 2015
View reviewed changes
Comment thread api/src/org/apache/cloudstack/api/command/user/snapshot/RevertSnapshotCmd.java Outdated

yadvr Aug 25, 2015

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

this might cause API response issues.

yadvr commented Aug 25, 2015

Copy link
Copy Markdown
Member

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

karuturi commented Aug 26, 2015

Copy link
Copy Markdown
Member

Can you add some tests please?

asfbot commented Aug 28, 2015

Copy link
Copy Markdown

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

ustcweizhou commented Aug 28, 2015

Copy link
Copy Markdown
Contributor Author

@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 commented Aug 28, 2015

Copy link
Copy Markdown
Contributor

LGTM

asfbot commented Aug 28, 2015

Copy link
Copy Markdown

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

ustcweizhou commented Sep 1, 2015

Copy link
Copy Markdown
Contributor Author

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

wido commented Sep 1, 2015

Copy link
Copy Markdown
Contributor

No, seems good. We can merge this.

ustcweizhou closed this Sep 1, 2015
yadvr pushed a commit to shapeblue/cloudstack that referenced this pull request Sep 1, 2015
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 commented Sep 8, 2015

Copy link
Copy Markdown
Member

@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 mentioned this pull request Nov 22, 2020
Closed
yadvr pushed a commit that referenced this pull request Jan 20, 2021
Fixing form not closing on submit (#732)
bed8fab 
Signed-off-by: Rohit Yadav <rohit.yadav@shapeblue.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
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

Back | FazBrowse Home | New Git URL