Change in vdsm[ovirt-3.5]: vdsm-tool: suppoort dependencies between ModuleConfigure

Wednesday, 27 August 2014

Hello Dan Kenigsberg,

I'd like you to do a code review.  Please visit

    http://gerrit.ovirt.org/32050

to review the following change.

Change subject: vdsm-tool: suppoort dependencies between ModuleConfigure
......................................................................

vdsm-tool: suppoort dependencies between ModuleConfigure

Change-Id: Ibf63ce9aa3ca8edb82091d09b976e8e23896524e
Bug-Url: https://bugzilla.redhat.com/show_bug.cgi?id=1127877
Signed-off-by: Mooli Tayer <mtayer(a)redhat.com&gt;
Reviewed-on: http://gerrit.ovirt.org/31423
Reviewed-by: Dan Kenigsberg <danken(a)redhat.com&gt;
---
M lib/vdsm/tool/configurator.py
M lib/vdsm/tool/configurators/__init__.py
M tests/toolTests.py
3 files changed, 192 insertions(+), 11 deletions(-)


  git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/50/32050/1

diff --git a/lib/vdsm/tool/configurator.py b/lib/vdsm/tool/configurator.py
index c52ab07..1e185f6 100644
--- a/lib/vdsm/tool/configurator.py
+++ b/lib/vdsm/tool/configurator.py
@@ -17,6 +17,7 @@
 # Refer to the README and COPYING files for full details of the license
 #
 
+from collections import deque
 import argparse
 import sys
 import traceback
@@ -31,12 +32,13 @@
     sanlock
 
 
-__configurers = {
-    m.getName(): m for m in (
-        libvirt.Libvirt(),
-        sanlock.Sanlock()
-    )
-}
+def _getConfigurers():
+    return {
+        m.getName(): m for m in (
+            libvirt.Libvirt(),
+            sanlock.Sanlock()
+        )
+    }
 
 
 @expose("configure")
@@ -167,20 +169,64 @@
         raise InvalidRun("Remove configuration failed")
 
 
+def _add_dependencies(modulesNames):
+    queue = deque(modulesNames)
+    retNames = set(queue)
+
+    while queue:
+        next_ = queue.popleft()
+        try:
+            requiredNames = _getConfigurers()[next_].getRequires()
+        except KeyError:
+            raise KeyError(
+                "error: argument --module: "
+                "invalid choice: %s (choose from %s)\n" % (next_,
modulesNames)
+            )
+
+        for requiredName in requiredNames:
+            if requiredName not in retNames:
+                retNames.add(requiredName)
+                queue.append(requiredName)
+
+    return retNames
+
+
+def _sort_modules(modulesNames):
+    # Greedy topological sort algorithm(Dijkstra).
+    # At each step go over all tasks and find a task that can be executed
+    # before all others. If at any point there is none, there is a circle!
+    # Note: there's an improved performance variant, but this is good enough.
+    modulesNames = set(modulesNames)
+    sortedModules = []
+    while modulesNames:
+
+        for c in modulesNames:
+            requires = _getConfigurers()[c].getRequires()
+            if requires.issubset(set(sortedModules)):
+                modulesNames.remove(c)
+                sortedModules.append(c)
+                break
+
+        else:
+            raise RuntimeError("Dependency circle found!")
+
+    return sortedModules
+
+
 def _parse_args(action, *args):
     parser = argparse.ArgumentParser('vdsm-tool %s' % (action))
     parser.add_argument(
         '--module',
         dest='modules',
-        choices=__configurers.keys(),
         default=[],
         metavar='STRING',
         action='append',
         help=(
             'Specify the module to run the action on '
-            '(e.g %(choices)s).\n'
+            '(e.g %s).\n'
             'If non is specified, operation will run for '
             'all related modules.'
+            % _getConfigurers().keys()
         ),
     )
     if action == "configure":
@@ -191,8 +237,13 @@
             action='store_true',
             help='Force configuration, trigger services restart',
         )
+
     args = parser.parse_args(args)
     if not args.modules:
-        args.modules = __configurers.keys()
-    args.modules = [__configurers[cName] for cName in args.modules]
+        args.modules = _getConfigurers().keys()
+
+    args.modules = _sort_modules(_add_dependencies(args.modules))
+
+    args.modules = [_getConfigurers()[cName] for cName in args.modules]
+
     return args
diff --git a/lib/vdsm/tool/configurators/__init__.py
b/lib/vdsm/tool/configurators/__init__.py
index 41bc1d0..219d119 100644
--- a/lib/vdsm/tool/configurators/__init__.py
+++ b/lib/vdsm/tool/configurators/__init__.py
@@ -64,3 +64,6 @@
 
     def removeConf(self):
         pass
