diff --git a/doc/source/inventory/configure-inventory.rst b/doc/source/inventory/configure-inventory.rst index 6ae9fdc870..5281a19e11 100644 --- a/doc/source/inventory/configure-inventory.rst +++ b/doc/source/inventory/configure-inventory.rst @@ -40,6 +40,17 @@ Like ``--check``, this flag is not invoked when running from ansible. Configuration constraints ~~~~~~~~~~~~~~~~~~~~~~~~~ +Group memberships +----------------- + +When adding groups, keep the following in mind: + + * A group can contain hosts + * A group can contain child groups + +However, groups cannot contain child groups and hosts. + + The lxc_hosts Group ------------------- diff --git a/lib/generate.py b/lib/generate.py index 0da9cac8a3..3ead52307d 100755 --- a/lib/generate.py +++ b/lib/generate.py @@ -121,9 +121,16 @@ class LxcHostsDefined(Exception): return self.message +class GroupConflict(Exception): + pass + + def _parse_belongs_to(key, belongs_to, inventory): """Parse all items in a `belongs_to` list. + This function assumes the key defined is a group that has child subgroups, + *not* a group with hosts defined in the group configuration. + :param key: ``str`` Name of key to append to a given entry :param belongs_to: ``list`` List of items to iterate over :param inventory: ``dict`` Living dictionary of inventory @@ -843,6 +850,38 @@ def _check_lxc_hosts(config): logger.debug("lxc_hosts group not defined") +def _check_group_branches(config, physical_skel): + """Ensure that groups have either hosts or child groups, not both + + The inventory skeleton population assumes that groups will either have + hosts as "leaves", or other groups as children, not both. This function + ensures this invariant is met by comparing the configuration to the + physical skeleton definition. + + :param config: ``dict`` The contents of the user configuration file. Keys + present in this dict are assumed to be groups containing host entries. + :param config: ``dict`` The physical skeleton tree, defining parent/child + relationships between groups. Values in the 'belong_to' key are + assumed to be parents of other groups. + :raises GroupConflict: + """ + logging.debug("Checking group branches match expectations") + for group, relations in physical_skel.items(): + if 'belongs_to' not in relations: + continue + parents = relations['belongs_to'] + for parent in parents: + if parent in config.keys(): + message = ( + "Group {parent} has a child group {child}, " + "but also has host entries in user configuration. " + "Hosts cannot be sibling with groups." + ).format(parent=parent, child=group) + raise GroupConflict(message) + logging.debug("Group branches ok.") + return True + + def _check_config_settings(cidr_networks, config, container_skel): """check preciseness of config settings @@ -1001,6 +1040,11 @@ def main(config=None, check=False, debug=False, environment=None, **kwargs): ip.set_used_ips(user_defined_config, inventory) user_defined_setup(user_defined_config, inventory) skel_setup(environment, inventory) + + _check_group_branches( + user_defined_config, + environment.get('physical_skel') + ) logger.debug("Loading physical skel.") skel_load( environment.get('physical_skel'), diff --git a/releasenotes/notes/group_branches-281e8d5fe2a54425.yaml b/releasenotes/notes/group_branches-281e8d5fe2a54425.yaml new file mode 100644 index 0000000000..ee1b2dc8e6 --- /dev/null +++ b/releasenotes/notes/group_branches-281e8d5fe2a54425.yaml @@ -0,0 +1,5 @@ +--- +fixes: + - Errors relating to groups containing both hosts and other groups as + children now raise a more descriptive error. See inventory documentation + for more details. Fixes bug #1646136 diff --git a/tests/test_inventory.py b/tests/test_inventory.py index ea32d38b6e..80f780e794 100644 --- a/tests/test_inventory.py +++ b/tests/test_inventory.py @@ -1,4 +1,3 @@ -#!/usr/bin/env python import collections import copy @@ -18,6 +17,7 @@ LIB_DIR = 'lib' sys.path.append(path.join(os.getcwd(), LIB_DIR)) sys.path.append(path.join(os.getcwd(), INV_DIR)) +import dictutils import dynamic_inventory import filesystem as fs import generate as di @@ -1217,5 +1217,88 @@ class TestConfigMatchesEnvironment(unittest.TestCase): self.assertIn(key, config.keys()) +class TestInventoryGroupConstraints(unittest.TestCase): + def setUp(self): + self.env = fs.load_environment(BASE_ENV_DIR, {}) + + def test_group_with_hosts_dont_have_children(self): + """Require that groups have children groups or hosts, not both.""" + inventory = get_inventory() + + # This should only work on groups, but stuff like '_meta' and 'all' + # are in here, too. + for key, values in inventory.items(): + # The keys for children/hosts can exist, the important part is being empty lists. + has_children = bool(inventory.get('children')) + has_hosts = bool(inventory.get('hosts')) + + self.assertFalse(has_children and has_hosts) + + def _create_bad_env(self, env): + # This environment setup is used because it was reported with + # bug #1646136 + override = """ + physical_skel: + local-compute_containers: + belongs_to: + - compute_containers + local-compute_hosts: + belongs_to: + - compute_hosts + rbd-compute_containers: + belongs_to: + - compute_containers + rbd-compute_hosts: + belongs_to: + - compute_hosts + """ + + bad_env = yaml.load(override) + + # This is essentially what load_environment does, after all the file + # system walking + dictutils.merge_dict(env, bad_env) + + return env + + def test_group_with_hosts_and_children_fails(self): + """Integration test making sure the whole script fails.""" + env = self._create_bad_env(self.env) + + + config = get_config() + + kwargs = { + 'load_environment': mock.DEFAULT, + 'load_user_configuration': mock.DEFAULT + } + + with mock.patch.multiple('filesystem', **kwargs) as mocks: + mocks['load_environment'].return_value = env + mocks['load_user_configuration'].return_value = config + + with self.assertRaises(di.GroupConflict) as context: + get_inventory() + + def test_group_validation_unit(self): + env = self._create_bad_env(self.env) + + config = get_config() + + with self.assertRaises(di.GroupConflict): + di._check_group_branches(config, env['physical_skel']) + + def test_group_validation_no_config(self): + result = di._check_group_branches(None, self.env) + self.assertTrue(result) + + def test_group_validation_passes_defaults(self): + config = get_config() + + result = di._check_group_branches(config, self.env['physical_skel']) + + self.assertTrue(result) + + if __name__ == '__main__': unittest.main()