Change in vdsm[master]: ifcfg: re-enable IPv6 before device configuration, or disabl...

edwardh at redhat.com edwardh at redhat.com
Mon Mar 21 21:42:54 UTC 2016


Edward Haas has posted comments on this change.

Change subject: ifcfg: re-enable IPv6 before device configuration, or disable afterwards
......................................................................


Patch Set 9: Code-Review-1

(3 comments)

https://gerrit.ovirt.org/#/c/54555/9/lib/vdsm/network/configurators/__init__.py
File lib/vdsm/network/configurators/__init__.py:

Line 216: wait_for_device
I don't understand this one.
The caller creates a device and then asks to wait for a device to be created? What if it was already created?
If you want to make sure a device is created before proceeding (blocking on it), it needs to be done in the creation context.

I know there are races with links coming up, but not with their creation. Do we really need to be so pedantic?


https://gerrit.ovirt.org/#/c/54555/9/lib/vdsm/network/configurators/ifcfg.py
File lib/vdsm/network/configurators/ifcfg.py:

Line 805:                              args=(iface, cgroup))
Line 806:         t.daemon = True
Line 807:         t.start()
Line 808:     else:
Line 809:         if not iface.master and (iface.ipv4 or iface.ipv6):
Nice catch
Line 810:             if iface.ipv4:
Line 811:                 expected_event = {'label': iface.name, 'family': 'inet',
Line 812:                                   'scope': 'global'}
Line 813:             elif iface.ipv6:


https://gerrit.ovirt.org/#/c/54555/9/tests/functional/networkTests.py
File tests/functional/networkTests.py:

Line 1879:     @permutations([[([4], [6])],
Line 1880:                    [([6], [4])],
Line 1881:                    [([4], [4, 6], [4])]])
Line 1882:     @cleanupNet
Line 1883:     def testStaticNetworkConfig(self, ip_reconfigurations):
I find it hard to read this test and if it will fail, it will be painful to understand which scenario failed.

Can you please split this test into multiple ones?
Each should check a specific scenario and have a name that fits it.
Line 1884:         with dummyIf(1) as nics:
Line 1885:             nic, = nics
Line 1886:             IPv4 = dict(nic=nic, bootproto='none', ipaddr=IP_ADDRESS,
Line 1887:                         netmask=IP_MASK, gateway=IP_GATEWAY)


-- 
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: 9
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ondřej Svoboda <osvoboda at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Edward Haas <edwardh at redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Ondřej Svoboda <osvoboda at redhat.com>
Gerrit-Reviewer: Petr Horáček <phoracek at redhat.com>
Gerrit-Reviewer: gerrit-hooks <automation at ovirt.org>
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list