From: Andrew Schoen Date: Wed, 26 Nov 2014 15:44:23 +0000 (-0600) Subject: Various bug fixes while running tests on this branch in the octo lab. X-Git-Tag: 1.1.0~1067^2~22^2~5 X-Git-Url: http://git.apps.os.sepia.ceph.com/?a=commitdiff_plain;h=d3f3b786ea01d719e593faa9581d8bd9b8251477;p=teuthology.git Various bug fixes while running tests on this branch in the octo lab. Made YamlConfig work more closely like a dictionary by extending collections.MutableMapping. It still needs some work to be able to be used interchangeably with a dict - most noteably with yaml.safe_dump. Decided to just use a dict instead of JobConfig for args['']. Too many other codepaths are assuming that config is a dict. Signed-off-by: Andrew Schoen --- diff --git a/scripts/run.py b/scripts/run.py index cf857ec8b6..cb6fbfa004 100644 --- a/scripts/run.py +++ b/scripts/run.py @@ -1,7 +1,7 @@ """ usage: teuthology --help teuthology --version - teuthology [options] ... + teuthology [options] [--] ... Run ceph integration tests diff --git a/teuthology/config.py b/teuthology/config.py index 54c28513f4..3091b4b7d2 100644 --- a/teuthology/config.py +++ b/teuthology/config.py @@ -1,6 +1,7 @@ import os import yaml import logging +import collections def init_logging(): @@ -10,7 +11,7 @@ def init_logging(): log = init_logging() -class YamlConfig(object): +class YamlConfig(collections.MutableMapping): """ A configuration object populated by parsing a yaml file, with optional default values. @@ -79,15 +80,34 @@ class YamlConfig(object): """ return str(self) + def get(self, key, default=None): + """ + A function that acts like dict.get(). Will first check self._conf, then + _defaults for the given key. + + :param key: The key to fetch + :returns: The value for the given key, or the default kwarg if not found + """ + result = self.__getattr__(key) + if not result: + return default + return result + def __str__(self): return yaml.safe_dump(self._conf, default_flow_style=False).strip() + def __repr__(self): + return self.__str__() + def __getitem__(self, name): - return self._conf.__getitem__(name) + return self.__getattr__(name) def __getattr__(self, name): return self._conf.get(name, self._defaults.get(name)) + def __contains__(self, name): + return self._conf.__contains__(name) + def __setattr__(self, name, value): if name.endswith('_conf') or name in ('yaml_path'): object.__setattr__(self, name, value) @@ -97,6 +117,18 @@ class YamlConfig(object): def __delattr__(self, name): del self._conf[name] + def __len__(self): + return self._conf.__len__() + + def __iter__(self): + return self._conf.__iter__() + + def __setitem__(self, name, value): + self._conf.__setitem__(name, value) + + def __delitem__(self, name): + self._conf.__delitem__(name) + class TeuthologyConfig(YamlConfig): """ @@ -128,12 +160,13 @@ class JobConfig(YamlConfig): class FakeNamespace(YamlConfig): - """ This class is meant to behave like a argparse Namespace with an attached - teuthology config at the teuthology_config property. It mimics the old - way of doing things with argparse and teuthology.misc.read_config. + """ + This class is meant to behave like a argparse Namespace with an attached + teuthology config at the teuthology_config property. It mimics the old + way of doing things with argparse and teuthology.misc.read_config. - We'll use this as a stop-gap as we refactor commands but allow the tasks - to still be passed a single namespace object for the time being. + We'll use this as a stop-gap as we refactor commands but allow the tasks + to still be passed a single namespace object for the time being. """ def __init__(self, config_dict=None, yaml_path=None): if not yaml_path: @@ -146,8 +179,9 @@ class FakeNamespace(YamlConfig): self._conf = self._clean_config(config_dict) def _clean_config(self, config_dict): - """ Makes sure that the keys of config_dict are able to be used. For example - the "--" prefix of a docopt dict isn't valid and won't populate correctly. + """ + Makes sure that the keys of config_dict are able to be used. For example + the "--" prefix of a docopt dict isn't valid and won't populate correctly. """ result = dict() for key, value in config_dict.iteritems(): diff --git a/teuthology/misc.py b/teuthology/misc.py index 48766a1f12..aab0d89ed6 100644 --- a/teuthology/misc.py +++ b/teuthology/misc.py @@ -112,6 +112,9 @@ def merge_configs(config_paths): """ conf_dict = dict() for conf_path in config_paths: + if not os.path.exists(conf_path): + log.debug("The config path {0} does not exist, skipping.".format(conf_path)) + continue with file(conf_path) as partial_file: partial_dict = yaml.safe_load(partial_file) try: diff --git a/teuthology/run.py b/teuthology/run.py index bd98874e08..bc11b68d95 100644 --- a/teuthology/run.py +++ b/teuthology/run.py @@ -14,7 +14,7 @@ from .nuke import nuke from .run_tasks import run_tasks from .repo_utils import fetch_qa_suite from .results import email_results -from .config import JobConfig, FakeNamespace +from .config import FakeNamespace log = logging.getLogger(__name__) @@ -100,9 +100,10 @@ def fetch_tasks_if_needed(job_config): def setup_config(config_paths): - """ Takes a list of config yaml files and combines them - into a single dictionary. Processes / validates the dictionary and then - returns a JobConfig instance. + """ + Takes a list of config yaml files and combines them + into a single dictionary. Processes / validates the dictionary and then + returns a JobConfig instance. """ config = merge_configs(config_paths) @@ -121,12 +122,13 @@ def setup_config(config_paths): '%d targets are needed for all roles but found %d listed.' % ( roles, targets) - return JobConfig.from_dict(config) + return config def get_machine_type(machine_type, config): - """ If no machine_type is given, find the appropriate machine_type - from the given config. + """ + If no machine_type is given, find the appropriate machine_type + from the given config. """ if machine_type is None: fallback_default = config.get('machine_type', 'plana') @@ -146,9 +148,10 @@ def get_summary(owner, description): def validate_tasks(config): - """ Ensures that config tasks do not include 'kernal'. + """ + Ensures that config tasks do not include 'kernel'. - If there is not tasks, return an empty list. + If there is not tasks, return an empty list. """ if 'tasks' not in config: log.warning('No tasks specified. Continuing anyway...') @@ -160,6 +163,8 @@ def validate_tasks(config): 'of the tasks list') assert 'kernel' not in task, msg + return config["tasks"] + def get_initial_tasks(lock, config, machine_type): init_tasks = [] @@ -237,47 +242,47 @@ def report_outcome(config, archive, summary, fake_ctx): sys.exit(1) -def main(ctx): - set_up_logging(ctx["--verbose"], ctx["--archive"]) +def main(args): + set_up_logging(args["--verbose"], args["--archive"]) - if ctx["--owner"] is None: - ctx["--owner"] = get_user() + if args["--owner"] is None: + args["--owner"] = get_user() - ctx[""] = setup_config(ctx[""]) + args[""] = setup_config(args[""]) - write_initial_metadata(ctx["--archive"], ctx[""], ctx["--name"], ctx["--description"], ctx["--owner"]) - report.try_push_job_info(ctx[""], dict(status='running')) + write_initial_metadata(args["--archive"], args[""], args["--name"], args["--description"], args["--owner"]) + report.try_push_job_info(args[""], dict(status='running')) - machine_type = get_machine_type(ctx["--machine-type"]) + machine_type = get_machine_type(args["--machine-type"], args[""]) - if ctx["--block"]: - assert ctx["--lock"], \ + if args["--block"]: + assert args["--lock"], \ 'the --block option is only supported with the --lock option' - log.debug('\n '.join(['Config:', ] + yaml.safe_dump( - ctx[""], default_flow_style=False).splitlines())) + log.debug('\n '.join(['Config:', ] + yaml.safe_dump(args[""], default_flow_style=False).splitlines())) - ctx["summary"] = get_summary(ctx["--owner"], ctx["--description"]) + args["summary"] = get_summary(args["--owner"], args["--description"]) - ctx[""]["tasks"] = validate_tasks(ctx[""]) + args[""]["tasks"] = validate_tasks(args[""]) - init_tasks = get_initial_tasks(ctx["--lock"], ctx[""], machine_type) + init_tasks = get_initial_tasks(args["--lock"], args[""], machine_type) - ctx[""]['tasks'].insert(0, init_tasks) + # prepend init_tasks to the front of the task list + args[""]['tasks'][:0] = init_tasks - if ctx["--suite-path"] is not None: - ctx[""]['suite_path'] = ctx["--suite-path"] + if args["--suite-path"] is not None: + args[""]['suite_path'] = args["--suite-path"] # fetches the tasks and returns a new suite_path if needed - ctx[""]["suite_path"] = fetch_tasks_if_needed(ctx[""]) + args[""]["suite_path"] = fetch_tasks_if_needed(args[""]) # create a FakeNamespace instance that mimics the old argparse way of doing things # we do this so we can pass it to run_tasks without porting those tasks to the # new way of doing things right now - fake_ctx = FakeNamespace(ctx) + fake_ctx = FakeNamespace(args) try: - run_tasks(tasks=ctx[""]['tasks'], ctx=fake_ctx) + run_tasks(tasks=args[""]['tasks'], ctx=fake_ctx) finally: # print to stdout the results and possibly send an email on any errors - report_outcome(ctx[""], ctx["--archive"], ctx["summary"], fake_ctx) + report_outcome(args[""], args["--archive"], args["summary"], fake_ctx) diff --git a/teuthology/test/test_config.py b/teuthology/test/test_config.py index 727b432411..8bb8c149a0 100644 --- a/teuthology/test/test_config.py +++ b/teuthology/test/test_config.py @@ -18,6 +18,14 @@ class TestYamlConfig(object): conf_obj = self.test_class.from_dict(in_dict) assert conf_obj.foo == 'bar' + def test_contains(self): + in_dict = dict(foo='bar') + conf_obj = self.test_class.from_dict(in_dict) + conf_obj.bar = "foo" + assert "bar" in conf_obj + assert "foo" in conf_obj + assert "baz" not in conf_obj + def test_to_dict(self): in_dict = dict(foo='bar') conf_obj = self.test_class.from_dict(in_dict) @@ -48,6 +56,24 @@ class TestYamlConfig(object): del conf_obj.foo assert conf_obj.foo is None + def test_get(self): + conf_obj = self.test_class() + conf_obj.foo = "bar" + assert conf_obj.get("foo") == "bar" + assert conf_obj.get("not_there", "default") == "default" + + def test_assignment(self): + conf_obj = self.test_class() + conf_obj["foo"] = "bar" + assert conf_obj["foo"] == "bar" + assert conf_obj.foo == "bar" + + def test_used_with_update(self): + d = dict() + conf_obj = self.test_class.from_dict({"foo": "bar"}) + d.update(conf_obj) + assert d["foo"] == "bar" + class TestTeuthologyConfig(TestYamlConfig): def setup(self): @@ -101,8 +127,10 @@ class TestFakeNamespace(TestYamlConfig): and that defaults are properly used. However, we won't check all the defaults. """ - conf_obj = self.test_class(dict()) - assert conf_obj.teuthology_config + conf_obj = self.test_class(dict(foo="bar")) + assert conf_obj["foo"] == "bar" + assert conf_obj.foo == "bar" assert conf_obj.teuthology_config.archive_base assert not conf_obj.teuthology_config.automated_scheduling assert conf_obj.teuthology_config.ceph_git_base_url == 'https://github.com/ceph/' + assert conf_obj.teuthology_config["ceph_git_base_url"] == 'https://github.com/ceph/' diff --git a/teuthology/test/test_misc.py b/teuthology/test/test_misc.py index 7671006c3b..504912c56a 100644 --- a/teuthology/test/test_misc.py +++ b/teuthology/test/test_misc.py @@ -97,14 +97,16 @@ class TestHostnames(object): class TestMergeConfigs(object): """ Tests merge_config and deep_merge in teuthology.misc """ + @patch("os.path.exists") @patch("yaml.safe_load") @patch("__builtin__.file") - def test_merge_configs(self, m_file, m_safe_load): + def test_merge_configs(self, m_file, m_safe_load, m_exists): """ Only tests with one yaml file being passed, mainly just to test the loop logic. The actual merge will be tested in subsequent tests. """ expected = {"a": "b", "b": "c"} + m_exists.return_value = True m_safe_load.return_value = expected result = misc.merge_configs(["path/to/config1"]) assert result == expected diff --git a/teuthology/test/test_run.py b/teuthology/test/test_run.py index d43d0002a0..a9f874e01d 100644 --- a/teuthology/test/test_run.py +++ b/teuthology/test/test_run.py @@ -25,7 +25,7 @@ class TestRun(object): m_merge_configs.return_value = config result = run.setup_config(["some/config.yaml"]) assert m_merge_configs.called - assert result.job_id == "1" + assert result["job_id"] == "1" assert result["foo"] == "bar" @patch("teuthology.run.merge_configs") @@ -33,8 +33,8 @@ class TestRun(object): config = {"targets": range(4), "roles": range(2)} m_merge_configs.return_value = config result = run.setup_config(["some/config.yaml"]) - assert result.targets - assert result.roles + assert result["targets"] == [0, 1, 2, 3] + assert result["roles"] == [0, 1] @patch("teuthology.run.merge_configs") def test_setup_config_targets_invalid(self, m_merge_configs): @@ -80,6 +80,11 @@ class TestRun(object): result = run.validate_tasks({}) assert result == [] + def test_validate_tasks_valid(self): + expected = {"foo": "bar"} + result = run.validate_tasks({"tasks": expected}) + assert result == expected + def test_get_initial_tasks_invalid(self): with pytest.raises(AssertionError): run.get_initial_tasks(True, {"targets": "can't be here"}, "machine_type")