Change in vdsm[master]: net: OVS netinfo
edwardh at redhat.com
edwardh at redhat.com
Fri May 6 20:59:43 UTC 2016
Edward Haas has posted comments on this change.
Change subject: net: OVS netinfo
......................................................................
Patch Set 26: Code-Review-1
(7 comments)
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' key?
Does not need to be in this patch..
Hmm.. would it fit as a bridge property instead of a port level?
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: return bond_info
Line 186:
Line 187:
Line 188: def get_netinfo(ovs_info=None):
Sorry for this nit, but could you please order the funcs from top to bottom (public first and so on..)
Line 189:
Line 190: if ovs_info is None:
Line 191: ovs_info = OvsInfo()
Line 192:
PS26, Line 190: if ovs_info is None:
: ovs_info = OvsInfo()
Why would we need a default for this?
IMO instantiating an object like this better be controlled by the caller.. less surprises.
PS26, Line 201: for port, port_attrs in six.iteritems(ports):
: if port_attrs['level'] == NORTHBOUND:
For southbound we use ovs_info.southbound_port and for northbound we use this?
PS26, Line 203: port
Really confusing... this is the northbound port name that represents the network name.
That needs to be expressed somehow...
https://gerrit.ovirt.org/#/c/56972/26/tests/network/ovs_info_test.py
File tests/network/ovs_info_test.py:
PS26, Line 208: @MonkeyPatch(info.OvsInfo, '__init__', mocked_OvsInfo_init)
This requires mocking the OvsInfo object, not its __init__.
But a proper test should have mocked the driver.
(Because we should have unit tested OvsDB by mocking the driver, testing OvsInfo using the same mock and so on...
PS26, Line 209: _get_iface_info
I understand that it is easier to monkeypatch it like this, but it is a bad practice to patch/mock private funcs/methods.
It makes the test strongly coupled to the CUD.
You should mock/patch only public modules/classes/funcs, preferably not the ones from the same module you test.
In this case, I guess you need to monkeypatch all the funcs called to process the info.
This is also the reason why IP related stuff should have been done outside the info module.
--
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: 26
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