Change in vdsm[master]: virt: Correct VM state before vm.cont() in _recover()

Shmuel Leib Melamud has uploaded a new change for review.

Change subject: virt: Correct VM state before vm.cont() in _recover()
......................................................................

virt: Correct VM state before vm.cont() in _recover()

In SourceThread._recover(), if error occured while hibernating a VM, set
VM status to PAUSED before calling vm.cont(). Otherwise, vm.cont() will
fail to run in SAVING_STATE state and the VM will be left paused.

Change-Id: I5b1c7b4eecacf87ece48dc563fd2da294af0510b
Bug-Url: https://bugzilla.redhat.com/show_bug.cgi?id=1238536
Signed-off-by: Shmuel Melamud 
---
M vdsm/virt/migration.py
1 file changed, 1 insertion(+), 0 deletions(-)

git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/27/47527/1

diff --git a/vdsm/virt/migration.py b/vdsm/virt/migration.py
index 32caccb..f950a6f 100644
--- a/vdsm/virt/migration.py
+++ b/vdsm/virt/migration.py
@@ -213,6 +213,7 @@
                 self.log.exception("Failed to destroy remote VM")
         # if the guest was stopped before migration, we need to cont it
         if self.hibernating:
+            self._vm.lastStatus = vmstatus.PAUSED
             self._vm.cont()
         # either way, migration has finished
         self._vm.lastStatus = vmstatus.UP

[e-mail] has posted comments on this change.

Change subject: virt: Correct VM state before vm.cont() in _recover()
......................................................................

Patch Set 1:

* Update tracker::#1238536::OK
* Check Bug-Url::OK
* Check Public Bug::#1238536::OK, public bug
* Check Product::#1238536::OK, Correct classification oVirt
* Check TM::SKIP, not in a monitored branch (ovirt-3.5 ovirt-3.4 ovirt-3.3 ovirt-3.2)
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])

Shmuel Leib Melamud has posted comments on this change.

Change subject: virt: Correct VM state before vm.cont() in _recover()
......................................................................

Patch Set 1: Verified+1

Francesco Romani has posted comments on this change.

Change subject: virt: Correct VM state before vm.cont() in _recover()
......................................................................

Patch Set 1: Code-Review-1

(1 comment)

now that this works, let's check better alternatives. -1  for visibility.

https://gerrit.ovirt.org/#/c/47527/1/vdsm/virt/migration.py
File vdsm/virt/migration.py:

Line 212:             except Exception:
Line 213:                 self.log.exception("Failed to destroy remote VM")
Line 214:         # if the guest was stopped before migration, we need to cont it
Line 215:         if self.hibernating:
Line 216:             self._vm.lastStatus = vmstatus.PAUSED
IMO better to reset this status when/if the hook raises, with a comment that reminds us it is pretty much a workaround.
Line 217:             self._vm.cont()
Line 218:         # either way, migration has finished
Line 219:         self._vm.lastStatus = vmstatus.UP
Line 220:         self._vm.send_status_event()

Shmuel Leib Melamud has posted comments on this change.

Change subject: virt: Correct VM state before vm.cont() in _recover()
......................................................................

Patch Set 1:

(1 comment)

https://gerrit.ovirt.org/#/c/47527/1/vdsm/virt/migration.py
File vdsm/virt/migration.py:

Line 212:             except Exception:
Line 213:                 self.log.exception("Failed to destroy remote VM")
Line 214:         # if the guest was stopped before migration, we need to cont it
Line 215:         if self.hibernating:
Line 216:             self._vm.lastStatus = vmstatus.PAUSED
> IMO better to reset this status when/if the hook raises, with a comment tha

This may happen not only when the hook raises. self._vm._dom.save(fname) may also raise an exception and result will be the same. Thus, the exceptions must be catched at higher level than _startUnderlyingMigration() - in run().

But exceptions are already catched in run() and _recover() is used in all cases to reset VM to the state it had before migration started. IMHO it is better to keep all recovery code in one place.

Also, _vm.pause(SAVING_STATE) is invoked in _prepareGuest() called from run(). It is logical to change the state back on the same level.

But I agree, it is not the best place. The best place should be _vm.cont(). We have _vm.pause(SAVING_STATE) that means "pause the VM and put it into SAVING_STATE instead of PAUSED". So we need also the opposite - _vm.cont(beforeState=SAVING_STATE) that means "continue the VM that is currently in SAVING_STATE instead of PAUSED".

Or we can just take away the status check from _vm.cont().
Line 217:             self._vm.cont()
Line 218:         # either way, migration has finished
Line 219:         self._vm.lastStatus = vmstatus.UP
Line 220:         self._vm.send_status_event()

[e-mail] has posted comments on this change.

Change subject: virt: Correct VM state before vm.cont() in _recover()
......................................................................

Patch Set 2:

* #1238536::Update tracker: OK
* Check Bug-Url::OK
* Check Public Bug::#1238536::OK, public bug
* Check Product::#1238536::OK, Correct classification oVirt
* Check TM::SKIP, not in a monitored branch (ovirt-3.5 ovirt-3.4 ovirt-3.3 ovirt-3.2)
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])

Shmuel Leib Melamud has posted comments on this change.

Change subject: virt: Correct VM state before vm.cont() in _recover()
......................................................................

Patch Set 2: Verified+1

Francesco Romani has posted comments on this change.

Change subject: virt: Correct VM state before vm.cont() in _recover()
......................................................................

Patch Set 2: Code-Review+1

I'm not enthusiast about beforeState, but since I can't suggest anythign better, here goes my preliminar ACK.

Change in vdsm[master]: virt: Allow _recover() to call vm.cont() in any VM state

[Nir Soffer]

Nir Soffer has submitted this change and it was merged.

Change subject: virt: Allow _recover() to call vm.cont() in any VM state
......................................................................

virt: Allow _recover() to call vm.cont() in any VM state

By default, cont() returns error if the VM is in one of the following
states:

vmstatus.MIGRATION_SOURCE
    Migration is in progress, VM status should not be changed till the
    migration finishes.

vmstatus.SAVING_STATE
    Hibernation is in progress, VM status should not be changed till the
    hibernation finishes.

vmstatus.DOWN
    VM is down, continuing is not possible from this state.

But if an error occured when performing an operation on a VM and
SourceThread._recover() is called, it must be able to call vm.cont() for
recovery purposes regardless of the VM's status.

To implement this, ignoreStatus parameter was added to vm.cont(). When
set to True, it forces vm.cont() to run even in the states listed above.

Change-Id: I5b1c7b4eecacf87ece48dc563fd2da294af0510b
Bug-Url: https://bugzilla.redhat.com/show_bug.cgi?id=1238536
Signed-off-by: Shmuel Melamud 
Reviewed-on: https://gerrit.ovirt.org/47527
Continuous-Integration: Jenkins CI
Reviewed-by: Nir Soffer 
Reviewed-by: Francesco Romani 
---
M vdsm/virt/migration.py
M vdsm/virt/vm.py
2 files changed, 29 insertions(+), 4 deletions(-)

Approvals:
  Nir Soffer: Looks good to me, but someone else must approve
  Jenkins CI: Passed CI tests
  Shmuel Leib Melamud: Verified
  Francesco Romani: Looks good to me, approved

Change in vdsm[master]: virt: Allow _recover() to call vm.cont() in any VM state

gerrit-hooks has posted comments on this change.

Change subject: virt: Allow _recover() to call vm.cont() in any VM state
......................................................................

Patch Set 7:

* #1238536::Update tracker: OK
* Set MODIFIED::bug 1238536::::#1238536::::OK