Change in vdsm[master]: PARTIAL: explicitly close libvirt connection after use

[Dan Kenigsberg]

Dan Kenigsberg has uploaded a new change for review.

Change subject: PARTIAL: explicitly close libvirt connection after use
......................................................................

PARTIAL: explicitly close libvirt connection after use

Otherwise, libvirt cries about

: error : virNetSocketReadWire:1184 : End of file while reading data: Input/output error

TODO: fix other usages of libvirtconnection.get().

Change-Id: I2b15f3ab017828a63960ba9311fa2bf6bfab4729
Signed-off-by: Dan Kenigsberg 
---
M lib/vdsm/tool/dummybr.py
1 file changed, 6 insertions(+), 5 deletions(-)

git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/92/17192/1

diff --git a/lib/vdsm/tool/dummybr.py b/lib/vdsm/tool/dummybr.py
index 762e726..68a9568 100644
--- a/lib/vdsm/tool/dummybr.py
+++ b/lib/vdsm/tool/dummybr.py
@@ -20,6 +20,7 @@

import os
+import contextlib

from vdsm.netinfo import DUMMY_BRIDGE
 from vdsm import libvirtconnection, utils, constants
@@ -34,11 +35,11 @@

def addBridgeToLibvirt(bridgeName):
-    conn = libvirtconnection.get(None, False)
-    if bridgeName not in conn.listNetworks():
-        conn.networkCreateXML(
-            '''<network><name>%s</name><forward mode='bridge'/><bridge '''
-            '''name='%s'/></network>''' % (bridgeName, bridgeName))
+    with contextlib.closing(libvirtconnection.get(None, False)) as conn:
+        if bridgeName not in conn.listNetworks():
+            conn.networkCreateXML(
+                '''<network><name>%s</name><forward mode='bridge'/><bridge '''
+                '''name='%s'/></network>''' % (bridgeName, bridgeName))

@expose('dummybr')
 def main():

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: PARTIAL: explicitly close libvirt connection after use
......................................................................

Patch Set 1:

Build Successful

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

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

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

Antoni Segura Puimedon has posted comments on this change.

Change subject: PARTIAL: explicitly close libvirt connection after use
......................................................................

Patch Set 1: I would prefer that you didn't submit this

I'd rather modify the connection object in libvirtconnection.py in the following way:

Top of the module:

import new

and before return connection do:

connection.__enter__ = new.instancemethod(lambda self: self, connection,
                                              connection.__class__)
    def conn_close(self, *args):
        self.close()

connection.__exit__ = new.instancemethod(conn_close, connection,
                                              connection.__class__)

This will allow the current usage as well as:

with libvirtconnection.get() as conn:
    conn.xxxx

and automatically close.

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: PARTIAL: explicitly close libvirt connection after use
......................................................................

Patch Set 1:

Why using `new` is better than returning contextlib.closing(conn) in libvirtconnection.get()?

I thought of doing something like this, but it requires more thought, as at the moment, libvirtconnection.get() may return an existing instance, that is used in another context. In that case, we should not close the connection when it falls out of scope - we should have some kind of refcounting.

Antoni Segura Puimedon has posted comments on this change.

Change subject: PARTIAL: explicitly close libvirt connection after use
......................................................................

Patch Set 1:

The difference is that using "new" allows you not to change the old usage, i.e., you still return a conn object on which you can execute queries. However, if you use it with "with" it will behave like a context manager.

As for the refcounting, I agree, we should probably do better connection management.

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: PARTIAL: explicitly close libvirt connection after use
......................................................................

Patch Set 1:

Toni, I still do not get why explicitly setting conn.__enter__ is any better than

def get():
  bla
  conn = foo
  return closing(conn)

(not that we can actually use either of these due to the refcount issue)

Itamar Heim has posted comments on this change.

Change subject: PARTIAL: explicitly close libvirt connection after use
......................................................................

Patch Set 1:

ping - still relevant?

Change in vdsm[master]: Explicitly close libvirt connections at exist

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: Explicitly close libvirt connections at exist
......................................................................

Patch Set 2: Code-Review-1 Verified-1

Build Failed

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/7073/ : FAILURE

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/7975/ : FAILURE

http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/7864/ : UNSTABLE

Change in vdsm[master]: Explicitly close libvirt connections at exist

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: Explicitly close libvirt connections at exist
......................................................................

Patch Set 3:

Build Failed

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/7074/ : FAILURE

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

http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/7865/ : SUCCESS

Change in vdsm[master]: Explicitly close libvirt connections at exist

Yeela Kaplan has posted comments on this change.

Change subject: Explicitly close libvirt connections at exist
......................................................................

Patch Set 3: Code-Review+1

Change in vdsm[master]: Explicitly close libvirt connections at exist

Yaniv Bronhaim has posted comments on this change.

