Change in vdsm[master]: net: report expected devices in OVS netinfo
phoracek at redhat.com
phoracek at redhat.com
Tue May 10 16:24:24 UTC 2016
Petr Horáček has posted comments on this change.
Change subject: net: report expected devices in OVS netinfo
......................................................................
Patch Set 10: Verified+1
(9 comments)
Passed ovs_info_test.py
https://gerrit.ovirt.org/#/c/57067/9/lib/vdsm/network/ovs/info.py
File lib/vdsm/network/ovs/info.py:
Line 236: net_ports.append(expected_vlan_name)
Line 237: else:
Line 238: net_ports.append(southbound)
Line 239:
Line 240: for port, port_attrs in six.iteritems(ports):
> Try to use an immutable form and give it a more informative name, like _sha
Done
Line 241: if (port_attrs['tag'] == net_tag and
Line 242: port_attrs['level'] not in (SOUTHBOUND, NORTHBOUND)):
Line 243: net_ports.append(port)
Line 244:
PS9, Line 248: ort, por
> Why is it copied?
Done
Line 249: bond_attrs = ports[port]['bond']
Line 250: bond_info = {
Line 251: 'slaves': bond_attrs['slaves'],
Line 252: # TODO: what should we report when no slave is active?
Line 253: 'active_slave': (bond_attrs['active_slave'] or
> T think that for bridgeless networks, the bridge needs to be removed in the
fake_bridgeless is a separated function, extension for this faking. If we choose to not include bridges if it is to be 'bridgeless', we need to access RunningConfig from here. I prefer step-by-step approach.
Line 254: bond_attrs['slaves'][0]),
Line 255: 'opts': _to_bond_opts(bond_attrs['mode'], bond_attrs['lacp']),
Line 256: 'switch': 'ovs'
Line 257: }
PS9, Line 255: s': _to_bond_opts(bond_a
> northbound iface is included here?
It is not. At least on 3.6 we do not report bridge in networks/bridges 'ports'.
PS9, Line 253: 'active_slave': (bond_attrs['active_slave'] or
: bond_attrs['slaves'][0]),
: 'opts': _to_bond_opts(bond_attrs['mode'], bond_attrs['lacp']),
: 'switch': 'ovs'
: }
: bond_info.update(EMPTY_PORT_INFO)
: return bond_info
> Please extract to a func:
Done
PS9, Line 261:
: def _to_bond_opts(mode, lacp):
: custom_opts = []
: if mode:
: custom_opts.append('ovs_mode:%s' % mode)
: if lacp:
: custom_opts.append('ovs_lacp:%s' % lacp)
: return {'custom': ','.join(custom_opts)} if custom_opts else {}
:
:
> Please extract to a func.
Done
PS9, Line 272: ipv4gateway = get_gateway(routes, iface, family=4)
: ipv4addr, ipv4netmask, ipv4addrs, ipv6addrs = getIpInfo(
: iface, addresses, ipv4gateway)
: is_dhcpv4, is_dhcpv6 = dhcp_status(iface, addresses)
:
: return {'mtu': getMtu(iface), 'addr': ipv4addr, 'ipv4addrs': ipv4addrs,
: 'gateway': ipv4gateway,
> I am not clear what are we faking here.
Done. Described in get_netinfo() docstring.
Line 284
Line 285
Line 286
Line 287
Line 288
> We should prefer the immutable version when it is possible, it is cleaner a
Done
https://gerrit.ovirt.org/#/c/57067/9/tests/network/ovs_info_test.py
File tests/network/ovs_info_test.py:
PS9, Line 282: rtE
> We do not need it anymore, it can be dropped.
Done
--
To view, visit https://gerrit.ovirt.org/57067
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I5bdf8c0cd4eb2f11dfd8145d82b0d9f866595583
Gerrit-PatchSet: 10
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Petr Horáček <phoracek 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: 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