You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cloudstack.apache.org by GitBox <gi...@apache.org> on 2020/04/15 19:12:19 UTC
[GitHub] [cloudstack] ggoodrich-ipp opened a new pull request #4032: Suspending the VM prior to deleting snapshots to avoid corruption, th…
ggoodrich-ipp opened a new pull request #4032: Suspending the VM prior to deleting snapshots to avoid corruption, th…
URL: https://github.com/apache/cloudstack/pull/4032
…en resuming
## Description
<!--- Describe your changes in detail -->
These changes are related to PR #3194, but include suspending/resuming the VM when doing a VM snapshot as well, when deleting a VM snapshot, as it is performing the same operations via Libvirt. Also, there was an issue with the UI/localization changes in the prior PR, as that PR was altering the Volume snapshot behavior, but was altering the VM snapshot wording. Both have been altered in this PR.
Issuing this in response to the work happening in PR #4029.
<!-- For new features, provide link to FS, dev ML discussion etc. -->
<!-- In case of bug fix, the expected and actual behaviours, steps to reproduce. -->
<!-- When "Fixes: #<id>" is specified, the issue/PR will automatically be closed when this PR gets merged -->
<!-- For addressing multiple issues/PRs, use multiple "Fixes: #<id>" -->
<!-- Fixes: # -->
## Types of changes
<!--- What types of changes does your code introduce? Put an `x` in all the boxes that apply: -->
- [ ] Breaking change (fix or feature that would cause existing functionality to change)
- [ ] New feature (non-breaking change which adds functionality)
- [x] Bug fix (non-breaking change which fixes an issue)
- [x] Enhancement (improves an existing feature and functionality)
- [ ] Cleanup (Code refactoring and cleanup, that may add test cases)
## Screenshots (if appropriate):
## How Has This Been Tested?
<!-- Please describe in detail how you tested your changes. -->
<!-- Include details of your testing environment, and the tests you ran to -->
<!-- see how your change affects other areas of the code, etc. -->
<!-- Please read the [CONTRIBUTING](https://github.com/apache/cloudstack/blob/master/CONTRIBUTING.md) document -->
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [cloudstack] rhtyd commented on a change in pull request #4032: Suspending the VM prior to deleting snapshots to avoid corruption, th…
Posted by GitBox <gi...@apache.org>.
rhtyd commented on a change in pull request #4032: Suspending the VM prior to deleting snapshots to avoid corruption, th…
URL: https://github.com/apache/cloudstack/pull/4032#discussion_r409490687
##########
File path: plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtDeleteVMSnapshotCommandWrapper.java
##########
@@ -51,14 +52,28 @@ public Answer execute(final DeleteVMSnapshotCommand cmd, final LibvirtComputingR
final KVMStoragePoolManager storagePoolMgr = libvirtComputingResource.getStoragePoolMgr();
Domain dm = null;
DomainSnapshot snapshot = null;
+ DomainInfo.DomainState state = null;
+ boolean didDelete = false;
+ Connect conn = null;
try {
final LibvirtUtilitiesHelper libvirtUtilitiesHelper = libvirtComputingResource.getLibvirtUtilitiesHelper();
- Connect conn = libvirtUtilitiesHelper.getConnection();
+ conn = libvirtUtilitiesHelper.getConnection();
dm = libvirtComputingResource.getDomain(conn, vmName);
snapshot = dm.snapshotLookupByName(cmd.getTarget().getSnapshotName());
+ // Suspend the VM prior to deleting the snapshot to guard against corruption
+ dm.suspend();
Review comment:
Should we check if VM is running? (i.e. not in any other states)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [cloudstack] ggoodrich-ipp commented on a change in pull request #4032: Suspending the VM prior to deleting snapshots to avoid corruption, th…
Posted by GitBox <gi...@apache.org>.
ggoodrich-ipp commented on a change in pull request #4032: Suspending the VM prior to deleting snapshots to avoid corruption, th…
URL: https://github.com/apache/cloudstack/pull/4032#discussion_r409551472
##########
File path: plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtDeleteVMSnapshotCommandWrapper.java
##########
@@ -93,12 +108,26 @@ public Answer execute(final DeleteVMSnapshotCommand cmd, final LibvirtComputingR
} else if (snapshot == null) {
s_logger.debug("Can not find vm snapshot " + cmd.getTarget().getSnapshotName() + " on vm: " + vmName + ", return true");
return new DeleteVMSnapshotAnswer(cmd, cmd.getVolumeTOs());
+ } else if (didDelete) {
+ s_logger.error("Failed to resume vm after delete snapshot " + cmd.getTarget().getSnapshotName() + " on vm: " + vmName + " return true : " + e);
+ return new DeleteVMSnapshotAnswer(cmd, cmd.getVolumeTOs());
}
s_logger.warn(msg, e);
return new DeleteVMSnapshotAnswer(cmd, false, msg);
} finally {
if (dm != null) {
+ // Make sure if the VM is paused, then resume it, in case we got an exception during our delete() and didn't have the chance before
+ try {
+ dm = libvirtComputingResource.getDomain(conn, vmName);
Review comment:
I was under the impression that dm needed to be refreshed after a state change, as I saw this same thing being done in KVMStorageProcessor -> backupSnapshot(). Rather than validate that it was necessary or not, I decided that safer is always better, as refreshing the domain variable won't cause harm.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [cloudstack] rhtyd commented on a change in pull request #4032: Suspending the VM prior to deleting snapshots to avoid corruption, th…
Posted by GitBox <gi...@apache.org>.
rhtyd commented on a change in pull request #4032: Suspending the VM prior to deleting snapshots to avoid corruption, th…
URL: https://github.com/apache/cloudstack/pull/4032#discussion_r409490834
##########
File path: plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtDeleteVMSnapshotCommandWrapper.java
##########
@@ -51,14 +52,28 @@ public Answer execute(final DeleteVMSnapshotCommand cmd, final LibvirtComputingR
final KVMStoragePoolManager storagePoolMgr = libvirtComputingResource.getStoragePoolMgr();
Domain dm = null;
DomainSnapshot snapshot = null;
+ DomainInfo.DomainState state = null;
+ boolean didDelete = false;
+ Connect conn = null;
try {
final LibvirtUtilitiesHelper libvirtUtilitiesHelper = libvirtComputingResource.getLibvirtUtilitiesHelper();
- Connect conn = libvirtUtilitiesHelper.getConnection();
+ conn = libvirtUtilitiesHelper.getConnection();
dm = libvirtComputingResource.getDomain(conn, vmName);
snapshot = dm.snapshotLookupByName(cmd.getTarget().getSnapshotName());
+ // Suspend the VM prior to deleting the snapshot to guard against corruption
+ dm.suspend();
Review comment:
May wrap in a try-with?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [cloudstack] andrijapanicsb commented on issue #4032: Suspending the VM prior to deleting snapshots to avoid corruption, th…
Posted by GitBox <gi...@apache.org>.
andrijapanicsb commented on issue #4032: Suspending the VM prior to deleting snapshots to avoid corruption, th…
URL: https://github.com/apache/cloudstack/pull/4032#issuecomment-614597521
@weizhouapache does this one makes more sense/checks than the #4033 - i.e. shall we work on this one?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [cloudstack] DaanHoogland commented on issue #4032: Suspending the VM prior to deleting snapshots to avoid corruption, th…
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on issue #4032: Suspending the VM prior to deleting snapshots to avoid corruption, th…
URL: https://github.com/apache/cloudstack/pull/4032#issuecomment-614609895
as we are in release phase already and #4033 is simpler let's go with that. the logic is essentially the same.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [cloudstack] DaanHoogland commented on issue #4032: Suspending the VM prior to deleting snapshots to avoid corruption, th…
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on issue #4032: Suspending the VM prior to deleting snapshots to avoid corruption, th…
URL: https://github.com/apache/cloudstack/pull/4032#issuecomment-614598596
@ggoodrich-ipp my suggestion there ^^ won't catch all possible conflicts, and won't give us a full mutex, but for the case we are handling now it should do. (cc @weizhouapache @rhtyd again)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [cloudstack] blueorangutan commented on issue #4032: Suspending the VM prior to deleting snapshots to avoid corruption, th…
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on issue #4032: Suspending the VM prior to deleting snapshots to avoid corruption, th…
URL: https://github.com/apache/cloudstack/pull/4032#issuecomment-614585867
@andrijapanicsb a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [cloudstack] DaanHoogland commented on a change in pull request #4032: Suspending the VM prior to deleting snapshots to avoid corruption, th…
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on a change in pull request #4032: Suspending the VM prior to deleting snapshots to avoid corruption, th…
URL: https://github.com/apache/cloudstack/pull/4032#discussion_r409492530
##########
File path: plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtDeleteVMSnapshotCommandWrapper.java
##########
@@ -51,14 +52,28 @@ public Answer execute(final DeleteVMSnapshotCommand cmd, final LibvirtComputingR
final KVMStoragePoolManager storagePoolMgr = libvirtComputingResource.getStoragePoolMgr();
Domain dm = null;
DomainSnapshot snapshot = null;
+ DomainInfo.DomainState state = null;
+ boolean didDelete = false;
+ Connect conn = null;
try {
final LibvirtUtilitiesHelper libvirtUtilitiesHelper = libvirtComputingResource.getLibvirtUtilitiesHelper();
- Connect conn = libvirtUtilitiesHelper.getConnection();
+ conn = libvirtUtilitiesHelper.getConnection();
dm = libvirtComputingResource.getDomain(conn, vmName);
snapshot = dm.snapshotLookupByName(cmd.getTarget().getSnapshotName());
+ // Suspend the VM prior to deleting the snapshot to guard against corruption
+ dm.suspend();
Review comment:
does this mean you agree with my statement above, @rhtyd ?
the try catch won't work, it will succeed leaving a corrupt disk image (according to @andrijapanicsb 's tests)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [cloudstack] andrijapanicsb commented on issue #4032: Suspending the VM prior to deleting snapshots to avoid corruption, th…
Posted by GitBox <gi...@apache.org>.
andrijapanicsb commented on issue #4032: Suspending the VM prior to deleting snapshots to avoid corruption, th…
URL: https://github.com/apache/cloudstack/pull/4032#issuecomment-614585418
@blueorangutan package
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [cloudstack] rhtyd commented on a change in pull request #4032: Suspending the VM prior to deleting snapshots to avoid corruption, th…
Posted by GitBox <gi...@apache.org>.
rhtyd commented on a change in pull request #4032: Suspending the VM prior to deleting snapshots to avoid corruption, th…
URL: https://github.com/apache/cloudstack/pull/4032#discussion_r409492088
##########
File path: plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtDeleteVMSnapshotCommandWrapper.java
##########
@@ -93,12 +108,26 @@ public Answer execute(final DeleteVMSnapshotCommand cmd, final LibvirtComputingR
} else if (snapshot == null) {
s_logger.debug("Can not find vm snapshot " + cmd.getTarget().getSnapshotName() + " on vm: " + vmName + ", return true");
return new DeleteVMSnapshotAnswer(cmd, cmd.getVolumeTOs());
+ } else if (didDelete) {
+ s_logger.error("Failed to resume vm after delete snapshot " + cmd.getTarget().getSnapshotName() + " on vm: " + vmName + " return true : " + e);
+ return new DeleteVMSnapshotAnswer(cmd, cmd.getVolumeTOs());
}
s_logger.warn(msg, e);
return new DeleteVMSnapshotAnswer(cmd, false, msg);
} finally {
if (dm != null) {
+ // Make sure if the VM is paused, then resume it, in case we got an exception during our delete() and didn't have the chance before
+ try {
+ dm = libvirtComputingResource.getDomain(conn, vmName);
Review comment:
dm is already not null, why overwrite it again?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [cloudstack] blueorangutan commented on issue #4032: Suspending the VM prior to deleting snapshots to avoid corruption, th…
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on issue #4032: Suspending the VM prior to deleting snapshots to avoid corruption, th…
URL: https://github.com/apache/cloudstack/pull/4032#issuecomment-614598203
Packaging result: ✔centos7 ✔debian. JID-1172
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services