diff --git a/metalsmith/_provisioner.py b/metalsmith/_provisioner.py index 50ae8db..8bef9e9 100644 --- a/metalsmith/_provisioner.py +++ b/metalsmith/_provisioner.py @@ -14,7 +14,6 @@ # limitations under the License. import logging -import sys from openstack import connection from openstack import exceptions as os_exc @@ -113,7 +112,10 @@ class Provisioner(object): candidates, predicate): """Build a list of candidate nodes for allocation.""" if candidates: - nodes = [self._get_node(node) for node in candidates] + try: + nodes = [self._get_node(node) for node in candidates] + except os_exc.ResourceNotFound as exc: + raise exceptions.InvalidNode(str(exc)) else: nodes = list(self.connection.baremetal.nodes( details=True, @@ -149,9 +151,14 @@ class Provisioner(object): 'with traits %(traits)s and candidate nodes %(candidates)s', {'rsc': resource_class, 'traits': traits, 'candidates': candidates}) - allocation = self.connection.baremetal.create_allocation( - name=hostname, candidate_nodes=candidates, - resource_class=resource_class, traits=traits) + try: + allocation = self.connection.baremetal.create_allocation( + name=hostname, candidate_nodes=candidates, + resource_class=resource_class, traits=traits) + except os_exc.SDKException as exc: + # Re-raise the expected exception class + raise exceptions.ReservationFailed( + 'Failed to create an allocation: %s' % exc) node = None try: @@ -171,18 +178,15 @@ class Provisioner(object): node = self._patch_reserved_node(node, allocation, hostname, capabilities) except Exception as exc: - exc_info = sys.exc_info() - - try: + with _utils.reraise_os_exc( + exceptions.ReservationFailed, + 'Failed to delete failed allocation') as expected: LOG.error('Processing allocation %(alloc)s for node %(node)s ' 'failed: %(exc)s; deleting allocation', {'alloc': _utils.log_res(allocation), - 'node': _utils.log_res(node), 'exc': exc}) + 'node': _utils.log_res(node), 'exc': exc}, + exc_info=not expected) self.connection.baremetal.delete_allocation(allocation) - except Exception: - LOG.exception('Failed to delete failed allocation') - - six.reraise(*exc_info) LOG.debug('Reserved node: %s', node) return node, allocation @@ -427,17 +431,12 @@ class Provisioner(object): self.connection.baremetal.set_node_provision_state( node, 'active', config_drive=cd) except Exception: - exc_info = sys.exc_info() - - if clean_up_on_failure: - try: + with _utils.reraise_os_exc( + exceptions.DeploymentFailed) as expected: + if clean_up_on_failure: LOG.error('Deploy attempt failed on node %s, cleaning up', - _utils.log_res(node)) + _utils.log_res(node), exc_info=not expected) self._clean_up(node, nics=nics) - except Exception: - LOG.exception('Clean up failed') - - six.reraise(*exc_info) LOG.info('Provisioning started on node %s', _utils.log_res(node)) @@ -466,14 +465,20 @@ class Provisioner(object): (more precisely, until the operation times out on server side). :return: List of updated :py:class:`metalsmith.Instance` objects if all succeeded. - :raises: `openstack.exceptions.ResourceTimeout` if deployment times - out for any node. - :raises: `openstack.exceptions.SDKException` if deployment fails - for any node. + :raises: :py:class:`metalsmith.exceptions.DeploymentFailed` + if deployment fails or times out. + :raises: :py:class:`metalsmith.exceptions.InstanceNotFound` + if requested nodes cannot be found. """ nodes = [self._find_node_and_allocation(n)[0] for n in nodes] - nodes = self.connection.baremetal.wait_for_nodes_provision_state( - nodes, 'active', timeout=timeout) + try: + nodes = self.connection.baremetal.wait_for_nodes_provision_state( + nodes, 'active', timeout=timeout) + except os_exc.ResourceTimeout as exc: + raise exceptions.DeploymentTimeout(str(exc)) + except os_exc.SDKException as exc: + raise exceptions.DeploymentFailed(str(exc)) + # Using _get_instance in case the deployment started by something # external that uses allocations. return [self._get_instance(node) for node in nodes] @@ -533,6 +538,12 @@ class Provisioner(object): :param wait: How many seconds to wait for the process to finish, None to return immediately. :return: the latest `Node` object. + :raises: :py:class:`metalsmith.exceptions.DeploymentFailed` + if undeployment fails. + :raises: :py:class:`metalsmith.exceptions.DeploymentTimeout` + if undeployment times out. + :raises: :py:class:`metalsmith.exceptions.InstanceNotFound` + if requested node cannot be found. """ node = self._find_node_and_allocation(node)[0] if self._dry_run: @@ -540,16 +551,23 @@ class Provisioner(object): return self._clean_up(node) - node = self.connection.baremetal.set_node_provision_state( - node, 'deleted', wait=False) + try: + node = self.connection.baremetal.set_node_provision_state( + node, 'deleted', wait=False) - LOG.info('Deleting started for node %s', _utils.log_res(node)) + LOG.info('Deleting started for node %s', _utils.log_res(node)) + + if wait is None: + return node - if wait is not None: node = self.connection.baremetal.wait_for_nodes_provision_state( [node], 'available', timeout=wait)[0] - LOG.info('Node %s undeployed successfully', _utils.log_res(node)) + except os_exc.ResourceTimeout as exc: + raise exceptions.DeploymentTimeout(str(exc)) + except os_exc.SDKException as exc: + raise exceptions.DeploymentFailed(str(exc)) + LOG.info('Node %s undeployed successfully', _utils.log_res(node)) return node def show_instance(self, instance_id): @@ -557,7 +575,7 @@ class Provisioner(object): :param instance_id: hostname, UUID or node name. :return: :py:class:`metalsmith.Instance` object. - :raises: :py:class:`metalsmith.exceptions.InvalidInstance` + :raises: :py:class:`metalsmith.exceptions.InstanceNotFound` if the instance is not a valid instance. """ return self.show_instances([instance_id])[0] @@ -571,8 +589,9 @@ class Provisioner(object): :param instances: list of hostnames, UUIDs or node names. :return: list of :py:class:`metalsmith.Instance` objects in the same order as ``instances``. - :raises: :py:class:`metalsmith.exceptions.InvalidInstance` - if one of the instances are not valid instances. + :raises: :py:class:`metalsmith.exceptions.InstanceNotFound` + if one of the instances cannot be found or the found node is + not a valid instance. """ result = [self._get_instance(inst) for inst in instances] # NOTE(dtantsur): do not accept node names as valid instances if they @@ -580,7 +599,7 @@ class Provisioner(object): missing = [inst for (res, inst) in zip(result, instances) if res.state == _instance.InstanceState.UNKNOWN] if missing: - raise exceptions.InvalidInstance( + raise exceptions.InstanceNotFound( "Node(s)/instance(s) %s are not valid instances" % ', '.join(map(str, missing))) return result @@ -611,28 +630,41 @@ class Provisioner(object): return node def _find_node_and_allocation(self, node, refresh=False): - if (not isinstance(node, six.string_types) - or not _utils.is_hostname_safe(node)): - return self._get_node(node, refresh=refresh), None - try: - allocation = self.connection.baremetal.get_allocation(node) - except os_exc.ResourceNotFound: - return self._get_node(node, refresh=refresh), None - else: - if allocation.node_id: + if (not isinstance(node, six.string_types) + or not _utils.is_hostname_safe(node)): + return self._get_node(node, refresh=refresh), None + + try: + allocation = self.connection.baremetal.get_allocation(node) + except os_exc.ResourceNotFound: + return self._get_node(node, refresh=refresh), None + except os_exc.ResourceNotFound as exc: + raise exceptions.InstanceNotFound(str(exc)) + + if allocation.node_id: + try: return (self.connection.baremetal.get_node(allocation.node_id), allocation) - else: - raise exceptions.InvalidInstance( - 'Allocation %s exists but is not associated ' - 'with a node' % node) + except os_exc.ResourceNotFound: + raise exceptions.InstanceNotFound( + 'Node %(node)s associated with allocation ' + '%(alloc)s was not found' % + {'node': allocation.node_id, + 'alloc': allocation.id}) + else: + raise exceptions.InstanceNotFound( + 'Allocation %s exists but is not associated ' + 'with a node' % node) def _get_instance(self, ident): node, allocation = self._find_node_and_allocation(ident) if allocation is None and node.allocation_id: - allocation = self.connection.baremetal.get_allocation( - node.allocation_id) + try: + allocation = self.connection.baremetal.get_allocation( + node.allocation_id) + except os_exc.ResourceNotFound as exc: + raise exceptions.InstanceNotFound(str(exc)) return _instance.Instance(self.connection, node, allocation=allocation) diff --git a/metalsmith/_utils.py b/metalsmith/_utils.py index 1bfda50..db930c3 100644 --- a/metalsmith/_utils.py +++ b/metalsmith/_utils.py @@ -13,13 +13,20 @@ # See the License for the specific language governing permissions and # limitations under the License. +import contextlib +import logging import re +import sys +from openstack import exceptions as os_exc import six from metalsmith import exceptions +LOG = logging.getLogger(__name__) + + def log_res(res): if res is None: return None @@ -122,3 +129,19 @@ def hostname_for(node, allocation=None): return allocation.name else: return default_hostname(node) + + +@contextlib.contextmanager +def reraise_os_exc(reraise_as, failure_message='Clean up failed'): + exc_info = sys.exc_info() + is_expected = isinstance(exc_info[1], os_exc.SDKException) + + try: + yield is_expected + except Exception: + LOG.exception(failure_message) + + if is_expected: + raise reraise_as(str(exc_info[1])) + else: + six.reraise(*exc_info) diff --git a/metalsmith/exceptions.py b/metalsmith/exceptions.py index 4f60417..f85eeb3 100644 --- a/metalsmith/exceptions.py +++ b/metalsmith/exceptions.py @@ -67,28 +67,6 @@ class CapabilitiesNotFound(ReservationFailed): super(CapabilitiesNotFound, self).__init__(message) -class TraitsNotFound(ReservationFailed): - """DEPRECATED.""" - - def __init__(self, message, traits): - self.requested_traits = traits - super(TraitsNotFound, self).__init__(message) - - -class ValidationFailed(ReservationFailed): - """Validation failed for all requested nodes.""" - - -class NoNodesReserved(ReservationFailed): - """DEPRECATED.""" - - def __init__(self, nodes): - self.nodes = nodes - message = ('All the candidate nodes are already reserved ' - 'or failed validation') - super(NoNodesReserved, self).__init__(message) - - class InvalidImage(Error): """Requested image is invalid and cannot be used.""" @@ -105,13 +83,18 @@ class InvalidNode(Error): """This node cannot be deployed onto.""" -class DeploymentFailure(Error): - """DEPRECATED.""" - - def __init__(self, message, nodes): - self.nodes = nodes - super(DeploymentFailure, self).__init__(message) +class DeploymentFailed(Error): + """Deployment failed.""" -class InvalidInstance(Error): - """The node(s) does not have a metalsmith instance associated.""" +class DeploymentTimeout(DeploymentFailed): + """Timeout during deployment.""" + + +class InstanceNotFound(Error): + """Instance not found or node doesn't have an instance associated.""" + + +# Deprecated aliases +DeploymentFailure = DeploymentFailed +InvalidInstance = InstanceNotFound diff --git a/metalsmith/test/test_provisioner.py b/metalsmith/test/test_provisioner.py index aa76b61..e42b3ad 100644 --- a/metalsmith/test/test_provisioner.py +++ b/metalsmith/test/test_provisioner.py @@ -21,6 +21,7 @@ import testtools from metalsmith import _instance from metalsmith import _provisioner +from metalsmith import _utils from metalsmith import exceptions from metalsmith import instance_config from metalsmith import sources @@ -133,6 +134,13 @@ class TestGetFindNode(testtools.TestCase): self.assertIs(result, node) self.assertIsNone(alloc) + def test__find_node_and_allocation_by_node_not_found(self): + node = mock.Mock(spec=['id', 'name']) + self.api.baremetal.get_node.side_effect = os_exc.ResourceNotFound + self.assertRaises(exceptions.InstanceNotFound, + self.pr._find_node_and_allocation, node, + refresh=True) + def test__find_node_and_allocation_by_hostname(self): result, alloc = self.pr._find_node_and_allocation('node') self.assertIs(result, self.api.baremetal.get_node.return_value) @@ -148,9 +156,16 @@ class TestGetFindNode(testtools.TestCase): self.assertIsNone(alloc) self.api.baremetal.get_node.assert_called_once_with('node') + def test__find_node_and_allocation_by_hostname_node_in_allocation(self): + self.api.baremetal.get_node.side_effect = os_exc.ResourceNotFound + self.assertRaises(exceptions.InstanceNotFound, + self.pr._find_node_and_allocation, 'node') + self.api.baremetal.get_node.assert_called_once_with( + self.api.baremetal.get_allocation.return_value.node_id) + def test__find_node_and_allocation_by_hostname_bad_allocation(self): self.api.baremetal.get_allocation.return_value.node_id = None - self.assertRaises(exceptions.InvalidInstance, + self.assertRaises(exceptions.InstanceNotFound, self.pr._find_node_and_allocation, 'node') self.assertFalse(self.api.baremetal.get_node.called) @@ -195,6 +210,19 @@ class TestReserveNode(Base): self.assertFalse(self.api.baremetal.patch_node.called) self.assertFalse(self.api.baremetal.delete_allocation.called) + def test_create_allocation_failed(self): + self.api.baremetal.create_allocation.side_effect = ( + os_exc.SDKException('boom')) + + self.assertRaisesRegex(exceptions.ReservationFailed, 'boom', + self.pr.reserve_node, self.RSC) + + self.api.baremetal.create_allocation.assert_called_once_with( + name=None, candidate_nodes=None, + resource_class=self.RSC, traits=None) + self.assertFalse(self.api.baremetal.delete_allocation.called) + self.assertFalse(self.api.baremetal.patch_node.called) + def test_allocation_failed(self): self.api.baremetal.wait_for_allocation.side_effect = ( os_exc.SDKException('boom')) @@ -209,7 +237,7 @@ class TestReserveNode(Base): self.api.baremetal.create_allocation.return_value) self.assertFalse(self.api.baremetal.patch_node.called) - @mock.patch.object(_provisioner.LOG, 'exception', autospec=True) + @mock.patch.object(_utils.LOG, 'exception', autospec=True) def test_allocation_failed_clean_up_failed(self, mock_log): self.api.baremetal.delete_allocation.side_effect = RuntimeError() self.api.baremetal.wait_for_allocation.side_effect = ( @@ -269,7 +297,27 @@ class TestReserveNode(Base): self.api.baremetal.nodes.return_value = [expected] self.api.baremetal.patch_node.side_effect = os_exc.SDKException('boom') - self.assertRaisesRegex(os_exc.SDKException, 'boom', + self.assertRaisesRegex(exceptions.ReservationFailed, 'boom', + self.pr.reserve_node, self.RSC, + capabilities={'answer': '42'}) + + self.api.baremetal.create_allocation.assert_called_once_with( + name=None, candidate_nodes=[expected.id], + resource_class=self.RSC, traits=None) + self.api.baremetal.delete_allocation.assert_called_once_with( + self.api.baremetal.wait_for_allocation.return_value) + self.api.baremetal.patch_node.assert_called_once_with( + expected, [{'path': '/instance_info/capabilities', + 'op': 'add', 'value': {'answer': '42'}}]) + + def test_node_update_unexpected_exception(self): + expected = self._node(properties={'local_gb': 100, + 'capabilities': {'answer': '42'}}) + self.api.baremetal.get_node.return_value = expected + self.api.baremetal.nodes.return_value = [expected] + self.api.baremetal.patch_node.side_effect = RuntimeError('boom') + + self.assertRaisesRegex(RuntimeError, 'boom', self.pr.reserve_node, self.RSC, capabilities={'answer': '42'}) @@ -352,6 +400,16 @@ class TestReserveNode(Base): self.api.baremetal.wait_for_allocation.return_value.node_id) self.assertFalse(self.api.baremetal.patch_node.called) + def test_provided_node_not_found(self): + self.mock_get_node.side_effect = os_exc.ResourceNotFound + + self.assertRaises(exceptions.InvalidNode, self.pr.reserve_node, + self.RSC, candidates=['node1']) + + self.assertFalse(self.api.baremetal.nodes.called) + self.assertFalse(self.api.baremetal.create_allocation.called) + self.assertFalse(self.api.baremetal.patch_node.called) + def test_nodes_filtered(self): nodes = [self._node(resource_class='banana'), self._node(resource_class='compute'), @@ -1776,6 +1834,16 @@ class TestUnprovisionNode(Base): wait_mock.assert_called_once_with([self.node], 'available', timeout=3600) + def test_with_wait_failed(self): + for caught, expected in [(os_exc.ResourceTimeout, + exceptions.DeploymentTimeout), + (os_exc.SDKException, + exceptions.DeploymentFailed)]: + self.api.baremetal.wait_for_nodes_provision_state.side_effect = ( + caught) + self.assertRaises(expected, self.pr.unprovision_node, + self.node, wait=3600) + def test_without_allocation(self): self.node.allocation_id = None # Check that unrelated extra fields are not touched. @@ -1867,7 +1935,7 @@ class TestShowInstance(testtools.TestCase): self.node.provision_state = 'manageable' self.api.baremetal.get_allocation.side_effect = ( os_exc.ResourceNotFound()) - self.assertRaises(exceptions.InvalidInstance, + self.assertRaises(exceptions.InstanceNotFound, self.pr.show_instance, 'id1') self.api.baremetal.get_node.assert_called_once_with('id1') @@ -1881,6 +1949,17 @@ class TestWaitForProvisioning(Base): self.assertEqual([node], [inst.node for inst in result]) self.assertIsInstance(result[0], _instance.Instance) + def test_exceptions(self): + node = mock.Mock(spec=NODE_FIELDS) + + for caught, expected in [(os_exc.ResourceTimeout, + exceptions.DeploymentTimeout), + (os_exc.SDKException, + exceptions.DeploymentFailed)]: + self.api.baremetal.wait_for_nodes_provision_state.side_effect = ( + caught) + self.assertRaises(expected, self.pr.wait_for_provisioning, [node]) + class TestListInstances(Base): def setUp(self): diff --git a/releasenotes/notes/exceptions-395ae4f1ad4de6da.yaml b/releasenotes/notes/exceptions-395ae4f1ad4de6da.yaml new file mode 100644 index 0000000..5bf4e16 --- /dev/null +++ b/releasenotes/notes/exceptions-395ae4f1ad4de6da.yaml @@ -0,0 +1,8 @@ +--- +upgrade: + - | + Changes to consistently using exceptions from ``metalsmith.exceptions`` + rathen than exposing OpenStackSDK exceptions. +deprecations: + - | + The exception ``InvalidInstance`` has been renamed to ``InstanceNotFound``.