Performance: avoid set_fact in Kolla Ansible host vars

When generating host variable files for Kolla Ansible, we have some
heavy usage of set_fact to set variables for network interfaces, in a
play targeted at all hosts. There are also tasks using the fail action
plugin to perform verification.

At scale this has a significant impact, due to the number of tasks
executed against all hosts. These tasks are executed at the beginning of
many commands, so the scope is broad.  There is also some tricky logic
involved, which is difficult to express in Ansible/Jinja.

This change replaces the use of set_fact with a custom Ansible action
plugin. The plugin executes locally on the Ansible controller, and
returns a dict of Ansible facts to set for each host. The plugin is
executed once for each overcloud host, and returns all relevant facts.
The plugin also performs verification. Extraction into a Python module
allows for unit testing.

This has been shown to have a significant improvement on execution time,
particularly as the number of hosts reaches 100 or more.

Story: 2007993
Task: 40641

Change-Id: I443da1ae05fcca2d7d8dff7db563eeda37e9f502

ansible/action_plugins/kolla_ansible_host_vars.py

@@ -0,0 +1,19 @@
+# Copyright (c) 2020 StackHPC Ltd.
+#
+# Licensed under the Apache License, Version 2.0 (the "License"); you may
+# not use this file except in compliance with the License. You may obtain
+# a copy of the License at
+#
+#      http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+# License for the specific language governing permissions and limitations
+# under the License.
+
+__metaclass__ = type
+
+import kayobe.plugins.action.kolla_ansible_host_vars
+
+ActionModule = kayobe.plugins.action.kolla_ansible_host_vars.ActionModule

ansible/kolla-ansible.yml

@@ -127,30 +127,19 @@
     - kolla-ansible
   gather_facts: False
   tasks:
-    - name: Set bifrost network interface
-      set_fact:
-        kolla_bifrost_network_interface: "{{ provision_oc_net_name | net_interface | replace('-', '_') }}"
-      when: provision_oc_net_name in network_interfaces
-
-    - name: Validate seed Kolla Ansible network configuration
-      fail:
-        msg: >
-          The Kolla Ansible variable {{ item.var_name }}
-          ({{ item.description }}) is invalid. Value:
-          "{{ hostvars[inventory_hostname][item.var_name] | default('<undefined>') }}".
-      when:
-        - item.required | bool
-        - hostvars[inventory_hostname][item.var_name] is not defined or not hostvars[inventory_hostname][item.var_name]
-      with_items:
+    - name: Set Kolla Ansible host variables
+      kolla_ansible_host_vars:
+        interfaces:
           - var_name: "kolla_bifrost_network_interface"
-          description: "Bifrost network interface name"
+            description: "Bifrost provisioning network"
+            network: "{{ provision_oc_net_name }}"
             required: True
-
           # Strictly api_interface is not required but kolla-ansible currently
           # references it in prechecks.
-    - name: Set API network interface
-      set_fact:
-        kolla_api_interface: "{{ kolla_bifrost_network_interface }}"
+          - var_name: "kolla_api_interface"
+            description: "Bifrost provisioning network"
+            network: "{{ provision_oc_net_name }}"
+            required: True
 
     - import_role:
         name: kolla-ansible-host-vars
@@ -166,152 +155,76 @@
     - config-validation
     - kolla-ansible
   gather_facts: False
+  tasks:
+    - name: Set Kolla Ansible host variables
       vars:
+        require_ironic_networks: >-
+          {{ kolla_enable_ironic | bool and
+             inventory_hostname in groups['controllers'] }}
+        ironic_networks:
+          - network: "{{ provision_wl_net_name }}"
+            required: "{{ require_ironic_networks }}"
+          - network: "{{ cleaning_net_name }}"
+            required: "{{ require_ironic_networks }}"
         require_provider_networks: >-
           {{ kolla_enable_neutron | bool and
              (inventory_hostname in groups['network'] or
               (kolla_enable_neutron_provider_networks | bool and inventory_hostname in groups['compute'])) }}
