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