From 40cb090df6fe61ea0435657e50a352ea6ee9ea75 Mon Sep 17 00:00:00 2001 From: Boris Pavlovic Date: Mon, 4 Aug 2014 03:08:21 +0400 Subject: [PATCH] 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 | 83 +++++++++++------ rally/benchmark/runners/base.py | 14 +-- tests/benchmark/context/test_base.py | 131 ++++++++++++++++++--------- tests/benchmark/runners/test_base.py | 24 +++-- tests/fakes.py | 1 + 5 files changed, 162 insertions(+), 91 deletions(-) diff --git a/rally/benchmark/context/base.py b/rally/benchmark/context/base.py index 7abb74b6b9..02e4065b46 100644 --- a/rally/benchmark/context/base.py +++ b/rally/benchmark/context/base.py @@ -19,8 +19,11 @@ import jsonschema 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 @@ class Context(object): 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 @@ class Context(object): @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 @@ class Context(object): 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 @@ class ContextManager(object): 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. + 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)) - 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. + def setup(self): + """Creates benchmark environment from config.""" - :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) + self._visited = [] + for ctx in self._get_sorted_context_lst(): + self._visited.append(ctx) + ctx.setup() - 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 + 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() diff --git a/rally/benchmark/runners/base.py b/rally/benchmark/runners/base.py index cc02410314..707de21884 100644 --- a/rally/benchmark/runners/base.py +++ b/rally/benchmark/runners/base.py @@ -197,13 +197,6 @@ class ScenarioRunner(object): 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 @@ class ScenarioRunner(object): 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. diff --git a/tests/benchmark/context/test_base.py b/tests/benchmark/context/test_base.py index 36194fdd01..4f96133e4d 100644 --- a/tests/benchmark/context/test_base.py +++ b/tests/benchmark/context/test_base.py @@ -19,6 +19,7 @@ import mock from rally.benchmark.context import base from rally import exceptions +from rally import utils from tests import fakes from tests import test @@ -106,38 +107,31 @@ class BaseContextTestCase(test.TestCase): 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)) + + def test_all_context_have_ctxt_name(self): + for cls in utils.itersubclasses(base.Context): + self.assertNotEqual(cls.get_name(), "base", str(cls)) + + def test_lt(self): + self.assertTrue(base.Context < fakes.FakeContext) + self.assertFalse(fakes.FakeContext < base.Context) + self.assertFalse(base.Context < base.Context) + + def test_gt(self): + self.assertTrue(fakes.FakeContext > base.Context) + self.assertFalse(base.Context > fakes.FakeContext) + self.assertFalse(base.Context > base.Context) + + def test_eq(self): + self.assertFalse(base.Context == fakes.FakeContext) + self.assertTrue(base.Context == base.Context) + class ContextManagerTestCase(test.TestCase): - @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() - } - } - - cc = mock.MagicMock() - cc.__ctx_order__ = 10 - mock_get.return_value = cc - - mock_magic.return_value = 5 - - 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) - @mock.patch("rally.benchmark.context.base.Context.get_by_name") def test_validate(self, mock_get): config = { @@ -188,20 +182,73 @@ class ContextManagerTestCase(test.TestCase): self.assertRaises(exceptions.NoSuchContext, base.ContextManager.validate, config) - def test__magic(self): - 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": []}} - result = base.ContextManager._magic([], func, 2, 3) - self.assertEqual(result, 5) + manager = base.ContextManager(ctx_object) + result = manager.setup() - def test__magic_with_ctx(self): - ctx = [mock.MagicMock(), mock.MagicMock()] - func = lambda x, y: x + y + 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": []}} - expected = [mock.call.__enter__(), mock.call.setup(), - mock.call.__exit__(None, None, None)] - for c in ctx: - ctx[0].assert_has_calls(expected) + 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.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() diff --git a/tests/benchmark/runners/test_base.py b/tests/benchmark/runners/test_base.py index eedc3fc1a0..ea736dea72 100644 --- a/tests/benchmark/runners/test_base.py +++ b/tests/benchmark/runners/test_base.py @@ -17,7 +17,6 @@ import jsonschema 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 @@ class ScenarioRunnerTestCase(test.TestCase): 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 @@ class ScenarioRunnerTestCase(test.TestCase): } } - 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()) diff --git a/tests/fakes.py b/tests/fakes.py index 9d4d561e31..067e7af2dd 100644 --- a/tests/fakes.py +++ b/tests/fakes.py @@ -957,6 +957,7 @@ class FakeTimer(rally_utils.Timer): class FakeContext(base_ctx.Context): __ctx_name__ = "fake" + __ctx_order__ = 1 CONFIG_SCHEMA = { "type": "object",