Change in vdsm[master]: modularize configurators: draft.

nsoffer at redhat.com nsoffer at redhat.com
Wed Oct 8 21:25:27 UTC 2014


Nir Soffer has posted comments on this change.

Change subject: modularize configurators: draft.
......................................................................


Patch Set 2:

(28 comments)

This must be separated to different patches. The code suggested here is mostly regression compared with the current code - more complicated, less clear, and harder to work with for people who want to write a new configuration module.

The tests look nicer now, without the annoying duplication of the configuratior name, but now __repr__ is less useful which may make debugging test failures harder - so it seem that this should be solve in another way.

http://gerrit.ovirt.org/#/c/33914/2//COMMIT_MSG
Commit Message:

Line 10: It is not meant to be reviewed in depth.
Line 11: For a full review I will break this down to smaller subsequent changes.
Line 12: 
Line 13: Included here are a few changes I've been meaning to make for some time:
Line 14: * Turning configurators from classes to modules.
This is change 1
Line 15: * Extended documentation - I believe a configurator implementer
Line 16:       can now understand what needs doing and how.
Line 17: * Auto discovery of configurators.
Line 18: * Bugfix for an issue I found during testing: configurators_to_trigger cannot


Line 12: 
Line 13: Included here are a few changes I've been meaning to make for some time:
Line 14: * Turning configurators from classes to modules.
Line 15: * Extended documentation - I believe a configurator implementer
Line 16:       can now understand what needs doing and how.
This is change 2
Line 17: * Auto discovery of configurators.
Line 18: * Bugfix for an issue I found during testing: configurators_to_trigger cannot
Line 19:       be dict, since it destroys modules order.
Line 20: 


Line 13: Included here are a few changes I've been meaning to make for some time:
Line 14: * Turning configurators from classes to modules.
Line 15: * Extended documentation - I believe a configurator implementer
Line 16:       can now understand what needs doing and how.
Line 17: * Auto discovery of configurators.
This is change 3
Line 18: * Bugfix for an issue I found during testing: configurators_to_trigger cannot
Line 19:       be dict, since it destroys modules order.
Line 20: 
Line 21: The main issue of this draft is Classes morphing into Modules:


Line 15: * Extended documentation - I believe a configurator implementer
Line 16:       can now understand what needs doing and how.
Line 17: * Auto discovery of configurators.
Line 18: * Bugfix for an issue I found during testing: configurators_to_trigger cannot
Line 19:       be dict, since it destroys modules order.
This is change 4
Line 20: 
Line 21: The main issue of this draft is Classes morphing into Modules:
Line 22: * When a new implementation is needed, it should be sufficient to look at
Line 23:       configurator.py To understand what is needed. Code at package __init__


Line 17: * Auto discovery of configurators.
Line 18: * Bugfix for an issue I found during testing: configurators_to_trigger cannot
Line 19:       be dict, since it destroys modules order.
Line 20: 
Line 21: The main issue of this draft is Classes morphing into Modules:
This is change 5
Line 22: * When a new implementation is needed, it should be sufficient to look at
Line 23:       configurator.py To understand what is needed. Code at package __init__
Line 24:       that has lower visibility and  has been trimmed.
Line 25: * configurator.py contains the default values it uses.


Line 20: 
Line 21: The main issue of this draft is Classes morphing into Modules:
Line 22: * When a new implementation is needed, it should be sufficient to look at
Line 23:       configurator.py To understand what is needed. Code at package __init__
Line 24:       that has lower visibility and  has been trimmed.
Making the implementation more clear does not require using modules instead of classes.
Line 25: * configurator.py contains the default values it uses.
Line 26: * Modules are simpler and can use classes if they need (e.g libvirt),
Line 27:       the opposite is not true. Initialization is also simpler
Line 28:       (name() vs. name.Configurator).


Line 21: The main issue of this draft is Classes morphing into Modules:
Line 22: * When a new implementation is needed, it should be sufficient to look at
Line 23:       configurator.py To understand what is needed. Code at package __init__
Line 24:       that has lower visibility and  has been trimmed.
Line 25: * configurator.py contains the default values it uses.
This makes understanding the the classes harder, as you have to look in both the configurator module, and the configurator driver. Seems to contradict your point above about making it easier to understand.
Line 26: * Modules are simpler and can use classes if they need (e.g libvirt),
Line 27:       the opposite is not true. Initialization is also simpler
Line 28:       (name() vs. name.Configurator).
Line 29: 