+
+    def getRequires(self):
+        return set()
diff --git a/tests/toolTests.py b/tests/toolTests.py
index ad08ba8..bdd8a5d 100644
--- a/tests/toolTests.py
+++ b/tests/toolTests.py
@@ -17,7 +17,11 @@
 #
 # Refer to the README and COPYING files for full details of the license
 #
-from vdsm.tool.configurators import NOT_CONFIGURED, NOT_SURE
+from vdsm.tool import configurator
+from vdsm.tool.configurators import \
+    ModuleConfigure, \
+    NOT_CONFIGURED,\
+    NOT_SURE
 from vdsm.tool.configurators.configfile import ConfigFile, ParserWrapper
 from vdsm.tool.configurators.libvirt import Libvirt
 from vdsm.tool import upgrade
@@ -31,6 +35,129 @@
 dirName = os.path.dirname(os.path.realpath(__file__))
 
 
+class MockModuleConfigurator(ModuleConfigure):
+
+    def __init__(self, name, dependencies):
+        self._name = name
+        self._dependencies = dependencies
+
+    def __repr__(self):
+        return "name: %s, dependencies: %s" % (self._name, self._dependencies)
+
+    def getName(self):
+        return self._name
+
+    def getRequires(self):
+        return self._dependencies
+
+
+class ConfiguratorTests(TestCase):
+
+    @monkeypatch.MonkeyPatch(
+        configurator,
+        '_getConfigurers',
+        lambda:  {
+            'a': MockModuleConfigurator('a', set('b')),
+            'b': MockModuleConfigurator('b', set('a'))
+        }
+    )
+    def testDependencyCircle(self):
+        self.assertRaises(
+            RuntimeError,
+            configurator._parse_args,
+            'validate-config'
+        )
+
+    @monkeypatch.MonkeyPatch(
+        configurator,
+        '_getConfigurers',
+        lambda:  {
+            'a': MockModuleConfigurator('a', {'b',
'd'}),
+            'b': MockModuleConfigurator('b', {'c'}),
+            'c': MockModuleConfigurator('c', {'e',
'd'}),
+            'd': MockModuleConfigurator('d', {'e',
'e'}),
+            'e': MockModuleConfigurator('e', set()),
+            'f': MockModuleConfigurator('f', set()),
+
+        }
+    )
+    def testNormalDependencies(self):
+        modules = configurator._parse_args('validate-config').modules
+        # make sure this is indeed a topological sort.
+        before_m = set()
+        for m in modules:
+            for n in m.getRequires():
+                self.assertIn(n, before_m)
+            before_m.add(m.getName())
+
+    @monkeypatch.MonkeyPatch(
+        configurator,
+        '_getConfigurers',
+        lambda:  {
+            'a': MockModuleConfigurator('a', set()),
+            'b': MockModuleConfigurator('b', set()),
+            'c': MockModuleConfigurator('c', set())
+        }
+    )
+    def testNoDependencies(self):
+        configurator._parse_args('validate-config').modules
+
+    @monkeypatch.MonkeyPatch(
+        configurator,
+        '_getConfigurers',
+        lambda: {
+            'a': MockModuleConfigurator('a', {'b',
'c'}),
+            'b': MockModuleConfigurator('b', set()),
+            'c': MockModuleConfigurator('c', set())
+        }
+    )
+    def testDependenciesAdditionPositive(self):
+        modules = configurator._parse_args(
+            'validate-config',
+            '--module=a'
+        ).modules
+        modulesNames = [m.getName() for m in modules]
+        self.assertTrue('a' in modulesNames)
+        self.assertTrue('b' in modulesNames)
+        self.assertTrue('c' in modulesNames)
+
+    @monkeypatch.MonkeyPatch(
+        configurator,
+        '_getConfigurers',
+        lambda:  {
+            'a': MockModuleConfigurator('a', set()),
+            'b': MockModuleConfigurator('b', set()),
+            'c': MockModuleConfigurator('c', set())
+        }
+    )
+    def testDependenciesAdditionNegative(self):
+
+        modules = configurator._parse_args(
+            'validate-config',
+            '--module=a'
+        ).modules
+        moduleNames = [m.getName() for m in modules]
+        self.assertTrue('a' in moduleNames)
+        self.assertFalse('b' in moduleNames)
+        self.assertFalse('c' in moduleNames)
+
+    @monkeypatch.MonkeyPatch(
+        configurator,
+        '_getConfigurers',
+        lambda:  {
+            'a': MockModuleConfigurator('a', set()),
+        }
+    )
+    def testNonExistentModule(self):
+
+        self.assertRaises(
+            SystemExit,
+            configurator._parse_args,
+            'validate-config',
+            '--module=b'
+        )
+
+
 class LibvirtModuleConfigureTests(TestCase):
 
     test_env = {}


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: Ibf63ce9aa3ca8edb82091d09b976e8e23896524e
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: ovirt-3.5
Gerrit-Owner: mooli tayer <mtayer(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;

    

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011