Change subject: Explicitly close libvirt connections at exist
......................................................................

Patch Set 3:

(1 comment)

http://gerrit.ovirt.org/#/c/17192/3/lib/vdsm/libvirtconnection.py
File lib/vdsm/libvirtconnection.py:

Line 176: def __close_connections():
Line 177:     for conn in __connections.values():
Line 178:         conn.close()
Line 179: 
Line 180: atexit.register(__close_connections)
could be confusing.. don't you prefer something more tight to vdsm, and not being depended on the interpreter ? like maybe calling this part only on prepareForShutdown or on sigterm handling ?

Change in vdsm[master]: Explicitly close libvirt connections at exist

mooli tayer has posted comments on this change.

Change subject: Explicitly close libvirt connections at exist
......................................................................

Patch Set 3: Code-Review+1

Change in vdsm[master]: Explicitly close libvirt connections at exist

[Federico Simoncelli]

Federico Simoncelli has posted comments on this change.

Change subject: Explicitly close libvirt connections at exist
......................................................................

Patch Set 3:

(1 comment)

http://gerrit.ovirt.org/#/c/17192/3/lib/vdsm/libvirtconnection.py
File lib/vdsm/libvirtconnection.py:

Line 176: def __close_connections():
Line 177:     for conn in __connections.values():
Line 178:         conn.close()
Line 179: 
Line 180: atexit.register(__close_connections)
> could be confusing.. don't you prefer something more tight to vdsm, and not

+1 for prepareForShutdown (that in theory should already call stop_event_loop for cleanness).

Change in vdsm[master]: Explicitly close libvirt connections at exist

Antoni Segura Puimedon has posted comments on this change.

Change subject: Explicitly close libvirt connections at exist
......................................................................

Patch Set 3: Code-Review+1

Change in vdsm[master]: Explicitly close libvirt connections at exist

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: Explicitly close libvirt connections at exist
......................................................................

Patch Set 3:

(1 comment)

http://gerrit.ovirt.org/#/c/17192/3/lib/vdsm/libvirtconnection.py
File lib/vdsm/libvirtconnection.py:

Line 176: def __close_connections():
Line 177:     for conn in __connections.values():
Line 178:         conn.close()
Line 179: 
Line 180: atexit.register(__close_connections)
> +1 for prepareForShutdown (that in theory should already call stop_event_lo

I much prefer having this module standalone and usable outside of vdsm proper. I actually noticed the the stray connections causing libvirt errors after running `vdsm-tool dummybr` several times, where there's no prepareForShutdown.

Ack for calling stop_event_loop() in http://gerrit.ovirt.org/26532

Change in vdsm[master]: Explicitly close libvirt connections at exist

Yaniv Bronhaim has posted comments on this change.

Change subject: Explicitly close libvirt connections at exist
......................................................................

Patch Set 3: Code-Review+1

Change in vdsm[master]: Explicitly close libvirt connections at exist

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: Explicitly close libvirt connections at exist
......................................................................

Patch Set 3: Verified+1

Verified that the patch has no functional effect on VM lifecycle or vdsm-tool commands, while avoiding libvirt log errors.

Change in vdsm[master]: Explicitly close libvirt connections at exist

[Federico Simoncelli]

Federico Simoncelli has posted comments on this change.

Change subject: Explicitly close libvirt connections at exist
......................................................................

Patch Set 3: Code-Review+2

Change in vdsm[master]: Explicitly close libvirt connections at exist

[Dan Kenigsberg]

Dan Kenigsberg has submitted this change and it was merged.

Change subject: Explicitly close libvirt connections at exist
......................................................................

Explicitly close libvirt connections at exist

When a process (such as Vdsm, or `vdsm-tool dummybr`) exists while
holding an open libvirt connection, libvirt logs an error

: error : virNetSocketReadWire:1184 : End of file while reading data: Input/output error

This patch close all libvirt connections at process exit.

Change-Id: I2b15f3ab017828a63960ba9311fa2bf6bfab4729
Signed-off-by: Dan Kenigsberg 
Reviewed-on: http://gerrit.ovirt.org/17192
Reviewed-by: Yeela Kaplan 
Reviewed-by: mooli tayer 
Reviewed-by: Antoni Segura Puimedon 
Reviewed-by: Yaniv Bronhaim 
Reviewed-by: Federico Simoncelli 
---
M lib/vdsm/libvirtconnection.py
1 file changed, 8 insertions(+), 0 deletions(-)

Approvals:
  Yeela Kaplan: Looks good to me, but someone else must approve
  Yaniv Bronhaim: Looks good to me, but someone else must approve
  Antoni Segura Puimedon: Looks good to me, but someone else must approve
  Federico Simoncelli: Looks good to me, approved
  mooli tayer: Looks good to me, but someone else must approve
  Dan Kenigsberg: Verified