Change in vdsm[master]: tool: Make configurators more Pythonic

Wednesday, 20 August 2014

Nir Soffer has uploaded a new change for review.

Change subject: tool: Make configurators more Pythonic
......................................................................

tool: Make configurators more Pythonic

The configurations used to have Java like getters, which make the code
more clumsy then it should be. This patch convert the getters to class
variables.

The types returned by the getters were mutable, which may lead to
calling code to modify the returned value without any error. Now the
configurators return immutable types.

There was not documentation about the purpose and the semantics of the
getters, which makes it harder for new developer to modify this part.
Now ModuleConfigure document the class variables.

There should be no change in the behavior.

Change-Id: I16092da09d6763a8222bc941bd7d1501a7bb3bff
Signed-off-by: Nir Soffer <nsoffer(a)redhat.com&gt;
---
M lib/vdsm/tool/configurator.py
M lib/vdsm/tool/configurators/__init__.py
M lib/vdsm/tool/configurators/certificates.py
M lib/vdsm/tool/configurators/libvirt.py
M lib/vdsm/tool/configurators/sanlock.py
M tests/toolTests.py
6 files changed, 37 insertions(+), 40 deletions(-)


  git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/41/31741/1

diff --git a/lib/vdsm/tool/configurator.py b/lib/vdsm/tool/configurator.py
index 2f71075..132c866 100644
--- a/lib/vdsm/tool/configurator.py
+++ b/lib/vdsm/tool/configurator.py
@@ -35,7 +35,7 @@
 
 def _getConfigurers():
     return {
-        m.getName(): m for m in (
+        m.name: m for m in (
             certificates.Certificates(),
             libvirt.Libvirt(),
             sanlock.Sanlock(),
@@ -59,14 +59,14 @@
         override = args.force and isconfigured != CONFIGURED
         if not override and not c.validate():
             raise InvalidConfig(
-                "Configuration of %s is invalid" % c.getName()
+                "Configuration of %s is invalid" % c.name
             )
         if override or isconfigured == NOT_CONFIGURED:
             configurer_to_trigger.append(c)
 
     services = []
     for c in configurer_to_trigger:
-        for s in c.getServices():
+        for s in c.services:
             if service.service_status(s, False) == 0:
                 if not args.force:
                     raise InvalidRun(
@@ -82,7 +82,7 @@
     sys.stdout.write("\nRunning configure...\n")
     for c in configurer_to_trigger:
         c.configure()
-        sys.stdout.write("Reconfiguration of %s is done.\n" % (c.getName(),))
+        sys.stdout.write("Reconfiguration of %s is done.\n" % (c.name,))
 
     for s in reversed(services):
         service.service_start(s)
@@ -157,13 +157,13 @@
             c.removeConf()
             sys.stderr.write(
                 "removed configuration of module %s successfully\n" %
-                c.getName()
+                c.name
             )
 
         except Exception:
             sys.stderr.write(
                 "can't remove configuration of module %s\n" %
-                c.getName()
+                c.name
             )
             traceback.print_exc(file=sys.stderr)
             failed = True
@@ -178,7 +178,7 @@
     while queue:
         next_ = queue.popleft()
         try:
-            requiredNames = _getConfigurers()[next_].getRequires()
+            requiredNames = _getConfigurers()[next_].requires
         except KeyError:
             raise KeyError(
                 "error: argument --module: "
@@ -203,7 +203,7 @@
     while modulesNames:
 
         for c in modulesNames:
-            requires = _getConfigurers()[c].getRequires()
+            requires = _getConfigurers()[c].requires
             if requires.issubset(set(sortedModules)):
                 modulesNames.remove(c)
                 sortedModules.append(c)
diff --git a/lib/vdsm/tool/configurators/__init__.py
b/lib/vdsm/tool/configurators/__init__.py
index 49071ff..e9b28ac 100644
--- a/lib/vdsm/tool/configurators/__init__.py
+++ b/lib/vdsm/tool/configurators/__init__.py
@@ -44,11 +44,19 @@
 
 class ModuleConfigure(object):
 
-    def getName(self):
-        return None
+    # The module name to be used with the --module option.
+    # Must be overriden by subclass.
+    name = None
 
-    def getServices(self):
-        return []
+    # Names of other modules required by this module.
+    # May be overriden by subclass.
+    requires = frozenset()
+
+    # Services that must not run when this module is configured. The specified
+    # services will be stopped before this configurator is called, and will be
+    # started in reversed order when the configurator is done.
+    # May be overriden by subclass.
+    services = ()
 
     def validate(self):
         return True
@@ -61,6 +69,3 @@
 
     def removeConf(self):
         pass
-
-    def getRequires(self):
-        return set()
diff --git a/lib/vdsm/tool/configurators/certificates.py
b/lib/vdsm/tool/configurators/certificates.py
index 7a9a7c6..8690a4e 100644
--- a/lib/vdsm/tool/configurators/certificates.py
+++ b/lib/vdsm/tool/configurators/certificates.py
@@ -44,9 +44,7 @@
     Responsible for rolling out self signed certificates if vdsm's
     configuration is ssl_enabled and no certificates exist.
     """
-    # Todo: validate_ovirt_certs.py
-    def getName(self):
-        return 'certificates'
+    name = 'certificates'
 
     def validate(self):
         return self._certsExist()
diff --git a/lib/vdsm/tool/configurators/libvirt.py
b/lib/vdsm/tool/configurators/libvirt.py
index c1ee527..eebbb92 100644
--- a/lib/vdsm/tool/configurators/libvirt.py
+++ b/lib/vdsm/tool/configurators/libvirt.py
@@ -51,14 +51,12 @@
 
 class Libvirt(ModuleConfigure):
 
-    def getName(self):
-        return 'libvirt'
+    name = 'libvirt'
+    requires = {'certificates'}
+    services = ("vdsmd", "supervdsmd", "libvirtd")
 
     def _getFile(self, fname):
         return self.FILES[fname]['path']
-
-    def getServices(self):
-        return ["vdsmd", "supervdsmd", "libvirtd"]
 
     def configure(self):
         if os.getuid() != 0:
@@ -110,9 +108,6 @@
     def removeConf(self):
         for cfile, content in Libvirt.FILES.items():
             content['removeConf'](self, content['path'])
-
-    def getRequires(self):
-        return {'certificates'}
 
     def _getPersistedFiles(self):
         """
diff --git a/lib/vdsm/tool/configurators/sanlock.py
b/lib/vdsm/tool/configurators/sanlock.py
index 9225252..10c7760 100644
--- a/lib/vdsm/tool/configurators/sanlock.py
+++ b/lib/vdsm/tool/configurators/sanlock.py
@@ -36,11 +36,8 @@
 
     SANLOCK_GROUPS = (constants.QEMU_PROCESS_GROUP, constants.VDSM_GROUP)
 
-    def getName(self):
-        return 'sanlock'
-
-    def getServices(self):
-        return ['sanlock']
+    name = 'sanlock'
+    services = ('sanlock',)
 
     def configure(self):
         """
diff --git a/tests/toolTests.py b/tests/toolTests.py
index bdd8a5d..64d0a17 100644
--- a/tests/toolTests.py
+++ b/tests/toolTests.py
@@ -37,18 +37,20 @@
 
 class MockModuleConfigurator(ModuleConfigure):
 
-    def __init__(self, name, dependencies):
+    def __init__(self, name, requires):
         self._name = name
-        self._dependencies = dependencies
+        self._requires = requires
 
     def __repr__(self):
         return "name: %s, dependencies: %s" % (self._name, self._dependencies)
 
-    def getName(self):
+    @property
+    def name(self):
         return self._name
 
-    def getRequires(self):
-        return self._dependencies
+    @property
+    def requires(self):
+        return self._requires
 
 
 class ConfiguratorTests(TestCase):
@@ -86,9 +88,9 @@
         # make sure this is indeed a topological sort.
         before_m = set()
         for m in modules:
-            for n in m.getRequires():
+            for n in m.requires:
                 self.assertIn(n, before_m)
-            before_m.add(m.getName())
+            before_m.add(m.name)
 
     @monkeypatch.MonkeyPatch(
         configurator,
@@ -116,7 +118,7 @@
             'validate-config',
             '--module=a'
         ).modules
-        modulesNames = [m.getName() for m in modules]
+        modulesNames = [m.name for m in modules]
         self.assertTrue('a' in modulesNames)
         self.assertTrue('b' in modulesNames)
         self.assertTrue('c' in modulesNames)
@@ -136,7 +138,7 @@
             'validate-config',
             '--module=a'
         ).modules
-        moduleNames = [m.getName() for m in modules]
+        moduleNames = [m.name for m in modules]
         self.assertTrue('a' in moduleNames)
         self.assertFalse('b' in moduleNames)
         self.assertFalse('c' in moduleNames)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I16092da09d6763a8222bc941bd7d1501a7bb3bff
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer <nsoffer(a)redhat.com&gt;

    

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011