Dan Kenigsberg has posted comments on this change. Change subject: networkTests: Extend a test to also consider DHCPv6 ......................................................................
Patch Set 14: Code-Review-1 (4 comments) http://gerrit.ovirt.org/#/c/30532/14/tests/functional/dhcp.py File tests/functional/dhcp.py: Line 48: class Dnsmasq(): Line 49: def __init__(self): Line 50: self.proc = None Line 51: Line 52: def start(self, interface, family=4): what is the benefit of supplying family here? Why not always support both families? In any case, using 46 to signify "both families" is very confusing. the only benefit that I see is the falure on el6. For to avoid this, I suggest to pass a boolean include_ipv6, which could be dropped when we drop el6 support. Line 53: # -p 0 disable all the dnsmasq dns functionality Line 54: # --dhcp-option=3 don't send gateway address which would break routing Line 55: # -O 6 don't reply with any DNS servers either Line 56: # -d do not daemonize and log to stderr http://gerrit.ovirt.org/#/c/30532/14/tests/functional/networkTests.py File tests/functional/networkTests.py: Line 1835: {BONDING_NAME: {'remove': True}}, Line 1836: NOCHK) Line 1837: Line 1838: @permutations([[(True, 4)], [(True, 6)], [(True, 46)], Line 1839: [(False, 4)], [(False, 6)], [(False, 46)]]) what's family 46? In this level it can make sense, but we should define a local constant _BOTH_4_AND_6 to make the meaning clearer. Maybe even replace the arg with "families" which has values of set((4,) set((6,) set((4,6)) Line 1840: @cleanupNet Line 1841: @RequireVethMod Line 1842: @ValidateRunningAsRoot Line 1843: def testSetupNetworksAddDelDhcp(self, (bridged, family)): Line 1843: def testSetupNetworksAddDelDhcp(self, (bridged, family)): Line 1844: if (family != 4 Line 1845: and caps.getos() in (caps.OSName.RHEVH, caps.OSName.RHEL) Line 1846: and caps.osversion()['version'].startswith('6')): Line 1847: raise SkipTest('el6’s dnsmasq does not support DHCPv6') The char ’ is not ascii please drop it. \' is good enough. Line 1848: Line 1849: with vethIf() as (left, right): Line 1850: veth.setIP(left, IP_ADDRESS, IP_CIDR, 4) Line 1851: veth.setIP(left, IPv6_ADDRESS, IPv6_CIDR, 6) Line 1852: veth.setLinkUp(left) Line 1853: if family != 4: # needed for a link-local address Line 1854: veth.setLinkUp(right) Line 1855: with dnsmasqDhcp(left, 46): Line 1856: bootproto = family != 6 and 'dhcp' or 'none' with new syntax and 'families' suggestion this would look 'dhcp' if 4 in families else 'none' which I find much clearer. Line 1857: network = {NETWORK_NAME: {'nic': right, 'bridged': bridged, Line 1858: 'bootproto': bootproto, Line 1859: 'dhcpv6': family != 4, Line 1860: 'blockingdhcp': True}} -- To view, visit http://gerrit.ovirt.org/30532 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ic5d821edd54681a7a8c1013a90af61ae835baa39 Gerrit-PatchSet: 14 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Ondřej Svoboda <osvob...@redhat.com> Gerrit-Reviewer: Antoni Segura Puimedon <asegu...@redhat.com> Gerrit-Reviewer: Dan Kenigsberg <dan...@redhat.com> Gerrit-Reviewer: Ondřej Svoboda <osvob...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
