Change in vdsm[ovirt-3.6]: lib: use weakmethod to break reference cycles

fromani at redhat.com fromani at redhat.com
Tue Jan 19 12:35:31 UTC 2016


Hello Piotr Kliczewski, Nir Soffer,

I'd like you to do a code review.  Please visit

    https://gerrit.ovirt.org/52423

to review the following change.

Change subject: lib: use weakmethod to break reference cycles
......................................................................

lib: use weakmethod to break reference cycles

The libvirtconnection module replaces the methods
of libvirt.virDomain object (et. al.) to detect libvirt
errors, and to take action in presence of certain errors,
maybe dropping the connection, maybe killing Vdsm.

While the whole idea of this wrapping and how to properly
react to these events is debatable and should probably
rewritten, the immediate issue is that this wrapping is not safe,
and creates circular references in instance methods:

   +-------------------------------------+
   v                                     |
object -> wrapper -> tuple of arguments(self, method)

Since libvirt.virDomain defines the __del__ special method,
objects become uncollectable, and the net final effect is
a resource leak each time a VM is stopped.

This was observed for virDomain, but the wrapping of
virConnect has also the same problem.

This patch uses utils.weakmethod to break the reference cycle in a
backport-friandly manner, in order to make the objects collectable
again, and to avoid the leak.

The proper long-term fix is to rethink the wrapping done in
libvirtconnection.py, but this deserves more planning and
will be addressed in a future patch.

Change-Id: I06607e63d222bf3f8037f2bda115b48feb94cb1c
Bug-Url: https://bugzilla.redhat.com/1283999
Bug-Url: https://bugzilla.redhat.com/1269424
Backport-To: 3.6
Backport-To: 3.5
Reported-By: Eldad Marciano <emarcian at redhat.com>
Reported-By: Milan Zamazal <mzamazal at redhat.com>
Signed-off-by: Francesco Romani <fromani at redhat.com>
Reviewed-on: https://gerrit.ovirt.org/51630
Reviewed-by: Piotr Kliczewski <piotr.kliczewski at gmail.com>
Reviewed-by: Nir Soffer <nsoffer at redhat.com>
Continuous-Integration: Jenkins CI
---
M lib/vdsm/libvirtconnection.py
1 file changed, 5 insertions(+), 3 deletions(-)


  git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/23/52423/1

diff --git a/lib/vdsm/libvirtconnection.py b/lib/vdsm/libvirtconnection.py
index f7a3926..0836bf1 100644
--- a/lib/vdsm/libvirtconnection.py
+++ b/lib/vdsm/libvirtconnection.py
@@ -1,5 +1,5 @@
 #
-# Copyright 2009-2013 Red Hat, Inc.
+# Copyright 2009-2016 Red Hat, Inc.
 #
 # This program is free software; you can redistribute it and/or modify
 # it under the terms of the GNU General Public License as published by
@@ -126,7 +126,8 @@
                     for name in dir(ret):
                         method = getattr(ret, name)
                         if callable(method) and name[0] != '_':
-                            setattr(ret, name, wrapMethod(method))
+                            setattr(ret, name,
+                                    wrapMethod(utils.weakmethod(method)))
                 return ret
             except libvirt.libvirtError as e:
                 edom = e.get_error_domain()
@@ -167,7 +168,8 @@
             for name in dir(libvirt.virConnect):
                 method = getattr(conn, name)
                 if callable(method) and name[0] != '_':
-                    setattr(conn, name, wrapMethod(method))
+                    setattr(conn, name,
+                            wrapMethod(utils.weakmethod(method)))
             if target is not None:
                 for ev in (libvirt.VIR_DOMAIN_EVENT_ID_LIFECYCLE,
                            libvirt.VIR_DOMAIN_EVENT_ID_REBOOT,


-- 
To view, visit https://gerrit.ovirt.org/52423
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I06607e63d222bf3f8037f2bda115b48feb94cb1c
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: ovirt-3.6
Gerrit-Owner: Francesco Romani <fromani at redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczewski at gmail.com>


More information about the vdsm-patches mailing list