Remove magic() method from context manager

This magic methods brings a lot of issues:
*) Unclear code

*) Recursion and with construction that makes it hard
to understand traces & as well to have a proper workflow

*) Uncontrollable process of creation and deletion of context:
E.g. it's super hard to "abort" creation of context in the middle

*) create() & cleanup() are inseparable => which makes it impossible
to create "persistance context", that have to run separately
setup() & cleanup()

Change-Id: I071c53900441121767ac3550c5ee5790a358ae73

rally/benchmark/context/base.py

@@ -19,8 +19,11 @@
 import six
 
 from rally import exceptions
+from rally.openstack.common import log as logging
 from rally import utils
 
+LOG = logging.getLogger(__name__)
+
 
 @six.add_metaclass(abc.ABCMeta)
 class Context(object):
@@ -47,6 +50,15 @@ def __init__(self, context):
         self.context = context
         self.task = context["task"]
 
+    def __lt__(self, other):
+        return self.get_order() < other.get_order()
+
+    def __gt__(self, other):
+        return self.get_order() > other.get_order()
+
+    def __eq__(self, other):
+        return self.get_order() == other.get_order()
+
     @classmethod
     def validate(cls, config, non_hidden=False):
         if non_hidden and cls.__ctx_hidden__:
@@ -56,7 +68,14 @@ def validate(cls, config, non_hidden=False):
     @classmethod
     def validate_semantic(cls, config, admin=None, users=None, task=None):
         """Context semantic validation towards the deployment."""
-        pass
+
+    @classmethod
+    def get_name(cls):
+        return cls.__ctx_name__
+
+    @classmethod
+    def get_order(cls):
+        return cls.__ctx_order__
 
     @staticmethod
     def get_by_name(name):
@@ -84,14 +103,9 @@ def __exit__(self, exc_type, exc_value, exc_traceback):
 class ContextManager(object):
     """Create context environment and run method inside it."""
 
-    @staticmethod
-    def run(context, func, cls, method_name, args):
-        ctxlst = [Context.get_by_name(name) for name in context["config"]]
-        ctxlst = map(lambda ctx: ctx(context),
-                     sorted(ctxlst, key=lambda x: x.__ctx_order__))
-
-        return ContextManager._magic(ctxlst, func, cls,
-                                     method_name, context, args)
+    def __init__(self, context_obj):
+        self._visited = []
+        self.context_obj = context_obj
 
     @staticmethod
     def validate(context, non_hidden=False):
@@ -104,26 +118,37 @@ def validate_semantic(context, admin=None, users=None, task=None):
             Context.get_by_name(name).validate_semantic(config, admin=admin,
                                                         users=users, task=task)
 
-    @staticmethod
-    def _magic(ctxlst, func, *args):
-        """Some kind of contextlib.nested but with black jack & recursion.
-
-        This method uses recursion to build nested "with" from list of context
-        objects. As it's actually a combination of dark and voodoo magic I
-        called it "_magic". Please don't repeat at home.
-
-        :param ctxlst: list of instances of subclasses of Context
-        :param func: function that will be called inside this context
-        :param args: args that will be passed to function `func`
-        :returns: result of function call
-        """
-        if not ctxlst:
-            return func(*args)
-
-        with ctxlst[0]:
-            # TODO(boris-42): call of setup could be moved inside __enter__
-            #                 but it should be in try-except, and in except
-            #                 we should call by hand __exit__
-            ctxlst[0].setup()
-            tmp = ContextManager._magic(ctxlst[1:], func, *args)
-            return tmp
+    def _get_sorted_context_lst(self):
+        ctxlst = map(Context.get_by_name, self.context_obj["config"])
+        return sorted(map(lambda ctx: ctx(self.context_obj), ctxlst))
+
+    def setup(self):
+        """Creates benchmark environment from config."""
+
+        self._visited = []
+        for ctx in self._get_sorted_context_lst():
+            self._visited.append(ctx)
+            ctx.setup()
+
+        return self.context_obj
+
+    def cleanup(self):
+        """Destroys benchmark environment."""
+
+        ctxlst = self._visited or self._get_sorted_context_lst()
+        for ctx in ctxlst[::-1]:
+            try:
+                ctx.cleanup()
+            except Exception as e:
+                LOG.error("Context %s failed during cleanup." % ctx.get_name())
+                LOG.exception(e)
+
+    def __enter__(self):
+        try:
+            self.setup()
+        except Exception:
+            self.cleanup()
+            raise
+
+    def __exit__(self, exc_type, exc_value, exc_traceback):
+        self.cleanup()

rally/benchmark/runners/base.py

