diff --git a/ironic_staging_drivers/ansible/deploy.py b/ironic_staging_drivers/ansible/deploy.py index fc09b39..ac029c7 100644 --- a/ironic_staging_drivers/ansible/deploy.py +++ b/ironic_staging_drivers/ansible/deploy.py @@ -154,7 +154,8 @@ def _get_configdrive_path(basename): return os.path.join(CONF.tempdir, basename + '.cndrive') -def _get_node_ip(task): +def _get_node_ip_dhcp(task): + """Get node IP from DHCP provider.""" api = dhcp_factory.DHCPFactory().provider ip_addrs = api.get_ip_addresses(task) if not ip_addrs: @@ -169,6 +170,18 @@ def _get_node_ip(task): return ip_addrs[0] +def _get_node_ip_heartbeat(task): + callback_url = task.node.driver_internal_info.get('agent_url', '') + return urlparse.urlparse(callback_url).netloc.split(':')[0] + + +def _get_node_ip(task): + if CONF.ansible.use_ramdisk_callback: + return _get_node_ip_heartbeat(task) + else: + return _get_node_ip_dhcp(task) + + # some good code from agent def _reboot_and_finish_deploy(task): wait = CONF.ansible.post_deploy_get_power_state_retry_interval * 1000 @@ -323,7 +336,10 @@ def _prepare_variables(task): def _validate_clean_steps(steps, node_uuid): missing = [] for step in steps: - name = step.setdefault('name', 'unnamed') + name = step.get('name') + if not name: + missing.append({'name': 'undefined', 'field': 'name'}) + continue if 'interface' not in step: missing.append({'name': name, 'field': 'interface'}) args = step.get('args', {}) @@ -338,12 +354,17 @@ def _validate_clean_steps(steps, node_uuid): LOG.error(msg) raise exception.NodeCleaningFailure(node=node_uuid, reason=msg) + if len(set(s['name'] for s in steps)) != len(steps): + msg = _("Cleaning steps do not have unique names.") + LOG.error(msg) + raise exception.NodeCleaningFailure(node=node_uuid, + reason=msg) -def _get_clean_steps(task, interface=None, override_priorities=None): +def _get_clean_steps(node, interface=None, override_priorities=None): """Get cleaning steps.""" - clean_steps_file = task.node.driver_info.get('ansible_clean_steps_config', - DEFAULT_CLEAN_STEPS) + clean_steps_file = node.driver_info.get('ansible_clean_steps_config', + DEFAULT_CLEAN_STEPS) path = os.path.join(CONF.ansible.playbooks_path, clean_steps_file) try: with open(path) as f: @@ -351,9 +372,9 @@ def _get_clean_steps(task, interface=None, override_priorities=None): except Exception as e: msg = _('Failed to load clean steps from file ' '%(file)s: %(exc)s') % {'file': path, 'exc': e} - raise exception.NodeCleaningFailure(node=task.node.uuid, reason=msg) + raise exception.NodeCleaningFailure(node=node.uuid, reason=msg) - _validate_clean_steps(internal_steps, task.node.uuid) + _validate_clean_steps(internal_steps, node.uuid) steps = [] override = override_priorities or {} @@ -454,8 +475,9 @@ class AnsibleDeploy(base.DeployInterface): manager_utils.node_power_action(task, states.REBOOT) if CONF.ansible.use_ramdisk_callback: return states.DEPLOYWAIT + node = task.node - ip_addr = _get_node_ip(task) + ip_addr = _get_node_ip_dhcp(task) try: _deploy(task, ip_addr) except Exception as e: @@ -515,7 +537,7 @@ class AnsibleDeploy(base.DeployInterface): 'erase_devices_metadata': CONF.deploy.erase_devices_metadata_priority } - return _get_clean_steps(task, interface='deploy', + return _get_clean_steps(task.node, interface='deploy', override_priorities=new_priorities) def execute_clean_step(self, task, step): @@ -529,13 +551,14 @@ class AnsibleDeploy(base.DeployInterface): playbook, user, key = _parse_ansible_driver_info( task.node, action='clean') stepname = step['step'] - try: - ip_addr = node.driver_internal_info['ansible_cleaning_ip'] - except KeyError: - raise exception.NodeCleaningFailure(node=node.uuid, - reason='undefined node IP ' - 'addresses') - node_list = [(node.uuid, ip_addr, user, node.extra)] + + if (not CONF.ansible.use_ramdisk_callback and + 'ansible_cleaning_ip' in node.driver_internal_info): + node_address = node.driver_internal_info['ansible_cleaning_ip'] + else: + node_address = _get_node_ip(task) + + node_list = [(node.uuid, node_address, user, node.extra)] extra_vars = _prepare_extra_vars(node_list) LOG.debug('Starting cleaning step %(step)s on node %(node)s', @@ -576,7 +599,7 @@ class AnsibleDeploy(base.DeployInterface): if use_callback: return states.CLEANWAIT - ip_addr = _get_node_ip(task) + ip_addr = _get_node_ip_dhcp(task) LOG.debug('IP of node %(node)s is %(ip)s', {'node': node.uuid, 'ip': ip_addr}) driver_internal_info = node.driver_internal_info @@ -641,7 +664,7 @@ class AnsibleDeploy(base.DeployInterface): task.upgrade_lock(purpose='clean') node = task.node driver_internal_info = node.driver_internal_info - driver_internal_info['ansible_cleaning_ip'] = address + driver_internal_info['agent_url'] = callback_url node.driver_internal_info = driver_internal_info node.save() try: diff --git a/ironic_staging_drivers/tests/unit/ansible/test_deploy.py b/ironic_staging_drivers/tests/unit/ansible/test_deploy.py index c6f774a..58334aa 100644 --- a/ironic_staging_drivers/tests/unit/ansible/test_deploy.py +++ b/ironic_staging_drivers/tests/unit/ansible/test_deploy.py @@ -28,6 +28,7 @@ from ironic.tests.unit.objects import utils as object_utils from ironic_lib import utils as irlib_utils import mock from oslo_config import cfg +import six from ironic_staging_drivers.ansible import deploy as ansible_deploy @@ -51,7 +52,7 @@ DRIVER_INFO = { 'ansible_deploy_key_file': '/path/key', } DRIVER_INTERNAL_INFO = { - 'ansible_cleaning_ip': 'http://127.0.0.1/', + 'ansible_cleaning_ip': '127.0.0.1', 'is_whole_disk_image': True, 'clean_steps': [] } @@ -81,30 +82,69 @@ class TestAnsibleMethods(db_base.DbTestCase): ansible_deploy._parse_ansible_driver_info, self.node, 'test') - def test__get_node_ip(self): + def test__get_node_ip_dhcp(self): dhcp_provider_mock = mock.Mock() dhcp_factory.DHCPFactory._dhcp_provider = dhcp_provider_mock dhcp_provider_mock.get_ip_addresses.return_value = ['ip'] with task_manager.acquire(self.context, self.node.uuid) as task: - ansible_deploy._get_node_ip(task) + ansible_deploy._get_node_ip_dhcp(task) dhcp_provider_mock.get_ip_addresses.assert_called_once_with( task) - def test__get_node_ip_no_ip(self): + def test__get_node_ip_dhcp_no_ip(self): dhcp_provider_mock = mock.Mock() dhcp_factory.DHCPFactory._dhcp_provider = dhcp_provider_mock dhcp_provider_mock.get_ip_addresses.return_value = [] with task_manager.acquire(self.context, self.node.uuid) as task: self.assertRaises(exception.FailedToGetIPAddressOnPort, - ansible_deploy._get_node_ip, task) + ansible_deploy._get_node_ip_dhcp, task) - def test__get_node_ip_multiple_ip(self): + def test__get_node_ip_dhcp_multiple_ip(self): + # self.config(group='ansible', use_ramdisk_callback=False) + # di_info = self.node.driver_internal_info + # di_info.pop('ansible_cleaning_ip') + # self.node.driver_internal_info = di_info + # self.node.save() dhcp_provider_mock = mock.Mock() dhcp_factory.DHCPFactory._dhcp_provider = dhcp_provider_mock dhcp_provider_mock.get_ip_addresses.return_value = ['ip1', 'ip2'] with task_manager.acquire(self.context, self.node.uuid) as task: self.assertRaises(exception.InstanceDeployFailure, - ansible_deploy._get_node_ip, task) + ansible_deploy._get_node_ip_dhcp, task) + + def test__get_node_ip_heartbeat(self): + di_info = self.node.driver_internal_info + di_info['agent_url'] = 'http://1.2.3.4:5678' + self.node.driver_internal_info = di_info + self.node.save() + + with task_manager.acquire(self.context, self.node.uuid) as task: + self.assertEqual('1.2.3.4', + ansible_deploy._get_node_ip_heartbeat(task)) + + @mock.patch.object(ansible_deploy, '_get_node_ip_heartbeat', + return_value='127.0.0.1', autospec=True) + @mock.patch.object(ansible_deploy, '_get_node_ip_dhcp', + return_value='127.0.0.1', autospec=True) + def test__get_node_ip_callback(self, ip_dhcp_mock, ip_agent_mock): + self.config(group='ansible', use_ramdisk_callback=True) + with task_manager.acquire(self.context, self.node.uuid) as task: + res = ansible_deploy._get_node_ip(task) + self.assertEqual(0, ip_dhcp_mock.call_count) + ip_agent_mock.assert_called_once_with(task) + self.assertEqual('127.0.0.1', res) + + @mock.patch.object(ansible_deploy, '_get_node_ip_heartbeat', + return_value='127.0.0.1', autospec=True) + @mock.patch.object(ansible_deploy, '_get_node_ip_dhcp', + return_value='127.0.0.1', autospec=True) + def test__get_node_ip_no_callback(self, ip_dhcp_mock, ip_agent_mock): + self.config(group='ansible', use_ramdisk_callback=False) + with task_manager.acquire(self.context, self.node.uuid) as task: + res = ansible_deploy._get_node_ip(task) + self.assertEqual(0, ip_agent_mock.call_count) + ip_dhcp_mock.assert_called_once_with(task) + self.assertEqual('127.0.0.1', res) @mock.patch.object(utils, 'node_power_action', autospec=True) @mock.patch.object(fake.FakePower, 'get_power_state', @@ -326,6 +366,78 @@ class TestAnsibleMethods(db_base.DbTestCase): finish_deploy_mock.assert_called_once_with(task) clean_ramdisk_mock.assert_called_once_with(task) + def test__validate_clean_steps(self): + steps = [{"interface": "deploy", + "name": "foo", + "args": {"spam": {"required": True, "value": "ham"}}}, + {"name": "bar", + "interface": "deploy"}] + self.assertIsNone(ansible_deploy._validate_clean_steps( + steps, self.node.uuid)) + + def test__validate_clean_steps_missing(self): + steps = [{"name": "foo", + "interface": "deploy", + "args": {"spam": {"value": "ham"}, + "ham": {"required": True}}}, + {"name": "bar"}, + {"interface": "deploy"}] + exc = self.assertRaises(exception.NodeCleaningFailure, + ansible_deploy._validate_clean_steps, + steps, self.node.uuid) + self.assertIn("name foo, field ham.value", six.text_type(exc)) + self.assertIn("name bar, field interface", six.text_type(exc)) + self.assertIn("name undefined, field name", six.text_type(exc)) + + def test__validate_clean_steps_names_not_unique(self): + steps = [{"name": "foo", + "interface": "deploy"}, + {"name": "foo", + "interface": "deploy"}] + exc = self.assertRaises(exception.NodeCleaningFailure, + ansible_deploy._validate_clean_steps, + steps, self.node.uuid) + self.assertIn("unique names", six.text_type(exc)) + + @mock.patch.object(ansible_deploy.yaml, 'safe_load', autospec=True) + def test__get_clean_steps(self, load_mock): + steps = [{"interface": "deploy", + "name": "foo", + "args": {"spam": {"required": True, "value": "ham"}}}, + {"name": "bar", + "interface": "deploy", + "priority": 100}] + load_mock.return_value = steps + expected = [{"interface": "deploy", + "step": "foo", + "priority": 10, + "abortable": False, + "argsinfo": {"spam": {"required": True}}, + "args": {"spam": "ham"}}, + {"interface": "deploy", + "step": "bar", + "priority": 100, + "abortable": False, + "argsinfo": {}, + "args": {}}] + d_info = self.node.driver_info + d_info['ansible_clean_steps_config'] = 'custom_clean' + self.node.driver_info = d_info + self.node.save() + self.config(group='ansible', playbooks_path='/path/to/playbooks') + + with mock.patch.object(ansible_deploy, 'open', mock.mock_open(), + create=True) as open_mock: + self.assertEqual( + expected, + ansible_deploy._get_clean_steps( + self.node, interface="deploy", + override_priorities={"foo": 10})) + open_mock.assert_has_calls(( + mock.call('/path/to/playbooks/custom_clean'),)) + load_mock.assert_called_once_with( + open_mock().__enter__.return_value) + class TestAnsibleDeploy(db_base.DbTestCase): def setUp(self): @@ -384,7 +496,7 @@ class TestAnsibleDeploy(db_base.DbTestCase): power_mock.assert_called_once_with(task, states.REBOOT) @mock.patch.object(ansible_deploy, '_deploy', autospec=True) - @mock.patch.object(ansible_deploy, '_get_node_ip', + @mock.patch.object(ansible_deploy, '_get_node_ip_dhcp', return_value='127.0.0.1', autospec=True) @mock.patch.object(utils, 'node_power_action', autospec=True) def test_deploy_done(self, power_mock, get_ip_mock, deploy_mock): @@ -451,7 +563,7 @@ class TestAnsibleDeploy(db_base.DbTestCase): with task_manager.acquire(self.context, self.node.uuid) as task: steps = self.driver.get_clean_steps(task) get_clean_steps_mock.assert_called_once_with( - task, interface='deploy', + task.node, interface='deploy', override_priorities={ 'erase_devices': None, 'erase_devices_metadata': None}) @@ -471,7 +583,7 @@ class TestAnsibleDeploy(db_base.DbTestCase): with task_manager.acquire(self.context, self.node.uuid) as task: steps = self.driver.get_clean_steps(task) mock_get_clean_steps.assert_called_once_with( - task, interface='deploy', + task.node, interface='deploy', override_priorities={'erase_devices': 9, 'erase_devices_metadata': 98}) self.assertEqual(mock_steps, steps) @@ -491,6 +603,10 @@ class TestAnsibleDeploy(db_base.DbTestCase): DRIVER_INTERNAL_INFO['ansible_cleaning_ip'], 'test_u', {})]} prepare_extra_mock.return_value = ironic_nodes + di_info = self.node.driver_internal_info + di_info['agent_url'] = 'http://127.0.0.1' + self.node.driver_internal_info = di_info + self.node.save() with task_manager.acquire(self.context, self.node.uuid) as task: self.driver.execute_clean_step(task, step) @@ -502,28 +618,6 @@ class TestAnsibleDeploy(db_base.DbTestCase): run_playbook_mock.assert_called_once_with( 'test_pl', ironic_nodes, 'test_k', tags=['clean']) - @mock.patch.object(ansible_deploy, '_run_playbook', autospec=True) - @mock.patch.object(ansible_deploy, '_parse_ansible_driver_info', - return_value=('test_pl', 'test_u', 'test_k'), - autospec=True) - def test_execute_clean_step_no_ip(self, parse_driver_info_mock, - run_playbook_mock): - - step = {'priority': 10, 'interface': 'deploy', - 'step': 'erase_devices', 'tags': ['clean']} - driver_internal_info = dict(DRIVER_INTERNAL_INFO) - del driver_internal_info['ansible_cleaning_ip'] - self.node.driver_internal_info = driver_internal_info - self.node.save() - - with task_manager.acquire(self.context, self.node.uuid) as task: - self.assertRaises(exception.NodeCleaningFailure, - self.driver.execute_clean_step, task, step) - - parse_driver_info_mock.assert_called_once_with( - task.node, action='clean') - self.assertFalse(run_playbook_mock.called) - @mock.patch.object(ansible_deploy, '_parse_ansible_driver_info', return_value=('test_pl', 'test_u', 'test_k'), autospec=True) @@ -536,6 +630,10 @@ class TestAnsibleDeploy(db_base.DbTestCase): run_mock.side_effect = exception.InstanceDeployFailure('Boom') step = {'priority': 10, 'interface': 'deploy', 'step': 'erase_devices', 'args': {'tags': ['clean']}} + di_info = self.node.driver_internal_info + di_info['agent_url'] = 'http://127.0.0.1' + self.node.driver_internal_info = di_info + self.node.save() with task_manager.acquire(self.context, self.node.uuid) as task: self.driver.execute_clean_step(task, step) log_mock.error.assert_called_once_with( @@ -590,7 +688,7 @@ class TestAnsibleDeploy(db_base.DbTestCase): @mock.patch.object(ansible_deploy, '_parse_ansible_driver_info', return_value=('test_pl', 'test_u', 'test_k'), autospec=True) - @mock.patch.object(ansible_deploy, '_get_node_ip', + @mock.patch.object(ansible_deploy, '_get_node_ip_dhcp', return_value='127.0.0.1', autospec=True) @mock.patch.object(ansible_deploy, '_run_playbook', autospec=True) @mock.patch.object(utils, 'node_power_action', autospec=True)