Ondřej Svoboda has posted comments on this change. Change subject: ifcfg: re-enable IPv6 before device configuration, or disable afterwards ......................................................................
Patch Set 5: (5 comments) https://gerrit.ovirt.org/#/c/54555/5/lib/vdsm/network/configurators/__init__.py File lib/vdsm/network/configurators/__init__.py: Line 208: raise ConfigNetworkError(ERR_FAILED_IFUP, 'Device %s was not created ' Line 209: 'during a %ss timeout.' % (name, timeout)) Line 210: Line 211: Line 212: def enable_ipv6(device_name, enable=True, wait_for_ifup=False): > You are right. Sure, your version is more readable. Line 213: if ipv6_supported(): Line 214: if wait_for_ifup: Line 215: wait_for_device(device_name) Line 216: try: https://gerrit.ovirt.org/#/c/54555/5/lib/vdsm/network/configurators/ifcfg.py File lib/vdsm/network/configurators/ifcfg.py: 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: > This seems to be a common thing to all devices, we can locate it in _ifup o Only the top level device carries IP configuration so I have to query it to achieve consistency. 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/lib/vdsm/network/models.py File lib/vdsm/network/models.py: Line 444: # TODO: remove in favour of the explicit check below Line 445: def __nonzero__(self): Line 446: return bool(self.address or self.ipv6autoconf or self.dhcpv6) Line 447: Line 448: @property > You can suggest this replacement in a separated patch. I was afraid the use of __nonzero__ (replaced by __bool__ in Python 3) would make code review harder. But since you are aware now that I want to move to an explicit property, I will do this change to another patch. Line 449: def requested(self): Line 450: return bool(self.address or self.ipv6autoconf or self.dhcpv6) Line 451: Line 452: def __repr__(self): https://gerrit.ovirt.org/#/c/54555/5/tests/functional/networkTests.py File tests/functional/networkTests.py: Line 1922: if 6 in families: Line 1923: self.assertIn(IPv6_ADDRESS_AND_CIDR, test_net['ipv6addrs']) Line 1924: self.assertEqual(IPv6_GATEWAY, test_net['ipv6gateway']) Line 1925: else: Line 1926: self.assertEqual([], test_net['ipv6addrs']) > self.assertEqual(1, sysctl.is_disabled_ipv6(NETWORK_NAME)) I'll add this. Line 1927: Line 1928: with self.vdsm_net.pinger(): Line 1929: for families in ip_reconfigurations: Line 1930: run_test_instance(self, families) 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(): > I got confused. Do we have two permutation levels here? Yes and no. Before, a network was configured and removed. Now, to simulate some anticipated scenarios of reconfiguration, the network is created e.g. as IPv4-only, then changed to dual-stack, and then back to IPv4-only. The key thing I want to test here is that IPv6 is successfully reenabled (after being disabled, say, on boot) and disabled again, none of which we had before. And only then the network is removed. Line 1929: for families in ip_reconfigurations: Line 1930: run_test_instance(self, families) Line 1931: Line 1932: delete = {NETWORK_NAME: {'remove': True}} -- 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 <osvob...@redhat.com> Gerrit-Reviewer: Dan Kenigsberg <dan...@redhat.com> Gerrit-Reviewer: Edward Haas <edwa...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Ondřej Svoboda <osvob...@redhat.com> 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/mailman/listinfo/vdsm-patches
