Document and test group membership constraints

This patch tries to document and test the following invariant that
should apply for all groups in the environment:

    A group should have either child groups, or hosts, not both.

Tests are introduced to confirm and enforce this behavior, based on the
reported failing configuration.

A new function, _check_group_branches, is added which will detect a
failing scenario before code that uses it is executed. If a conflict is
found, a GroupConflict exception will be raised indicating why.

The _check_group_branches function was placed before the physical skel
loading because the problem occurs during that phase; placing this in
the _check_config_settings function would be checking after the error's
already been raised.

A note was also added to _parse_belongs_to to communicate it's implicit
dependence on state as checked by _check_group_branches.

Parent-Id: I1e746bfbda430076459d757039bc21f9df6a4a8a
Change-Id: I7830915fbdf9ed814846b69b1293729fb59ece79
Closes-Bug: #1646136

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
 -------------------
 

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'),

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

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()