-  tasks:
-    - name: Set API network interface
-      set_fact:
-        kolla_network_interface: "{{ internal_net_name | net_interface | replace('-', '_')  }}"
-        kolla_api_interface: "{{ internal_net_name | net_interface | replace('-', '_')  }}"
-      when: internal_net_name in network_interfaces
-
-    - name: Set storage network interface
-      set_fact:
-        kolla_storage_interface: "{{ storage_net_name | net_interface | replace('-', '_')  }}"
-      when: storage_net_name in network_interfaces
-
-    - name: Set cluster network interface
-      set_fact:
-        kolla_cluster_interface: "{{ storage_mgmt_net_name | net_interface | replace('-', '_')  }}"
-      when: storage_mgmt_net_name in network_interfaces
-
-    - name: Set Swift storage network interface
-      set_fact:
-        kolla_swift_storage_interface: "{{ swift_storage_net_name | net_interface | replace('-', '_')  }}"
-      when: swift_storage_net_name in network_interfaces
-
-    - name: Set Swift cluster network interface
-      set_fact:
-        kolla_swift_replication_interface: "{{ swift_storage_replication_net_name | net_interface | replace('-', '_')  }}"
-      when: swift_storage_replication_net_name in network_interfaces
-
-    - name: Set provision network interface
-      set_fact:
-        kolla_provision_interface: "{{ provision_wl_net_name | net_interface | replace('-', '_')  }}"
-      when: provision_wl_net_name in network_interfaces
-
-    - name: Set inspector dnsmasq network interface
-      set_fact:
-        kolla_inspector_dnsmasq_interface: "{{ inspection_net_name | net_interface | replace('-', '_')  }}"
-      when: inspection_net_name in network_interfaces
-
-    - name: Set DNS network interface
-      set_fact:
-        kolla_dns_interface: "{{ public_net_name | net_interface | replace('-', '_')  }}"
-      when: public_net_name in network_interfaces
-
-    - name: Set tunnel network interface
-      set_fact:
-        kolla_tunnel_interface: "{{ tunnel_net_name | net_interface | replace('-', '_') }}"
-      when: tunnel_net_name in network_interfaces
-
-    - name: Set external VIP interface
-      set_fact:
-        kolla_external_vip_interface: "{{ public_net_name | net_interface | replace('-', '_') }}"
-      when: public_net_name in network_interfaces
-
-    - name: Initialise facts containing the network host interfaces
-      set_fact:
-        # Initialise the following lists.
-        kolla_neutron_interfaces: []
-        kolla_neutron_bridge_names: []
-        kolla_neutron_external_interfaces: []
-
-    # When these networks are VLANs, we need to use the underlying tagged
-    # bridge interface rather than the untagged interface. We therefore
-    # strip the .<vlan> suffix of the interface name. We use a union here
-    # as a single tagged interface may be shared between these networks.
-    - name: Set a fact containing the interfaces to be plugged to the Neutron OVS bridges
-      set_fact:
-        kolla_neutron_interfaces: >
-          {{ kolla_neutron_interfaces |
-             union([item | net_interface | replace('.' ~ item | net_vlan | default('!nomatch!'), '')]) |
-             list }}
-      with_items: "{{ [provision_wl_net_name, cleaning_net_name] + external_net_names | unique | list }}"
-      when: item in network_interfaces
-
-    - name: Set facts containing the Neutron bridge and interface names
-      vars:
-        is_bridge: "{{ item in (network_interfaces | net_select_bridges | map('net_interface')) }}"
-        # For a bridge, use a veth pair connected to the bridge. Otherwise use
-        # the interface directly.
-        external_interface_name: "{{ (network_patch_prefix ~ item ~ network_patch_suffix_ovs) if is_bridge else item }}"
-      set_fact:
-        kolla_neutron_bridge_names: >
-          {{ kolla_neutron_bridge_names +
-             [item ~ network_bridge_suffix_ovs] }}
-        kolla_neutron_external_interfaces: >
-          {{ kolla_neutron_external_interfaces +
-             [external_interface_name] }}
-      with_items: "{{ kolla_neutron_interfaces }}"
-
-    - name: Validate overcloud host Kolla Ansible network configuration
-      fail:
-        msg: >
-          The Kolla Ansible variable {{ item.var_name }}
-          ({{ item.description }}) is invalid. Value:
-          "{{ hostvars[inventory_hostname][item.var_name] | default('<undefined>') }}".
-      when:
-        - item.required | bool
-        - hostvars[inventory_hostname][item.var_name] is not defined or not hostvars[inventory_hostname][item.var_name]
-      with_items:
-        - var_name: "kolla_api_interface"
-          description: "API network interface name"
+        # This expression generates a list containing an item for each network
+        # in external_net_names, in the format required by the
+        # external_networks argument of the kolla_ansible_host_vars action
+        # plugin.
+        provider_networks: >-
+          {{ dict(external_net_names |
+                  zip_longest([], fillvalue=require_provider_networks)) |
+             dict2items(key_name='network', value_name='required') }}
+      kolla_ansible_host_vars:
+        interfaces:
+          - var_name: "kolla_network_interface"
+            description: "Default network"
+            network: "{{ internal_net_name }}"
             required: True