@@ -197,13 +197,6 @@ def _run_scenario(self, cls, method_name, context, args):
                   where each result is a dictionary
         """
 
-    def _wrap_run_scenario(self, cls, method_name, context, args):
-        """Whole scenario time measurement without context preparation."""
-
-        with rutils.Timer() as timer:
-            self._run_scenario(cls, method_name, context, args)
-        return timer.duration()
-
     def run(self, name, context, args):
         cls_name, method_name = name.split(".", 1)
         cls = scenario_base.Scenario.get_by_name(cls_name)
@@ -225,9 +218,10 @@ def run(self, name, context, args):
 
         args = cls.preprocess(method_name, context_obj, args)
 
-        return base_ctx.ContextManager.run(context_obj,
-                                           self._wrap_run_scenario,
-                                           cls, method_name, args)
+        with base_ctx.ContextManager(context_obj):
+            with rutils.Timer() as timer:
+                self._run_scenario(cls, method_name, context_obj, args)
+            return timer.duration()
 
     def _send_result(self, result):
         """Send partial result to consumer.

tests/benchmark/context/test_base.py

@@ -19,6 +19,7 @@
 
 from rally.benchmark.context import base
 from rally import exceptions
+from rally import utils
 from tests import fakes
 from tests import test
 
@@ -106,37 +107,30 @@ def test_with_statement(self):
 
         ctx.cleanup.assert_called_once_with()
 
+    def test_all_context_have_ctxt_order(self):
+        for cls in utils.itersubclasses(base.Context):
+            self.assertNotEqual(cls.get_order(), 0, str(cls))
 
-class ContextManagerTestCase(test.TestCase):
+    def test_all_context_have_ctxt_name(self):
+        for cls in utils.itersubclasses(base.Context):
+            self.assertNotEqual(cls.get_name(), "base", str(cls))
 
-    @mock.patch("rally.benchmark.context.base.ContextManager._magic")
-    @mock.patch("rally.benchmark.context.base.Context.get_by_name")
-    def test_run(self, mock_get, mock_magic):
-        context = {
-            "config": {
-                "a": mock.MagicMock(),
-                "b": mock.MagicMock()
-            }
-        }
+    def test_lt(self):
+        self.assertTrue(base.Context < fakes.FakeContext)
+        self.assertFalse(fakes.FakeContext < base.Context)
+        self.assertFalse(base.Context < base.Context)
 
-        cc = mock.MagicMock()
-        cc.__ctx_order__ = 10
-        mock_get.return_value = cc
+    def test_gt(self):
+        self.assertTrue(fakes.FakeContext > base.Context)
+        self.assertFalse(base.Context > fakes.FakeContext)
+        self.assertFalse(base.Context > base.Context)
 
-        mock_magic.return_value = 5
+    def test_eq(self):
+        self.assertFalse(base.Context == fakes.FakeContext)
+        self.assertTrue(base.Context == base.Context)
 
-        result = base.ContextManager.run(context, lambda x, y: x + y, type,
-                                         "fake_method", {"fake": "value"})
-        self.assertEqual(result, 5)
 
-        mock_get.assert_has_calls([
-            mock.call("a"),
-            mock.call("b"),
-        ], any_order=True)
-        mock_get.assert_has_calls([
-            mock.call()(context),
-            mock.call()(context),
-        ], any_order=True)
+class ContextManagerTestCase(test.TestCase):
 
     @mock.patch("rally.benchmark.context.base.Context.get_by_name")
     def test_validate(self, mock_get):
@@ -188,20 +182,73 @@ def test_validate__non_existing_context(self):
         self.assertRaises(exceptions.NoSuchContext,
                           base.ContextManager.validate, config)
 
-    def test__magic(self):
-        func = lambda x, y: x + y
-
-        result = base.ContextManager._magic([], func, 2, 3)
-        self.assertEqual(result, 5)
-
-    def test__magic_with_ctx(self):
-        ctx = [mock.MagicMock(), mock.MagicMock()]
-        func = lambda x, y: x + y
+    @mock.patch("rally.benchmark.context.base.Context.get_by_name")
+    def test_setup(self, mock_get_by_name):
+        mock_context = mock.MagicMock()
+        mock_get_by_name.return_value = mock_context
+        ctx_object = {"config": {"a": [], "b": []}}
+
+        manager = base.ContextManager(ctx_object)
+        result = manager.setup()
+
+        self.assertEqual(result, ctx_object)
+        mock_get_by_name.assert_has_calls([mock.call("a"), mock.call("b")])
+        mock_context.assert_has_calls([mock.call(ctx_object),
+                                       mock.call(ctx_object)])
+        self.assertEqual([mock_context(), mock_context()], manager._visited)
+        mock_context.return_value.assert_has_calls([mock.call.setup(),
+                                                    mock.call.setup()])
 
-        result = base.ContextManager._magic(ctx, func, 2, 3)
-        self.assertEqual(result, 5)
+    @mock.patch("rally.benchmark.context.base.Context.get_by_name")
+    def test_cleanup(self, mock_get_by_name):
+        mock_context = mock.MagicMock()
+        mock_get_by_name.return_value = mock_context
+        ctx_object = {"config": {"a": [], "b": []}}
+
+        manager = base.ContextManager(ctx_object)
+        manager.cleanup()
+        mock_get_by_name.assert_has_calls([mock.call("a"), mock.call("b")])
+        mock_context.assert_has_calls([mock.call(ctx_object),
+                                       mock.call(ctx_object)])
+        mock_context.return_value.assert_has_calls([mock.call.cleanup(),
+                                                    mock.call.cleanup()])
 