Line 24:       that has lower visibility and  has been trimmed.
Line 25: * configurator.py contains the default values it uses.
Line 26: * Modules are simpler and can use classes if they need (e.g libvirt),
Line 27:       the opposite is not true. Initialization is also simpler
Line 28:       (name() vs. name.Configurator).
Modules are indeed simpler, and this can be a good point when the consumers of this are absolute beginners (e.g. kids learning to program). Since the developers using this should be more skilled, the difference is insignificant.
Line 29: 
Line 30: I tested this code on el6.6 and fe20.
Line 31: Only tested build and tests for now. Manual validation will follow
Line 32: once patch is no longer a draft.


Line 25: * configurator.py contains the default values it uses.
Line 26: * Modules are simpler and can use classes if they need (e.g libvirt),
Line 27:       the opposite is not true. Initialization is also simpler
Line 28:       (name() vs. name.Configurator).
Line 29: 
Why are you suggesting a patch that does 5 unrelated changes?

This waste everybody time, and if merged, will make it very hard to understand the changes later and fix the new bugs.

Please separate this to at least 4 indipendent patches handling the different changes.

I ordered the changes by their importance:

1. Fix configurators_to_trigger cannot be dict, since it destroys modules order. We may need to backport this fix. Did you consider using ordereddict?

This does not required moving to modules

2. Removal of the unneeded name property. This is indeed useless as the class importing the module already know the module name.

This can be useful when the Python module name cannot be used as the --module argument, but current code does not require it.

This does not required moving to modules

3. Extended documentation - not sure what you mean by that, since the base class in __init__.py is already documented, but lets this patch.

This does not required moving to modules

4. Automatic discovery of modules - I don't know why we need this - this will complicate the code for no reason, for loading the 3 modules we have. But seeing this in separate clean patch we can evaluate this change.

This does not required moving to modules.

5. Moving to modules instead of classes - I don't see any reason to do this, but as a separate patch it will be easy to evaluate.
Line 30: I tested this code on el6.6 and fe20.
Line 31: Only tested build and tests for now. Manual validation will follow
Line 32: once patch is no longer a draft.
Line 33: 


http://gerrit.ovirt.org/#/c/33914/2/lib/vdsm/tool/configurator.py
File lib/vdsm/tool/configurator.py:

Line 72: [optional function] configure:
Line 73:     A function configuring module aspects to run vdsm.
Line 74: 
Line 75: [optional function] removeConf:
Line 76:     A function cleaning up the configuration done for this module.
Making everything optional make the calling code work harder for no reason. A module writer has to provide the methods and attributes.

Using a well documented base class is the Pythonic way to handle this. Why do you try to find ways which are not supported by the language?

Also having the documentation in the module driving the configuration modules require the authors of configuration modules to look in this file, which is a bad idea.

