Change in vdsm[master]: net: OVS configurator

edwardh at redhat.com edwardh at redhat.com
Mon Apr 18 09:36:53 UTC 2016


Edward Haas has posted comments on this change.

Change subject: net: OVS configurator
......................................................................


Patch Set 2:

(16 comments)

https://gerrit.ovirt.org/#/c/55958/2/lib/vdsm/network/ovs/configurators/__init__.py
File lib/vdsm/network/ovs/configurators/__init__.py:

Line 16: #
Line 17: # Refer to the README and COPYING files for full details of the license
Line 18: #
Line 19: from __future__ import absolute_import
Line 20: from __future__ import print_function
> why don't we import it from one line?
That is the convention everywhere else, I prefer to stay with that.
(There may be also a script that checks the absolute_import line)
Line 21: 
Line 22: from importlib import import_module
Line 23: from pkgutil import iter_modules
Line 24: 


Line 33: _CONFIGURATORS = {}
Line 34: for _, module, _ in iter_modules([__path__[0]]):
Line 35:     _CONFIGURATORS[module] = import_module('{}.{}'.format(__name__, module))
Line 36:     if not hasattr(_CONFIGURATORS[module], 'name'):
Line 37:         setattr(_CONFIGURATORS[module], 'name', module)
> IMO we should import module only when it is requested.
The attr part is not needed, removing it.

It is simpler to import all drivers when the package is first imported. Importing from within the driver factory (create) does not seem right: create is called multiple times, and the import part is required only once.


https://gerrit.ovirt.org/#/c/55958/2/lib/vdsm/network/ovs/configurators/api.py
File lib/vdsm/network/ovs/configurators/api.py:

PS2, Line 31: commands
> Commands Transaction and '.'
Done


PS2, Line 35: commands
> Commands Transaction and '.'
Done


PS2, Line 76: list_bridge_info
> list_br_info?
Too similar to the previous one.. (list_br)
I could called it list_bridge_db, but it sounded strange to me.

Just following the naming from ovs-vsctl


https://gerrit.ovirt.org/#/c/55958/2/lib/vdsm/network/ovs/configurators/vsctl.py
File lib/vdsm/network/ovs/configurators/vsctl.py:

Line 16: #
Line 17: # Refer to the README and COPYING files for full details of the license
Line 18: #
Line 19: # The implementation has been inspired by openstack OVS access
Line 20: 
> drop empty line before imports, such details triggers something inside me..
Done
Line 21: from __future__ import absolute_import
Line 22: 
Line 23: import collections
Line 24: import json


PS2, Line 47: return
> i'd use `pass` to emphasise that we do nothing (but i'm not sure about pass
pass will not exit the func, while return does.
If there are no commands in the transaction, we can exit early.


Line 57:             raise ConfigNetworkError(
Line 58:                 ne.ERR_BAD_PARAMS,
Line 59:                 'Executing commands failed: %s' % '\n'.join(err))
Line 60:         if out is None:
Line 61:             return out
> `return` is enough
Done
Line 62: 
Line 63:         for i, row in enumerate(out):
Line 64:             self.commands[i].result = row
Line 65:         return [cmd.result for cmd in self.commands]


Line 73:     def __init__(self, cmd):
Line 74:         self.cmd = cmd
Line 75: 
Line 76:     def execute(self):
Line 77:         with Transaction() as t:
> is there any other reason to use context manager then saving one line of .c
Yes:
- It is consistent with the multi command version.
- The 'commit' is  omitted.
  - Less lines and nicer.
Line 78:             t.add(self)
Line 79:         return self.result
Line 80: 
Line 81:     @property


Line 88: 
Line 89: 
Line 90: class CommandResultDB(Command):
Line 91: 
Line 92:     def __init__(self, cmd):
> is this override needed? if we drop it, Command.__init__ will be used impli
Done
Line 93:         super(CommandResultDB, self).__init__(cmd)
Line 94: 
Line 95:     @Command.result.setter
Line 96:     def result(self, data):


Line 100:             return
Line 101: 
Line 102:         # try:
Line 103:         jdata = json.loads(data)
Line 104:         # except (ValueError, TypeError):
> we should use it without try, exec. if it explodes, we need code fix.
I'm not sure..
If we cannot decode the output, maybe we should return None and log an error...
It's a bit harsh to raise an exception on a report command when the caller is probably not going to take care of exceptions.
Line 105: 
Line 106:         headings = jdata['headings']
Line 107:         data = jdata['data']
Line 108:         results = []


Line 145:         if bridge:
Line 146:             command.append(bridge)
Line 147:         return CommandResultDB(command)
Line 148: 
Line 149:     def show(self):
> could we use list commands instead of this? it is well readable for a human
I don't understand.
Could you give an example?

If you mean Command('show', 'something') and use '*' to convert it to a list? This will be nice here but will be a big overdoing for the rest.
Line 150:         return Command(['show'])
Line 151: 
Line 152:     def add_vlan(self, bridge, vlan, fake_bridge_name=None, may_exist=False):
Line 153:         command = []


PS2, Line 209: python
> Python, '.'
Done


https://gerrit.ovirt.org/#/c/55958/2/tests/network/ovs_configurators_test.py
File tests/network/ovs_configurators_test.py:

Line 45:     bridges = ovsdb.list_bridge_info().execute()
Line 46: 
Line 47:     with ovsdb.transaction() as t:
Line 48:         for bridge in bridges:
Line 49:             t.add(ovsdb.del_br(bridge['name']))
> we want to remove only TEST_BRIDGE, the rest should stay in place.
Right, but at the same time I do not want this code to change if I add ovs bridges to the tests.

I'll change TEST_BRIDGE to TEST_BRIDGES and make it a tuple.
Line 50: 
Line 51:     if _saved_openvswitch_status == _DOWN:
Line 52:         service.service_stop('openvswitch')
Line 53: 


Line 84:             trans.add(cmd_del_br)
Line 85:             trans.add(cmd_list_bridge_info)
Line 86: 
Line 87:         self.assertEqual(0, len(cmd_list_bridge_info.result))
Line 88:         self.assertEqual([], cmd_list_bridge_info.result)
> the previous line is not necessary. [] != [something] is explicit enough
Done
Line 89: 
Line 90: 
Line 91: @attr(type='integration')
Line 92: class TestOvsApiWithSingleRealBridge(VdsmTestCase):


https://gerrit.ovirt.org/#/c/55958/2/vdsm.spec.in
File vdsm.spec.in:

PS2, Line 1176: %{python_sitelib}/%{vdsm_name}/network/ovs/switch.py*
              : %{python_sitelib}/%{vdsm_name}/network/ovs/validator.py*
> not added by this patch.
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ic24a9cea8d50e801de12da1c9cfcd2d195aed684
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Edward Haas <edwardh 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