Fix node scheduling with pipe in hostvars

If Ansible hostvars contain a pipe (|) character, this can cause
problems during scheduling as Ansible fails during Jinja templating.
This is probably a bug in Ansible/Jinja.

The particular case where this was hit was when using screen, the
TERMCAP environment variable gets set to something beginning with
'SC|screen|VT'.

This change addresses the issue by moving the capture of hostvars inside
the tenks_update_state action plugin rather than evaluating them in a
playbook.

Change-Id: Ibef91d9ef499c8741b61a170672a23f530a600bb

ansible/action_plugins/tenks_update_state.py

@@ -34,8 +34,6 @@ class ActionModule(ActionBase):
             * Scheduling specifications of nodes by type onto hypervisors.
 
         The following task arguments are accepted:
-            :hypervisor_vars: A dict of hostvars for each hypervisor, keyed
-                              by hypervisor hostname. Required.
             :specs: A list of node specifications to be instantiated. Required.
             :node_types: A dict mapping node type names to a dict of properties
                          of that type.
@@ -59,6 +57,11 @@ class ActionModule(ActionBase):
 
         self.args = self._task.args
         self.localhost_vars = task_vars['hostvars']['localhost']
+        self.hypervisor_vars = {
+            hv: hv_hostvars
+            for hv, hv_hostvars in task_vars['hostvars'].items()
+            if hv in task_vars['groups']['hypervisors']
+        }
         self._validate_args()
 
         if self.args['prune_only']:
