Antoni Segura Puimedon has uploaded a new change for review.
Change subject: BZ#840594 Fix netinfo when libvirt bridge does not exist. ......................................................................
BZ#840594 Fix netinfo when libvirt bridge does not exist.
In the case where a bridge that is configured in libvirt is manually removed or renamed, netinfo throws an exception when trying to discover which ports are assigned to the bridge.
This patch catches the OSError exception of trying to list the ports of a non- existing bridge interface, effectively filtering the non-existing interface from the network capabilities.
Change-Id: I2538a8ce2fafd8efed834b87b20a63d058afd96b Signed-off-by: Antoni S. Puimedon asegurap@redhat.com --- M vdsm/netinfo.py 1 file changed, 14 insertions(+), 4 deletions(-)
git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/27/7127/1
diff --git a/vdsm/netinfo.py b/vdsm/netinfo.py index 4fbd13a..40483ed 100644 --- a/vdsm/netinfo.py +++ b/vdsm/netinfo.py @@ -19,6 +19,7 @@ #
import os +import errno import glob import ethtool import shlex @@ -241,10 +242,19 @@ nets = networks() for netname in nets.iterkeys(): if nets[netname]['bridged']: - devname = netname - d['networks'][netname] = { 'ports': ports(devname), - 'stp': bridge_stp_state(devname), - 'cfg': getIfaceCfg(devname), } + try: + devname = netname + d['networks'][netname] = { 'ports': ports(devname), + 'stp': bridge_stp_state(devname), + 'cfg': getIfaceCfg(devname), } + except OSError, e: + # If the bridge reported by libvirt does not exist anymore, do + # not report it, as this already assures that the bridge is not + # added d['networks'] + if e == errno.ENOENT: + continue + else: + raise else: devname = nets[netname]['iface'] d['networks'][netname] = {}
-- To view, visit http://gerrit.ovirt.org/7127 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: newchange Gerrit-Change-Id: I2538a8ce2fafd8efed834b87b20a63d058afd96b Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Antoni Segura Puimedon asegurap@redhat.com
Igor Lvovsky has posted comments on this change.
Change subject: BZ#840594 Fix netinfo when libvirt bridge does not exist. ......................................................................
Patch Set 1: I would prefer that you didn't submit this
(2 inline comments)
.................................................... File vdsm/netinfo.py Line 253: # added d['networks'] Line 254: if e == errno.ENOENT: Line 255: continue Line 256: else: Line 257: raise hmmm, I am not really like it. what about just check whether this bridge exists: if not devname in bridges(): continue Line 258: else: Line 259: devname = nets[netname]['iface'] Line 260: d['networks'][netname] = {} Line 261:
Line 269: 'bridged': nets[netname]['bridged'], Line 270: 'addr': getaddr(devname), Line 271: 'netmask': getnetmask(devname), Line 272: 'gateway': routes.get(devname, '0.0.0.0'), Line 273: 'mtu': getMtu(devname), }) Actually, you will fail here too for such situation Line 274: Line 275: d['bridges'] = dict([(bridge, { Line 276: 'ports': ports(bridge), Line 277: 'stp': bridge_stp_state(bridge),
-- To view, visit http://gerrit.ovirt.org/7127 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I2538a8ce2fafd8efed834b87b20a63d058afd96b Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Antoni Segura Puimedon asegurap@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: Livnat Peer lpeer@redhat.com
Igor Lvovsky has posted comments on this change.
Change subject: BZ#840594 Fix netinfo when libvirt bridge does not exist. ......................................................................
Patch Set 1: (1 inline comment)
.................................................... File vdsm/netinfo.py Line 269: 'bridged': nets[netname]['bridged'], Line 270: 'addr': getaddr(devname), Line 271: 'netmask': getnetmask(devname), Line 272: 'gateway': routes.get(devname, '0.0.0.0'), Line 273: 'mtu': getMtu(devname), }) ah, sorry my bad. this one under same 'for' statement Line 274: Line 275: d['bridges'] = dict([(bridge, { Line 276: 'ports': ports(bridge), Line 277: 'stp': bridge_stp_state(bridge),
-- To view, visit http://gerrit.ovirt.org/7127 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I2538a8ce2fafd8efed834b87b20a63d058afd96b Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Antoni Segura Puimedon asegurap@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: Livnat Peer lpeer@redhat.com
Antoni Segura Puimedon has posted comments on this change.
Change subject: BZ#840594 Fix netinfo when libvirt bridge does not exist. ......................................................................
Patch Set 1: (1 inline comment)
.................................................... File vdsm/netinfo.py Line 253: # added d['networks'] Line 254: if e == errno.ENOENT: Line 255: continue Line 256: else: Line 257: raise It is a case of "It is easier to ask for forgiveness than permission". http://docs.python.org/glossary.html#term-eafp http://en.wikipedia.org/wiki/Python_syntax_and_semantics#Exceptions
The usual Python style would be to put it in the way I did (in contrast to the Java style).
As for performance, it depends if the exceptional case is very common, i.e., how likely it is for a call to this method to happen when the user to has manually modified a bridge vs. no manual modification has happened. Line 258: else: Line 259: devname = nets[netname]['iface'] Line 260: d['networks'][netname] = {} Line 261:
-- To view, visit http://gerrit.ovirt.org/7127 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I2538a8ce2fafd8efed834b87b20a63d058afd96b Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Antoni Segura Puimedon asegurap@redhat.com Gerrit-Reviewer: Antoni Segura Puimedon asegurap@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: Livnat Peer lpeer@redhat.com
Dan Kenigsberg has posted comments on this change.
Change subject: BZ#840594 Fix netinfo when libvirt bridge does not exist. ......................................................................
Patch Set 1: I would prefer that you didn't submit this
(2 inline comments)
.................................................... File vdsm/netinfo.py Line 242: nets = networks() Line 243: for netname in nets.iterkeys(): Line 244: if nets[netname]['bridged']: Line 245: try: Line 246: devname = netname this assignment should certainly go out of the try-block Line 247: d['networks'][netname] = { 'ports': ports(devname), Line 248: 'stp': bridge_stp_state(devname), Line 249: 'cfg': getIfaceCfg(devname), } Line 250: except OSError, e:
Line 253: # added d['networks'] Line 254: if e == errno.ENOENT: Line 255: continue Line 256: else: Line 257: raise Toni, you are correct about Python-ness, however, swallowing the exception is not recommended. Could you log the error (and its trace back?) notice that the try-except block contains more than just the ports() function - other things may fail, and when they do, I'd like to know that.
if the log proves too dirty and too common, we can later remove the traceback. Line 258: else: Line 259: devname = nets[netname]['iface'] Line 260: d['networks'][netname] = {} Line 261:
-- To view, visit http://gerrit.ovirt.org/7127 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I2538a8ce2fafd8efed834b87b20a63d058afd96b Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Antoni Segura Puimedon asegurap@redhat.com Gerrit-Reviewer: Antoni Segura Puimedon asegurap@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: Livnat Peer lpeer@redhat.com
Antoni Segura Puimedon has posted comments on this change.
Change subject: BZ#840594 Fix netinfo when libvirt bridge does not exist. ......................................................................
Patch Set 1: (2 inline comments)
Added some comments.
.................................................... File vdsm/netinfo.py Line 242: nets = networks() Line 243: for netname in nets.iterkeys(): Line 244: if nets[netname]['bridged']: Line 245: try: Line 246: devname = netname Done Line 247: d['networks'][netname] = { 'ports': ports(devname), Line 248: 'stp': bridge_stp_state(devname), Line 249: 'cfg': getIfaceCfg(devname), } Line 250: except OSError, e:
Line 253: # added d['networks'] Line 254: if e == errno.ENOENT: Line 255: continue Line 256: else: Line 257: raise In principle at least for bridge_stp_state the error should be for the same cause, but in getIfaceCfg it might be different. I will add the log as requested and resubmit. Line 258: else: Line 259: devname = nets[netname]['iface'] Line 260: d['networks'][netname] = {} Line 261:
-- To view, visit http://gerrit.ovirt.org/7127 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I2538a8ce2fafd8efed834b87b20a63d058afd96b Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Antoni Segura Puimedon asegurap@redhat.com Gerrit-Reviewer: Antoni Segura Puimedon asegurap@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: Livnat Peer lpeer@redhat.com
Dan Kenigsberg has posted comments on this change.
Change subject: BZ#840594 Fix netinfo when libvirt bridge does not exist. ......................................................................
Patch Set 2: I would prefer that you didn't submit this
(2 inline comments)
.................................................... File vdsm/netinfo.py Line 250: except OSError, e: Line 251: # If the bridge reported by libvirt does not exist anymore, do Line 252: # not report it, as this already assures that the bridge is not Line 253: # added d['networks'] Line 254: if e == errno.ENOENT: I'm pretty sure you have to
e.errno == errno.ENOENT
would you add a unit test for netinfo.get(), and this case specifically? Line 255: logging.info('An exception occurred while trying to get ' Line 256: 'information from a bridged network.', Line 257: exc_info=True) Line 258: continue
Line 251: # If the bridge reported by libvirt does not exist anymore, do Line 252: # not report it, as this already assures that the bridge is not Line 253: # added d['networks'] Line 254: if e == errno.ENOENT: Line 255: logging.info('An exception occurred while trying to get ' this is a bit of a long story: I prefer more informative strings such as
'obtaining info for net %s' Line 256: 'information from a bridged network.', Line 257: exc_info=True) Line 258: continue Line 259: else:
-- To view, visit http://gerrit.ovirt.org/7127 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I2538a8ce2fafd8efed834b87b20a63d058afd96b Gerrit-PatchSet: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Antoni Segura Puimedon asegurap@redhat.com Gerrit-Reviewer: Antoni Segura Puimedon asegurap@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: Livnat Peer lpeer@redhat.com
oVirt Jenkins CI Server has posted comments on this change.
Change subject: BZ#840594 Fix netinfo when libvirt bridge does not exist. ......................................................................
Patch Set 3:
Build Failed
http://jenkins.ovirt.info/job/patch_vdsm_unit_tests/458/ : ABORTED
-- To view, visit http://gerrit.ovirt.org/7127 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I2538a8ce2fafd8efed834b87b20a63d058afd96b Gerrit-PatchSet: 3 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Antoni Segura Puimedon asegurap@redhat.com Gerrit-Reviewer: Antoni Segura Puimedon asegurap@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: Livnat Peer lpeer@redhat.com Gerrit-Reviewer: oVirt Jenkins CI Server
Dan Kenigsberg has posted comments on this change.
Change subject: BZ#840594 Fix netinfo when libvirt bridge does not exist. ......................................................................
Patch Set 3: I would prefer that you didn't submit this
(2 inline comments)
.................................................... File vdsm/netinfo.py Line 242: nets = networks() Line 243: for netname in nets.iterkeys(): Line 244: if nets[netname]['bridged']: Line 245: try: Line 246: devname = netname this has got back into the try-block. Line 247: d['networks'][netname] = { 'ports': ports(devname), Line 248: 'stp': bridge_stp_state(devname), Line 249: 'cfg': getIfaceCfg(devname), } Line 250: except OSError, e:
Line 250: except OSError, e: Line 251: # If the bridge reported by libvirt does not exist anymore, do Line 252: # not report it, as this already assures that the bridge is not Line 253: # added d['networks'] Line 254: print e.errno seems that you have posted a private version. Line 255: print errno.ENOENT Line 256: if e.errno == errno.ENOENT: Line 257: logging.info('Obtaining info for net %s.', devname, Line 258: exc_info=True)
-- To view, visit http://gerrit.ovirt.org/7127 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I2538a8ce2fafd8efed834b87b20a63d058afd96b Gerrit-PatchSet: 3 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Antoni Segura Puimedon asegurap@redhat.com Gerrit-Reviewer: Antoni Segura Puimedon asegurap@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: Livnat Peer lpeer@redhat.com Gerrit-Reviewer: oVirt Jenkins CI Server
oVirt Jenkins CI Server has posted comments on this change.
Change subject: BZ#840594 Fix netinfo when libvirt bridge does not exist. ......................................................................
Patch Set 4:
Build Failed
http://jenkins.ovirt.info/job/patch_vdsm_unit_tests/502/ : ABORTED
-- To view, visit http://gerrit.ovirt.org/7127 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I2538a8ce2fafd8efed834b87b20a63d058afd96b Gerrit-PatchSet: 4 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Antoni Segura Puimedon asegurap@redhat.com Gerrit-Reviewer: Antoni Segura Puimedon asegurap@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: Livnat Peer lpeer@redhat.com Gerrit-Reviewer: oVirt Jenkins CI Server
Dan Kenigsberg has posted comments on this change.
Change subject: BZ#840594 Fix netinfo when libvirt bridge does not exist. ......................................................................
Patch Set 4: Looks good to me, approved
-- To view, visit http://gerrit.ovirt.org/7127 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I2538a8ce2fafd8efed834b87b20a63d058afd96b Gerrit-PatchSet: 4 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Antoni Segura Puimedon asegurap@redhat.com Gerrit-Reviewer: Antoni Segura Puimedon asegurap@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: Livnat Peer lpeer@redhat.com Gerrit-Reviewer: oVirt Jenkins CI Server
Dan Kenigsberg has posted comments on this change.
Change subject: BZ#840594 Fix netinfo when libvirt bridge does not exist. ......................................................................
Patch Set 4: Verified
verified with libvirt net without a compatible bridge.
-- To view, visit http://gerrit.ovirt.org/7127 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I2538a8ce2fafd8efed834b87b20a63d058afd96b Gerrit-PatchSet: 4 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Antoni Segura Puimedon asegurap@redhat.com Gerrit-Reviewer: Antoni Segura Puimedon asegurap@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: Livnat Peer lpeer@redhat.com Gerrit-Reviewer: oVirt Jenkins CI Server
Dan Kenigsberg has submitted this change and it was merged.
Change subject: BZ#840594 Fix netinfo when libvirt bridge does not exist. ......................................................................
BZ#840594 Fix netinfo when libvirt bridge does not exist.
In the case where a bridge that is configured in libvirt is manually removed or renamed, netinfo throws an exception when trying to discover which ports are assigned to the bridge.
This patch catches the OSError exception of trying to list the ports of a non- existing bridge interface, effectively filtering the non-existing interface from the network capabilities.
Also adds a unittest for getting network information of non-existing bridges.
Change-Id: I2538a8ce2fafd8efed834b87b20a63d058afd96b Signed-off-by: Antoni S. Puimedon asegurap@redhat.com --- M tests/netinfoTests.py M vdsm/netinfo.py 2 files changed, 23 insertions(+), 3 deletions(-)
Approvals: Dan Kenigsberg: Verified; Looks good to me, approved
-- To view, visit http://gerrit.ovirt.org/7127 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: merged Gerrit-Change-Id: I2538a8ce2fafd8efed834b87b20a63d058afd96b Gerrit-PatchSet: 4 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Antoni Segura Puimedon asegurap@redhat.com Gerrit-Reviewer: Antoni Segura Puimedon asegurap@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: Livnat Peer lpeer@redhat.com Gerrit-Reviewer: oVirt Jenkins CI Server
vdsm-patches@lists.fedorahosted.org