-        - var_name: "kolla_external_vip_interface"
-          description: "External network interface name"
-          required: "{{ inventory_hostname in groups['network'] }}"
+          - var_name: "kolla_api_interface"
+            description: "API network"
+            network: "{{ internal_net_name }}"
+            required: True
+          - var_name: "kolla_storage_interface"
+            description: "Storage network"
+            network: "{{ storage_net_name }}"
+            required: False
+          - var_name: "kolla_cluster_interface"
+            description: "Cluster network"
+            network: "{{ storage_mgmt_net_name }}"
+            required: False
+          - var_name: "kolla_swift_storage_interface"
+            description: "Swift storage network"
+            network: "{{ swift_storage_net_name }}"
+            required: False
+          - var_name: "kolla_swift_replication_interface"
+            description: "Swift storage replication network"
+            network: "{{ swift_storage_replication_net_name }}"
+            required: False
           - var_name: "kolla_provision_interface"
-          description: "Bare metal provisioning network interface name"
+            description: "Bare metal provisioning network"
+            network: "{{ provision_wl_net_name }}"
             required: "{{ kolla_enable_ironic | bool and inventory_hostname in groups['controllers'] }}"
           - var_name: "kolla_inspector_dnsmasq_interface"
-          description: "Bare metal introspection network interface name"
+            description: "Bare metal introspection network"
+            network: "{{ inspection_net_name }}"
             required: "{{ kolla_enable_ironic | bool and inventory_hostname in groups['controllers'] }}"
-        - var_name: "kolla_neutron_bridge_names"
-          description: "List of Neutron bridge names"
-          required: "{{ require_provider_networks }}"
-        - var_name: "kolla_neutron_external_interfaces"
-          description: "List of Neutron interface names"
-          required: "{{ require_provider_networks }}"
-
-    - name: Validate Kolla Ansible Neutron bridge and interface configuration
-      fail:
-        msg: >
-          The Kolla Ansible variable {{ item.0.var_name }}
-          ({{ item.0.description }}) is invalid. Value:
-          "{{ item.1 | default('<undefined>') }}".
-      when:
-        - item.0.required | bool
-        - item.1 is not defined or not item.1
-      with_subelements:
-        - - var_name: "kolla_neutron_bridge_names"
-            value: "{{ kolla_neutron_bridge_names }}"
-            description: "List of Neutron bridge names"
-            required: "{{ require_provider_networks }}"
-          - var_name: "kolla_neutron_external_interfaces"
-            value: "{{ kolla_neutron_external_interfaces }}"
-            description: "List of Neutron interface names"
-            required: "{{ require_provider_networks }}"
-        - value
-
-    # Kolla ansible expects these variables to be comma-separated lists.
-    - name: Update facts containing the Neutron bridge and interface names
-      set_fact:
-        kolla_neutron_bridge_names: "{{ kolla_neutron_bridge_names | join(',') }}"
-        kolla_neutron_external_interfaces: "{{ kolla_neutron_external_interfaces | join(',') }}"
+          - var_name: "kolla_dns_interface"
+            description: "DNS network"
+            network: "{{ public_net_name }}"
+            required: False
+          - var_name: "kolla_tunnel_interface"
+            description: "Tunnel network"
+            network: "{{ tunnel_net_name }}"
+            required: False
+          - var_name: "kolla_external_vip_interface"
+            description: "External network"
+            network: "{{ public_net_name }}"
+            required: "{{ inventory_hostname in groups['network'] }}"
+        external_networks: "{{ ironic_networks + provider_networks }}"
 
     - import_role:
         name: kolla-ansible-host-vars
