Suspending the VM prior to deleting snapshots to avoid corruption, th… by ggoodrich-ipp · Pull Request #4032 · apache/cloudstack

ggoodrich-ipp · 2020-04-15T19:12:18Z

…en resuming

Description

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.

Types of changes

Breaking change (fix or feature that would cause existing functionality to change)
New feature (non-breaking change which adds functionality)
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?

…en resuming

andrijapanicsb · 2020-04-16T11:12:19Z

@blueorangutan package

blueorangutan · 2020-04-16T11:13:09Z

@andrijapanicsb a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

andrijapanicsb · 2020-04-16T11:34:53Z

@weizhouapache does this one makes more sense/checks than the #4033 - i.e. shall we work on this one?

DaanHoogland

we should only start/resume a VM if it was running before we suspended it, not if it was stopped or paused by some other operation.
I suggest we add a var with oldState and check if oldState.equals(DomainInfo.DomainState.VIR_DOMAIN_RUNNING) before the resume statements.

blueorangutan · 2020-04-16T11:36:34Z

Packaging result: ✔centos7 ✔debian. JID-1172

DaanHoogland · 2020-04-16T11:37:31Z

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

yadvr · 2020-04-16T11:43:09Z

            snapshot = dm.snapshotLookupByName(cmd.getTarget().getSnapshotName());

+            // Suspend the VM prior to deleting the snapshot to guard against corruption
+            dm.suspend();


Should we check if VM is running? (i.e. not in any other states)

May wrap in a try-with?

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)

yadvr · 2020-04-16T11:45:49Z

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


dm is already not null, why overwrite it again?

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.

yadvr

Left some remarks, needs testing/verification. LGTM otherwise.
@weizhouapache @DaanHoogland can you comment as well?

DaanHoogland · 2020-04-16T12:03:21Z

as we are in release phase already and #4033 is simpler let's go with that. the logic is essentially the same.

yadvr · 2020-06-11T12:01:03Z

@ggoodrich-ipp can you fix the conflicts, thanks.

ggoodrich-ipp · 2020-06-11T13:57:07Z

@ggoodrich-ipp can you fix the conflicts, thanks.

Conflicts resolved.

yadvr · 2020-06-12T04:48:46Z

@blueorangutan package

blueorangutan · 2020-06-12T04:50:24Z

@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

blueorangutan · 2020-06-12T05:45:30Z

Packaging result: ✖centos7 ✖debian. JID-1353

yadvr · 2020-06-17T02:30:56Z

@ggoodrich-ipp could you check if quiescevm is true in the createSnapshot API? Would that be better than doing it by default?

ggoodrich-ipp · 2020-06-18T16:54:04Z

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

DaanHoogland

minor remarks

DaanHoogland · 2020-07-28T07:40:01Z

+                // 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);
+                }
+


can you extract this in a separate method please?

DaanHoogland · 2020-07-28T07:44:11Z

+            didDelete = true;
+
+            // Resume the VM
+            dm = libvirtComputingResource.getDomain(conn, vmName);
+            state = dm.getInfo().state;
+            if (state == DomainInfo.DomainState.VIR_DOMAIN_PAUSED) {
+                dm.resume();
+            }


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.

DaanHoogland · 2020-07-28T07:46:19Z

+            } 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());


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.

