Change in vdsm[master]: net: OVS netinfo

phoracek at redhat.com phoracek at redhat.com
Mon May 9 13:36:27 UTC 2016


Petr Horáček has posted comments on this change.

Change subject: net: OVS netinfo
......................................................................


Patch Set 27: Verified+1

(7 comments)

Passed ovs_info_test.py

https://gerrit.ovirt.org/#/c/56972/26/lib/vdsm/network/ovs/info.py
File lib/vdsm/network/ovs/info.py:

Line 109: 
Line 110:         return {'slaves': slaves, 'active_slave': active_slave,
Line 111:                 'fake_iface': fake_iface, 'mode': mode, 'lacp': lacp}
Line 112: 
Line 113:     def southbound_port(self, bridge):
> Perhaps we should cache this info at the bridge level with a 'southbond' ke
I'd prefer to do that in another patch. There is also another problem: What should we do if there are multiple southbound ports set? It should never happen, but it sounds like bug danger.
Line 114:         for port, attrs in six.iteritems(self.bridges[bridge]['ports']):
Line 115:             if attrs['level'] == SOUTHBOUND:
Line 116:                 return port
Line 117:         return None


Line 184: 
Line 185: def _get_bond_info(port, bond_attrs, addresses, routes):
Line 186:     bond_info = {
Line 187:         'slaves': bond_attrs['slaves'],
Line 188:         # TODO: what should we report when no slave is active?
> Sorry for this nit, but could you please order the funcs from top to bottom
Done
Line 189:         'active_slave': (bond_attrs['active_slave'] or
Line 190:                          bond_attrs['slaves'][0]),
Line 191:         'opts': _to_bond_opts(bond_attrs['mode'], bond_attrs['lacp']),
Line 192:         'switch': 'ovs'


PS26, Line 190:                          bond_attrs['slaves'][0]),
              :         'opts': _to_bond_opt
> Why would we need a default for this?
Done


PS26, Line 201: def _to_bond_opts(mode, lacp):
              :     opts = {}
> For southbound we use ovs_info.southbound_port and for northbound we use th
Done


PS26, Line 203: 
> Really confusing... this is the northbound port name that represents the ne
Done


https://gerrit.ovirt.org/#/c/56972/26/tests/network/ovs_info_test.py
File tests/network/ovs_info_test.py:

PS26, Line 208:                                       else '::'))
> This requires mocking the OvsInfo object, not its __init__.
Done


PS26, Line 209: getIpInfo',
> I understand that it is easier to monkeypatch it like this, but it is a bad
Done


-- 
To view, visit https://gerrit.ovirt.org/56972
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia5dd351faf108fd04afa78208d9c34451a856d4a
Gerrit-PatchSet: 27
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