Change in vdsm[ovirt-3.5]: tool: Create configurators once

ybronhei at redhat.com ybronhei at redhat.com
Sun Aug 31 21:26:22 UTC 2014


Hello Nir Soffer, Dan Kenigsberg,

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

    http://gerrit.ovirt.org/32204

to review the following change.

Change subject: tool: Create configurators once
......................................................................

tool: Create configurators once

Commit e5e80540e8 replaced the configurators dict with a function,
creating a configuration dict on each call. The code added in this
commit invoke the function many times for each run on the tool, creating
10's of instances of the configurators. This change is not needed to
acomplish the purpuse of that patch, and is rather pointless.

This patch reverts the unrelated change, keeping the configurations in a
module constant. This change also simplify the tests.

Change-Id: I6e0e258dc28557661bf85734f000e43f20b1e512
Signed-off-by: Nir Soffer <nsoffer at redhat.com>
Reviewed-on: http://gerrit.ovirt.org/31739
Reviewed-by: Yaniv Bronhaim <ybronhei at redhat.com>
Reviewed-by: Dan Kenigsberg <danken at redhat.com>
---
M lib/vdsm/tool/configurator.py
M tests/toolTests.py
2 files changed, 23 insertions(+), 24 deletions(-)


  git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/04/32204/1

diff --git a/lib/vdsm/tool/configurator.py b/lib/vdsm/tool/configurator.py
index 83e7d41..c40375e 100644
--- a/lib/vdsm/tool/configurator.py
+++ b/lib/vdsm/tool/configurator.py
@@ -33,12 +33,11 @@
     sanlock
 
 
-def _getConfigurers():
-    return dict((m.getName(), m) for m in (
-        certificates.Certificates(),
-        libvirt.Libvirt(),
-        sanlock.Sanlock(),
-    ))
+_CONFIGURATORS = dict((m.getName(), m) for m in (
+    certificates.Certificates(),
+    libvirt.Libvirt(),
+    sanlock.Sanlock(),
+))
 
 
 @expose("configure")
@@ -177,9 +176,9 @@
     while queue:
         next_ = queue.popleft()
         try:
-            requiredNames = _getConfigurers()[next_].getRequires()
+            requiredNames = _CONFIGURATORS[next_].getRequires()
         except KeyError:
-            available = ', '.join(sorted(_getConfigurers()))
+            available = ', '.join(sorted(_CONFIGURATORS))
             raise UsageError(
                 "error: argument --module: invalid choice: %s\n"
                 "(available: %s)\n" % (next_, available)
@@ -203,7 +202,7 @@
     while modulesNames:
 
         for c in modulesNames:
-            requires = _getConfigurers()[c].getRequires()
+            requires = _CONFIGURATORS[c].getRequires()
             if requires.issubset(set(sortedModules)):
                 modulesNames.remove(c)
                 sortedModules.append(c)
@@ -228,7 +227,7 @@
             '(e.g %s).\n'
             'If non is specified, operation will run for '
             'all related modules.'
-            % _getConfigurers().keys()
+            % _CONFIGURATORS.keys()
         ),
     )
     if action == "configure":
@@ -242,10 +241,10 @@
 
     args = parser.parse_args(args)
     if not args.modules:
-        args.modules = _getConfigurers().keys()
+        args.modules = _CONFIGURATORS.keys()
 
     args.modules = _sort_modules(_add_dependencies(args.modules))
 
-    args.modules = [_getConfigurers()[cName] for cName in args.modules]
+    args.modules = [_CONFIGURATORS[cName] for cName in args.modules]
 
     return args
diff --git a/tests/toolTests.py b/tests/toolTests.py
index 502af03..51ad8ed 100644
--- a/tests/toolTests.py
+++ b/tests/toolTests.py
@@ -57,8 +57,8 @@
 
     @monkeypatch.MonkeyPatch(
         configurator,
-        '_getConfigurers',
-        lambda:  {
+        '_CONFIGURATORS',
+        {
             'a': MockModuleConfigurator('a', set(['b'])),
             'b': MockModuleConfigurator('b', set(['a']))
         }
@@ -72,8 +72,8 @@
 
     @monkeypatch.MonkeyPatch(
         configurator,
-        '_getConfigurers',
-        lambda:  {
+        '_CONFIGURATORS',
+        {
             'a': MockModuleConfigurator('a', set(['b', 'd'])),
             'b': MockModuleConfigurator('b', set(['c'])),
             'c': MockModuleConfigurator('c', set(['e', 'd'])),
@@ -94,8 +94,8 @@
 
     @monkeypatch.MonkeyPatch(
         configurator,
-        '_getConfigurers',
-        lambda:  {
+        '_CONFIGURATORS',
+        {
             'a': MockModuleConfigurator('a', set()),
             'b': MockModuleConfigurator('b', set()),
             'c': MockModuleConfigurator('c', set())
@@ -106,8 +106,8 @@
 
     @monkeypatch.MonkeyPatch(
         configurator,
-        '_getConfigurers',
-        lambda: {
+        '_CONFIGURATORS',
+        {
             'a': MockModuleConfigurator('a', set(['b', 'c'])),
             'b': MockModuleConfigurator('b', set()),
             'c': MockModuleConfigurator('c', set())
@@ -125,8 +125,8 @@
 
     @monkeypatch.MonkeyPatch(
         configurator,
-        '_getConfigurers',
-        lambda:  {
+        '_CONFIGURATORS',
+        {
             'a': MockModuleConfigurator('a', set()),
             'b': MockModuleConfigurator('b', set()),
             'c': MockModuleConfigurator('c', set())
@@ -145,8 +145,8 @@
 
     @monkeypatch.MonkeyPatch(
         configurator,
-        '_getConfigurers',
-        lambda:  {
+        '_CONFIGURATORS',
+        {
             'libvirt': MockModuleConfigurator('libvirt', set()),
             'sanlock': MockModuleConfigurator('sanlock', set()),
         }


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I6e0e258dc28557661bf85734f000e43f20b1e512
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: ovirt-3.5
Gerrit-Owner: Yaniv Bronhaim <ybronhei at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>


More information about the vdsm-patches mailing list