DaanHoogland · 2020-07-28T07:47:52Z

                    }
                } catch (final Exception ex) {
-                    s_logger.debug("Failed to delete snapshots on primary", ex);
+                    s_logger.error("Failed to delete snapshots on primary", ex);


ggoodrich-ipp · 2021-01-28T17:28:06Z

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!

DaanHoogland · 2021-01-29T14:01:15Z

travis is reporting a checkstyle error but i cannot find it closing

DaanHoogland · 2021-01-29T14:01:37Z

re-opening to re-kick travis

weizhouapache · 2021-02-01T10:28:58Z

@ggoodrich-ipp could you please fix the build error ?

yadvr · 2021-02-19T08:45:57Z

@blueorangutan package

blueorangutan · 2021-02-19T08:46:50Z

@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

blueorangutan · 2021-02-19T08:57:29Z

Packaging result: ✖centos7 ✖centos8 ✖debian. JID-2745

yadvr · 2021-02-19T10:33:19Z

@blueorangutan package

blueorangutan · 2021-02-19T10:33:50Z

@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

blueorangutan · 2021-02-19T12:18:27Z

Packaging result: ✖centos7 ✖centos8 ✖debian. JID-2759

shwstppr · 2021-02-22T10:06:36Z

@blueorangutan package

blueorangutan · 2021-02-22T10:07:58Z

@shwstppr a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

blueorangutan · 2021-02-22T11:21:08Z

Packaging result: ✖centos7 ✖centos8 ✖debian. JID-2780

blueorangutan · 2021-02-22T13:06:13Z

Packaging result: ✔centos7 ✖centos8 ✔debian. JID-2783

shwstppr · 2021-02-22T15:54:16Z

@blueorangutan test

blueorangutan · 2021-02-22T15:54:59Z

@shwstppr a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

blueorangutan · 2021-02-23T00:53:14Z

Trillian test result (tid-3594)
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

shwstppr

LGTM on code changes and test results. May need manual testing

yadvr · 2021-02-23T10:59:56Z

@weizhouapache have you done any manual testing on this?

yadvr · 2021-02-25T16:40:16Z

Did another round of code review, LGTM

Suspending the VM prior to deleting snapshots to avoid corruption, th…

eaab9ae

…en resuming

DaanHoogland requested changes Apr 16, 2020

View reviewed changes

yadvr reviewed Apr 16, 2020

View reviewed changes

yadvr requested a review from weizhouapache April 16, 2020 11:46

andrijapanicsb mentioned this pull request Apr 16, 2020

Bring back vm.suspend during deleting VM snapshot #4029

Merged

svenvogel added type:bug type:enhancement component:kvm component:primary-storage labels Apr 17, 2020

Merge branch 'master' into gjg-snapshot-pause-vm

30bd160

yadvr requested review from DaanHoogland and yadvr June 12, 2020 04:48

DaanHoogland reviewed Jul 28, 2020

View reviewed changes

PaulAngus removed the type:enhancement label Nov 7, 2020

Making some adjustments based upon review

43f1c87

DaanHoogland added this to the 4.15.1.0 milestone Jan 26, 2021

DaanHoogland closed this Jan 29, 2021

DaanHoogland reopened this Jan 29, 2021

weizhouapache approved these changes Jan 29, 2021

View reviewed changes

Comment thread ...in/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtDeleteVMSnapshotCommandWrapper.java

Fixing import

633438f

yadvr closed this Feb 19, 2021

yadvr reopened this Feb 19, 2021

shwstppr approved these changes Feb 23, 2021

View reviewed changes

yadvr closed this Feb 23, 2021

yadvr reopened this Feb 23, 2021

yadvr merged commit af0f642 into apache:4.15 Feb 25, 2021

Conversation

ggoodrich-ipp commented Apr 15, 2020 • edited by PaulAngus Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Types of changes

Screenshots (if appropriate):

How Has This Been Tested?

Uh oh!

andrijapanicsb commented Apr 16, 2020

Uh oh!

blueorangutan commented Apr 16, 2020

Uh oh!

andrijapanicsb commented Apr 16, 2020

Uh oh!

DaanHoogland left a comment

Choose a reason for hiding this comment

Uh oh!

blueorangutan commented Apr 16, 2020

Uh oh!

DaanHoogland commented Apr 16, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yadvr left a comment

Choose a reason for hiding this comment

Uh oh!

DaanHoogland commented Apr 16, 2020

Uh oh!

yadvr commented Jun 11, 2020

Uh oh!

ggoodrich-ipp commented Jun 11, 2020

Uh oh!

yadvr commented Jun 12, 2020

Uh oh!

blueorangutan commented Jun 12, 2020

Uh oh!

blueorangutan commented Jun 12, 2020

Uh oh!

yadvr commented Jun 17, 2020

Uh oh!

ggoodrich-ipp commented Jun 18, 2020

Uh oh!

DaanHoogland left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ggoodrich-ipp commented Jan 28, 2021

Uh oh!

DaanHoogland commented Jan 29, 2021

Uh oh!

DaanHoogland commented Jan 29, 2021

Uh oh!

Uh oh!

weizhouapache commented Feb 1, 2021

Uh oh!

yadvr commented Feb 19, 2021

Uh oh!

blueorangutan commented Feb 19, 2021

Uh oh!

blueorangutan commented Feb 19, 2021

Uh oh!

yadvr commented Feb 19, 2021

Uh oh!

blueorangutan commented Feb 19, 2021

ggoodrich-ipp commented Apr 15, 2020 •

edited by PaulAngus

Loading

DaanHoogland left a comment •

edited

Loading