Bring test coverage to 90% and keep it there

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):
+    def wait_for_node_state(self, node, state, timeout):
-        self.ironic.node.wait_for_provision_state(_node_id(node), 'active',
+        self.ironic.node.wait_for_provision_state(_node_id(node),
+                                                  state,
                                                   timeout=timeout)
 
 

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)
-                    self._clean_up(node, created_ports)
-                except Exception:
-                    LOG.exception('Clean up failed')
 
         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)
-                    self._clean_up(node, created_ports)
+
-                except Exception:
-                    LOG.exception('Clean up failed, delete and detach ports '
-                                  '%s manually', 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(
+            self._api.wait_for_node_state(node, 'available', timeout=wait)
-                node.uuid, 'available', timeout=max(0, wait))
 
-        self._clean_up(node)
+        self._api.release_node(node)
         LOG.info('Node %s undeployed successfully', _utils.log_node(node))

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

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()
+        super(Base, self).setUp()
-        self.api = mock.Mock(spec=['list_nodes', 'reserve_node',
-                                   'validate_node'])
         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):
+class TestProvisionNode(Base):
-
-    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'
 
     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)

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 \