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