Change in vdsm[master]: vm: set self._dom to None after destroying vm

[Jarod.w]

Jarod.w has uploaded a new change for review.

Change subject: vm: set self._dom to None after destroying vm
......................................................................

vm: set self._dom to None after destroying vm

When receiving 'vmDestory' command, we deal with it using the following
steps:
* invoke VM::releaseVm() in VM::destroy
* invoke _dom.destroyFlags()/_dom.destroy() to destroy vm in VM::releaseVm()
* When libvirt is done for destroying vm, it emits VIR_DOMAIN_EVENT_STOPPED
_SHUTDOWN/DESTROYED(due to different flows)
* VDSM receives the signal, it invokes VM::_onQemuDeath().
* invoke VM::releaseVm() in VM::_onQemuDeath().
Unfortunately, it doesn't set self._dom to None after destroying vm in
VM::releaseVm(). So, it cause the following libvirterror when invoking
VM::releaseVm() in VM::_onQemuDeath():

libvirtconnection::101::libvirtconnection::(wrapper) Unknown libvirterror: ecode:
42 edom: 10 level: 2 message: Domain not found: no domain with matching uuid
'1813cf7d-d62b-4cc2-86fc-de469e0e37eb' (xxx)

To ignore the libvirterror, the patch will set self._dom to None after destroying
vm.

Change-Id: I4eca4c14e44a2a95628bf8e45a7a0a64d56446b8
Signed-off-by: jarod.w 
---
M vdsm/vm.py
1 file changed, 2 insertions(+), 0 deletions(-)

git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/14/23014/1

diff --git a/vdsm/vm.py b/vdsm/vm.py
index 9381ccf..cd7f3ee 100644
--- a/vdsm/vm.py
+++ b/vdsm/vm.py
@@ -4511,6 +4511,8 @@
                                           "gracefully", self.conf['vmId'])
                             time.sleep(30)
                             self._dom.destroy()
+                    finally:
+                            self._dom = None
             except libvirt.libvirtError as e:
                 if e.get_error_code() == libvirt.VIR_ERR_NO_DOMAIN:
                     self.log.warning("libvirt domain not found", exc_info=True)

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: vm: set self._dom to None after destroying vm
......................................................................

Patch Set 1:

Build Successful

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/6566/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/5673/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/6479/ : SUCCESS

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: vm: set self._dom to None after destroying vm
......................................................................

Patch Set 1: Code-Review-1

(1 comment)

....................................................
File vdsm/vm.py
Line 4511:                                           "gracefully", self.conf['vmId'])
Line 4512:                             time.sleep(30)
Line 4513:                             self._dom.destroy()
Line 4514:                     finally:
Line 4515:                             self._dom = None
we must not leak the virDomain object before we know it has been successfully destroyed. destroy() might have failed now (due to a libvirt connection error) and would succeed on the next attempt.
Line 4516:             except libvirt.libvirtError as e:
Line 4517:                 if e.get_error_code() == libvirt.VIR_ERR_NO_DOMAIN:
Line 4518:                     self.log.warning("libvirt domain not found", exc_info=True)
Line 4519:                 else:

[Jarod.w]

Jarod.w has posted comments on this change.

Change subject: vm: set self._dom to None after destroying vm
......................................................................

Patch Set 1:

(1 comment)

....................................................
File vdsm/vm.py
Line 4511:                                           "gracefully", self.conf['vmId'])
Line 4512:                             time.sleep(30)
Line 4513:                             self._dom.destroy()
Line 4514:                     finally:
Line 4515:                             self._dom = None
When getting VIR_DOMAIN_EVENT_STOPPED_SHUTDOWN/DESTROYED signals, we can think that it has been successfully destroyed, right? If so, we
might need to set self._dom None in _onQemuDeath.
Line 4516:             except libvirt.libvirtError as e:
Line 4517:                 if e.get_error_code() == libvirt.VIR_ERR_NO_DOMAIN:
Line 4518:                     self.log.warning("libvirt domain not found", exc_info=True)
Line 4519:                 else:

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: vm: set self._dom to None after destroying vm
......................................................................

Patch Set 2:

Build Successful

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/6620/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/5727/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/6533/ : SUCCESS

[Vinzenz Feenstra]

Vinzenz Feenstra has posted comments on this change.

Change subject: vm: set self._dom to None after destroying vm
......................................................................

Patch Set 2:

(1 comment)

Minor typo in the commit message. Glad to see this error going away.

Thanks!