If you have good documentation and a base class, you don't need to know the code driving your code.
Line 77: """
Line 78: # Defaults behaviour for module configurators:
Line 79: _DEFAULTS = {
Line 80:     'requires': frozenset,


Line 82:     'validate': lambda: True,
Line 83:     'isconfigured': lambda: NOT_CONFIGURED,
Line 84:     'configure': lambda: None,
Line 85:     'removeConf': lambda: None,
Line 86: }
This is less clear then the current ModuleConfigurator class, and even worse, not the standard way to design this. Authors of the new modules will have to waste time understanding your strange way of doing this.
Line 87: 
Line 88: _CONFIGURATORS = dict(
Line 89:     (name, __import__(join(name, path), globals(), locals(), [], -1))
Line 90:     for path, name in (  # Use importlib once python >= 2.7


Line 89:     (name, __import__(join(name, path), globals(), locals(), [], -1))
Line 90:     for path, name in (  # Use importlib once python >= 2.7
Line 91:         (splitext(p)[0], basename(splitext(p)[0])) for p in
Line 92:         glob.glob("%s/*.py" % CONFIGURATORS_PACKAGE)
Line 93:         if basename(p) != "configfile"))
This is horribly complicated - please write this properly, one logic operation per line.

The stupid and simple:
    import libvirt
    import sanlock
    import certificates

Is much better then this unneeded dynamic code. Why do we need this complexity?
Line 94: 
Line 95: for mod in _CONFIGURATORS.values():
Line 96:     for k, v in _DEFAULTS:
Line 97:         if not hasattr(mod, k):


Line 94: 
Line 95: for mod in _CONFIGURATORS.values():
Line 96:     for k, v in _DEFAULTS:
Line 97:         if not hasattr(mod, k):
Line 98:             setattr(mod, k, v)
You are re-implementing inheritance here - why? Python gives you this builtin if you use classes.
Line 99: 
Line 100: 
Line 101: @expose("configure")
Line 102: @requiresRoot


Line 109:     args = _parse_args(*args)
Line 110:     configurer_to_trigger = []
Line 111: 
Line 112:     sys.stdout.write("\nChecking configuration status...\n\n")
Line 113:     for name, c in args.modules:
Having (name, module) tuple is making it harder for all functions in this module.

We would like that module will have a name property instead, like it was before. This is very easy to do either dynamically or statically.

This is a regression compared with the current code.
Line 114:         isconfigured = c.isconfigured()
Line 115:         override = args.force and isconfigured != CONFIGURED
Line 116:         if not override and not c.validate():
Line 117:             raise InvalidConfig(


Line 117:             raise InvalidConfig(
Line 118:                 "Configuration of %s is invalid" % name
Line 119:             )
Line 120:         if override or isconfigured == NOT_CONFIGURED:
Line 121:             configurer_to_trigger.append((name, c))
Again, now it is more complicated because c.name is not avaialble.
Line 122: 
Line 123:     services = []
Line 124:     for name, c in configurer_to_trigger:
Line 125:         for s in c.services:


Line 155:     """
Line 156:     ret = True
Line 157:     args = _parse_args(*args)
Line 158: 
Line 159:     m = [n for n, c in args.modules if c.isconfigured() == NOT_CONFIGURED]
This is less clear than the current code. Please use proper "name" here.
Line 160: 
Line 161:     if m:
Line 162:         sys.stdout.write(
Line 163:             "Modules %s are not configured\n " % ', '.join(m),


Line 190:     """
Line 191:     ret = True
Line 192:     args = _parse_args(*args)
Line 193: 
Line 194:     m = [n for n, c in args.modules if not c.validate()]
Again less clear.
Line 195: 
Line 196:     if m:
Line 197:         sys.stdout.write(
Line 198:             "Modules %s contains invalid configuration\n " % ', '.join(m),


Line 305:         args.modules = _CONFIGURATORS.keys()
Line 306: 
Line 307:     args.modules = _sort_modules(_add_dependencies(args.modules))
Line 308: 
Line 309:     args.modules = ((name, _CONFIGURATORS[name]) for name in args.modules)
More complex then before - why?
Line 310: 


http://gerrit.ovirt.org/#/c/33914/2/lib/vdsm/tool/configurators/certificates.py
File lib/vdsm/tool/configurators/certificates.py:

Line 82:             'vdsm/vdsm.conf'
Line 83:         )
Line 84:     )
Line 85:     return not config.getboolean('vars', 'ssl') or\
Line 86:         os.path.isfile(CERT_FILE)
No significant change here, both old and new code are the same. Feel free to make this refactoring if you think it will be easier to maintain this.


http://gerrit.ovirt.org/#/c/33914/2/lib/vdsm/tool/configurators/libvirt.py
File lib/vdsm/tool/configurators/libvirt.py:

Line 46: if utils.isOvirtNode():
Line 47:     from ovirt.node.utils.fs import Config as NodeCfg
Line 48: 
Line 49: 
Line 50: class _Libvirt(object):
This is a bad class name - what does libvirt._Libvirt object does?

libvirt.Configurator is very clear - it configure libvirt.

This is a regression compared with the current code.
Line 51: 
Line 52:     def _getFile(self, fname):
Line 53:         return self.FILES[fname]['path']
Line 54: 


Line 498:     }
Line 499: 
Line 500: 
Line 501: instance = _Libvirt()
Line 502: requires = frozenset(('certificates',))
This is writable by other modules, unlike the @property used before - regression.
Line 503: services = ("vdsmd", "supervdsmd", "libvirtd")
Line 504: isconfigured = instance.isconfigured
Line 505: validate = instance.validate
Line 506: configure = instance.configure


Line 499: 
Line 500: 
Line 501: instance = _Libvirt()
Line 502: requires = frozenset(('certificates',))
Line 503: services = ("vdsmd", "supervdsmd", "libvirtd")
This is writable by other modules, unlike the @property used before - regression.
Line 504: isconfigured = instance.isconfigured
Line 505: validate = instance.validate
Line 506: configure = instance.configure


Line 503: services = ("vdsmd", "supervdsmd", "libvirtd")
Line 504: isconfigured = instance.isconfigured
Line 505: validate = instance.validate
Line 506: configure = instance.configure
Line 507: removeConf = instance.removeConf
This boilerplate is sad - we did not have any of this before, why do we have it now - regression.

It would be more clear if the module had real function:

def validate():
    return instance.validate()

The way you do it save few lines, but makes understanding the module interface much harder. Please do not use this shortcuts - readability is the most important quality.


http://gerrit.ovirt.org/#/c/33914/2/lib/vdsm/tool/configurators/sanlock.py
File lib/vdsm/tool/configurators/sanlock.py:

Line 97:                 configured = CONFIGURED
Line 98:             else:
Line 99:                 raise
Line 100: 
Line 101:     return configured
No significant difference here, use this is you think it will be easier to maintain.


http://gerrit.ovirt.org/#/c/33914/2/tests/toolTests.py
File tests/toolTests.py:

Line 50:     def requires(self):
Line 51:         return self._requires
Line 52: 
Line 53:     def __repr__(self):
Line 54:         return "requires: %s" % (self._requires)
Why did you remove the name from the output? seems like regression. This simple line show that it is nice to have a name in the configurator context.

Having a name is easy, as a module can use its __file__ attribute, so there is no reason why a configurator module cannot tell its name.
Line 55: 
Line 56:     def validate(self):
Line 57:         return not self._should_fail
Line 58: 


Line 53:     def __repr__(self):
Line 54:         return "requires: %s" % (self._requires)
Line 55: 
Line 56:     def validate(self):
Line 57:         return not self._should_fail
This nice change is not related to any of the other changes, please separate testing cleanups to its own patch.
Line 58: 
Line 59:     def isconfigured(self):
Line 60:         if self._should_fail:
Line 61:             return NOT_CONFIGURED


Line 75:     @monkeypatch.MonkeyPatch(
Line 76:         configurator,
Line 77:         '_CONFIGURATORS',
Line 78:         {
Line 79:             'a': MockModuleConfigurator(('b',)),
It is nice to avoid the using name twice when creating a configurator, but the name is useful when debugging. So this is not the place to save typing.

The bad thing in this code is the need to create a dict like this:

    {"a": Configurator("a"), "b": Configurator("b")}

You can solve this like this:

    dict(c.name, c for c in [Configurator("a"), Configurator("b")])

To avoid repeating this, you can create a function:

    def dict_by_name(*configurators):
        return dict(c.name, c for c in configurators)

This can be part of the removing configuratior name property patch (if you indeed remove the name), or a separate patch if the name is available, for cleaning the tests.
Line 80:             'b': MockModuleConfigurator(('a',))
Line 81:         }
Line 82:     )
Line 83:     def testDependencyCircle(self):


Line 254:             'lib/vdsm/tool/libvirtd.logrotate'
Line 255:         )
Line 256: 
Line 257:         for key, val in self.test_env.items():
Line 258:             libvirt._Libvirt.FILES[key]['path'] = val
Naming regression, not required by any of the changes you suggest in the commit message.
Line 259: 
Line 260:         self._setConfig(
Line 261:             ('QLCONF', 'libvirtd'),
Line 262:             ('LDCONF', 'qemu_sanlock'),


-- 
To view, visit http://gerrit.ovirt.org/33914
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I66b2b2b94431f290f922a2e5e7670be290763e59
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: mooli tayer <mtayer at redhat.com>
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap at redhat.com>
Gerrit-Reviewer: Dima Kuznetsov <dkuznets at redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani at redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi at redhat.com>
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei at redhat.com>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-Reviewer: mooli tayer <mtayer at redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list