Ondřej Svoboda has posted comments on this change. Change subject: ifcfg: re-enable IPv6 before device configuration, or disable afterwards ......................................................................
Patch Set 5: (6 comments) https://gerrit.ovirt.org/#/c/54555/5/lib/vdsm/network/configurators/__init__.py File lib/vdsm/network/configurators/__init__.py: > I would recommend considering that when it will be needed, not now. At the First, I'll apply your suggestions to the function as it stands here. And since we probably don't want other configurators disadvantaged in the strict tests, I'll try to complete the implementation in them, seeing for myself if there is the need for common code, or not. Line 1: # Copyright 2013-2014 Red Hat, Inc. 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 https://gerrit.ovirt.org/#/c/54555/5/lib/vdsm/network/configurators/ifcfg.py File lib/vdsm/network/configurators/ifcfg.py: Line 50: Line 51: if utils.isOvirtNode(): Line 52: from ovirt.node.utils import fs as node_fs Line 53: Line 54: from . import Configurator, enable_ipv6, getEthtoolOpts > Please do not fix this in this context. Done Line 55: from . import dhclient Line 56: from . import libvirt Line 57: from ..errors import ConfigNetworkError, ERR_FAILED_IFUP Line 58: from ..models import Nic, Bridge, IPv4, IPv6 Line 100: self.runningConfig.save() Line 101: self.runningConfig = None Line 102: Line 103: def configureBridge(self, bridge, **opts): Line 104: if bridge.ipv6.requested: > As I see it, if no IPv6 has been set for a network, none of its devices sho For the record, I believe we all agreed to keep to my all-or-nothing approach. If this is not true, please remind me. One more point perhaps: while we can rely on the kernel not changing its networking behaviour, we cannot be sure about any higher-level components (NetworkManager/systemd-networkd/others?). So if we always set disable_ipv6 on all devices below the bridge and expected networking to work, we might run into problems someday. I really want the property to have the "natural" value, thus for all devices to get their link-local addresses. Unless, by our rule, the top-level device ("the network") had IPv6 disabled. IPv6 is enabled, "natural", on current systems by default, and this is very unlikely to change in the foreseeable future. Line 105: enable_ipv6(bridge.name) Line 106: self.configApplier.addBridge(bridge, **opts) Line 107: ifdown(bridge.name) Line 108: if bridge.port: https://gerrit.ovirt.org/#/c/54555/5/tests/functional/networkTests.py File tests/functional/networkTests.py: Line 1924: self.assertEqual(IPv6_GATEWAY, test_net['ipv6gateway']) Line 1925: else: Line 1926: self.assertEqual([], test_net['ipv6addrs']) Line 1927: Line 1928: with self.vdsm_net.pinger(): > Sounds good to me. I gave the helper function a better name. I think it is sufficient. Line 1929: for families in ip_reconfigurations: Line 1930: run_test_instance(self, families) Line 1931: Line 1932: delete = {NETWORK_NAME: {'remove': True}} Line 1930: self, > this is not a method; do not pass self. Done Line 2194: enable_ipv6 > Better use sysctl (and in all other places) Why? enable_ipv6 protects from trying to touch /proc sysctl when IPv6 is disabled on boot. It doesn't wait for the NIC by default, so it is actually a useful wrapper even in tests. -- To view, visit https://gerrit.ovirt.org/54555 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Idddfb096e6ea384dbe6655c5c4178d4884a8db85 Gerrit-PatchSet: 5 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Ondřej Svoboda <[email protected]> Gerrit-Reviewer: Dan Kenigsberg <[email protected]> Gerrit-Reviewer: Edward Haas <[email protected]> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Ondřej Svoboda <[email protected]> Gerrit-Reviewer: Petr Horáček <[email protected]> Gerrit-Reviewer: gerrit-hooks <[email protected]> Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