@@ -319,6 +232,3 @@
         kolla_ansible_pass_through_host_vars: "{{ kolla_overcloud_inventory_pass_through_host_vars }}"
         kolla_ansible_pass_through_host_vars_map: "{{ kolla_overcloud_inventory_pass_through_host_vars_map }}"
         kolla_ansible_inventory_path: "{{ kolla_config_path }}/inventory/overcloud"
-        # Kolla ansible expects these variables to be comma-separated lists.
-        kolla_neutron_bridge_names: "{{ kolla_neutron_bridge_names | join(',') }}"
-        kolla_neutron_external_interfaces: "{{ kolla_neutron_external_interfaces | join(',') }}"

kayobe/plugins/action/kolla_ansible_host_vars.py

@@ -0,0 +1,161 @@
+# Copyright (c) 2020 StackHPC Ltd.
+#
+# Licensed under the Apache License, Version 2.0 (the "License"); you may
+# not use this file except in compliance with the License. You may obtain
+# a copy of the License at
+#
+#      http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+# License for the specific language governing permissions and limitations
+# under the License.
+
+from ansible.plugins.action import ActionBase
+
+
+class ConfigError(Exception):
+    pass
+
+
+class ActionModule(ActionBase):
+    """Kolla Ansible host vars action plugin
+
+    This class provides an Ansible action module that returns facts
+    representing host variables to be passed to Kolla Ansible.
+    """
+
+    TRANSFERS_FILES = False
+
+    def run(self, tmp=None, task_vars=None):
+        if task_vars is None:
+            task_vars = dict()
+
+        result = super(ActionModule, self).run(tmp, task_vars)
+        del tmp  # tmp no longer has any effect
+
+        # Module arguments:
+        # interfaces: a list of dicts, each containing 'network', 'required',
+        #             and 'description' keys. Each describes a Kolla Ansible
+        #             interface variable.
+        # external_networks: a list of dicts, each containing 'network', and
+        #                    'required' keys. Each describes an external
+        #                    network.
+        interfaces = self._task.args["interfaces"]
+        external_networks = self._task.args.get('external_networks', [])
+        result.update(self._run(interfaces, external_networks))
+        return result
+
+    def _run(self, interfaces, external_networks):
+        result = {}
+        facts = {}
+        errors = []
+
+        # Kolla Ansible interface facts.
+        for interface in interfaces:
+            try:
+                iface = self._get_interface_fact(interface["network"],
+                                                 interface["required"],
+                                                 interface["description"])
+                if iface:
+                    facts[interface["var_name"]] = iface
+            except ConfigError as e:
+                errors.append(str(e))
+
+        # Build a list of external network interfaces.
+        external_interfaces = []
+        for network in external_networks:
+            try:
+                iface = self._get_external_interface(network["network"],
+                                                     network["required"])
+                if iface and iface not in external_interfaces:
+                    external_interfaces.append(iface)
+            except ConfigError as e:
+                errors.append(str(e))
+
+        if external_interfaces:
+            facts.update(self._get_external_interface_facts(
+                external_interfaces))
+
+        result['changed'] = False
+        if errors:
+            result['failed'] = True
+            result['msg'] = "; ".join(errors)
+        else:
+            result['ansible_facts'] = facts
+            result['_ansible_facts_cacheable'] = False
+        return result
+
+    def _get_interface_fact(self, net_name, required, description):
+        # Check whether the network is mapped to this host.
+        condition = "{{ '%s' in network_interfaces }}" % net_name
+        condition = self._templar.template(condition)
+        if condition:
+            # Get the network interface for this network.
+            iface = ("{{ '%s' | net_interface }}" % net_name)
+            iface = self._templar.template(iface)
+            if iface:
+                # Ansible fact names replace dashes with underscores.
+                # FIXME(mgoddard): Is this still required?
+                iface = iface.replace('-', '_')
+            if required and not iface:
+                msg = ("Required network '%s' (%s) does not have an interface "
+                       "configured for this host" % (net_name, description))
+                raise ConfigError(msg)
+            return iface
+        elif required:
+            msg = ("Required network '%s' (%s) is not mapped to this host" %
+                   (net_name, description))
+            raise ConfigError(msg)
+
+    def _get_external_interface(self, net_name, required):
+        condition = "{{ '%s' in network_interfaces }}" % net_name
+        condition = self._templar.template(condition)
+        if condition:
+            iface = self._templar.template("{{ '%s' | net_interface }}" %
+                                           net_name)
+            if iface:
+                # When these networks are VLANs, we need to use the
+                # underlying tagged bridge interface rather than the
+                # untagged interface. We therefore strip the .<vlan> suffix
+                # of the interface name. We use a union here as a single
+                # tagged interface may be shared between these networks.
+                vlan = self._templar.template("{{ '%s' | net_vlan }}" %
+                                              net_name)
+                if vlan and iface.endswith(".%s" % vlan):
+                    iface = iface.replace(".%s" % vlan, "")
+                return iface
+            elif required:
+                raise ConfigError("Required external network '%s' does not "
+                                  "have an interface configured for this host"
+                                  % net_name)
+        elif required:
+            raise ConfigError("Required external network '%s' is not mapped "
+                              "to this host" % net_name)
+
+    def _get_external_interface_facts(self, external_interfaces):
+        neutron_bridge_names = []
+        neutron_external_interfaces = []
+        bridge_suffix = self._templar.template(
+            "{{ network_bridge_suffix_ovs }}")
+        patch_prefix = self._templar.template("{{ network_patch_prefix }}")
+        patch_suffix = self._templar.template("{{ network_patch_suffix_ovs }}")
+        for interface in external_interfaces:
+            is_bridge = ("{{ '%s' in (network_interfaces |"
+                         "net_select_bridges |"
+                         "map('net_interface')) }}" % interface)
+            is_bridge = self._templar.template(is_bridge)
+            neutron_bridge_names.append(interface + bridge_suffix)
+            # For a bridge, use a veth pair connected to the bridge. Otherwise
+            # use the interface directly.
+            if is_bridge:
+                external_interface = patch_prefix + interface + patch_suffix
+            else:
+                external_interface = interface
+            neutron_external_interfaces.append(external_interface)
+        return {
+            "kolla_neutron_bridge_names": ",".join(neutron_bridge_names),
+            "kolla_neutron_external_interfaces": ",".join(
+                neutron_external_interfaces),
+        }

