Edward Haas has posted comments on this change. Change subject: net: disable unused IPv6 in ip.address.add() ......................................................................
Patch Set 6: Code-Review-1 (5 comments) https://gerrit.ovirt.org/#/c/59002/6/lib/vdsm/network/ip/address.py File lib/vdsm/network/ip/address.py: Line 172: ipwrapper.addrAdd(iface, ipv4.address, ipv4.netmask) Line 173: if ipv4.gateway and ipv4.defaultRoute: Line 174: ipwrapper.routeAdd(['default', 'via', ipv4.gateway]) Line 175: if ipv6: Line 176: if ipv6.address: Please extract into its own func. Line 177: ipv6addr, ipv6netmask = ipv6.address.split('/') Line 178: ipwrapper.addrAdd(iface, ipv6addr, ipv6netmask, family=6) Line 179: if ipv6.gateway: Line 180: ipwrapper.routeAdd(['default', 'via', ipv6.gateway], PS6, Line 186: ipv6_supported This can also be moved here. It will be better for netinfo to be dependent on ip and not the opposite. Line 183: with open('/proc/sys/net/ipv6/conf/%s/autoconf' % iface, Line 184: 'w') as ipv6_autoconf: Line 185: ipv6_autoconf.write('1' if ipv6.ipv6autoconf else '0') Line 186: elif netinfo_misc.ipv6_supported(): Line 187: _wait_for_device(iface) # see 5665f9dd0bd0e97c14a70acf54c5646d33b3ee42 It makes no sense doing this here. As with ifcfg handling, it needs to be done at creation point, not before using it. If the iface does not exists, even setting the ip will not succeed. See _ifup(). Can we be optimistic and assume that ovs does create the ifaces when the command returns? Line 188: sysctl.disable_ipv6(iface) Line 189: Line 190: Line 191: def flush(iface): https://gerrit.ovirt.org/#/c/59002/6/tests/network/netfunctestlib.py File tests/network/netfunctestlib.py: PS6, Line 272: 'ipv6addr' not in attrs and 'ipv6autoconf' not in attrs and : 'dhcpv6' not in attrs and misc.ipv6_supported() Extract to a func Line 286: def assertStaticIPv6(self, netattrs, ipinfo): Line 287: self.assertIn(netattrs['ipv6addr'], ipinfo['ipv6addrs']) Line 288: Line 289: def assertDisabledIPv6(self, ipinfo): Line 290: self.assertEqual([], ipinfo['ipv6addrs']) This actually show that there is no IPv6 address. We may have a scenario where IPv6 autoconf or DHCPv6 is enabled but no address was acquired. I think we need a TODO here: We need to report if IPv6 is enabled on iface/host and differentiate that from not acquiring an address. Line 291: Line 292: def assertLinksUp(self, net, attrs): Line 293: switch = attrs.get('switch', 'legacy') Line 294: if switch == 'legacy': -- To view, visit https://gerrit.ovirt.org/59002 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I6ea45e23f5a331cbb205cbb40f96dc3b4bc92958 Gerrit-PatchSet: 6 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Petr Horáček <phora...@redhat.com> Gerrit-Reviewer: Dan Kenigsberg <dan...@redhat.com> Gerrit-Reviewer: Edward Haas <edwa...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Petr Horáček <phora...@redhat.com> Gerrit-Reviewer: gerrit-hooks <automat...@ovirt.org> Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org
