Edward Haas has posted comments on this change. Change subject: ifcfg: write current DNS information to a management network's ifcfg file ......................................................................
Patch Set 4: Code-Review-1 (5 comments) https://gerrit.ovirt.org/#/c/61184/4/lib/vdsm/network/configurators/ifcfg.py File lib/vdsm/network/configurators/ifcfg.py: Line 570: if bridge.duid_source and dhclient.supports_duid_file(): Line 571: duid_source_file = dhclient.LEASE_FILE.format( Line 572: '', bridge.duid_source) Line 573: conf += 'DHCLIENTARGS="-df %s"\n' % duid_source_file Line 574: if bridge.nameservers: Why is it only under addBridge? Seem to me like it is generic and needs to be handled in _createConfFile while only the 'top level' device has nameservers set. Line 575: # TODO: PEERDNS=yes, to be on the safe side in the future? Line 576: # Write DNS1= and DNS2=, if known Line 577: for i, nameserver in enumerate(bridge.nameservers[0:2], 1): Line 578: conf += 'DNS{}={}\n'.format(i, nameserver) PS4, Line 575: PEERDNS=yes, to be on the safe side in the future? > PEERDNS is just like IPV6INIT. Currently, if is isn't present, it is assume This should have the same logic as DEFROUTE, this way we cover DHCP as well. Lets do it in a separate patch. PS4, Line 579: DOMAIN= > "DOMAIN=ovirt.org redhat.com" handling will be added in another patch. Yes, Why do we need to handle this? https://gerrit.ovirt.org/#/c/61184/4/lib/vdsm/network/legacy_switch.py File lib/vdsm/network/legacy_switch.py: Line 160: top_net_dev.ipv6 = IPv6( Line 161: ipv6addr, ipv6gateway, defaultRoute, ipv6autoconf, dhcpv6) Line 162: top_net_dev.blockingdhcp = (configurator._inRollback or Line 163: utils.tobool(blockingdhcp)) Line 164: if defaultRoute: This heuristic needs to be explained. I do not see the reason why you need to do it only when defaultRoute is set at this stage. Line 165: top_net_dev.nameservers = _netinfo.nameservers Line 166: return top_net_dev Line 167: Line 168: https://gerrit.ovirt.org/#/c/61184/4/tests/network/config_network_test.py File tests/network/config_network_test.py: Line 101: 'fakebr1': {'ports': ['bond00']}, Line 102: 'fakebr2': {'ports': ['eth7.1']} Line 103: }, Line 104: 'bondings': {'bond00': {'slaves': ['eth5', 'eth6']}}, Line 105: 'dnss': ['10.20.30.40', '20.30.40.50', '30.40.50.60'], Why not an empty list? (What happens when there are no DNS entries? What does the code today show in netinfo?) Line 106: } Line 107: Line 108: fakeInfo = netinfo.cache.CachingNetInfo(_netinfo) Line 109: nic = 'eth2' -- To view, visit https://gerrit.ovirt.org/61184 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ie10ee7938b26a7f3b2b7be80bc1a2a83cd1c376c Gerrit-PatchSet: 4 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Ondřej Svoboda <[email protected]> Gerrit-Reviewer: Edward Haas <[email protected]> Gerrit-Reviewer: Fabian Deutsch <[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/admin/lists/[email protected]
