Merge pull request #2 from locationlabs/IG-8617_improve_vars_plugin

Improve vars plugin

README.md

@@ -1,5 +1,14 @@
 # cumulus
 
+## Vars plugin
+
+This role employs the use of a `vars_plugin` for slave interfaces used in a bond to inherit
+`mstpctl` parameters from the bond interface, as well as set their `alias_name` to
+_Master:<bond_name>_. 
+
+NOTE: Ansible variable precedence means if one sets one of these attributes of an interface in the
+host_vars, that will override the vars plugin.
+
 ## NTP
 
 `cumulus_ntp_servers`: List of ntp servers to use - Default: Cumulus ntp servers

tasks/main.yml

@@ -3,9 +3,9 @@
     that: "{{ item.that }}"
     msg: "{{ item.msg }}"
   with_items:
-    - that: "{{ config | validate_interfaces }}"
+    - that: "{{ config is valid_interface }}"
       msg: "Invalid interface name detected: longer than 15 characters or starts with a number"
-    - that: "{{ config | validate_802_1x }}"
+    - that: "{{ config is valid_802_1x }}"
       msg: "Invalid 802.1X configuration"
 
 - name: Configure NTP

test_plugins/validate.py

@@ -1,26 +1,25 @@
 from __future__ import (absolute_import, division, print_function)
 __metaclass__ = type
 
-from ansible import errors
 
-
-def validate_interfaces(configuration):
+def valid_interface(configuration):
     """Validate interface configuration
 
     Checks the following:
     * interface name is 15 characters or less
     * interface name does not start with a number
     """
-    for config_section in [ s for s in ("bonds", "interfaces") if s in configuration.keys() ]:
+    for config_section in [s for s in ("bonds", "interfaces") if s in configuration.keys()]:
         for interface in configuration[config_section]:
-            if len(interface) > 15 :
+            if len(interface) > 15:
                 return False
             elif interface[0].isdigit():
                 return False
             else:
                 return True
 
-def validate_802_1x(configuration):
+
+def valid_802_1x(configuration):
     """Validate 802.1X parameters
 
     Checks the following when dot1x hash exists:
@@ -44,11 +43,12 @@ def validate_802_1x(configuration):
     else:
         return True
 
+
 class TestModule(object):
     ''' Ansible file jinja2 tests '''
 
     def tests(self):
         return {
-            'validate_interfaces': validate_interfaces,
-            'validate_802_1x': validate_802_1x,
+            'valid_interface': valid_interface,
+            'valid_802_1x': valid_802_1x,
         }

vars_plugins/cumulus.py

@@ -2,50 +2,60 @@
 __metaclass__ = type
 
 from distutils.version import LooseVersion
-from six import iteritems, itervalues
+from six import iteritems, itervalues, iterkeys
 from ansible import __version__
 from ansible.errors import AnsibleError
 from ansible.inventory.group import Group
 from ansible.inventory.host import Host
 from ansible.plugins.vars.host_group_vars import VarsModule as AnsibleVarsModule
+from ansible.utils.vars import combine_vars
+
 
 if LooseVersion(__version__) < LooseVersion("2.4"):
     raise AnsibleError('Cumulus vars plugin requires Ansible 2.4 or newer')
 
 
 class VarsModule(AnsibleVarsModule):
     '''
-    Looks for bonds with mstpctl settings and configures the slave interfaces
-    with the same settings.
+    Adds configuration to slave interfaces of a bond:
+      * Sets alias_name of interface to Master:<bond_name>
+      * Copies mstpctl settings from bond to slave interfaces if they exist
     '''
 
     def get_vars(self, loader, path, entities, cache=True):
-        self._display.debug('in CumulusVarsModule')
+        self._display.debug('in cumulus get_vars()')
         new_data = {}
         for entity in entities:
             if isinstance(entity, Host) or isinstance(entity, Group):
-                data = super(VarsModule, self).get_vars(loader, path, entities)
-                if isinstance(data, dict):
-                    if 'config' in data.keys():
+                host_group_vars_data = super(VarsModule, self).get_vars(loader, path, entities)
+                if isinstance(host_group_vars_data, dict):
+                    if 'config' in host_group_vars_data.keys():
                         self._display.debug('config data found')
-                        if 'bonds' in data['config'].keys():
+                        if 'bonds' in host_group_vars_data['config'].keys():
                             interfaces = {}
-                            for bond in itervalues(data['config']['bonds']):
-                                # FIXME: alias_name doenst work with os-atl2
-                                self._display.debug('bond: %s found' % bond) #['alias_name'])
+                            # This works because of explaination at
+                            # https://docs.python.org/2/library/stdtypes.html#dict.items and six
+                            # is just a wrapper around them
+                            for bond, bond_name in zip(itervalues(host_group_vars_data['config']['bonds']),
+                                                       iterkeys(host_group_vars_data['config']['bonds'])):
+                                self._display.debug('bond: %s found' % bond_name)
                                 mstpctl_settings = {
                                     key: value
                                     for key, value in iteritems(bond)
                                     if key.startswith('mstpctl')
                                 }
-                                if mstpctl_settings:
-                                    self._display.debug(
-                                        'Configuring slave interface(s): %s' % ''.join(
-                                            bond['slaves']
-                                        )
-                                    )
+
+                                if 'slaves' in bond:
                                     for slave_iface in bond['slaves']:
-                                        interfaces[slave_iface] = mstpctl_settings
+                                        if mstpctl_settings:
+                                            self._display.debug(
+                                                'Configuring slave interface(s): %s' % ' '.join(
+                                                    bond['slaves']
+                                                )
+                                            )
+                                            interfaces[slave_iface] = combine_vars(mstpctl_settings, {'alias_name': 'Master:%s' % bond_name})
+                                        else:
+                                            interfaces[slave_iface] = {'alias_name': 'Master:%s' % bond_name}
 
                             if interfaces:
                                 self._display.debug('slave interfaces configured')