Topology: check that node groups/wildcards are non-empty (#527)

Router and destination node sets defined in topology.conf may use
NodeSet groups and wildcards but any route definition with an
empty node set when we build the tree will be ignored.

Add checks for this in the code and add a note to the documentation.

Fixes #527.

doc/sphinx/tools/clush.rst

@@ -207,14 +207,16 @@ Configuration
 """""""""""""
 
 The system-wide library configuration file **/etc/clustershell/topology.conf**
-defines the routes of default command propagation tree. It is recommended that
-all connections between parent and children nodes are carefully
+defines available/preferred routes for the command propagation tree. It is
+recommended that all connections between parent and children nodes are carefully
 pre-configured, for example, to avoid any SSH warnings when connecting (if
 using the default SSH remote worker, of course).
 
 .. highlight:: ini
 
-The content of the topology.conf file should look like this::
+The file **topology.conf** is used to define a set of routes under a
+``[routes]`` section. Think of it as a routing table but for cluster
+commands. Node sets should be used when possible, for example::
 
   [routes]
   rio0: rio[10-13]
@@ -223,14 +225,17 @@ The content of the topology.conf file should look like this::
 
 .. highlight:: text
 
-This file defines the following topology graph::
+The example above defines the following topology graph::
 
     rio0
     |- rio[10-11]
     |  `- rio[100-240]
     `- rio[12-13]
        `- rio[300-440]
 
+:ref:`nodeset-groups` and :ref:`node-wildcards` are supported in
+**topology.conf**, but any route definition with an empty node set
+is ignored (a message is printed in debug mode in that case).
 
 At runtime, ClusterShell will pick an initial propagation tree from this
 topology graph definition and the current root node. Multiple admin/root

doc/sphinx/tools/nodeset.rst

@@ -910,6 +910,8 @@ like::
 A similar option is available with :ref:`clush-tool`, see
 :ref:`selecting all nodes with clush <clush-all-nodes>`.
 
+.. _node-wildcards:
+
 Node wildcards
 """"""""""""""
 

lib/ClusterShell/Topology.py

@@ -42,9 +42,14 @@
     # Python 2 compat
     import ConfigParser as configparser
 
+import logging
+
 from ClusterShell.NodeSet import NodeSet
 
 
+LOGGER = logging.getLogger(__name__)
+
+
 class TopologyError(Exception):
     """topology parser error to report invalid configurations or parsing
     errors
@@ -105,6 +110,7 @@ def add_child(self, child):
         its parent
         """
         assert isinstance(child, TopologyNodeGroup)
+        assert len(child.nodeset) > 0, "empty nodeset in child"
 
         if child in self._children:
             return
@@ -347,7 +353,6 @@ def add_route(self, src_ns, dst_ns):
         assert isinstance(src_ns, NodeSet)
         assert isinstance(dst_ns, NodeSet)
 
-        #print 'adding %s -> %s' % (str(src_ns), str(dst_ns))
         self._routing.add_route(TopologyRoute(src_ns, dst_ns))
 
     def dest(self, from_nodeset):
@@ -456,7 +461,17 @@ def _build_graph(self):
         """
         self.graph = TopologyGraph()
         for src, dst in self._topology:
-            self.graph.add_route(NodeSet(src), NodeSet(dst))
+            # GH#527: router and destination node sets may use NodeSet groups
+            # but we ignore any empty sets
+            src_ns = NodeSet(src)
+            if not src_ns:
+                LOGGER.debug('Failed to resolve router node set: %s', src)
+            dst_ns = NodeSet(dst)
+            if not dst_ns:
+                LOGGER.debug('Failed to resolve destination node set "%s" for' \
+                             'router node set %s', dst, src_ns)
+            if src_ns and dst_ns:
+                self.graph.add_route(src_ns, dst_ns)
 
     def tree(self, root, force_rebuild=False):
         """Return a previously generated propagation tree or build it if