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 <[email protected]> Gerrit-Reviewer: Antoni Segura Puimedon <[email protected]> Gerrit-Reviewer: Assaf Muller <[email protected]> Gerrit-Reviewer: Dan Kenigsberg <[email protected]> Gerrit-Reviewer: Ondřej Svoboda <[email protected]> Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