@@ -88,7 +91,7 @@ class ActionModule(ActionBase):
         ensure the generated indices are consistent.
         """
         state = self.args['state']
-        for hostname, hostvars in six.iteritems(self.args['hypervisor_vars']):
+        for hostname, hostvars in six.iteritems(self.hypervisor_vars):
             # The desired mappings given in the Tenks configuration. These do
             # not include IDXs which are an implementation detail of Tenks.
             specified_mappings = hostvars['physnet_mappings']
@@ -136,11 +139,11 @@ class ActionModule(ActionBase):
 
         if self.localhost_vars['cmd'] != 'teardown':
             # Ensure all hosts exist in state.
-            for hostname in self.args['hypervisor_vars']:
+            for hostname in self.hypervisor_vars:
                 self.args['state'].setdefault(hostname, {})
                 self.args['state'][hostname].setdefault('nodes', [])
             # Now create all the required new nodes.
-            scheduler = RoundRobinScheduler(self.args['hypervisor_vars'],
+            scheduler = RoundRobinScheduler(self.hypervisor_vars,
                                             self.args['state'])
             namer = Namer(self.args['state'])
             self._create_nodes(scheduler, namer)
@@ -209,7 +212,7 @@ class ActionModule(ActionBase):
         if self.args is None:
             self.args = {}
 
-        REQUIRED_ARGS = {'hypervisor_vars', 'specs', 'node_types'}
+        REQUIRED_ARGS = {'specs', 'node_types'}
         # Var names and their defaults.
         OPTIONAL_ARGS = [
             ('node_name_prefix', 'tk'),
@@ -230,7 +233,7 @@ class ActionModule(ActionBase):
                     e = "The parameter '%s' must be specified." % arg
                     raise AnsibleActionFail(to_text(e))
 
-            if not self.args['hypervisor_vars']:
+            if not self.hypervisor_vars:
                 e = ("There are no hosts in the 'hypervisors' group to which "
                      "we can schedule.")
                 raise AnsibleActionFail(to_text(e))

ansible/schedule.yml

@@ -17,14 +17,6 @@
       when: item.type not in node_types
       loop: "{{ specs }}"
 
-    # Creates a dict mapping each hypervisor's hostname to its hostvars, to be
-    # used during scheduling.
-    - name: Collect hypervisor hostvars
-      set_fact:
-        hypervisor_vars: >-
-          {{ hypervisor_vars | default({}) | combine({item: hostvars[item]}) }}
-      loop: "{{ groups['hypervisors'] }}"
-
     - name: Check if an existing state file exists
       stat:
         path: "{{ state_file_path }}"
@@ -38,7 +30,6 @@
 
     - name: Get updated state
       tenks_update_state:
-        hypervisor_vars: "{{ hypervisor_vars }}"
         node_name_prefix: "{{ node_name_prefix | default(omit) }}"
         node_types: "{{ node_types }}"
         specs: "{{ specs }}"

releasenotes/notes/fix-schedule-templating-05e4dcee8a1771b7.yaml

@@ -0,0 +1,5 @@
+---
+fixes:
+  - |
+    Fixes an issue with node scheduling when Ansible hostvars contain a pipe
+    (``|``) character.

tests/test_tenks_update_state.py

@@ -73,7 +73,7 @@ class TestTenksUpdateState(unittest.TestCase):
                 },
             },
         ]
-        self.hypervisor_vars = {
+        self.mod.hypervisor_vars = {
             'foo': {
                 'physnet_mappings': {
                     'physnet0': 'dev0',
@@ -83,7 +83,6 @@ class TestTenksUpdateState(unittest.TestCase):
             },
         }
         self.mod.args = {
-            'hypervisor_vars': self.hypervisor_vars,
             'node_types': self.node_types,
             'node_name_prefix': 'test_node_pfx',
             'specs': self.specs,
@@ -102,7 +101,7 @@ class TestTenksUpdateState(unittest.TestCase):
                          expected_indices)
 
     def test__set_physnet_idxs_no_state_two_hosts(self):
-        self.hypervisor_vars['bar'] = self.hypervisor_vars['foo']
+        self.mod.hypervisor_vars['bar'] = self.mod.hypervisor_vars['foo']
         self.mod._set_physnet_idxs()
         expected_indices = {
             'physnet0': 0,
@@ -112,12 +111,12 @@ class TestTenksUpdateState(unittest.TestCase):
                              expected_indices)
 
     def test_set_physnet_idxs__no_state_two_hosts_different_nets(self):
-        self.hypervisor_vars['bar'] = self.hypervisor_vars['foo']
-        self.hypervisor_vars['foo']['physnet_mappings'].update({
+        self.mod.hypervisor_vars['bar'] = self.mod.hypervisor_vars['foo']
+        self.mod.hypervisor_vars['foo']['physnet_mappings'].update({
             'physnet1': 'dev1',
             'physnet2': 'dev2',
         })
-        self.hypervisor_vars['bar']['physnet_mappings'].update({
+        self.mod.hypervisor_vars['bar']['physnet_mappings'].update({
             'physnet2': 'dev2',
         })
         self.mod._set_physnet_idxs()
@@ -128,12 +127,12 @@ class TestTenksUpdateState(unittest.TestCase):
             six.assertCountEqual(self, idxs, set(idxs))
 
     def test_set_physnet_idxs__idx_maintained_after_removal(self):
-        self.hypervisor_vars['foo']['physnet_mappings'].update({
+        self.mod.hypervisor_vars['foo']['physnet_mappings'].update({
             'physnet1': 'dev1',
         })
         self.mod._set_physnet_idxs()
         physnet1_idx = self.args['state']['foo']['physnet_indices']['physnet1']
-        del self.hypervisor_vars['foo']['physnet_mappings']['physnet0']
+        del self.mod.hypervisor_vars['foo']['physnet_mappings']['physnet0']
         self.mod._set_physnet_idxs()
         self.assertEqual(
             physnet1_idx,
@@ -159,11 +158,11 @@ class TestTenksUpdateState(unittest.TestCase):
         for node in nodes:
             self.assertGreaterEqual(
                 node['ipmi_port'],
-                self.hypervisor_vars['foo']['ipmi_port_range_start']
+                self.mod.hypervisor_vars['foo']['ipmi_port_range_start']
             )
             self.assertLessEqual(
                 node['ipmi_port'],
-                self.hypervisor_vars['foo']['ipmi_port_range_end']
+                self.mod.hypervisor_vars['foo']['ipmi_port_range_end']
             )
             self.assertNotIn(node['ipmi_port'], used_ipmi_ports)
             used_ipmi_ports.add(node['ipmi_port'])
@@ -185,7 +184,7 @@ class TestTenksUpdateState(unittest.TestCase):
         self.assertEqual(created_state, self.args['state'])
 
     def test__process_specs_multiple_hosts(self):
-        self.hypervisor_vars['bar'] = self.hypervisor_vars['foo']
+        self.mod.hypervisor_vars['bar'] = self.mod.hypervisor_vars['foo']
         self.mod._process_specs()
         foo_nodes = self.args['state']['foo']['nodes']
         bar_nodes = self.args['state']['bar']['nodes']
@@ -227,7 +226,7 @@ class TestTenksUpdateState(unittest.TestCase):
             self.assertEqual(expected_state, self.args['state'])
 
     def test__process_specs_no_hypervisors(self):
-        self.args['hypervisor_vars'] = {}
+        self.mod.hypervisor_vars = {}
         self.assertRaises(AnsibleActionFail, self.mod._process_specs)
 
     def test__process_specs_no_hypervisors_on_physnet(self):
@@ -236,9 +235,10 @@ class TestTenksUpdateState(unittest.TestCase):
 
     def test__process_specs_one_hypervisor_on_physnet(self):
         self.node_types['type0']['physical_networks'].append('another_pn')
-        self.hypervisor_vars['bar'] = copy.deepcopy(
-            self.hypervisor_vars['foo'])
-        self.hypervisor_vars['bar']['physnet_mappings']['another_pn'] = 'dev1'
+        self.mod.hypervisor_vars['bar'] = copy.deepcopy(
+            self.mod.hypervisor_vars['foo'])
+        self.mod.hypervisor_vars['bar']['physnet_mappings']['another_pn'] = (
+            'dev1')
         self.mod._process_specs()
 
         # Check all nodes were scheduled to the hypervisor connected to the
@@ -248,8 +248,8 @@ class TestTenksUpdateState(unittest.TestCase):
 
     def test__process_specs_not_enough_ports(self):
         # Give 'foo' only a single IPMI port to allocate.
-        self.hypervisor_vars['foo']['ipmi_port_range_start'] = 123
-        self.hypervisor_vars['foo']['ipmi_port_range_end'] = 123
+        self.mod.hypervisor_vars['foo']['ipmi_port_range_start'] = 123
+        self.mod.hypervisor_vars['foo']['ipmi_port_range_end'] = 123
         self.assertRaises(AnsibleActionFail, self.mod._process_specs)
 
     def test__process_specs_node_name_prefix(self):
@@ -276,7 +276,7 @@ class TestTenksUpdateState(unittest.TestCase):
 
     def test__process_specs_node_name_prefix_multiple_hosts(self):
         self.specs[0]['node_name_prefix'] = 'foo-prefix'
-        self.hypervisor_vars['bar'] = self.hypervisor_vars['foo']
+        self.mod.hypervisor_vars['bar'] = self.mod.hypervisor_vars['foo']
         self.mod._process_specs()
         foo_nodes = self.args['state']['foo']['nodes']
         bar_nodes = self.args['state']['bar']['nodes']