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
This commit is contained in:
Boris Pavlovic
parent
24218d9ff8
commit
40cb090df6
@ -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()
|
||||
|
||||
@ -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.
|
||||
|
||||
@ -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()
|
||||
|
||||
@ -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())
|
||||
|
||||
@ -957,6 +957,7 @@ class FakeTimer(rally_utils.Timer):
|
||||
class FakeContext(base_ctx.Context):
|
||||
|
||||
__ctx_name__ = "fake"
|
||||
__ctx_order__ = 1
|
||||
|
||||
CONFIG_SCHEMA = {
|
||||
"type": "object",
|
||||
|
||||
Loading…
Reference in New Issue
Block a user