From ae4f9d20233b87e6c3ae3560763b1e5302b24200 Mon Sep 17 00:00:00 2001 From: Mikhail Dubov Date: Wed, 1 Oct 2014 16:45:45 +0400 Subject: [PATCH] Fix messages in rally exceptions Many exceptions in rally.exceptions contain "%(message)s" in their msg_fmt attribute. When raised with message="...", however, these Exceptions just assign the passed string to the message attribute instead of formatting it with msg_fmt. We fix this issue here and also refactor a bit the scenario classes: * Remove the unused InvalidTaskConfigException; * Move the DummyScenarioException to the dummy bechmark scenario module; * Move the Tempest* exceptions to the corresponding modules Change-Id: I551f38edd5ec7bc67dd952a2203508f64c4b2518 --- rally/benchmark/context/images.py | 2 +- rally/benchmark/context/tempest.py | 5 +- rally/benchmark/engine.py | 2 +- rally/benchmark/processing/utils.py | 2 +- rally/benchmark/scenarios/base.py | 7 ++- rally/benchmark/scenarios/dummy/dummy.py | 8 ++- rally/benchmark/scenarios/tempest/utils.py | 8 ++- rally/deploy/engines/fuel.py | 2 +- rally/exceptions.py | 54 +++++++------------ .../verification/verifiers/tempest/config.py | 14 +++-- .../verification/verifiers/tempest/tempest.py | 6 ++- tests/unit/benchmark/context/test_tempest.py | 6 ++- .../benchmark/scenarios/dummy/test_dummy.py | 5 +- .../verification/verifiers/test_config.py | 7 ++- 14 files changed, 63 insertions(+), 65 deletions(-) diff --git a/rally/benchmark/context/images.py b/rally/benchmark/context/images.py index d1527b1a3c..8ea12cd3d6 100644 --- a/rally/benchmark/context/images.py +++ b/rally/benchmark/context/images.py @@ -111,4 +111,4 @@ class ImageGenerator(base.Context): message = _( "The image service is unavailable, Reason: %(reason)s") % { "reason": six.text_type(e)} - raise exceptions.InvalidScenarioArgument(message=message) + raise exceptions.InvalidScenarioArgument(message) diff --git a/rally/benchmark/context/tempest.py b/rally/benchmark/context/tempest.py index 351f546d18..2663279a62 100644 --- a/rally/benchmark/context/tempest.py +++ b/rally/benchmark/context/tempest.py @@ -23,6 +23,7 @@ from rally import exceptions from rally.openstack.common.gettextutils import _ from rally.openstack.common import log as logging from rally import utils +from rally.verification.verifiers.tempest import config from rally.verification.verifiers.tempest import tempest LOG = logging.getLogger(__name__) @@ -48,11 +49,11 @@ class Tempest(base.Context): self.verifier.install() if not self.verifier.is_configured(): self.verifier.generate_config_file() - except exceptions.TempestSetupFailure: + except tempest.TempestSetupFailure: msg = _("Failing to install tempest.") LOG.error(msg) raise exceptions.BenchmarkSetupFailure(msg) - except exceptions.TempestConfigCreationFailure: + except config.TempestConfigCreationFailure: msg = _("Failing to configure tempest.") LOG.error(msg) raise exceptions.BenchmarkSetupFailure(msg) diff --git a/rally/benchmark/engine.py b/rally/benchmark/engine.py index d875c5381c..8107ec548b 100644 --- a/rally/benchmark/engine.py +++ b/rally/benchmark/engine.py @@ -170,7 +170,7 @@ class BenchmarkEngine(object): except Exception as e: log = [str(type(e)), str(e), json.dumps(traceback.format_exc())] self.task.set_failed(log=log) - raise exceptions.InvalidTaskException(message=str(e)) + raise exceptions.InvalidTaskException(str(e)) def _get_runner(self, config): runner = config.get("runner", {}) diff --git a/rally/benchmark/processing/utils.py b/rally/benchmark/processing/utils.py index f7dabc38ea..fc89060c0c 100644 --- a/rally/benchmark/processing/utils.py +++ b/rally/benchmark/processing/utils.py @@ -27,7 +27,7 @@ def mean(values): """ if not values: raise exceptions.InvalidArgumentsException( - message="the list should be non-empty") + "the list should be non-empty") return math.fsum(values) / len(values) diff --git a/rally/benchmark/scenarios/base.py b/rally/benchmark/scenarios/base.py index a480db8153..c98a350fde 100644 --- a/rally/benchmark/scenarios/base.py +++ b/rally/benchmark/scenarios/base.py @@ -126,11 +126,10 @@ class Scenario(object): try: result = validator(config, clients=clients, task=task) except Exception as e: - raise exceptions.InvalidScenarioArgument(message=e) + raise exceptions.InvalidScenarioArgument(e) else: if not result.is_valid: - raise exceptions.InvalidScenarioArgument( - message=result.msg) + raise exceptions.InvalidScenarioArgument(result.msg) @classmethod def validate(cls, name, config, admin=None, users=None, task=None): @@ -207,7 +206,7 @@ class Scenario(object): """ if not 0 <= min_sleep <= max_sleep: raise exceptions.InvalidArgumentsException( - message="0 <= min_sleep <= max_sleep") + "0 <= min_sleep <= max_sleep") sleep_time = random.uniform(min_sleep, max_sleep) time.sleep(sleep_time) diff --git a/rally/benchmark/scenarios/dummy/dummy.py b/rally/benchmark/scenarios/dummy/dummy.py index fc047c8a47..3f8bb326e9 100644 --- a/rally/benchmark/scenarios/dummy/dummy.py +++ b/rally/benchmark/scenarios/dummy/dummy.py @@ -18,6 +18,10 @@ from rally.benchmark import validation from rally import exceptions +class DummyScenarioException(exceptions.RallyException): + msg_fmt = _("Dummy scenario expected exception: '%(msg)s'") + + class Dummy(base.Scenario): """Benchmarks for testing Rally benchmark engine at scale.""" @@ -47,7 +51,7 @@ class Dummy(base.Scenario): :param size_of_message: the size of the message. """ - raise exceptions.DummyScenarioException("M" * size_of_message) + raise DummyScenarioException("M" * size_of_message) @validation.number("exception_probability", minval=0, maxval=1, integer_only=False, nullable=True) @@ -63,7 +67,7 @@ class Dummy(base.Scenario): """ if random.random() < exception_probability: - raise exceptions.DummyScenarioException( + raise DummyScenarioException( "Dummy Scenario Exception: Probability: %s" % exception_probability ) diff --git a/rally/benchmark/scenarios/tempest/utils.py b/rally/benchmark/scenarios/tempest/utils.py index 25b37a590b..32b18547aa 100644 --- a/rally/benchmark/scenarios/tempest/utils.py +++ b/rally/benchmark/scenarios/tempest/utils.py @@ -24,6 +24,10 @@ from rally import exceptions from rally.openstack.common.gettextutils import _ +class TempestBenchmarkFailure(exceptions.RallyException): + msg_fmt = _("Failed tempest test(s): '%(message)s'") + + def tempest_log_wrapper(func): @functools.wraps(func) def inner_func(scenario_obj, *args, **kwargs): @@ -46,10 +50,10 @@ def tempest_log_wrapper(func): scenario_obj._add_atomic_actions("test_execution", total.get("time")) if total.get("errors") or total.get("failures"): - raise exceptions.TempestBenchmarkFailure([ + raise TempestBenchmarkFailure([ test for test in six.itervalues(tests) if test["status"] == "FAIL"]) else: - raise exceptions.TempestBenchmarkFailure(_("No information")) + raise TempestBenchmarkFailure(_("No information")) return inner_func diff --git a/rally/deploy/engines/fuel.py b/rally/deploy/engines/fuel.py index 06b17504f2..603d93ce1b 100644 --- a/rally/deploy/engines/fuel.py +++ b/rally/deploy/engines/fuel.py @@ -140,7 +140,7 @@ class FuelEngine(engine.EngineFactory): if 'compute' not in self.config['nodes']: if 'cinder+compute' not in self.config['nodes']: raise exceptions.ValidationError( - message=_('At least one compute is required.')) + _('At least one compute is required.')) def _get_nodes(self, key): if key not in self.config['nodes']: diff --git a/rally/exceptions.py b/rally/exceptions.py index 2058a9f641..81787937cc 100644 --- a/rally/exceptions.py +++ b/rally/exceptions.py @@ -46,27 +46,29 @@ class RallyException(Exception): def __init__(self, message=None, **kwargs): self.kwargs = kwargs - if 'code' not in self.kwargs: + if "code" not in self.kwargs: try: - self.kwargs['code'] = self.code + self.kwargs["code"] = self.code except AttributeError: pass - if not message: - try: - message = self.msg_fmt % kwargs - except KeyError: - exc_info = sys.exc_info() - # kwargs doesn't match a variable in the message - # log the issue and the kwargs - msg = "kwargs don't match in string format operation: %s" - LOG.debug(msg % kwargs, exc_info=exc_info) + if "%(message)s" in self.msg_fmt: + kwargs = dict(kwargs.items() + {"message": message}.items()) - if CONF.fatal_exception_format_errors: - raise exc_info[0], exc_info[1], exc_info[2] - else: - # at least get the core message out if something happened - message = self.msg_fmt + try: + message = self.msg_fmt % kwargs + except KeyError: + exc_info = sys.exc_info() + # kwargs doesn't match a variable in the message + # log the issue and the kwargs + msg = "kwargs don't match in string format operation: %s" + LOG.debug(msg % kwargs, exc_info=exc_info) + + if CONF.fatal_exception_format_errors: + raise exc_info[0], exc_info[1], exc_info[2] + else: + # at least get the core message out if something happened + message = message or self.msg_fmt super(RallyException, self).__init__(message) @@ -98,10 +100,6 @@ class InvalidTaskException(InvalidConfigException): msg_fmt = _("This config is invalid: `%(message)s`") -class InvalidTaskConfigException(InvalidTaskException): - msg_fmt = _("This config has invalid schema: `%(message)s`") - - class NotFoundScenarios(InvalidTaskException): msg_fmt = _("There are no benchmark scenarios with names: `%(names)s`.") @@ -209,26 +207,10 @@ class InvalidScenarioArgument(RallyException): msg_fmt = _("Invalid scenario argument: '%(message)s'") -class TempestConfigCreationFailure(RallyException): - msg_fmt = _("Unable create tempest.conf: '%(message)s'") - - -class TempestSetupFailure(RallyException): - msg_fmt = _("Unable to setup tempest: '%(message)s'") - - -class TempestBenchmarkFailure(RallyException): - msg_fmt = _("Failed tempest test(s): '%(message)s'") - - class BenchmarkSetupFailure(RallyException): msg_fmt = _("Unable to setup benchmark: '%(message)s'") -class DummyScenarioException(RallyException): - msg_fmt = _("Dummy scenario expected exception: '%(message)s'") - - class ValidationError(RallyException): msg_fmt = _("Validation error: %(message)s") diff --git a/rally/verification/verifiers/tempest/config.py b/rally/verification/verifiers/tempest/config.py index 979c0c4d92..7f03388d01 100644 --- a/rally/verification/verifiers/tempest/config.py +++ b/rally/verification/verifiers/tempest/config.py @@ -46,6 +46,10 @@ CONF = cfg.CONF CONF.register_opts(image_opts, 'image') +class TempestConfigCreationFailure(exceptions.RallyException): + msg_fmt = _("Unable create tempest.conf: '%(message)s'") + + class TempestConf(object): def __init__(self, deploy_id): @@ -57,7 +61,7 @@ class TempestConf(object): msg = (_("Admin permission is required to generate tempest " "configuration file. User %s doesn't have admin role.") % self.endpoint['username']) - raise exceptions.TempestConfigCreationFailure(message=msg) + raise TempestConfigCreationFailure(msg) self.available_services = [service['name'] for service in self.keystoneclient. service_catalog.get_data()] @@ -82,7 +86,7 @@ class TempestConf(object): except requests.ConnectionError as err: msg = _('Error on downloading cirros image, possibly' ' no connection to Internet with message %s') % str(err) - raise exceptions.TempestConfigCreationFailure(message=msg) + raise TempestConfigCreationFailure(msg) if response.status_code == 200: with open(self.img_path + '.tmp', 'wb') as img_file: for chunk in response.iter_content(chunk_size=1024): @@ -97,7 +101,7 @@ class TempestConf(object): else: msg = _('Error on downloading cirros image, ' 'HTTP error code %s') % response.getcode() - raise exceptions.TempestConfigCreationFailure(message=msg) + raise TempestConfigCreationFailure(msg) def _get_url(self, servicename): for service in self.keystoneclient.auth_ref['serviceCatalog']: @@ -138,7 +142,7 @@ class TempestConf(object): msg = _('There are no desired images (cirros) or only one and ' 'new image could not be created.\n' 'Reason: %s') % e.message - raise exceptions.TempestConfigCreationFailure(message=msg) + raise TempestConfigCreationFailure(msg) self.conf.set(section_name, 'image_ref', image_list[0].id) self.conf.set(section_name, 'image_ref_alt', image_list[1].id) @@ -157,7 +161,7 @@ class TempestConf(object): msg = _('There are no desired flavors or only one and ' 'new flavor could not be created.\n' 'Reason: %s') % e.message - raise exceptions.TempestConfigCreationFailure(message=msg) + raise TempestConfigCreationFailure(msg) self.conf.set(section_name, 'flavor_ref', flavor_list[0].id) self.conf.set(section_name, 'flavor_ref_alt', flavor_list[1].id) diff --git a/rally/verification/verifiers/tempest/tempest.py b/rally/verification/verifiers/tempest/tempest.py index 19f8620004..53dd847566 100644 --- a/rally/verification/verifiers/tempest/tempest.py +++ b/rally/verification/verifiers/tempest/tempest.py @@ -30,6 +30,10 @@ from rally.verification.verifiers.tempest import subunit2json LOG = logging.getLogger(__name__) +class TempestSetupFailure(exceptions.RallyException): + msg_fmt = _("Unable to setup tempest: '%(message)s'") + + class Tempest(object): tempest_base_path = os.path.join(os.path.expanduser("~"), @@ -130,7 +134,7 @@ class Tempest(object): self._initialize_testr() except subprocess.CalledProcessError as e: self.uninstall() - raise exceptions.TempestSetupFailure("failed cmd: '%s'", e.cmd) + raise TempestSetupFailure("failed cmd: '%s'" % e.cmd) else: print("Tempest has been successfully installed!") diff --git a/tests/unit/benchmark/context/test_tempest.py b/tests/unit/benchmark/context/test_tempest.py index 8936012a30..bfdaad1262 100644 --- a/tests/unit/benchmark/context/test_tempest.py +++ b/tests/unit/benchmark/context/test_tempest.py @@ -19,6 +19,8 @@ import mock from rally.benchmark.context import tempest from rally import exceptions +from rally.verification.verifiers.tempest import config +from rally.verification.verifiers.tempest import tempest as tempest_verifier from tests.unit import test @@ -55,7 +57,7 @@ class TempestContextTestCase(test.TestCase): @mock.patch(TEMPEST + ".Tempest.install") def test_setup_failure_on_tempest_installation( self, mock_install, mock_is_installed, mock_is_cfg, mock_mkdir): - mock_install.side_effect = exceptions.TempestSetupFailure() + mock_install.side_effect = tempest_verifier.TempestSetupFailure() benchmark = tempest.Tempest(self.context) @@ -68,7 +70,7 @@ class TempestContextTestCase(test.TestCase): @mock.patch(TEMPEST + ".Tempest.generate_config_file") def test_setup_failure_on_tempest_configuration( self, mock_gen, mock_is_installed, mock_is_cfg, mock_mkdir): - mock_gen.side_effect = exceptions.TempestConfigCreationFailure() + mock_gen.side_effect = config.TempestConfigCreationFailure() benchmark = tempest.Tempest(self.context) diff --git a/tests/unit/benchmark/scenarios/dummy/test_dummy.py b/tests/unit/benchmark/scenarios/dummy/test_dummy.py index 2f6e75d805..53c20827d2 100644 --- a/tests/unit/benchmark/scenarios/dummy/test_dummy.py +++ b/tests/unit/benchmark/scenarios/dummy/test_dummy.py @@ -14,7 +14,6 @@ import mock from rally.benchmark.scenarios.dummy import dummy -from rally import exceptions from tests.unit import test @@ -34,7 +33,7 @@ class DummyTestCase(test.TestCase): scenario = dummy.Dummy() size_of_message = 5 - self.assertRaises(exceptions.DummyScenarioException, + self.assertRaises(dummy.DummyScenarioException, scenario.dummy_exception, size_of_message) def test_dummy_exception_probability(self): @@ -46,7 +45,7 @@ class DummyTestCase(test.TestCase): # should always raise an exception as probability is 1 for i in range(100): - self.assertRaises(exceptions.DummyScenarioException, + self.assertRaises(dummy.DummyScenarioException, scenario.dummy_exception_probability, {'exception_probability': 1}) diff --git a/tests/unit/verification/verifiers/test_config.py b/tests/unit/verification/verifiers/test_config.py index 30e2b933f2..78df027e66 100644 --- a/tests/unit/verification/verifiers/test_config.py +++ b/tests/unit/verification/verifiers/test_config.py @@ -18,7 +18,6 @@ import os import mock from oslo.config import cfg -from rally import exceptions from rally.verification.verifiers.tempest import config from tests.unit import fakes from tests.unit import test @@ -74,7 +73,7 @@ class ConfigTestCase(test.TestCase): mock_result = mock.MagicMock() mock_result.status_code = 404 mock_requests.get.return_value = mock_result - self.assertRaises(exceptions.TempestConfigCreationFailure, + self.assertRaises(config.TempestConfigCreationFailure, self.conf_generator._load_img) def test__get_url(self): @@ -149,7 +148,7 @@ class ConfigTestCase(test.TestCase): mock_novaclient.flavors.list.return_value = [] mock_novaclient.flavors.create.side_effect = Exception() mock_nova.Client.return_value = mock_novaclient - self.assertRaises(exceptions.TempestConfigCreationFailure, + self.assertRaises(config.TempestConfigCreationFailure, self.conf_generator._set_compute_flavors) @mock.patch("rally.osclients.glance") @@ -186,7 +185,7 @@ class ConfigTestCase(test.TestCase): mock_glanceclient.images.list.return_value = [] mock_glanceclient.images.create.side_effect = Exception() mock_glance.Client.return_value = mock_glanceclient - self.assertRaises(exceptions.TempestConfigCreationFailure, + self.assertRaises(config.TempestConfigCreationFailure, self.conf_generator._set_compute_images) def test__set_compute_ssh_connect_method_if_neutron(self):