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 2021/02/19 08:45:57 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:
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)
- [ ] 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
[GitHub] [cloudstack] ggoodrich-ipp commented on pull request #4032: Suspending the VM prior to deleting snapshots to avoid corruption, th…
Posted by GitBox <gi...@apache.org>.
ggoodrich-ipp commented on pull request #4032:
URL: https://github.com/apache/cloudstack/pull/4032#issuecomment-767046339
> @ggoodrich-ipp what issue does this pr solve ?
> Delete a vm snapshot twice ?
Well, I think another PR managed to get in there to suspend the VM, but this PR is specifically around suspending, then resuming the VM when doing a snapshot.
----------------------------------------------------------------
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
[GitHub] [cloudstack] rhtyd closed pull request #4032: Suspending the VM prior to deleting snapshots to avoid corruption, th…
Posted by GitBox <gi...@apache.org>.
rhtyd closed pull request #4032:
URL: https://github.com/apache/cloudstack/pull/4032
----------------------------------------------------------------
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
[GitHub] [cloudstack] weizhouapache commented on pull request #4032: Suspending the VM prior to deleting snapshots to avoid corruption, th…
Posted by GitBox <gi...@apache.org>.
weizhouapache commented on pull request #4032:
URL: https://github.com/apache/cloudstack/pull/4032#issuecomment-770751087
@ggoodrich-ipp could you please fix the build error ?
----------------------------------------------------------------
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
[GitHub] [cloudstack] blueorangutan commented on pull request #4032: Suspending the VM prior to deleting snapshots to avoid corruption, th…
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4032:
URL: https://github.com/apache/cloudstack/pull/4032#issuecomment-783787706
<b>Trillian test result (tid-3594)</b>
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 30418 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4032-t3594-kvm-centos7.zip
Smoke tests completed. 86 look OK, 0 have error(s)
Only failed tests results shown below:
Test | Result | Time (s) | Test File
--- | --- | --- | ---
----------------------------------------------------------------
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
[GitHub] [cloudstack] weizhouapache 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>.
weizhouapache commented on a change in pull request #4032:
URL: https://github.com/apache/cloudstack/pull/4032#discussion_r567104868
##########
File path: plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtDeleteVMSnapshotCommandWrapper.java
##########
@@ -52,18 +52,33 @@ public Answer execute(final DeleteVMSnapshotCommand cmd, final LibvirtComputingR
final KVMStoragePoolManager storagePoolMgr = libvirtComputingResource.getStoragePoolMgr();
Domain dm = null;
DomainSnapshot snapshot = null;
+ DomainInfo.DomainState oldState = null;
Review comment:
@ggoodrich-ipp
should it be "DomainState" instead of "DomainInfo.DomainState" ?
----------------------------------------------------------------
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
[GitHub] [cloudstack] blueorangutan commented on pull request #4032: Suspending the VM prior to deleting snapshots to avoid corruption, th…
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4032:
URL: https://github.com/apache/cloudstack/pull/4032#issuecomment-643063240
@rhtyd 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
[GitHub] [cloudstack] rhtyd merged pull request #4032: Suspending the VM prior to deleting snapshots to avoid corruption, th…
Posted by GitBox <gi...@apache.org>.
rhtyd merged pull request #4032:
URL: https://github.com/apache/cloudstack/pull/4032
----------------------------------------------------------------
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
[GitHub] [cloudstack] blueorangutan commented on pull request #4032: Suspending the VM prior to deleting snapshots to avoid corruption, th…
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4032:
URL: https://github.com/apache/cloudstack/pull/4032#issuecomment-781988877
@rhtyd 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
[GitHub] [cloudstack] blueorangutan commented on pull request #4032: Suspending the VM prior to deleting snapshots to avoid corruption, th…
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4032:
URL: https://github.com/apache/cloudstack/pull/4032#issuecomment-783302934
Packaging result: ✖centos7 ✖centos8 ✖debian. JID-2780
----------------------------------------------------------------
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
[GitHub] [cloudstack] rhtyd closed pull request #4032: Suspending the VM prior to deleting snapshots to avoid corruption, th…
Posted by GitBox <gi...@apache.org>.
rhtyd closed pull request #4032:
URL: https://github.com/apache/cloudstack/pull/4032
----------------------------------------------------------------
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
[GitHub] [cloudstack] rhtyd commented on pull request #4032: Suspending the VM prior to deleting snapshots to avoid corruption, th…
Posted by GitBox <gi...@apache.org>.
rhtyd commented on pull request #4032:
URL: https://github.com/apache/cloudstack/pull/4032#issuecomment-642598175
@ggoodrich-ipp can you fix the conflicts, thanks.
----------------------------------------------------------------
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
[GitHub] [cloudstack] ggoodrich-ipp commented on pull request #4032: Suspending the VM prior to deleting snapshots to avoid corruption, th…
Posted by GitBox <gi...@apache.org>.
ggoodrich-ipp commented on pull request #4032:
URL: https://github.com/apache/cloudstack/pull/4032#issuecomment-767046339
> @ggoodrich-ipp what issue does this pr solve ?
> Delete a vm snapshot twice ?
Well, I think another PR managed to get in there to suspend the VM, but this PR is specifically around suspending, then resuming the VM when doing a snapshot.
----------------------------------------------------------------
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
[GitHub] [cloudstack] DaanHoogland commented on pull request #4032: Suspending the VM prior to deleting snapshots to avoid corruption, th…
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on pull request #4032:
URL: https://github.com/apache/cloudstack/pull/4032#issuecomment-769822134
travis is reporting a checkstyle error but i cannot find it closing
----------------------------------------------------------------
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
[GitHub] [cloudstack] shwstppr commented on pull request #4032: Suspending the VM prior to deleting snapshots to avoid corruption, th…
Posted by GitBox <gi...@apache.org>.
shwstppr commented on pull request #4032:
URL: https://github.com/apache/cloudstack/pull/4032#issuecomment-783473914
@blueorangutan test
----------------------------------------------------------------
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
[GitHub] [cloudstack] blueorangutan commented on pull request #4032: Suspending the VM prior to deleting snapshots to avoid corruption, th…
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4032:
URL: https://github.com/apache/cloudstack/pull/4032#issuecomment-783474364
@shwstppr a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke 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
[GitHub] [cloudstack] blueorangutan commented on pull request #4032: Suspending the VM prior to deleting snapshots to avoid corruption, th…
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4032:
URL: https://github.com/apache/cloudstack/pull/4032#issuecomment-783257187
@shwstppr 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
[GitHub] [cloudstack] DaanHoogland commented on pull request #4032: Suspending the VM prior to deleting snapshots to avoid corruption, th…
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on pull request #4032:
URL: https://github.com/apache/cloudstack/pull/4032#issuecomment-769822337
re-opening to re-kick travis
----------------------------------------------------------------
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
[GitHub] [cloudstack] shwstppr commented on pull request #4032: Suspending the VM prior to deleting snapshots to avoid corruption, th…
Posted by GitBox <gi...@apache.org>.
shwstppr commented on pull request #4032:
URL: https://github.com/apache/cloudstack/pull/4032#issuecomment-783256306
@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
[GitHub] [cloudstack] shwstppr commented on pull request #4032: Suspending the VM prior to deleting snapshots to avoid corruption, th…
Posted by GitBox <gi...@apache.org>.
shwstppr commented on pull request #4032:
URL: https://github.com/apache/cloudstack/pull/4032#issuecomment-766714686
@ggoodrich-ipp can you please resolve conflicts
----------------------------------------------------------------
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
[GitHub] [cloudstack] ggoodrich-ipp commented on pull request #4032: Suspending the VM prior to deleting snapshots to avoid corruption, th…
Posted by GitBox <gi...@apache.org>.
ggoodrich-ipp commented on pull request #4032:
URL: https://github.com/apache/cloudstack/pull/4032#issuecomment-642671150
> @ggoodrich-ipp can you fix the conflicts, thanks.
Conflicts resolved.
----------------------------------------------------------------
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
[GitHub] [cloudstack] ggoodrich-ipp commented on pull request #4032: Suspending the VM prior to deleting snapshots to avoid corruption, th…
Posted by GitBox <gi...@apache.org>.
ggoodrich-ipp commented on pull request #4032:
URL: https://github.com/apache/cloudstack/pull/4032#issuecomment-646168701
> @ggoodrich-ipp could you check if `quiescevm` is true in the `createSnapshot` API? Would that be better than doing it by default?
That seems questionable, due to the potential for corruption, in this case. I wouldn't think we would want an option that could lead to corruption.
----------------------------------------------------------------
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
[GitHub] [cloudstack] rhtyd commented on pull request #4032: Suspending the VM prior to deleting snapshots to avoid corruption, th…
Posted by GitBox <gi...@apache.org>.
rhtyd commented on pull request #4032:
URL: https://github.com/apache/cloudstack/pull/4032#issuecomment-784116493
@weizhouapache have you done any manual testing on this?
----------------------------------------------------------------
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
[GitHub] [cloudstack] blueorangutan commented on pull request #4032: Suspending the VM prior to deleting snapshots to avoid corruption, th…
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4032:
URL: https://github.com/apache/cloudstack/pull/4032#issuecomment-781928084
@rhtyd 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
[GitHub] [cloudstack] ggoodrich-ipp commented on pull request #4032: Suspending the VM prior to deleting snapshots to avoid corruption, th…
Posted by GitBox <gi...@apache.org>.
ggoodrich-ipp commented on pull request #4032:
URL: https://github.com/apache/cloudstack/pull/4032#issuecomment-756424559
@DaanHoogland @rhtyd I have made some adjustments to this PR. Thoughts?
----------------------------------------------------------------
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
[GitHub] [cloudstack] blueorangutan commented on pull request #4032: Suspending the VM prior to deleting snapshots to avoid corruption, th…
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4032:
URL: https://github.com/apache/cloudstack/pull/4032#issuecomment-643078058
Packaging result: ✖centos7 ✖debian. JID-1353
----------------------------------------------------------------
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
[GitHub] [cloudstack] blueorangutan commented on pull request #4032: Suspending the VM prior to deleting snapshots to avoid corruption, th…
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4032:
URL: https://github.com/apache/cloudstack/pull/4032#issuecomment-781933852
Packaging result: ✖centos7 ✖centos8 ✖debian. JID-2745
----------------------------------------------------------------
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
[GitHub] [cloudstack] weizhouapache commented on pull request #4032: Suspending the VM prior to deleting snapshots to avoid corruption, th…
Posted by GitBox <gi...@apache.org>.
weizhouapache commented on pull request #4032:
URL: https://github.com/apache/cloudstack/pull/4032#issuecomment-766808374
@ggoodrich-ipp what issue does this pr solve ?
Delete a vm snapshot twice ?
----------------------------------------------------------------
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
[GitHub] [cloudstack] DaanHoogland closed pull request #4032: Suspending the VM prior to deleting snapshots to avoid corruption, th…
Posted by GitBox <gi...@apache.org>.
DaanHoogland closed pull request #4032:
URL: https://github.com/apache/cloudstack/pull/4032
----------------------------------------------------------------
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
[GitHub] [cloudstack] rhtyd commented on pull request #4032: Suspending the VM prior to deleting snapshots to avoid corruption, th…
Posted by GitBox <gi...@apache.org>.
rhtyd commented on pull request #4032:
URL: https://github.com/apache/cloudstack/pull/4032#issuecomment-781927615
@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
[GitHub] [cloudstack] weizhouapache commented on pull request #4032: Suspending the VM prior to deleting snapshots to avoid corruption, th…
Posted by GitBox <gi...@apache.org>.
weizhouapache commented on pull request #4032:
URL: https://github.com/apache/cloudstack/pull/4032#issuecomment-766808374
@ggoodrich-ipp what issue does this pr solve ?
Delete a vm snapshot twice ?
----------------------------------------------------------------
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
[GitHub] [cloudstack] rhtyd commented on pull request #4032: Suspending the VM prior to deleting snapshots to avoid corruption, th…
Posted by GitBox <gi...@apache.org>.
rhtyd commented on pull request #4032:
URL: https://github.com/apache/cloudstack/pull/4032#issuecomment-786037814
Did another round of code review, LGTM
----------------------------------------------------------------
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
[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:
URL: https://github.com/apache/cloudstack/pull/4032#discussion_r461380678
##########
File path: plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtDeleteVMSnapshotCommandWrapper.java
##########
@@ -97,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);
+ state = dm.getInfo().state;
+ if (state == DomainInfo.DomainState.VIR_DOMAIN_PAUSED) {
+ dm.resume();
+ }
+ } catch (LibvirtException e) {
+ s_logger.error("Failed to resume vm after delete snapshot " + cmd.getTarget().getSnapshotName() + " on vm: " + vmName + " return true : " + e);
+ }
+
Review comment:
can you extract this in a separate method please?
##########
File path: plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtDeleteVMSnapshotCommandWrapper.java
##########
@@ -97,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());
Review comment:
here a var `didDelete` leads us to conclude that the current exception thrown is caused by `dm.resume()`. this boolean should be named some thing like `tryingResume` instead. better would be to investigate the nature of the exception.
##########
File path: plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/KVMStorageProcessor.java
##########
@@ -1029,7 +1029,7 @@ public Answer backupSnapshot(final CopyCommand cmd) {
}
}
} catch (final Exception ex) {
- s_logger.debug("Failed to delete snapshots on primary", ex);
+ s_logger.error("Failed to delete snapshots on primary", ex);
Review comment:
:+1:
##########
File path: plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtDeleteVMSnapshotCommandWrapper.java
##########
@@ -63,6 +66,14 @@ public Answer execute(final DeleteVMSnapshotCommand cmd, final LibvirtComputingR
dm.suspend(); // suspend the vm to avoid image corruption
snapshot.delete(0); // only remove this snapshot, not children
+ didDelete = true;
+
+ // Resume the VM
+ dm = libvirtComputingResource.getDomain(conn, vmName);
+ state = dm.getInfo().state;
+ if (state == DomainInfo.DomainState.VIR_DOMAIN_PAUSED) {
+ dm.resume();
+ }
Review comment:
resume action leads to an implicit assumption which must be guarded during maintenance by future generations: failing resume is concluded by examining a var named `didDelete`. dangerous assumption even with the current logic computing. see below.
----------------------------------------------------------------
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
[GitHub] [cloudstack] ggoodrich-ipp commented on pull request #4032: Suspending the VM prior to deleting snapshots to avoid corruption, th…
Posted by GitBox <gi...@apache.org>.
ggoodrich-ipp commented on pull request #4032:
URL: https://github.com/apache/cloudstack/pull/4032#issuecomment-769247937
I see @DaanHoogland re-targeted this to 4.15, which must have resolved the conflicts. Are there any other issues with this PR that need to be addressed prior to approval, @weizhouapache @shwstppr ? Let me know, and I'll try to accommodate. Thanks!
----------------------------------------------------------------
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
[GitHub] [cloudstack] rhtyd commented on pull request #4032: Suspending the VM prior to deleting snapshots to avoid corruption, th…
Posted by GitBox <gi...@apache.org>.
rhtyd commented on pull request #4032:
URL: https://github.com/apache/cloudstack/pull/4032#issuecomment-781988610
@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
[GitHub] [cloudstack] blueorangutan commented on pull request #4032: Suspending the VM prior to deleting snapshots to avoid corruption, th…
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4032:
URL: https://github.com/apache/cloudstack/pull/4032#issuecomment-782040381
Packaging result: ✖centos7 ✖centos8 ✖debian. JID-2759
----------------------------------------------------------------
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
[GitHub] [cloudstack] rhtyd commented on pull request #4032: Suspending the VM prior to deleting snapshots to avoid corruption, th…
Posted by GitBox <gi...@apache.org>.
rhtyd commented on pull request #4032:
URL: https://github.com/apache/cloudstack/pull/4032#issuecomment-643062751
@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
[GitHub] [cloudstack] rhtyd commented on pull request #4032: Suspending the VM prior to deleting snapshots to avoid corruption, th…
Posted by GitBox <gi...@apache.org>.
rhtyd commented on pull request #4032:
URL: https://github.com/apache/cloudstack/pull/4032#issuecomment-645109835
@ggoodrich-ipp could you check if `quiescevm` is true in the `createSnapshot` API? Would that be better than doing it by default?
----------------------------------------------------------------
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
[GitHub] [cloudstack] shwstppr commented on pull request #4032: Suspending the VM prior to deleting snapshots to avoid corruption, th…
Posted by GitBox <gi...@apache.org>.
shwstppr commented on pull request #4032:
URL: https://github.com/apache/cloudstack/pull/4032#issuecomment-766714686
@ggoodrich-ipp can you please resolve conflicts
----------------------------------------------------------------
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
[GitHub] [cloudstack] blueorangutan commented on pull request #4032: Suspending the VM prior to deleting snapshots to avoid corruption, th…
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4032:
URL: https://github.com/apache/cloudstack/pull/4032#issuecomment-783360172
Packaging result: ✔centos7 ✖centos8 ✔debian. JID-2783
----------------------------------------------------------------
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