Ondřej Svoboda has posted comments on this change.
Change subject: netinfo: Determine bootproto from dhclient's lease files
......................................................................
Patch Set 9:
(11 comments)
http://gerrit.ovirt.org/#/c/23098/9/lib/vdsm/netinfo.py
File lib/vdsm/netinfo.py:
Line 49: NET_CONF_BACK_DIR = constants.P_VDSM_LIB + 'netconfback/'
Line 50: NET_LOGICALNET_CONF_BACK_DIR = NET_CONF_BACK_DIR + 'logicalnetworks/'
Line 51:
Line 52: # where dhclient stores its DHCP lease files as NetworkManager's slave
Line 53: DHCLIENT_LEASES_GLOB = '/var/lib/NetworkManager/dhclient-*.lease'
this can be declared as _PRIVATE
Done
Line 54:
Line 55: NET_CONF_PREF = NET_CONF_DIR + 'ifcfg-'
Line 56: PROC_NET_VLAN = '/proc/net/vlan/'
Line 57: NET_PATH = '/sys/class/net'
Line 545: 'mtu': str(link.mtu),
Line 546: 'netmask': ipv4netmask}
Line 547:
Line 548:
Line 549: def
getDhclientIfaces(leaseFilesGlob='/var/lib/dhclient/dhclient.leases',
do we use this default argument anywhere? I'd rather pass it
explicitly the
Done
Line 550: version=''):
Line 551: """Returns a set of interfaces configured using dhclient.
Line 552:
Line 553: dhclient stores DHCP leases to file(s) whose names can be specified by the
Line 546: 'netmask': ipv4netmask}
Line 547:
Line 548:
Line 549: def
getDhclientIfaces(leaseFilesGlob='/var/lib/dhclient/dhclient.leases',
Line 550: version=''):
An argument ipv6=False would be much clearer.
Done
Line 551: """Returns a set of interfaces configured using dhclient.
Line 552:
Line 553: dhclient stores DHCP leases to file(s) whose names can be specified by the
Line 554: leaseFilesGlob parameter, e.g.
'/var/lib/NetworkManager/dhclient6-*.lease'.
Line 575: elif line == LEASE:
Line 576: insideLease = True
Line 577: elif line == '}\n':
Line 578: insideLease = False
Line 579: except IOError:
never silently ignore such an exception - always log it.
Done
Line 580: pass
Line 581:
Line 582: return interfaces
Line 583:
http://gerrit.ovirt.org/#/c/23098/9/tests/functional/dhcp.py
File tests/functional/dhcp.py:
Line 1: # Copyright 2014 Red Hat, Inc.
some of this code is from 2013
Done
Line 2: #
Line 3: # This program is free software; you can redistribute it and/or modify
Line 4: # it under the terms of the GNU General Public License as published by
Line 5: # the Free Software Foundation; either version 2 of the License, or
Line 34: class DhcpError(Exception):
Line 35: pass
Line 36:
Line 37:
Line 38: def runDnsmasq(interface, dhcpRangeFrom, dhcpRangeTo):
I liked the former abstraction. Would you keep it?
Done
Line 39: # --dhcp-option=3 don't send gateway address which would break routing
Line 40: # -k do not daemonize
Line 41: # -p 0 disable all the dnsmasq dns functionality
Line 42: proc = execCmd([_DNSMASQ_BINARY.cmd, '-d',
'--dhcp-authoritative',
Line 61: """Creating our own 'connection' with a static address
prevents Network
Line 62: Manager from running dhclient on the interface.
Line 63:
Line 64: And so it does not interfere with dhclient we are going to
run."""
Line 65: proc = execCmd([_NM_CLI_BINARY.cmd, 'connection', 'add',
please use
Done
Line 66: 'type', 'ethernet', 'ifname',
interface,
Line 67: 'con-name', connection, 'autoconnect',
'yes',
Line 68: 'ip4', '12.34.56.78'], sync=True)
Line 69:
Line 64: And so it does not interfere with dhclient we are going to
run."""
Line 65: proc = execCmd([_NM_CLI_BINARY.cmd, 'connection', 'add',
Line 66: 'type', 'ethernet', 'ifname',
interface,
Line 67: 'con-name', connection, 'autoconnect',
'yes',
Line 68: 'ip4', '12.34.56.78'], sync=True)
sync=True is the default. no reason to put it here.
Done
Line 69:
Line 70: if proc[0]: # return code
Line 71: raise SkipTest('Could not add a placeholder NM connection.')
Line 72:
http://gerrit.ovirt.org/#/c/23098/9/tests/functional/firewall.py
File tests/functional/firewall.py:
Line 69: veth, '-p', 'udp', '--sport',
'68', '--dport',
Line 70: '67', '-j', 'ACCEPT'])
Line 71: elif _serviceRunning('firewalld'):
Line 72: _execCmdChecker([_FIREWALLD_BINARY.cmd, '--zone=trusted',
Line 73: '--remove-interface=' + veth])
we should at least log an error on the "else" block.
Printing it out as it should not cause the test to fail.
Line 74:
Line 75:
Line 76: def _serviceRunning(name):
Line 77: ret, _, _ = execCmd([_SERVICE_BINARY.cmd, name, 'status'])
http://gerrit.ovirt.org/#/c/23098/9/tests/functional/networkTests.py
File tests/functional/networkTests.py:
Line 105: if not pgrep('NetworkManager'): # NO-OP if NM is not running
Line 106: try:
Line 107: yield
Line 108: finally:
Line 109: pass # 'return' eats exceptions
so you should avoid the whole try-finally block.
Done
Line 110:
Line 111: else:
Line 112: connectionName = 'placeholder-' + interface
Line 113: try:
Line 110:
Line 111: else:
Line 112: connectionName = 'placeholder-' + interface
Line 113: try:
Line 114: dhcp.addNMplaceholderConnection(interface, connectionName)
currently, this function is a generator with too "yield"s
and is a bit conf
Done
Line 115: yield
Line 116: finally:
Line 117: dhcp.removeNMplaceholderConnection(connectionName)
Line 118:
--
To view, visit
http://gerrit.ovirt.org/23098
To unsubscribe, visit
http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I5fbc48a0adf5f40120a72ec2c4cc2fc80b7226b8
Gerrit-PatchSet: 9
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ondřej Svoboda <osvoboda(a)redhat.com>
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap(a)redhat.com>
Gerrit-Reviewer: Assaf Muller <amuller(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Ondřej Svoboda <osvoboda(a)redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes