Dan Kenigsberg has posted comments on this change.
Change subject: network: ifcfg: synchronous ifup
......................................................................
Patch Set 7:
(4 comments)
https://gerrit.ovirt.org/#/c/37366/7//COMMIT_MSG
Commit Message:
Line 4: Commit: Petr Horáček <phoracek(a)redhat.com>
Line 5: CommitDate: 2015-07-10 09:58:38 +0200
Line 6:
Line 7: network: ifcfg: synchronous ifup
Line 8:
please state why is it important.
Also, we must state why it is problematic - if we set up multiple networks, this patch
makes us wait for each one at a time. How long does it take?
Line 9: Change-Id: I0c90a556f5fc52c1b8e675986ac39b6032ca0197
https://gerrit.ovirt.org/#/c/37366/7/vdsm/network/configurators/ifcfg.py
File vdsm/network/configurators/ifcfg.py:
Line 168: self.configApplier.addNic(nic, **opts)
Line 169: self._addSourceRoute(nic)
Line 170: if nic.bond is None:
Line 171: is_vlanned = netinfo.isVlanned(nic.name)
Line 172: if not is_vlanned:
I'm not sure that the intermediate variable is helpful, but it is surely unrelated to
the patch.
Line 173: ifdown(nic.name)
Line 174: _ifup(nic)
Line 175:
Line 176: def removeBridge(self, bridge):
Line 848: raise Exception('Error translating bond mode.')
Line 849:
Line 850:
Line 851: def _netmask2prefix(netmask):
Line 852: return sum([bin(int(x)).count('1') for x in
netmask.split('.')])
this is correct only if the netmask does not have "holes". That's fine, but
it should be expressed in a docstring.
Line 853:
Line 854:
Line 855: @contextmanager
Line 856: def _wait_for_event(expected_event, timeout=120):
Line 852: return sum([bin(int(x)).count('1') for x in
netmask.split('.')])
Line 853:
Line 854:
Line 855: @contextmanager
Line 856: def _wait_for_event(expected_event, timeout=120):
this default is a bit too much. I think that a few seconds are more than enough.
Line 857: with monitor.Monitor(groups=('ipv4-ifaddr', 'ipv6-ifaddr'),
Line 858: timeout=timeout) as mon:
Line 859: try:
Line 860: yield
--
To view, visit
https://gerrit.ovirt.org/37366
To unsubscribe, visit
https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I0c90a556f5fc52c1b8e675986ac39b6032ca0197
Gerrit-PatchSet: 7
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Petr Horáček <phoracek(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Petr Horáček <phoracek(a)redhat.com>
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: Yes