....................................................
Commit Message
Line 5: CommitDate: 2014-01-08 16:07:21 +0800
Line 6: 
Line 7: vm: set self._dom to None after destroying vm
Line 8: 
Line 9: When receiving 'vmDestory' command, we deal with it using the following
vmDestroy
Line 10: steps:
Line 11: * invoke VM::releaseVm() in VM::destroy
Line 12: * invoke _dom.destroyFlags()/_dom.destroy() to destroy vm in VM::releaseVm()
Line 13: * When libvirt is done for destroying vm, it emits VIR_DOMAIN_EVENT_STOPPED

[Jarod.w]

Jarod.w has posted comments on this change.

Change subject: vm: set self._dom to None after destroying vm
......................................................................

Patch Set 2:

(1 comment)

....................................................
Commit Message
Line 5: CommitDate: 2014-01-08 16:07:21 +0800
Line 6: 
Line 7: vm: set self._dom to None after destroying vm
Line 8: 
Line 9: When receiving 'vmDestory' command, we deal with it using the following
Done
Line 10: steps:
Line 11: * invoke VM::releaseVm() in VM::destroy
Line 12: * invoke _dom.destroyFlags()/_dom.destroy() to destroy vm in VM::releaseVm()
Line 13: * When libvirt is done for destroying vm, it emits VIR_DOMAIN_EVENT_STOPPED

[Jarod.w]

Jarod.w has posted comments on this change.

Change subject: vm: set self._dom to None after destroying vm
......................................................................

Patch Set 3: Verified+1

Test passed on:
libvirt: 1.0.5.7
qemu: 1.5.3.0-2.355

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: vm: set self._dom to None after destroying vm
......................................................................

Patch Set 3:

Build Successful

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/6653/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/5760/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/6566/ : SUCCESS

Francesco Romani has posted comments on this change.

Change subject: vm: set self._dom to None after destroying vm
......................................................................

Patch Set 3:

I'm in for this.

[Vinzenz Feenstra]

Vinzenz Feenstra has posted comments on this change.

Change subject: vm: set self._dom to None after destroying vm
......................................................................

Patch Set 3: Code-Review+1

Francesco Romani has posted comments on this change.

Change subject: vm: set self._dom to None after destroying vm
......................................................................

Patch Set 3: Code-Review+1

For some reasons -likely just irrational- I don't really like to reset _dom this way, but on the other hand I can't see any mistake in the patch, and the issue addressed here is real. So, I'll just keep the discussion purely technical, and on this basis and I'm ok with the patch.

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: vm: set self._dom to None after destroying vm
......................................................................

Patch Set 3: Code-Review+2

[Dan Kenigsberg]

Dan Kenigsberg has submitted this change and it was merged.

Change subject: vm: set self._dom to None after destroying vm
......................................................................

vm: set self._dom to None after destroying vm

When receiving 'vmDestroy' command, we deal with it using the following
steps:
* invoke VM::releaseVm() in VM::destroy
* invoke _dom.destroyFlags()/_dom.destroy() to destroy vm in VM::releaseVm()
* When libvirt is done for destroying vm, it emits VIR_DOMAIN_EVENT_STOPPED
_SHUTDOWN/DESTROYED(due to different flows)
* VDSM receives the signal, it invokes VM::_onQemuDeath().
* invoke VM::releaseVm() in VM::_onQemuDeath().
Unfortunately, it doesn't set self._dom to None after destroying vm in
VM::releaseVm(). So, it cause the following libvirterror when invoking
VM::releaseVm() in VM::_onQemuDeath():

libvirtconnection::101::libvirtconnection::(wrapper) Unknown libvirterror: ecode:
42 edom: 10 level: 2 message: Domain not found: no domain with matching uuid
'1813cf7d-d62b-4cc2-86fc-de469e0e37eb' (xxx)

To ignore the libvirterror, the patch will set self._dom to None when getting signals
(VIR_DOMAIN_EVENT_STOPPED_SHUTDOWN/DESTROYED).

Change-Id: I4eca4c14e44a2a95628bf8e45a7a0a64d56446b8
Signed-off-by: jarod.w 
Reviewed-on: http://gerrit.ovirt.org/23014
Reviewed-by: Vinzenz Feenstra 
Reviewed-by: Francesco Romani 
Reviewed-by: Dan Kenigsberg 
---
M vdsm/vm.py
1 file changed, 1 insertion(+), 0 deletions(-)

Approvals:
  Vinzenz Feenstra: Looks good to me, but someone else must approve
  Dan Kenigsberg: Looks good to me, approved
  Francesco Romani: Looks good to me, but someone else must approve
  Jarod.w: Verified