-        expected = [mock.call.__enter__(), mock.call.setup(),
-                    mock.call.__exit__(None, None, None)]
-        for c in ctx:
-            ctx[0].assert_has_calls(expected)
+    @mock.patch("rally.benchmark.context.base.Context.get_by_name")
+    def test_cleanp_exception(self, mock_get_by_name):
+        mock_context = mock.MagicMock()
+        mock_context.cleanup.side_effect = Exception()
+        mock_get_by_name.return_value = mock_context
+        ctx_object = {"config": {"a": [], "b": []}}
+        manager = base.ContextManager(ctx_object)
+        manager.cleanup()
+
+        mock_get_by_name.assert_has_calls([mock.call("a"), mock.call("b")])
+        mock_context.assert_has_calls([mock.call(ctx_object),
+                                       mock.call(ctx_object)])
+        mock_context.return_value.assert_has_calls([mock.call.cleanup(),
+                                                    mock.call.cleanup()])
+
+    @mock.patch("rally.benchmark.context.base.ContextManager.cleanup")
+    @mock.patch("rally.benchmark.context.base.ContextManager.setup")
+    def test_with_statement(self, mock_setup, mock_cleanup):
+        with base.ContextManager(mock.MagicMock()):
+            mock_setup.assert_called_once_with()
+            mock_setup.reset_mock()
+            self.assertFalse(mock_cleanup.called)
+        self.assertFalse(mock_setup.called)
+        mock_cleanup.assert_called_once_with()
+
+    @mock.patch("rally.benchmark.context.base.ContextManager.cleanup")
+    @mock.patch("rally.benchmark.context.base.ContextManager.setup")
+    def test_with_statement_excpetion_during_setup(self, mock_setup,
+                                                   mock_cleanup):
+        mock_setup.side_effect = Exception("abcdef")
+
+        try:
+            with base.ContextManager(mock.MagicMock()):
+                pass
+        except Exception:
+            pass
+        finally:
+            mock_setup.assert_called_once_with()
+            mock_cleanup.assert_called_once_with()

tests/benchmark/runners/test_base.py

@@ -17,7 +17,6 @@
 import mock
 
 from rally.benchmark.runners import base
-from rally.benchmark.runners import constant
 from rally.benchmark.runners import serial
 from rally.benchmark.scenarios import base as scenario_base
 from rally import consts
@@ -221,17 +220,23 @@ def test_validate_default_runner(self, mock_validate):
                 config,
                 serial.SerialScenarioRunner.CONFIG_SCHEMA)
 
-    @mock.patch("rally.benchmark.runners.base.osclients")
-    @mock.patch("rally.benchmark.runners.base.base_ctx.ContextManager")
-    def test_run(self, mock_ctx_manager, mock_osclients):
-        runner = constant.ConstantScenarioRunner(
+    @mock.patch("rally.benchmark.runners.base.rutils.Timer.duration")
+    @mock.patch("rally.benchmark.runners.base.base_ctx.ContextManager.setup")
+    @mock.patch("rally.benchmark.runners.base.base_ctx.ContextManager.cleanup")
+    def test_run(self, mock_setup, mock_cleanup, mock_duration):
+        mock_duration.return_value = 10
+        runner = serial.SerialScenarioRunner(
             mock.MagicMock(),
             self.fake_endpoints,
             mock.MagicMock())
+
+        runner._run_scenario = mock.MagicMock()
+
         scenario_name = "NovaServers.boot_server_from_volume_and_delete"
         config_kwargs = {"image": {"id": 1}, "flavor": {"id": 1}}
-        runner.run(scenario_name, {"some_ctx": 2}, config_kwargs)
+        result = runner.run(scenario_name, {"some_ctx": 2}, config_kwargs)
 
+        self.assertEqual(result, mock_duration.return_value)
         self.assertEqual(list(runner.result_queue), [])
 
         cls_name, method_name = scenario_name.split(".", 1)
@@ -246,12 +251,11 @@ def test_run(self, mock_ctx_manager, mock_osclients):
             }
         }
 
-        expected = [context_obj, runner._wrap_run_scenario, cls,
-                    method_name, config_kwargs]
-        mock_ctx_manager.run.assert_called_once_with(*expected)
+        runner._run_scenario.assert_called_once_with(
+            cls, method_name, context_obj, config_kwargs)
 
     def test_runner_send_result_exception(self):
-        runner = constant.ConstantScenarioRunner(
+        runner = serial.SerialScenarioRunner(
             mock.MagicMock(),
             self.fake_endpoints,
             mock.MagicMock())

tests/fakes.py

@@ -957,6 +957,7 @@ def duration(self):
 class FakeContext(base_ctx.Context):
 
     __ctx_name__ = "fake"
+    __ctx_order__ = 1
 
     CONFIG_SCHEMA = {
         "type": "object",