mooli tayer has uploaded a new change for review.
Change subject: libvirtconnection: ping libvirt upon disconnection ......................................................................
libvirtconnection: ping libvirt upon disconnection
Upon recieving an error from libvirt we should ping it (using getLibVersion) and only upon failure treat it as down.
Libvirt error codes do not distinguish recoverable from non-recoverable libvirt errors.
Change-Id: Ia489e46dd8ce4c70c888988d17b86311d3c4b935 Bug-Url: https://bugzilla.redhat.com/show_bug.cgi?id=982634 Signed-off-by: Mooli Tayer mtayer@redhat.com --- M lib/vdsm/libvirtconnection.py A tests/libvirtconnectionTests.py 2 files changed, 136 insertions(+), 13 deletions(-)
git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/44/19444/1
diff --git a/lib/vdsm/libvirtconnection.py b/lib/vdsm/libvirtconnection.py index cdba57c..b129c03 100644 --- a/lib/vdsm/libvirtconnection.py +++ b/lib/vdsm/libvirtconnection.py @@ -70,15 +70,15 @@ Wrap methods of connection object so that they catch disconnection, and take the current process down. """ - def wrapMethod(f): - def wrapper(*args, **kwargs): + def wrapMethod(conn, f): + def wrapper(vdsmLibvirtPing=False, *args, **kwargs): try: ret = f(*args, **kwargs) if isinstance(ret, libvirt.virDomain): for name in dir(ret): method = getattr(ret, name) if callable(method) and name[0] != '_': - setattr(ret, name, wrapMethod(method)) + setattr(ret, name, wrapMethod(conn, method)) return ret except libvirt.libvirtError as e: edom = e.get_error_domain() @@ -90,15 +90,19 @@ libvirt.VIR_ERR_NO_CONNECT, libvirt.VIR_ERR_INVALID_CONN) if edom in EDOMAINS and ecode in ECODES: - log.error('connection to libvirt broken.' - ' ecode: %d edom: %d', ecode, edom) - if killOnFailure: - log.error('taking calling process down.') - os.kill(os.getpid(), signal.SIGTERM) - else: - log.debug('Unknown libvirterror: ecode: %d edom: %d ' - 'level: %d message: %s', ecode, edom, - e.get_error_level(), e.get_error_message()) + if not vdsmLibvirtPing: + conn.getLibVersion(vdsmLibvirtPing=True) + else: + log.error('connection to libvirt broken.' + ' ecode: %d edom: %d', ecode, edom) + if killOnFailure: + log.error('taking calling process down.') + os.kill(os.getpid(), signal.SIGTERM) + else: + raise + log.debug('Unknown libvirterror: ecode: %d edom: %d ' + 'level: %d message: %s', ecode, edom, + e.get_error_level(), e.get_error_message()) raise wrapper.__name__ = f.__name__ wrapper.__doc__ = f.__doc__ @@ -128,7 +132,7 @@ 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(conn, method)) if target is not None: for ev in (libvirt.VIR_DOMAIN_EVENT_ID_LIFECYCLE, libvirt.VIR_DOMAIN_EVENT_ID_REBOOT, diff --git a/tests/libvirtconnectionTests.py b/tests/libvirtconnectionTests.py new file mode 100644 index 0000000..280dad9 --- /dev/null +++ b/tests/libvirtconnectionTests.py @@ -0,0 +1,119 @@ +# +# Copyright 2012 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 +# the Free Software Foundation; either version 2 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write to the Free Software +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA +# 02110-1301 USA +# +# Refer to the README and COPYING files for full details of the license +# + +from vdsm import libvirtconnection +from testrunner import VdsmTestCase as TestCaseBase +import os +from monkeypatch import MonkeyPatch + + +class TerminationException(Exception): + pass + + +class LibvirtMock: + VIR_CRED_AUTHNAME = 1 + VIR_CRED_PASSPHRASE = 2 + VIR_FROM_RPC = 3 + VIR_FROM_REMOTE = 4 + VIR_ERR_SYSTEM_ERROR = 5 + VIR_ERR_INTERNAL_ERROR = 6 + VIR_ERR_NO_CONNECT = 7 + VIR_ERR_INVALID_CONN = 8 + + class libvirtError(Exception): + def get_error_code(self): + return LibvirtMock.VIR_ERR_SYSTEM_ERROR + + def get_error_domain(self): + return LibvirtMock.VIR_FROM_RPC + + def get_error_level(self): + return 3 + + def get_error_message(self): + return '' + + class virConnect: + failGetLibVersion = False + failNodeDeviceLookupByName = False + + def nodeDeviceLookupByName(self): + if LibvirtMock.virConnect.failNodeDeviceLookupByName: + print('nodeDeviceLookupByName called, exception') + + raise LibvirtMock.libvirtError() + else: + print('nodeDeviceLookupByName called, success') + return '' + + def getLibVersion(self): + if LibvirtMock.virConnect.failGetLibVersion: + print('getLibVersion called, exception') + raise LibvirtMock.libvirtError() + else: + print('getLibVersion called, success') + return '' + + class virDomain(): + pass + + def openAuth(self, *args): + return LibvirtMock.virConnect() + + +def _kill(*args): + raise TerminationException() + + +class testLibvirtconnection(TestCaseBase): + @MonkeyPatch(libvirtconnection, 'libvirt', LibvirtMock()) + def testCallSucceeded(self): + """Positive test - libvirtMock does not raise any errors""" + LibvirtMock.virConnect.failGetLibVersion = False + LibvirtMock.virConnect.failNodeDeviceLookupByName = False + connection = libvirtconnection.get() + connection.nodeDeviceLookupByName() + + @MonkeyPatch(libvirtconnection, 'libvirt', LibvirtMock()) + def testCallFailedConnectionUp(self): + """ + libvirtMock will raise an error when nodeDeviceLookupByName is called. + When getLibVersion is called (used by libvirtconnection to recognize disconnections) + it will not raise an error -> in that case an error should be raised ('Unknown libvirterror'). + """ + connection = libvirtconnection.get(killOnFailure=True) + LibvirtMock.virConnect.failNodeDeviceLookupByName = True + LibvirtMock.virConnect.failGetLibVersion = False + self.assertRaises(LibvirtMock.libvirtError, connection.nodeDeviceLookupByName) + + @MonkeyPatch(libvirtconnection, 'libvirt', LibvirtMock()) + @MonkeyPatch(os, 'kill', _kill) + def testCallFailedConnectionDown(self): + """ + libvirtMock will raise an error when nodeDeviceLookupByName is called. + When getLibVersion is called (used by libvirtconnection to recognize disconnections) + it will also raise an error -> in that case os.kill should be called('connection to libvirt broken.'). + """ + connection = libvirtconnection.get(killOnFailure=True) + LibvirtMock.virConnect.failNodeDeviceLookupByName = True + LibvirtMock.virConnect.failGetLibVersion = True + self.assertRaises(TerminationException, connection.nodeDeviceLookupByName) \ No newline at end of file
oVirt Jenkins CI Server has posted comments on this change.
Change subject: libvirtconnection: ping libvirt upon disconnection ......................................................................
Patch Set 1: Code-Review-1 Verified-1
Build Failed
http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/4570/ : FAILURE
http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/4492/ : UNSTABLE
http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/3684/ : FAILURE
oVirt Jenkins CI Server has posted comments on this change.
Change subject: libvirtconnection: ping libvirt upon disconnection ......................................................................
Patch Set 2: Code-Review-1 Verified-1
Build Failed
http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/4571/ : FAILURE
http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/4493/ : UNSTABLE
http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/3685/ : FAILURE
oVirt Jenkins CI Server has posted comments on this change.
Change subject: libvirtconnection: ping libvirt upon disconnection ......................................................................
Patch Set 3:
Build Successful
http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/4572/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/4494/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/3686/ : SUCCESS
Saggi Mizrahi has posted comments on this change.
Change subject: libvirtconnection: ping libvirt upon disconnection ......................................................................
Patch Set 3:
(6 comments)
Good job overall Kudos for writing tests!!
.................................................... File lib/vdsm/libvirtconnection.py Line 70: Wrap methods of connection object so that they catch disconnection, and Line 71: take the current process down. Line 72: """ Line 73: def wrapMethod(conn, f): Line 74: def wrapper(vdsmLibvirtPing=False, *args, **kwargs): Took me a while to get what "vdsmLibvirtPing" means maybe "pingLibvirt", it would align better with what the method does instead of who is using it. Also it would flip the default to true which is always a bit nicer Line 75: try: Line 76: ret = f(*args, **kwargs) Line 77: if isinstance(ret, libvirt.virDomain): Line 78: for name in dir(ret):
Line 90: libvirt.VIR_ERR_NO_CONNECT, Line 91: libvirt.VIR_ERR_INVALID_CONN) Line 92: if edom in EDOMAINS and ecode in ECODES: Line 93: if not vdsmLibvirtPing: Line 94: conn.getLibVersion(vdsmLibvirtPing=True) Since this can hang, it should be asynchronous. Don't forget to lock so you only ping once (YOPO!) Line 95: else: Line 96: log.error('connection to libvirt broken.' Line 97: ' ecode: %d edom: %d', ecode, edom) Line 98: if killOnFailure:
Line 92: if edom in EDOMAINS and ecode in ECODES: Line 93: if not vdsmLibvirtPing: Line 94: conn.getLibVersion(vdsmLibvirtPing=True) Line 95: else: Line 96: log.error('connection to libvirt broken.' warning Line 97: ' ecode: %d edom: %d', ecode, edom) Line 98: if killOnFailure: Line 99: log.error('taking calling process down.') Line 100: os.kill(os.getpid(), signal.SIGTERM)
Line 95: else: Line 96: log.error('connection to libvirt broken.' Line 97: ' ecode: %d edom: %d', ecode, edom) Line 98: if killOnFailure: Line 99: log.error('taking calling process down.') critical Line 100: os.kill(os.getpid(), signal.SIGTERM) Line 101: else: Line 102: raise Line 103: log.debug('Unknown libvirterror: ecode: %d edom: %d '
Line 104: 'level: %d message: %s', ecode, edom, Line 105: e.get_error_level(), e.get_error_message()) Line 106: raise Line 107: wrapper.__name__ = f.__name__ Line 108: wrapper.__doc__ = f.__doc__ a bit unrelated but still. If you could do it in another patch while you are there.
http://docs.python.org/2/library/functools.html#functools.wraps Line 109: return wrapper Line 110: Line 111: def req(credentials, user_data): Line 112: passwd = file(constants.P_VDSM_LIBVIRT_PASSWD).readline().rstrip("\n")
.................................................... File tests/libvirtconnectionTests.py Line 28: class TerminationException(Exception): Line 29: pass Line 30: Line 31: Line 32: class LibvirtMock: please use new style classes
short story: inherit from object long story: http://docs.python.org/release/2.5.2/ref/node33.html Line 33: VIR_CRED_AUTHNAME = 1 Line 34: VIR_CRED_PASSPHRASE = 2 Line 35: VIR_FROM_RPC = 3 Line 36: VIR_FROM_REMOTE = 4
Dan Kenigsberg has posted comments on this change.
Change subject: libvirtconnection: ping libvirt upon disconnection ......................................................................
Patch Set 3: Code-Review-1
(3 comments)
reminder: -1 does not mean "I hate your patch". It means: "I read it, and it is not completely ready".
Thanks for your patch!
.................................................... File lib/vdsm/libvirtconnection.py Line 89: libvirt.VIR_ERR_INTERNAL_ERROR, Line 90: libvirt.VIR_ERR_NO_CONNECT, Line 91: libvirt.VIR_ERR_INVALID_CONN) Line 92: if edom in EDOMAINS and ecode in ECODES: Line 93: if not vdsmLibvirtPing: the logic here is confusing. I'd expect that if the function is told to ping libvirt, it would do it:
if pingLibvirt: conn.getLibVersion()
(I think Saggi has alluded to this, too) Line 94: conn.getLibVersion(vdsmLibvirtPing=True) Line 95: else: Line 96: log.error('connection to libvirt broken.' Line 97: ' ecode: %d edom: %d', ecode, edom)
Line 90: libvirt.VIR_ERR_NO_CONNECT, Line 91: libvirt.VIR_ERR_INVALID_CONN) Line 92: if edom in EDOMAINS and ecode in ECODES: Line 93: if not vdsmLibvirtPing: Line 94: conn.getLibVersion(vdsmLibvirtPing=True) Why should we not block? If getLibVersion() blocks (due to libvirt's sudden hang), there is no reason to return a value. Libvirt could have hung a microsecond earlier, when the wrapped method was called. Line 95: else: Line 96: log.error('connection to libvirt broken.' Line 97: ' ecode: %d edom: %d', ecode, edom) Line 98: if killOnFailure:
.................................................... File tests/libvirtconnectionTests.py Line 1: # Line 2: # Copyright 2012 Red Hat, Inc. it's 2013 Line 3: # Line 4: # This program is free software; you can redistribute it and/or modify Line 5: # it under the terms of the GNU General Public License as published by Line 6: # the Free Software Foundation; either version 2 of the License, or
Yaniv Bronhaim has posted comments on this change.
Change subject: libvirtconnection: ping libvirt upon disconnection ......................................................................
Patch Set 3:
(4 comments)
.................................................... File lib/vdsm/libvirtconnection.py Line 70: Wrap methods of connection object so that they catch disconnection, and Line 71: take the current process down. Line 72: """ Line 73: def wrapMethod(conn, f): Line 74: def wrapper(vdsmLibvirtPing=False, *args, **kwargs): I prefer to omit this variable and don't wrap getLibVersion at all. when testing connectivity we'll call to conn.getLibVersion and if libvirtError is raise we can suicide. Line 75: try: Line 76: ret = f(*args, **kwargs) Line 77: if isinstance(ret, libvirt.virDomain): Line 78: for name in dir(ret):
Line 90: libvirt.VIR_ERR_NO_CONNECT, Line 91: libvirt.VIR_ERR_INVALID_CONN) Line 92: if edom in EDOMAINS and ecode in ECODES: Line 93: if not vdsmLibvirtPing: Line 94: conn.getLibVersion(vdsmLibvirtPing=True) why block? doesn't matter much .. first fail ping we'll die, doesn't matter if in parallel another ping call runs Line 95: else: Line 96: log.error('connection to libvirt broken.' Line 97: ' ecode: %d edom: %d', ecode, edom) Line 98: if killOnFailure:
Line 92: if edom in EDOMAINS and ecode in ECODES: Line 93: if not vdsmLibvirtPing: Line 94: conn.getLibVersion(vdsmLibvirtPing=True) Line 95: else: Line 96: log.error('connection to libvirt broken.' not related to the patch much, change it in another patch Line 97: ' ecode: %d edom: %d', ecode, edom) Line 98: if killOnFailure: Line 99: log.error('taking calling process down.') Line 100: os.kill(os.getpid(), signal.SIGTERM)
Line 104: 'level: %d message: %s', ecode, edom, Line 105: e.get_error_level(), e.get_error_message()) Line 106: raise Line 107: wrapper.__name__ = f.__name__ Line 108: wrapper.__doc__ = f.__doc__ just use @wraps(f) in the same patch you change the logs level Line 109: return wrapper Line 110: Line 111: def req(credentials, user_data): Line 112: passwd = file(constants.P_VDSM_LIBVIRT_PASSWD).readline().rstrip("\n")
Yaniv Bronhaim has posted comments on this change.
Change subject: libvirtconnection: ping libvirt upon disconnection ......................................................................
Patch Set 3:
(1 comment)
.................................................... File lib/vdsm/libvirtconnection.py Line 90: libvirt.VIR_ERR_NO_CONNECT, Line 91: libvirt.VIR_ERR_INVALID_CONN) Line 92: if edom in EDOMAINS and ecode in ECODES: Line 93: if not vdsmLibvirtPing: Line 94: conn.getLibVersion(vdsmLibvirtPing=True) btw, why not using conn = __connections.get(id(target)) -
you have target here, instead of passing conn Line 95: else: Line 96: log.error('connection to libvirt broken.' Line 97: ' ecode: %d edom: %d', ecode, edom) Line 98: if killOnFailure:
oVirt Jenkins CI Server has posted comments on this change.
Change subject: libvirtconnection: ping libvirt upon disconnection ......................................................................
Patch Set 4:
Build Successful
http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/4573/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/4495/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/3688/ : SUCCESS
oVirt Jenkins CI Server has posted comments on this change.
Change subject: libvirtconnection: ping libvirt upon disconnection ......................................................................
Patch Set 5:
Build Successful
http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/4574/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/4496/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/3689/ : SUCCESS
Yaniv Bronhaim has posted comments on this change.
Change subject: libvirtconnection: ping libvirt upon disconnection ......................................................................
Patch Set 5:
(3 comments)
.................................................... File lib/vdsm/libvirtconnection.py Line 70: Wrap methods of connection object so that they catch disconnection, and Line 71: take the current process down. Line 72: """ Line 73: def wrapMethod(f): Line 74: def wrapper(_pingLibvirt=True, *args, **kwargs): underscore is to specified private variables in an object or globals. please omit it from this context Line 75: try: Line 76: ret = f(*args, **kwargs) Line 77: if isinstance(ret, libvirt.virDomain): Line 78: for name in dir(ret):
Line 89: libvirt.VIR_ERR_INTERNAL_ERROR, Line 90: libvirt.VIR_ERR_NO_CONNECT, Line 91: libvirt.VIR_ERR_INVALID_CONN) Line 92: if edom in EDOMAINS and ecode in ECODES: Line 93: if _pingLibvirt: Don't you agree with me that omitting this _pingLibvirt parameter that we pass to each method and instead not wrapping getLibVersion is better here? The only catch is if someone calls the getLibVersion and expects the wrapping flow, which doesn't really hurt here. The caller will see the regular exception. Line 94: conn = __connections.get(id(target)) Line 95: conn.getLibVersion(_pingLibvirt=False) Line 96: else: Line 97: log.error('connection to libvirt broken.'
Line 91: libvirt.VIR_ERR_INVALID_CONN) Line 92: if edom in EDOMAINS and ecode in ECODES: Line 93: if _pingLibvirt: Line 94: conn = __connections.get(id(target)) Line 95: conn.getLibVersion(_pingLibvirt=False) much better, even in one line: __connections.get(id(target)).getLibVersion(_pingLibvirt=False) Line 96: else: Line 97: log.error('connection to libvirt broken.' Line 98: ' ecode: %d edom: %d', ecode, edom) Line 99: if killOnFailure:
Yaniv Bronhaim has posted comments on this change.
Change subject: libvirtconnection: ping libvirt upon disconnection ......................................................................
Patch Set 5:
(2 comments)
.................................................... File tests/libvirtconnectionTests.py Line 36: VIR_FROM_REMOTE = 4 Line 37: VIR_ERR_SYSTEM_ERROR = 5 Line 38: VIR_ERR_INTERNAL_ERROR = 6 Line 39: VIR_ERR_NO_CONNECT = 7 Line 40: VIR_ERR_INVALID_CONN = 8 you can do VIR_CRED_AUTHNAME, VIR_CRED_PASSPHRASE, ... = range(n) Line 41: Line 42: class libvirtError(Exception): Line 43: def get_error_code(self): Line 44: return LibvirtMock.VIR_ERR_SYSTEM_ERROR
Line 46: def get_error_domain(self): Line 47: return LibvirtMock.VIR_FROM_RPC Line 48: Line 49: def get_error_level(self): Line 50: return 3 what is 3 ? VIR_FROM_RPC? Line 51: Line 52: def get_error_message(self): Line 53: return '' Line 54:
mooli tayer has posted comments on this change.
Change subject: libvirtconnection: ping libvirt upon disconnection ......................................................................
Patch Set 5:
(2 comments)
.................................................... File lib/vdsm/libvirtconnection.py Line 89: libvirt.VIR_ERR_INTERNAL_ERROR, Line 90: libvirt.VIR_ERR_NO_CONNECT, Line 91: libvirt.VIR_ERR_INVALID_CONN) Line 92: if edom in EDOMAINS and ecode in ECODES: Line 93: if _pingLibvirt: what if libvirt is down and gitLibVersion is called? we won't catch the error.
From the users perspective i'm concerned that in all other cases he gets one behavior and in gitLibVersion another and he knows nothing about it.
Line 94: conn = __connections.get(id(target)) Line 95: conn.getLibVersion(_pingLibvirt=False) Line 96: else: Line 97: log.error('connection to libvirt broken.'
.................................................... File tests/libvirtconnectionTests.py Line 46: def get_error_domain(self): Line 47: return LibvirtMock.VIR_FROM_RPC Line 48: Line 49: def get_error_level(self): Line 50: return 3 Nope, unrelated, this is just some log level. I'll add a variable for it to prevent confusion. Line 51: Line 52: def get_error_message(self): Line 53: return '' Line 54:
oVirt Jenkins CI Server has posted comments on this change.
Change subject: libvirtconnection: ping libvirt upon disconnection ......................................................................
Patch Set 6: Code-Review-1 Verified-1
Build Failed
http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/4577/ : FAILURE
http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/4499/ : UNSTABLE
http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/3692/ : FAILURE
oVirt Jenkins CI Server has posted comments on this change.
Change subject: libvirtconnection: ping libvirt upon disconnection ......................................................................
Patch Set 7: Code-Review-1 Verified-1
Build Failed
http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/4578/ : FAILURE
http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/4500/ : UNSTABLE
http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/3693/ : FAILURE
oVirt Jenkins CI Server has posted comments on this change.
Change subject: libvirtconnection: ping libvirt upon disconnection ......................................................................
Patch Set 8:
Build Successful
http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/4582/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/4504/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/3697/ : SUCCESS
oVirt Jenkins CI Server has posted comments on this change.
Change subject: libvirtconnection: ping libvirt upon disconnection ......................................................................
Patch Set 9:
Build Successful
http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/4585/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/4507/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/3700/ : SUCCESS
oVirt Jenkins CI Server has posted comments on this change.
Change subject: libvirtconnection: ping libvirt upon disconnection ......................................................................
Patch Set 10:
Build Successful
http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/4586/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/4508/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/3701/ : SUCCESS
Dan Kenigsberg has posted comments on this change.
Change subject: libvirtconnection: ping libvirt upon disconnection ......................................................................
Patch Set 10: Code-Review+1
Yaniv Bronhaim has posted comments on this change.
Change subject: libvirtconnection: ping libvirt upon disconnection ......................................................................
Patch Set 10:
(1 comment)
.................................................... File tests/Makefile.am Line 63: utilsTests.py \ Line 64: vdsClientTests.py \ Line 65: volumeTests.py \ Line 66: zombieReaperTests.py \ Line 67: libvirtconnectionTests.py \ you should put it in alpha-beit order Line 68: $(NULL) Line 69: Line 70: nodist_vdsmtests_PYTHON = \ Line 71: crossImportsTests.py \
mooli tayer has posted comments on this change.
Change subject: libvirtconnection: ping libvirt upon disconnection ......................................................................
Patch Set 10:
(1 comment)
.................................................... File tests/Makefile.am Line 63: utilsTests.py \ Line 64: vdsClientTests.py \ Line 65: volumeTests.py \ Line 66: zombieReaperTests.py \ Line 67: libvirtconnectionTests.py \ Well it's not really ordered, but I shouldn't make things any worse so I'll make another patch set. Later I'll submit another patch sorting this whole thing. Line 68: $(NULL) Line 69: Line 70: nodist_vdsmtests_PYTHON = \ Line 71: crossImportsTests.py \
Yaniv Bronhaim has posted comments on this change.
Change subject: libvirtconnection: ping libvirt upon disconnection ......................................................................
Patch Set 11: Code-Review+1
oVirt Jenkins CI Server has posted comments on this change.
Change subject: libvirtconnection: ping libvirt upon disconnection ......................................................................
Patch Set 11: Verified-1
Build Failed
http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/4602/ : FAILURE
http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/4524/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/3717/ : FAILURE
oVirt Jenkins CI Server has posted comments on this change.
Change subject: libvirtconnection: ping libvirt upon disconnection ......................................................................
Patch Set 12:
Build Successful
http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/4634/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/4556/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/3749/ : SUCCESS
mooli tayer has posted comments on this change.
Change subject: libvirtconnection: ping libvirt upon disconnection ......................................................................
Patch Set 12: Verified-1
What I did to test this patch:
a.) Wrote unit tests b.) Ran cross migration between two hosts running a total of 4 vms. I installed them from ovirt-stable repo and replaced libvirtconnection.py
I discovered that in patch set 11 and before the whole thing was one big bug: I wrote wrapper like so:
def wrapper(pingLibvirt=True, *args, **kwargs): ... ret = f(*args, **kwargs)
and the user of this class called this wrapper for example: conn.nwfilterLookupByName('someArg')
That would lead to: pingLibvirt='someArg' And worse args = []
Shame on me!
After the fix of this patch set the migration still does not work. not sure if it is related to this patch set or not. one host becomes non operational (the engine runs getCaps on it and gives the error: 'Host host_01 running without virtualization hardware acceleration' ) still checking this
Michal Skrivanek has posted comments on this change.
Change subject: libvirtconnection: ping libvirt upon disconnection ......................................................................
Patch Set 12:
(1 comment)
.................................................... File lib/vdsm/libvirtconnection.py Line 131: log.debug('trying to connect libvirt') Line 132: conn = utils.retry(libvirtOpenAuth, timeout=10, sleep=0.2) Line 133: __connections[id(target)] = conn Line 134: Line 135: setattr(conn, 'pingLibvirt', getattr(conn, 'getLibVersion')) since libvirtconnection is trying to be just a wrapper, IMHO it's better to avoid creating of new verbs...why not just use getLibVersion at line 95 Line 136: for name in dir(libvirt.virConnect): Line 137: method = getattr(conn, name) Line 138: if callable(method) and name[0] != '_': Line 139: setattr(conn, name, wrapMethod(method))
Yaniv Bronhaim has posted comments on this change.
Change subject: libvirtconnection: ping libvirt upon disconnection ......................................................................
Patch Set 12: Code-Review+1
(1 comment)
I like the new suggestion
.................................................... File lib/vdsm/libvirtconnection.py Line 131: log.debug('trying to connect libvirt') Line 132: conn = utils.retry(libvirtOpenAuth, timeout=10, sleep=0.2) Line 133: __connections[id(target)] = conn Line 134: Line 135: setattr(conn, 'pingLibvirt', getattr(conn, 'getLibVersion')) because then its wrapped as all other conn functions (line 139) and the call to getLibVersion will go through "wrapper" - we will end up with infinite loop that calls to getLibVersion each run of the except logic Line 136: for name in dir(libvirt.virConnect): Line 137: method = getattr(conn, name) Line 138: if callable(method) and name[0] != '_': Line 139: setattr(conn, name, wrapMethod(method))
Yaniv Bronhaim has posted comments on this change.
Change subject: libvirtconnection: ping libvirt upon disconnection ......................................................................
Patch Set 12: -Code-Review
(1 comment)
Until you figure the testing issue, and you must (I felt that it can be risky), I added another minor comment to consider.
Anyhow, this change need to solve the vdsm restart occurrence. Now the migration to unknown network won't lead to reset of vdsm, but it still won't work until the required network will be configured properly, only appropriate log should be shown in the log (That's what I expected to see)
.................................................... File lib/vdsm/libvirtconnection.py Line 131: log.debug('trying to connect libvirt') Line 132: conn = utils.retry(libvirtOpenAuth, timeout=10, sleep=0.2) Line 133: __connections[id(target)] = conn Line 134: Line 135: setattr(conn, 'pingLibvirt', getattr(conn, 'getLibVersion')) You need to catch here AttributeError. If the API is wrong and getLibVersion won't be there for any reason Line 136: for name in dir(libvirt.virConnect): Line 137: method = getattr(conn, name) Line 138: if callable(method) and name[0] != '_': Line 139: setattr(conn, name, wrapMethod(method))
mooli tayer has posted comments on this change.
Change subject: libvirtconnection: ping libvirt upon disconnection ......................................................................
Patch Set 12:
(1 comment)
Regarding my tests problem, I assume it is not related to my patch but to the issues with live migration disccussed on vdsm-devel.
.................................................... File lib/vdsm/libvirtconnection.py Line 131: log.debug('trying to connect libvirt') Line 132: conn = utils.retry(libvirtOpenAuth, timeout=10, sleep=0.2) Line 133: __connections[id(target)] = conn Line 134: Line 135: setattr(conn, 'pingLibvirt', getattr(conn, 'getLibVersion')) Yaniv is right, using getLibVersion creates an infinate loop. getLibVersion is a wrapped version at the point we need to call ping. If libvirt is down we will keep going through the code in line 94. Doing it this way is the best trade off I found between small fingerprint on the wrapped class and writing code that is simple.
Regarding the AttributeError, as saggy commented, libvirt api is not expected to change and in case it does we should any way let the exception propagate up. Line 136: for name in dir(libvirt.virConnect): Line 137: method = getattr(conn, name) Line 138: if callable(method) and name[0] != '_': Line 139: setattr(conn, name, wrapMethod(method))
Saggi Mizrahi has posted comments on this change.
Change subject: libvirtconnection: ping libvirt upon disconnection ......................................................................
Patch Set 12: Code-Review+1
I approve this as the major issue preventing people from grasping the change is that the original design of the module is backwards.
The whole idea of passing "target" is odd and makes everything look weird.
get() should just give an instance proxy object and than we use standard ways to share the proxy.
This proxy, if existed, would have done these checks and invalidated the connection it proxies.
The immediate next phase would be to change the design to that model so that future logic has a place to reside in.
Yaniv Bronhaim has posted comments on this change.
Change subject: libvirtconnection: ping libvirt upon disconnection ......................................................................
Patch Set 12: Code-Review+1
mooli tayer has posted comments on this change.
Change subject: libvirtconnection: ping libvirt upon disconnection ......................................................................
Patch Set 12: Verified+1
Verified: live migration seems to still be broken on master so I gave that up. Tested on the environment that originally produces this bug. (migration network right after it's creation, see bug)
Dan Kenigsberg has posted comments on this change.
Change subject: libvirtconnection: ping libvirt upon disconnection ......................................................................
Patch Set 12: Code-Review+2
Dan Kenigsberg has submitted this change and it was merged.
Change subject: libvirtconnection: ping libvirt upon disconnection ......................................................................
libvirtconnection: ping libvirt upon disconnection
Upon recieving an error from libvirt we should ping it (using getLibVersion) and only upon failure treat it as down.
Libvirt error codes do not distinguish recoverable from non-recoverable libvirt errors.
Change-Id: Ia489e46dd8ce4c70c888988d17b86311d3c4b935 Bug-Url: https://bugzilla.redhat.com/show_bug.cgi?id=982634 Signed-off-by: Mooli Tayer mtayer@redhat.com Reviewed-on: http://gerrit.ovirt.org/19444 Reviewed-by: Saggi Mizrahi smizrahi@redhat.com Reviewed-by: Yaniv Bronhaim ybronhei@redhat.com Reviewed-by: Dan Kenigsberg danken@redhat.com --- M lib/vdsm/libvirtconnection.py M tests/Makefile.am A tests/libvirtconnectionTests.py 3 files changed, 142 insertions(+), 9 deletions(-)
Approvals: Yaniv Bronhaim: Looks good to me, but someone else must approve Saggi Mizrahi: Looks good to me, but someone else must approve mooli tayer: Verified Dan Kenigsberg: Looks good to me, approved
vdsm-patches@lists.fedorahosted.org