From 8df4a68fb1e7d939a7489fc059386b68651f34fa Mon Sep 17 00:00:00 2001 From: Mark Goddard Date: Fri, 20 Mar 2020 10:59:30 +0000 Subject: [PATCH] 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 | 17 +++++---- ansible/schedule.yml | 9 ----- ...-schedule-templating-05e4dcee8a1771b7.yaml | 5 +++ tests/test_tenks_update_state.py | 36 +++++++++---------- 4 files changed, 33 insertions(+), 34 deletions(-) create mode 100644 releasenotes/notes/fix-schedule-templating-05e4dcee8a1771b7.yaml diff --git a/ansible/action_plugins/tenks_update_state.py b/ansible/action_plugins/tenks_update_state.py index 769a2b9..d2089ef 100644 --- a/ansible/action_plugins/tenks_update_state.py +++ b/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)) diff --git a/ansible/schedule.yml b/ansible/schedule.yml index 3241fa4..7415a80 100644 --- a/ansible/schedule.yml +++ b/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 }}" diff --git a/releasenotes/notes/fix-schedule-templating-05e4dcee8a1771b7.yaml b/releasenotes/notes/fix-schedule-templating-05e4dcee8a1771b7.yaml new file mode 100644 index 0000000..f02712a --- /dev/null +++ b/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. diff --git a/tests/test_tenks_update_state.py b/tests/test_tenks_update_state.py index 7426d38..42c5deb 100644 --- a/tests/test_tenks_update_state.py +++ b/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']