From f4fa6fabba88d6a738153357bcce55974a00cb1b Mon Sep 17 00:00:00 2001 From: Dmitry Tantsur Date: Fri, 7 Jun 2019 11:22:06 +0200 Subject: [PATCH] baremetal: raise more specific ResourceFailure in wait_for_* methods Raising a generic SDKException makes it harder to distinguish them from other failures. Also add missing docstrings. Change-Id: I576469b0bdb664512ce9d34d5217329f3fa62b64 --- openstack/baremetal/v1/_proxy.py | 6 ++++ openstack/baremetal/v1/allocation.py | 5 +++- openstack/baremetal/v1/node.py | 15 +++++++--- .../baremetal_introspection/v1/_proxy.py | 3 ++ .../v1/introspection.py | 5 +++- .../unit/baremetal/v1/test_allocation.py | 30 +++++++++++++++++++ .../tests/unit/baremetal/v1/test_node.py | 4 +-- .../baremetal_introspection/v1/test_proxy.py | 2 +- .../baremetal-wait-e4571cdb150b188a.yaml | 7 +++++ 9 files changed, 68 insertions(+), 9 deletions(-) create mode 100644 releasenotes/notes/baremetal-wait-e4571cdb150b188a.yaml diff --git a/openstack/baremetal/v1/_proxy.py b/openstack/baremetal/v1/_proxy.py index 842fd6241..01754b3d7 100644 --- a/openstack/baremetal/v1/_proxy.py +++ b/openstack/baremetal/v1/_proxy.py @@ -344,6 +344,9 @@ class Proxy(proxy.Proxy): :return: The list of :class:`~openstack.baremetal.v1.node.Node` instances that reached the requested state. + :raises: :class:`~openstack.exceptions.ResourceFailure` if a node + reaches an error state and ``abort_on_failed_state`` is ``True``. + :raises: :class:`~openstack.exceptions.ResourceTimeout` on timeout. """ log_nodes = ', '.join(n.id if isinstance(n, _node.Node) else n for n in nodes) @@ -895,6 +898,9 @@ class Proxy(proxy.Proxy): :returns: The instance of the allocation. :rtype: :class:`~openstack.baremetal.v1.allocation.Allocation`. + :raises: :class:`~openstack.exceptions.ResourceFailure` if allocation + fails and ``ignore_error`` is ``False``. + :raises: :class:`~openstack.exceptions.ResourceTimeout` on timeout. """ res = self._get_resource(_allocation.Allocation, allocation) return res.wait(self, timeout=timeout, ignore_error=ignore_error) diff --git a/openstack/baremetal/v1/allocation.py b/openstack/baremetal/v1/allocation.py index c4cf48762..19be5133f 100644 --- a/openstack/baremetal/v1/allocation.py +++ b/openstack/baremetal/v1/allocation.py @@ -76,6 +76,9 @@ class Allocation(_common.ListMixin, resource.Resource): state is considered successful and the call returns. :return: This :class:`Allocation` instance. + :raises: :class:`~openstack.exceptions.ResourceFailure` if allocation + fails and ``ignore_error`` is ``False``. + :raises: :class:`~openstack.exceptions.ResourceTimeout` on timeout. """ if self.state == 'active': return self @@ -86,7 +89,7 @@ class Allocation(_common.ListMixin, resource.Resource): self.fetch(session) if self.state == 'error' and not ignore_error: - raise exceptions.SDKException( + raise exceptions.ResourceFailure( "Allocation %(allocation)s failed: %(error)s" % {'allocation': self.id, 'error': self.last_error}) elif self.state != 'allocating': diff --git a/openstack/baremetal/v1/node.py b/openstack/baremetal/v1/node.py index 43f3d8acc..a02a00797 100644 --- a/openstack/baremetal/v1/node.py +++ b/openstack/baremetal/v1/node.py @@ -323,6 +323,10 @@ class Node(_common.ListMixin, resource.Resource): :return: This :class:`Node` instance. :raises: ValueError if ``config_drive``, ``clean_steps`` or ``rescue_password`` are provided with an invalid ``target``. + :raises: :class:`~openstack.exceptions.ResourceFailure` if the node + reaches an error state while waiting for the state. + :raises: :class:`~openstack.exceptions.ResourceTimeout` if timeout + is reached while waiting for the state. """ session = self._get_session(session) @@ -400,6 +404,9 @@ class Node(_common.ListMixin, resource.Resource): ``manageable`` transition is ``enroll`` again. :return: This :class:`Node` instance. + :raises: :class:`~openstack.exceptions.ResourceFailure` if the node + reaches an error state and ``abort_on_failed_state`` is ``True``. + :raises: :class:`~openstack.exceptions.ResourceTimeout` on timeout. """ for count in utils.iterate_timeout( timeout, @@ -469,8 +476,8 @@ class Node(_common.ListMixin, resource.Resource): ``manageable`` transition is ``enroll`` again. :return: ``True`` if the target state is reached - :raises: SDKException if ``abort_on_failed_state`` is ``True`` and - a failure state is reached. + :raises: :class:`~openstack.exceptions.ResourceFailure` if the node + reaches an error state and ``abort_on_failed_state`` is ``True``. """ # NOTE(dtantsur): microversion 1.2 changed None to available if (self.provision_state == expected_state @@ -481,7 +488,7 @@ class Node(_common.ListMixin, resource.Resource): return False if self.provision_state.endswith(' failed'): - raise exceptions.SDKException( + raise exceptions.ResourceFailure( "Node %(node)s reached failure state \"%(state)s\"; " "the last error is %(error)s" % {'node': self.id, 'state': self.provision_state, @@ -490,7 +497,7 @@ class Node(_common.ListMixin, resource.Resource): # "enroll" elif (expected_state == 'manageable' and self.provision_state == 'enroll' and self.last_error): - raise exceptions.SDKException( + raise exceptions.ResourceFailure( "Node %(node)s could not reach state manageable: " "failed to verify management credentials; " "the last error is %(error)s" % diff --git a/openstack/baremetal_introspection/v1/_proxy.py b/openstack/baremetal_introspection/v1/_proxy.py index 23b7ff2b8..7ad0b0796 100644 --- a/openstack/baremetal_introspection/v1/_proxy.py +++ b/openstack/baremetal_introspection/v1/_proxy.py @@ -125,6 +125,9 @@ class Proxy(proxy.Proxy): if the introspection reaches the ``error`` state. Otherwise the error state is considered successful and the call returns. :returns: :class:`~.introspection.Introspection` instance. + :raises: :class:`~openstack.exceptions.ResourceFailure` if + introspection fails and ``ignore_error`` is ``False``. + :raises: :class:`~openstack.exceptions.ResourceTimeout` on timeout. """ res = self._get_resource(_introspect.Introspection, introspection) return res.wait(self, timeout=timeout, ignore_error=ignore_error) diff --git a/openstack/baremetal_introspection/v1/introspection.py b/openstack/baremetal_introspection/v1/introspection.py index e560f38a5..88819f9f1 100644 --- a/openstack/baremetal_introspection/v1/introspection.py +++ b/openstack/baremetal_introspection/v1/introspection.py @@ -107,6 +107,9 @@ class Introspection(resource.Resource): if the introspection reaches the ``error`` state. Otherwise the error state is considered successful and the call returns. :return: This :class:`Introspection` instance. + :raises: :class:`~openstack.exceptions.ResourceFailure` if + introspection fails and ``ignore_error`` is ``False``. + :raises: :class:`~openstack.exceptions.ResourceTimeout` on timeout. """ if self._check_state(ignore_error): return self @@ -124,7 +127,7 @@ class Introspection(resource.Resource): def _check_state(self, ignore_error): if self.state == 'error' and not ignore_error: - raise exceptions.SDKException( + raise exceptions.ResourceFailure( "Introspection of node %(node)s failed: %(error)s" % {'node': self.id, 'error': self.error}) else: diff --git a/openstack/tests/unit/baremetal/v1/test_allocation.py b/openstack/tests/unit/baremetal/v1/test_allocation.py index 2146eed3d..5b42bb4ff 100644 --- a/openstack/tests/unit/baremetal/v1/test_allocation.py +++ b/openstack/tests/unit/baremetal/v1/test_allocation.py @@ -104,6 +104,36 @@ class TestWaitForAllocation(base.TestCase): self.assertIs(allocation, self.allocation) self.assertEqual(2, mock_fetch.call_count) + def test_failure(self, mock_fetch): + marker = [False] # mutable object to modify in the closure + + def _side_effect(allocation, session): + if marker[0]: + self.allocation.state = 'error' + self.allocation.last_error = 'boom!' + else: + marker[0] = True + + mock_fetch.side_effect = _side_effect + self.assertRaises(exceptions.ResourceFailure, + self.allocation.wait, self.session) + self.assertEqual(2, mock_fetch.call_count) + + def test_failure_ignored(self, mock_fetch): + marker = [False] # mutable object to modify in the closure + + def _side_effect(allocation, session): + if marker[0]: + self.allocation.state = 'error' + self.allocation.last_error = 'boom!' + else: + marker[0] = True + + mock_fetch.side_effect = _side_effect + allocation = self.allocation.wait(self.session, ignore_error=True) + self.assertIs(allocation, self.allocation) + self.assertEqual(2, mock_fetch.call_count) + def test_timeout(self, mock_fetch): self.assertRaises(exceptions.ResourceTimeout, self.allocation.wait, self.session, timeout=0.001) diff --git a/openstack/tests/unit/baremetal/v1/test_node.py b/openstack/tests/unit/baremetal/v1/test_node.py index 2d4764932..3a8732fcd 100644 --- a/openstack/tests/unit/baremetal/v1/test_node.py +++ b/openstack/tests/unit/baremetal/v1/test_node.py @@ -178,7 +178,7 @@ class TestNodeWaitForProvisionState(base.TestCase): mock_fetch.side_effect = _get_side_effect - self.assertRaisesRegex(exceptions.SDKException, + self.assertRaisesRegex(exceptions.ResourceFailure, 'failure state "deploy failed"', self.node.wait_for_provision_state, self.session, 'manageable') @@ -191,7 +191,7 @@ class TestNodeWaitForProvisionState(base.TestCase): mock_fetch.side_effect = _get_side_effect - self.assertRaisesRegex(exceptions.SDKException, + self.assertRaisesRegex(exceptions.ResourceFailure, 'failed to verify management credentials', self.node.wait_for_provision_state, self.session, 'manageable') diff --git a/openstack/tests/unit/baremetal_introspection/v1/test_proxy.py b/openstack/tests/unit/baremetal_introspection/v1/test_proxy.py index 92eb50eaf..77f9c2ea5 100644 --- a/openstack/tests/unit/baremetal_introspection/v1/test_proxy.py +++ b/openstack/tests/unit/baremetal_introspection/v1/test_proxy.py @@ -86,7 +86,7 @@ class TestWaitForIntrospection(base.TestCase): self.introspection.error = 'boom' mock_fetch.side_effect = _side_effect - self.assertRaisesRegex(exceptions.SDKException, 'boom', + self.assertRaisesRegex(exceptions.ResourceFailure, 'boom', self.proxy.wait_for_introspection, self.introspection) mock_fetch.assert_called_once_with(self.introspection, self.proxy) diff --git a/releasenotes/notes/baremetal-wait-e4571cdb150b188a.yaml b/releasenotes/notes/baremetal-wait-e4571cdb150b188a.yaml new file mode 100644 index 000000000..c104a7fec --- /dev/null +++ b/releasenotes/notes/baremetal-wait-e4571cdb150b188a.yaml @@ -0,0 +1,7 @@ +--- +fixes: + - | + The baremetal calls ``wait_for_nodes_provision_state``, + ``wait_for_allocation`` and the baremetal introspection call + ``wait_for_introspection`` now raise ``ResourceFailure`` on reaching + an error state instead of a generic ``SDKException``.