From 06826b80e98fa13f3fdf85f47071fbc24c3527dd Mon Sep 17 00:00:00 2001 From: Dmitry Tantsur Date: Wed, 9 May 2018 15:48:35 +0200 Subject: [PATCH] Bring test coverage to 90% and keep it there --- metalsmith/_os_api.py | 5 +- metalsmith/_provisioner.py | 87 ++++++------ metalsmith/_utils.py | 2 +- metalsmith/test/test_provisioner.py | 213 +++++++++++++++++++++++++--- tox.ini | 2 +- 5 files changed, 239 insertions(+), 70 deletions(-) diff --git a/metalsmith/_os_api.py b/metalsmith/_os_api.py index 4201536..4010a1d 100644 --- a/metalsmith/_os_api.py +++ b/metalsmith/_os_api.py @@ -124,8 +124,9 @@ class API(object): if not result['result']: raise RuntimeError('%s: %s' % (iface, result['reason'])) - def wait_for_active(self, node, timeout): - self.ironic.node.wait_for_provision_state(_node_id(node), 'active', + def wait_for_node_state(self, node, state, timeout): + self.ironic.node.wait_for_provision_state(_node_id(node), + state, timeout=timeout) diff --git a/metalsmith/_provisioner.py b/metalsmith/_provisioner.py index 4267ec1..3f559d5 100644 --- a/metalsmith/_provisioner.py +++ b/metalsmith/_provisioner.py @@ -124,7 +124,7 @@ class Provisioner(object): LOG.info('Provisioning started on node %s', _utils.log_node(node)) if wait is not None: - self._api.wait_for_active(node, timeout=wait) + self._api.wait_for_node_state(node, 'active', timeout=wait) # Update the node to return it's latest state node = self._api.get_node(node) @@ -132,16 +132,20 @@ class Provisioner(object): with excutils.save_and_reraise_exception(): LOG.error('Deploy attempt failed on node %s, cleaning up', _utils.log_node(node)) - try: - self._clean_up(node, created_ports) - except Exception: - LOG.exception('Clean up failed') + self._clean_up(node, created_ports) if wait is not None: LOG.info('Deploy succeeded on node %s', _utils.log_node(node)) return node + def _clean_up(self, node, created_ports): + try: + self._delete_ports(node, created_ports) + self._api.release_node(node) + except Exception: + LOG.exception('Clean up failed') + def _get_networks(self, network_refs): """Validate and get the networks.""" networks = [] @@ -154,39 +158,6 @@ class Provisioner(object): networks.append(network) return networks - def _clean_up(self, node, created_ports=None): - """Clean up a failed deployment.""" - if self._dry_run: - LOG.debug("Dry run, not cleaning up") - return - - if created_ports is None: - created_ports = node.extra.get(_CREATED_PORTS, []) - - for port_id in created_ports: - LOG.debug('Detaching port %(port)s from node %(node)s', - {'port': port_id, 'node': node.uuid}) - try: - self._api.detach_port_from_node(node.uuid, port_id) - except Exception as exc: - LOG.debug('Failed to remove VIF %(vif)s from node %(node)s, ' - 'assuming already removed: %(exc)s', - {'vif': port_id, 'node': _utils.log_node(node), - 'exc': exc}) - - LOG.debug('Deleting port %s', port_id) - try: - self._api.delete_port(port_id) - except Exception: - LOG.warning('Failed to delete neutron port %s', port_id) - - try: - self._api.release_node(node) - except Exception as exc: - LOG.warning('Failed to remove instance_uuid from node %(node)s, ' - 'assuming already removed: %(exc)s', - {'node': _utils.log_node(node), 'exc': exc}) - def _create_ports(self, node, networks): """Create and attach ports on given networks.""" created_ports = [] @@ -203,13 +174,31 @@ class Provisioner(object): except Exception: with excutils.save_and_reraise_exception(): LOG.error('Creating and binding ports failed, cleaning up') - try: - self._clean_up(node, created_ports) - except Exception: - LOG.exception('Clean up failed, delete and detach ports ' - '%s manually', created_ports) + self._clean_up(node, created_ports) + return created_ports + def _delete_ports(self, node, created_ports=None): + if created_ports is None: + created_ports = node.extra.get(_CREATED_PORTS, []) + + for port_id in created_ports: + LOG.debug('Detaching port %(port)s from node %(node)s', + {'port': port_id, 'node': node.uuid}) + try: + self._api.detach_port_from_node(node, port_id) + except Exception as exc: + LOG.debug('Failed to remove VIF %(vif)s from node %(node)s, ' + 'assuming already removed: %(exc)s', + {'vif': port_id, 'node': _utils.log_node(node), + 'exc': exc}) + + LOG.debug('Deleting port %s', port_id) + try: + self._api.delete_port(port_id) + except Exception: + LOG.warning('Failed to delete neutron port %s', port_id) + def unprovision_node(self, node, wait=None): """Unprovision a previously provisioned node. @@ -218,13 +207,17 @@ class Provisioner(object): None to return immediately. """ node = self._api.get_node(node) + if self._dry_run: + LOG.debug("Dry run, not unprovisioning") + return - self._api.node_action(node.uuid, 'deleted') + self._delete_ports(node) + + self._api.node_action(node, 'deleted') LOG.info('Deleting started for node %s', _utils.log_node(node)) if wait is not None: - self._api.ironic.node.wait_for_provision_state( - node.uuid, 'available', timeout=max(0, wait)) + self._api.wait_for_node_state(node, 'available', timeout=wait) - self._clean_up(node) + self._api.release_node(node) LOG.info('Node %s undeployed successfully', _utils.log_node(node)) diff --git a/metalsmith/_utils.py b/metalsmith/_utils.py index ba836a6..b9802d3 100644 --- a/metalsmith/_utils.py +++ b/metalsmith/_utils.py @@ -71,6 +71,6 @@ def get_root_disk(root_disk_size, node): "a positive integer, got %d" % root_disk_size) else: # allow for partitioning and config drive - root_disk_size = int(node.properties['local_gb']) - 2 + root_disk_size = int(node.properties['local_gb']) - 1 return root_disk_size diff --git a/metalsmith/test/test_provisioner.py b/metalsmith/test/test_provisioner.py index 9f938bd..8892c86 100644 --- a/metalsmith/test/test_provisioner.py +++ b/metalsmith/test/test_provisioner.py @@ -17,18 +17,30 @@ import mock import testtools from metalsmith import _exceptions +from metalsmith import _os_api from metalsmith import _provisioner -class TestReserveNode(testtools.TestCase): +class Base(testtools.TestCase): def setUp(self): - super(TestReserveNode, self).setUp() - self.api = mock.Mock(spec=['list_nodes', 'reserve_node', - 'validate_node']) + super(Base, self).setUp() self.pr = _provisioner.Provisioner(mock.Mock()) + self._reset_api_mock() + self.node = mock.Mock(spec=['name', 'uuid', 'properties', 'extra'], + uuid='000', properties={'local_gb': 100}, + extra={}) + self.node.name = 'control-0' + + def _reset_api_mock(self): + self.api = mock.Mock(spec=_os_api.API) + self.api.get_node.side_effect = lambda n: n + self.api.update_node.side_effect = lambda n, _u: n self.pr._api = self.api + +class TestReserveNode(Base): + def test_no_nodes(self): self.api.list_nodes.return_value = [] @@ -63,21 +75,184 @@ class TestReserveNode(testtools.TestCase): self.assertIs(node, expected) -class TestProvisionNode(testtools.TestCase): - - def setUp(self): - super(TestProvisionNode, self).setUp() - self.api = mock.Mock(spec=['get_node', 'get_image_info', 'get_network', - 'update_node', 'validate_node', - 'create_port', 'attach_port_to_node', - 'node_action', 'wait_for_active']) - self.api.get_node.side_effect = lambda n: n - self.api.update_node.side_effect = lambda n, _u: n - self.pr = _provisioner.Provisioner(mock.Mock()) - self.pr._api = self.api - self.node = mock.Mock(spec=['name', 'uuid', 'properties'], - uuid='000', properties={'local_gb': 100}) - self.node.name = 'control-0' +class TestProvisionNode(Base): def test_ok(self): self.pr.provision_node(self.node, 'image', ['network']) + + self.api.create_port.assert_called_once_with( + network_id=self.api.get_network.return_value.id) + self.api.attach_port_to_node.assert_called_once_with( + self.node.uuid, self.api.create_port.return_value.id) + image = self.api.get_image_info.return_value + updates = {'/instance_info/ramdisk': image.ramdisk_id, + '/instance_info/kernel': image.kernel_id, + '/instance_info/image_source': image.id, + '/instance_info/root_gb': 99, # 100 - 1 + '/instance_info/capabilities': {'boot_option': 'local'}, + '/extra/metalsmith_created_ports': [ + self.api.create_port.return_value.id + ]} + self.api.update_node.assert_called_once_with(self.node, updates) + self.api.validate_node.assert_called_once_with(self.node, + validate_deploy=True) + self.api.node_action.assert_called_once_with(self.node, 'active', + configdrive=mock.ANY) + self.assertFalse(self.api.wait_for_node_state.called) + self.assertFalse(self.api.release_node.called) + self.assertFalse(self.api.delete_port.called) + + def test_with_wait(self): + self.pr.provision_node(self.node, 'image', ['network'], wait=3600) + + self.api.create_port.assert_called_once_with( + network_id=self.api.get_network.return_value.id) + self.api.attach_port_to_node.assert_called_once_with( + self.node.uuid, self.api.create_port.return_value.id) + image = self.api.get_image_info.return_value + updates = {'/instance_info/ramdisk': image.ramdisk_id, + '/instance_info/kernel': image.kernel_id, + '/instance_info/image_source': image.id, + '/instance_info/root_gb': 99, # 100 - 1 + '/instance_info/capabilities': {'boot_option': 'local'}, + '/extra/metalsmith_created_ports': [ + self.api.create_port.return_value.id + ]} + self.api.update_node.assert_called_once_with(self.node, updates) + self.api.validate_node.assert_called_once_with(self.node, + validate_deploy=True) + self.api.node_action.assert_called_once_with(self.node, 'active', + configdrive=mock.ANY) + self.api.wait_for_node_state.assert_called_once_with(self.node, + 'active', + timeout=3600) + self.assertFalse(self.api.release_node.called) + self.assertFalse(self.api.delete_port.called) + + def test_dry_run(self): + self.pr._dry_run = True + self.pr.provision_node(self.node, 'image', ['network']) + + self.assertFalse(self.api.create_port.called) + self.assertFalse(self.api.attach_port_to_node.called) + self.assertFalse(self.api.update_node.called) + self.assertFalse(self.api.node_action.called) + self.assertFalse(self.api.wait_for_node_state.called) + self.assertFalse(self.api.release_node.called) + self.assertFalse(self.api.delete_port.called) + + def test_deploy_failure(self): + self.api.node_action.side_effect = RuntimeError('boom') + self.assertRaisesRegex(RuntimeError, 'boom', + self.pr.provision_node, self.node, + 'image', ['network'], wait=3600) + + self.assertFalse(self.api.wait_for_node_state.called) + self.api.release_node.assert_called_once_with(self.node) + self.api.delete_port.assert_called_once_with( + self.api.create_port.return_value.id) + self.api.detach_port_from_node.assert_called_once_with( + self.node, self.api.create_port.return_value.id) + + def test_port_creation_failure(self): + self.api.create_port.side_effect = RuntimeError('boom') + self.assertRaisesRegex(RuntimeError, 'boom', + self.pr.provision_node, self.node, + 'image', ['network'], wait=3600) + + self.assertFalse(self.api.node_action.called) + self.api.release_node.assert_called_once_with(self.node) + self.assertFalse(self.api.delete_port.called) + self.assertFalse(self.api.detach_port_from_node.called) + + def test_port_attach_failure(self): + self.api.attach_port_to_node.side_effect = RuntimeError('boom') + self.assertRaisesRegex(RuntimeError, 'boom', + self.pr.provision_node, self.node, + 'image', ['network'], wait=3600) + + self.assertFalse(self.api.node_action.called) + self.api.release_node.assert_called_once_with(self.node) + self.api.delete_port.assert_called_once_with( + self.api.create_port.return_value.id) + self.api.detach_port_from_node.assert_called_once_with( + self.node, self.api.create_port.return_value.id) + + @mock.patch.object(_provisioner.LOG, 'exception', autospec=True) + def test_failure_during_deploy_failure(self, mock_log_exc): + for failed_call in ['detach_port_from_node', + 'delete_port', 'release_node']: + self._reset_api_mock() + getattr(self.api, failed_call).side_effect = AssertionError() + self.api.node_action.side_effect = RuntimeError('boom') + self.assertRaisesRegex(RuntimeError, 'boom', + self.pr.provision_node, self.node, + 'image', ['network'], wait=3600) + + self.assertFalse(self.api.wait_for_node_state.called) + self.api.release_node.assert_called_once_with(self.node) + self.api.delete_port.assert_called_once_with( + self.api.create_port.return_value.id) + self.api.detach_port_from_node.assert_called_once_with( + self.node, self.api.create_port.return_value.id) + self.assertEqual(mock_log_exc.called, + failed_call == 'release_node') + + def test_invalid_image(self): + for result, error in [ + (None, 'does not exist'), + (mock.Mock(kernel_id=None), 'kernel_id is required'), + (mock.Mock(ramdisk_id=None), 'ramdisk_id is required') + ]: + self.api.get_image_info.return_value = result + self.assertRaisesRegex(_exceptions.InvalidImage, error, + self.pr.provision_node, + self.node, 'image', ['network']) + self.assertFalse(self.api.update_node.called) + self.assertFalse(self.api.node_action.called) + + def test_invalid_network(self): + self.api.get_network.return_value = None + self.assertRaises(_exceptions.InvalidNetwork, + self.pr.provision_node, + self.node, 'image', ['network']) + self.assertFalse(self.api.create_port.called) + self.assertFalse(self.api.node_action.called) + + +class TestUnprovisionNode(Base): + + def test_ok(self): + self.node.extra['metalsmith_created_ports'] = ['port1'] + self.pr.unprovision_node(self.node) + + self.api.delete_port.assert_called_once_with('port1') + self.api.detach_port_from_node.assert_called_once_with(self.node, + 'port1') + self.api.node_action.assert_called_once_with(self.node, 'deleted') + self.api.release_node.assert_called_once_with(self.node) + self.assertFalse(self.api.wait_for_node_state.called) + + def test_with_wait(self): + self.node.extra['metalsmith_created_ports'] = ['port1'] + self.pr.unprovision_node(self.node, wait=3600) + + self.api.delete_port.assert_called_once_with('port1') + self.api.detach_port_from_node.assert_called_once_with(self.node, + 'port1') + self.api.node_action.assert_called_once_with(self.node, 'deleted') + self.api.release_node.assert_called_once_with(self.node) + self.api.wait_for_node_state.assert_called_once_with(self.node, + 'available', + timeout=3600) + + def test_dry_run(self): + self.pr._dry_run = True + self.node.extra['metalsmith_created_ports'] = ['port1'] + self.pr.unprovision_node(self.node) + + self.assertFalse(self.api.node_action.called) + self.assertFalse(self.api.release_node.called) + self.assertFalse(self.api.delete_port.called) + self.assertFalse(self.api.detach_port_from_node.called) + self.assertFalse(self.api.wait_for_node_state.called) diff --git a/tox.ini b/tox.ini index 61f84fb..8b14e4b 100644 --- a/tox.ini +++ b/tox.ini @@ -7,7 +7,7 @@ deps = -r{toxinidir}/test-requirements.txt commands = coverage run --branch --include "metalsmith*" -m unittest discover metalsmith.test - coverage report -m + coverage report -m --fail-under 90 setenv = PYTHONDONTWRITEBYTECODE=1 passenv = http_proxy HTTP_PROXY https_proxy HTTPS_PROXY no_proxy NO_PROXY \ OS_USERNAME OS_PASSWORD OS_PROJECT_NAME OS_AUTH_URL \