kayobe/tests/unit/plugins/action/test_kolla_ansible_host_vars.py

@@ -0,0 +1,450 @@
+# Copyright (c) 2020 StackHPC Ltd.
+#
+# Licensed under the Apache License, Version 2.0 (the "License"); you may
+# not use this file except in compliance with the License. You may obtain
+# a copy of the License at
+#
+#      http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+# License for the specific language governing permissions and limitations
+# under the License.
+
+import copy
+import unittest
+
+import jinja2
+
+from kayobe.plugins.action import kolla_ansible_host_vars
+
+
+@jinja2.contextfilter
+def _net_interface(context, name):
+    return context.get(name + '_interface')
+
+
+@jinja2.contextfilter
+def _net_vlan(context, name):
+    return context.get(name + '_vlan')
+
+
+@jinja2.contextfilter
+def _net_select_bridges(context, names):
+    return [name for name in names
+            if (_net_interface(context, name) or "").startswith("br")]
+
+
+class FakeTemplar(object):
+
+    def __init__(self, variables):
+        self.variables = variables
+        self.env = jinja2.Environment()
+        self.env.filters['net_interface'] = _net_interface
+        self.env.filters['net_vlan'] = _net_vlan
+        self.env.filters['net_select_bridges'] = _net_select_bridges
+
+    def template(self, string):
+        template = self.env.from_string(string)
+        result = template.render(**self.variables)
+        return {
+            "None": None,
+            "True": True,
+            "False": False,
+        }.get(result, result)
+
+
+class TestCase(unittest.TestCase):
+
+    variables = {
+        "network_interfaces": [
+            "foo",
+            "bar",
+        ],
+        "foo_interface": "eth0",
+        "foo_vlan": 1,
+        "bar_interface": "eth1",
+        "bar_vlan": 2,
+        "network_bridge_suffix_ovs": "-ovs",
+        "network_patch_prefix": "p-",
+        "network_patch_suffix_ovs": "-ovs",
+    }
+
+    def _create_module(self, variables=None):
+        if not variables:
+            variables = self.variables
+        templar = FakeTemplar(variables)
+        return kolla_ansible_host_vars.ActionModule(None, None, None, None,
+                                                    templar, None)
+
+    def test__run_empty_args(self):
+        module = self._create_module()
+        result = module._run([], [])
+        expected = {
+            "changed": False,
+            "ansible_facts": {},
+            "_ansible_facts_cacheable": False,
+        }
+        self.assertEqual(expected, result)
+
+    def test__run_one_interface(self):
+        module = self._create_module()
+        interfaces = [{
+            "var_name": "kolla_foo_interface",
+            "network": "foo",
+            "description": "Foo network",
+            "required": False,
+        }]
+        result = module._run(interfaces, [])
+        expected = {
+            "changed": False,
+            "ansible_facts": {
+                "kolla_foo_interface": "eth0",
+            },
+            "_ansible_facts_cacheable": False,
+        }
+        self.assertEqual(expected, result)
+
+    def test__run_two_interfaces(self):
+        module = self._create_module()
+        interfaces = [{
+            "var_name": "kolla_foo_interface",
+            "network": "foo",
+            "description": "Foo network",
+            "required": False,
+        }, {
+            "var_name": "kolla_bar_interface",
+            "network": "bar",
+            "description": "Bar network",
+            "required": False,
+        }]
+        result = module._run(interfaces, [])
+        expected = {
+            "changed": False,
+            "ansible_facts": {
+                "kolla_foo_interface": "eth0",
+                "kolla_bar_interface": "eth1",
+            },
+            "_ansible_facts_cacheable": False,
+        }
+        self.assertEqual(expected, result)
+
+    def test__run_one_with_dashes(self):
+        variables = copy.deepcopy(self.variables)
+        variables["foo_interface"] = "eth-0"
+        module = self._create_module(variables)
+        interfaces = [{
+            "var_name": "kolla_foo_interface",
+            "network": "foo",
+            "description": "Foo network",
+            "required": False,
+        }]
+        result = module._run(interfaces, [])
+        expected = {
+            "changed": False,
+            "ansible_facts": {
+                "kolla_foo_interface": "eth_0",
+            },
+            "_ansible_facts_cacheable": False,
+        }
+        self.assertEqual(expected, result)
+
+    def test__run_interface_not_mapped(self):
+        module = self._create_module()
+        interfaces = [{
+            "var_name": "kolla_baz_interface",
+            "network": "baz",
+            "description": "Baz network",
+            "required": True,
+        }]
+        result = module._run(interfaces, [])
+        expected = {
+            "changed": False,
+            "failed": True,
+            "msg": ("Required network 'baz' (Baz network) is not mapped to "
+                    "this host"),
+        }
+        self.assertEqual(expected, result)
+
+    def test__run_interface_not_mapped_not_required(self):
+        module = self._create_module()
+        interfaces = [{
+            "var_name": "kolla_baz_interface",
+            "network": "baz",
+            "description": "Baz network",
+            "required": False,
+        }]
+        result = module._run(interfaces, [])
+        expected = {
+            "changed": False,
+            "ansible_facts": {},
+            "_ansible_facts_cacheable": False,
+        }
+        self.assertEqual(expected, result)
+
+    def test__run_interface_no_interface(self):
+        variables = copy.deepcopy(self.variables)
+        del variables["bar_interface"]
+        module = self._create_module(variables)
+        interfaces = [{
+            "var_name": "kolla_bar_interface",
+            "network": "bar",
+            "description": "Bar network",
+            "required": True,
+        }]
+        result = module._run(interfaces, [])
+        expected = {
+            "changed": False,
+            "failed": True,
+            "msg": ("Required network 'bar' (Bar network) does not have an "
+                    "interface configured for this host"),
+        }
+        self.assertEqual(expected, result)
+
+    def test__run_interface_no_interface_not_required(self):
+        variables = copy.deepcopy(self.variables)
+        del variables["bar_interface"]
+        module = self._create_module(variables)
+        interfaces = [{
+            "var_name": "kolla_bar_interface",
+            "network": "bar",
+            "description": "Bar network",
+            "required": False,
+        }]
+        result = module._run(interfaces, [])
+        expected = {
+            "changed": False,
+            "ansible_facts": {},
+            "_ansible_facts_cacheable": False,
+        }
+        self.assertEqual(expected, result)
+
+    def test__run_interface_no_interface_not_mapped(self):
+        variables = copy.deepcopy(self.variables)
+        del variables["bar_interface"]
+        module = self._create_module(variables)
+        interfaces = [{
+            "var_name": "kolla_bar_interface",
+            "network": "bar",
+            "description": "Bar network",
+            "required": True,
+        }, {
+            "var_name": "kolla_baz_interface",
+            "network": "baz",
+            "description": "Baz network",
+            "required": True,
+        }]
+        result = module._run(interfaces, [])
+        expected = {
+            "changed": False,
+            "failed": True,
+            "msg": ("Required network 'bar' (Bar network) does not have an "
+                    "interface configured for this host; Required network "
+                    "'baz' (Baz network) is not mapped to this host"),
+        }
+        self.assertEqual(expected, result)
+
+    def test_run_external_networks_one(self):
+        module = self._create_module()
+        external_networks = [{
+            "network": "foo",
+            "required": False,
+        }]
+        result = module._run([], external_networks)
+        expected = {
+            "changed": False,
+            "ansible_facts": {
+                "kolla_neutron_bridge_names": "eth0-ovs",
+                "kolla_neutron_external_interfaces": "eth0",
+            },
+            "_ansible_facts_cacheable": False,
+        }
+        self.assertEqual(expected, result)
+
+    def test_run_external_networks_two(self):
+        module = self._create_module()
+        external_networks = [{
+            "network": "foo",
+            "required": False,
+        }, {
+            "network": "bar",
+            "required": False,
+        }]
+        result = module._run([], external_networks)
+        expected = {
+            "changed": False,
+            "ansible_facts": {
+                "kolla_neutron_bridge_names": "eth0-ovs,eth1-ovs",
+                "kolla_neutron_external_interfaces": "eth0,eth1",
+            },
+            "_ansible_facts_cacheable": False,
+        }
+        self.assertEqual(expected, result)
+
+    def test_run_external_networks_two_same_interface(self):
+        variables = copy.deepcopy(self.variables)
+        variables["bar_interface"] = "eth0"
+        module = self._create_module(variables)
+        external_networks = [{
+            "network": "foo",
+            "required": False,
+        }, {
+            "network": "bar",
+            "required": False,
+        }]
+        result = module._run([], external_networks)
+        expected = {
+            "changed": False,
+            "ansible_facts": {
+                "kolla_neutron_bridge_names": "eth0-ovs",
+                "kolla_neutron_external_interfaces": "eth0",
+            },
+            "_ansible_facts_cacheable": False,
+        }
+        self.assertEqual(expected, result)
+
+    def test_run_external_networks_two_vlans(self):
+        variables = copy.deepcopy(self.variables)
+        variables["foo_interface"] = "eth0.1"
+        variables["bar_interface"] = "eth0.2"
+        module = self._create_module(variables)
+        external_networks = [{
+            "network": "foo",
+            "required": False,
+        }, {
+            "network": "bar",
+            "required": False,
+        }]
+        result = module._run([], external_networks)
+        expected = {
+            "changed": False,
+            "ansible_facts": {
+                "kolla_neutron_bridge_names": "eth0-ovs",
+                "kolla_neutron_external_interfaces": "eth0",
+            },
+            "_ansible_facts_cacheable": False,
+        }
+        self.assertEqual(expected, result)
+
+    def test_run_external_networks_bridge(self):
+        variables = copy.deepcopy(self.variables)
+        variables["foo_interface"] = "breth0"
+        module = self._create_module(variables)
+        external_networks = [{
+            "network": "foo",
+            "required": False,
+        }]
+        result = module._run([], external_networks)
+        expected = {
+            "changed": False,
+            "ansible_facts": {
+                "kolla_neutron_bridge_names": "breth0-ovs",
+                "kolla_neutron_external_interfaces": "p-breth0-ovs",
+            },
+            "_ansible_facts_cacheable": False,
+        }
+        self.assertEqual(expected, result)
+
+    def test_run_external_networks_bridge_vlan(self):
+        variables = copy.deepcopy(self.variables)
+        variables["foo_interface"] = "breth0.1"
+        variables["bar_interface"] = "breth0"
+        module = self._create_module(variables)
+        external_networks = [{
+            "network": "foo",
+            "required": False,
+        }]
+        result = module._run([], external_networks)
+        expected = {
+            "changed": False,
+            "ansible_facts": {
+                "kolla_neutron_bridge_names": "breth0-ovs",
+                "kolla_neutron_external_interfaces": "p-breth0-ovs",
+            },
+            "_ansible_facts_cacheable": False,
+        }
+        self.assertEqual(expected, result)
+
+    def test_run_external_networks_not_mapped(self):
+        module = self._create_module()
+        external_networks = [{
+            "network": "baz",
+            "required": True,
+        }]
+        result = module._run([], external_networks)
+        expected = {
+            "changed": False,
+            "failed": True,
+            "msg": ("Required external network 'baz' is not mapped to "
+                    "this host"),
+        }
+        self.assertEqual(expected, result)
+
+    def test_run_external_networks_not_mapped_not_required(self):
+        module = self._create_module()
+        external_networks = [{
+            "network": "baz",
+            "required": False,
+        }]
+        result = module._run([], external_networks)
+        expected = {
+            "changed": False,
+            "ansible_facts": {},
+            "_ansible_facts_cacheable": False,
+        }
+        self.assertEqual(expected, result)
+
+    def test_run_external_networks_no_interface(self):
+        variables = copy.deepcopy(self.variables)
+        del variables["bar_interface"]
+        module = self._create_module(variables)
+        external_networks = [{
+            "network": "bar",
+            "required": True,
+        }]
+        result = module._run([], external_networks)
+        expected = {
+            "changed": False,
+            "failed": True,
+            "msg": ("Required external network 'bar' does not have an "
+                    "interface configured for this host"),
+        }
+        self.assertEqual(expected, result)
+
+    def test_run_external_networks_no_interface_not_required(self):
+        variables = copy.deepcopy(self.variables)
+        del variables["bar_interface"]
+        module = self._create_module(variables)
+        external_networks = [{
+            "network": "bar",
+            "required": False,
+        }]
+        result = module._run([], external_networks)
+        expected = {
+            "changed": False,
+            "ansible_facts": {},
+            "_ansible_facts_cacheable": False,
+        }
+        self.assertEqual(expected, result)
+
+    def test_run_external_networks_not_mapped_no_interface(self):
+        variables = copy.deepcopy(self.variables)
+        del variables["bar_interface"]
+        module = self._create_module(variables)
+        external_networks = [{
+            "network": "baz",
+            "required": True,
+        }, {
+            "network": "bar",
+            "required": True,
+        }]
+        result = module._run([], external_networks)
+        expected = {
+            "changed": False,
+            "failed": True,
+            "msg": ("Required external network 'baz' is not mapped to "
+                    "this host; Required external network 'bar' does not "
+                    "have an interface configured for this host"),
+        }
+        self.assertEqual(expected, result)