From 7a3d93700e37a32257ea16ef5cca43e060d3ea9a Mon Sep 17 00:00:00 2001 From: dparalen Date: Fri, 8 Jan 2016 12:49:35 +0100 Subject: [PATCH] Introduce API for aborting introspection It is not currently possible to stop a running introspection. This may be annoying for the operator, considering the amount of time it takes for the bare metal node to call the continue API request, especially knowing the introspection will fail/time-out eventually (such as when debugging). This patch introduces a REST API endpoint "POST /v1/introspection//abort" in order to fill the gap. Upon the abort method call, following preconditions are checked: * there's a bare metal node matching the UUID * introspection was not finished for the node * introspection process is waiting for the node to give the continue call Following Responses are returned to the caller in case the preconditions were not met: * 404 Response in case node wasn't found * 409 Response (resource busy) in case the introspection process is not waiting for the Continue call Otherwise, a 202 Response is returned. When the abort method is processed, the node is powered off and it is black-listed in inspector's firewall to prevent it from booting the introspection image. This happens asynchronously. To prevent interference with the continue call processing, the processing method was updated to give a 400 Response to the introspection client in case continuing a finished or canceled introspection. Limitations: * IMPI credentials are never updated in case introspection was canceled * 202 response is returned even if the introspection was already finished * the endpoint differs from requested "DELETE /v1/introspection/" Links: [1] https://bugs.launchpad.net/ironic-inspector/+bug/1525235 Change-Id: If043171f0d292ae2775dc1f26233dd4911599247 Closes-Bug: #1525235 --- doc/source/http-api.rst | 21 +++- ironic_inspector/introspect.py | 51 +++++++++ ironic_inspector/main.py | 14 ++- ironic_inspector/process.py | 6 + ironic_inspector/test/functional.py | 27 +++++ ironic_inspector/test/test_introspect.py | 106 ++++++++++++++++++ ironic_inspector/test/test_main.py | 44 ++++++++ ironic_inspector/test/test_process.py | 12 ++ .../abort-introspection-ae5cb5a9fbacd2ac.yaml | 4 + 9 files changed, 283 insertions(+), 2 deletions(-) create mode 100644 releasenotes/notes/abort-introspection-ae5cb5a9fbacd2ac.yaml diff --git a/doc/source/http-api.rst b/doc/source/http-api.rst index d67636962..3199cec6f 100644 --- a/doc/source/http-api.rst +++ b/doc/source/http-api.rst @@ -51,7 +51,25 @@ Response body: JSON dictionary with keys: * ``finished`` (boolean) whether introspection is finished (``true`` on introspection completion or if it ends because of an error) -* ``error`` error string or ``null`` +* ``error`` error string or ``null``; ``Canceled by operator`` in + case introspection was aborted + + +Abort Running Introspection +~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +``POST /v1/introspection//abort`` abort running introspection. + +Requires X-Auth-Token header with Keystone token for authentication. + +Response: + +* 202 - accepted +* 400 - bad request +* 401, 403 - missing or invalid authentication +* 404 - node cannot be found +* 409 - inspector has locked this node for processing + Get Introspection Data ~~~~~~~~~~~~~~~~~~~~~~ @@ -295,3 +313,4 @@ Version History * **1.0** version of API at the moment of introducing versioning. * **1.1** adds endpoint to retrieve stored introspection data. * **1.2** endpoints for manipulating introspection rules. +* **1.3** endpoint for canceling running introspection diff --git a/ironic_inspector/introspect.py b/ironic_inspector/introspect.py index 16601514c..fedef3466 100644 --- a/ironic_inspector/introspect.py +++ b/ironic_inspector/introspect.py @@ -174,3 +174,54 @@ def _background_introspect_locked(ironic, node_info): LOG.info(_LI('Introspection environment is ready, manual power on is ' 'required within %d seconds'), CONF.timeout, node_info=node_info) + + +def abort(uuid, token=None): + """Abort running introspection. + + :param uuid: node uuid + :param token: authentication token + :raises: Error + """ + LOG.debug('Aborting introspection for node %s', uuid) + ironic = utils.get_client(token) + node_info = node_cache.get_node(uuid, ironic=ironic, locked=False) + + # check pending operations + locked = node_info.acquire_lock(blocking=False) + if not locked: + # Node busy --- cannot abort atm + raise utils.Error(_('Node is locked, please, retry later'), + node_info=node_info, code=409) + + utils.spawn_n(_abort, node_info, ironic) + + +def _abort(node_info, ironic): + # runs in background + if node_info.finished_at is not None: + # introspection already finished; nothing to do + LOG.info(_LI('Cannot abort introspection as it is already ' + 'finished'), node_info=node_info) + node_info.release_lock() + return + + # block this node from PXE Booting the introspection image + try: + firewall.update_filters(ironic) + except Exception as exc: + # Note(mkovacik): this will be retried in firewall update + # periodic task; we continue aborting + LOG.warning(_LW('Failed to update firewall filters: %s'), exc, + node_info=node_info) + + # finish the introspection + LOG.debug('Forcing power-off', node_info=node_info) + try: + ironic.node.set_power_state(node_info.uuid, 'off') + except Exception as exc: + LOG.warning(_LW('Failed to power off node: %s'), exc, + node_info=node_info) + + node_info.finished(error=_('Canceled by operator')) + LOG.info(_LI('Introspection aborted'), node_info=node_info) diff --git a/ironic_inspector/main.py b/ironic_inspector/main.py index df2dae811..c1c399d96 100644 --- a/ironic_inspector/main.py +++ b/ironic_inspector/main.py @@ -45,7 +45,7 @@ app = flask.Flask(__name__) LOG = utils.getProcessingLogger(__name__) MINIMUM_API_VERSION = (1, 0) -CURRENT_API_VERSION = (1, 2) +CURRENT_API_VERSION = (1, 3) _MIN_VERSION_HEADER = 'X-OpenStack-Ironic-Inspector-API-Minimum-Version' _MAX_VERSION_HEADER = 'X-OpenStack-Ironic-Inspector-API-Maximum-Version' _VERSION_HEADER = 'X-OpenStack-Ironic-Inspector-API-Version' @@ -209,6 +209,18 @@ def api_introspection(uuid): error=node_info.error or None) +@app.route('/v1/introspection//abort', methods=['POST']) +@convert_exceptions +def api_introspection_abort(uuid): + utils.check_auth(flask.request) + + if not uuidutils.is_uuid_like(uuid): + raise utils.Error(_('Invalid UUID value'), code=400) + + introspect.abort(uuid, token=flask.request.headers.get('X-Auth-Token')) + return '', 202 + + @app.route('/v1/introspection//data', methods=['GET']) @convert_exceptions def api_introspection_data(uuid): diff --git a/ironic_inspector/process.py b/ironic_inspector/process.py index 1447a0d62..ab22ddf3c 100644 --- a/ironic_inspector/process.py +++ b/ironic_inspector/process.py @@ -104,6 +104,12 @@ def process(introspection_data): LOG.info(_LI('Matching node is %s'), node_info.uuid, node_info=node_info, data=introspection_data) + if node_info.finished_at is not None: + # race condition or introspection canceled + raise utils.Error(_('Node processing already finished with ' + 'error: %s') % node_info.error, + node_info=node_info, code=400) + try: node = node_info.node() except exceptions.NotFound: diff --git a/ironic_inspector/test/functional.py b/ironic_inspector/test/functional.py index 7bc7597ca..821bd3bcd 100644 --- a/ironic_inspector/test/functional.py +++ b/ironic_inspector/test/functional.py @@ -159,6 +159,9 @@ class Base(base.NodeTest): def call_get_status(self, uuid): return self.call('get', '/v1/introspection/%s' % uuid).json() + def call_abort_introspect(self, uuid): + return self.call('post', '/v1/introspection/%s/abort' % uuid) + def call_continue(self, data): return self.call('post', '/v1/continue', data=data).json() @@ -361,6 +364,30 @@ class Test(Base): status = self.call_get_status(self.uuid) self.assertEqual({'finished': True, 'error': None}, status) + def test_abort_introspection(self): + self.call_introspect(self.uuid) + eventlet.greenthread.sleep(DEFAULT_SLEEP) + self.cli.node.set_power_state.assert_called_once_with(self.uuid, + 'reboot') + status = self.call_get_status(self.uuid) + self.assertEqual({'finished': False, 'error': None}, status) + + res = self.call_abort_introspect(self.uuid) + eventlet.greenthread.sleep(DEFAULT_SLEEP) + + self.assertEqual(res.status_code, 202) + status = self.call_get_status(self.uuid) + self.assertTrue(status['finished']) + self.assertEqual('Canceled by operator', status['error']) + + # Note(mkovacik): we're checking just this doesn't pass OK as + # there might be either a race condition (hard to test) that + # yields a 'Node already finished.' or an attribute-based + # look-up error from some pre-processing hooks because + # node_info.finished() deletes the look-up attributes only + # after releasing the node lock + self.call('post', '/v1/continue', self.data, expect_error=400) + @contextlib.contextmanager def mocked_server(): diff --git a/ironic_inspector/test/test_introspect.py b/ironic_inspector/test/test_introspect.py index dcbcba4e0..5875da378 100644 --- a/ironic_inspector/test/test_introspect.py +++ b/ironic_inspector/test/test_introspect.py @@ -416,3 +416,109 @@ class TestSetIpmiCredentials(BaseTest): self.assertRaises(utils.Error, introspect.introspect, self.uuid, new_ipmi_credentials=self.new_creds) + + +@mock.patch.object(utils, 'spawn_n', + lambda f, *a, **kw: f(*a, **kw) and None) +@mock.patch.object(firewall, 'update_filters', autospec=True) +@mock.patch.object(node_cache, 'get_node', autospec=True) +@mock.patch.object(utils, 'get_client', autospec=True) +class TestAbort(BaseTest): + def setUp(self): + super(TestAbort, self).setUp() + self.node_info.started_at = None + self.node_info.finished_at = None + + def test_ok(self, client_mock, get_mock, filters_mock): + cli = self._prepare(client_mock) + get_mock.return_value = self.node_info + self.node_info.acquire_lock.return_value = True + self.node_info.started_at = time.time() + self.node_info.finished_at = None + + introspect.abort(self.node.uuid) + + get_mock.assert_called_once_with(self.uuid, ironic=cli, + locked=False) + self.node_info.acquire_lock.assert_called_once_with(blocking=False) + filters_mock.assert_called_once_with(cli) + cli.node.set_power_state.assert_called_once_with(self.uuid, 'off') + self.node_info.finished.assert_called_once_with(error='Canceled ' + 'by operator') + + def test_node_not_found(self, client_mock, get_mock, filters_mock): + cli = self._prepare(client_mock) + exc = utils.Error('Not found.', code=404) + get_mock.side_effect = iter([exc]) + + self.assertRaisesRegexp(utils.Error, str(exc), + introspect.abort, self.uuid) + + self.assertEqual(0, filters_mock.call_count) + self.assertEqual(0, cli.node.set_power_state.call_count) + self.assertEqual(0, self.node_info.finished.call_count) + + def test_node_locked(self, client_mock, get_mock, filters_mock): + cli = self._prepare(client_mock) + get_mock.return_value = self.node_info + self.node_info.acquire_lock.return_value = False + self.node_info.started_at = time.time() + + self.assertRaisesRegexp(utils.Error, 'Node is locked, please, ' + 'retry later', introspect.abort, self.uuid) + + self.assertEqual(0, filters_mock.call_count) + self.assertEqual(0, cli.node.set_power_state.call_count) + self.assertEqual(0, self.node_info.finshed.call_count) + + def test_introspection_already_finished(self, client_mock, + get_mock, filters_mock): + cli = self._prepare(client_mock) + get_mock.return_value = self.node_info + self.node_info.acquire_lock.return_value = True + self.node_info.started_at = time.time() + self.node_info.finished_at = time.time() + + introspect.abort(self.uuid) + + self.assertEqual(0, filters_mock.call_count) + self.assertEqual(0, cli.node.set_power_state.call_count) + self.assertEqual(0, self.node_info.finshed.call_count) + + def test_firewall_update_exception(self, client_mock, get_mock, + filters_mock): + cli = self._prepare(client_mock) + get_mock.return_value = self.node_info + self.node_info.acquire_lock.return_value = True + self.node_info.started_at = time.time() + self.node_info.finished_at = None + filters_mock.side_effect = iter([Exception('Boom')]) + + introspect.abort(self.uuid) + + get_mock.assert_called_once_with(self.uuid, ironic=cli, + locked=False) + self.node_info.acquire_lock.assert_called_once_with(blocking=False) + filters_mock.assert_called_once_with(cli) + cli.node.set_power_state.assert_called_once_with(self.uuid, 'off') + self.node_info.finished.assert_called_once_with(error='Canceled ' + 'by operator') + + def test_node_power_off_exception(self, client_mock, get_mock, + filters_mock): + cli = self._prepare(client_mock) + get_mock.return_value = self.node_info + self.node_info.acquire_lock.return_value = True + self.node_info.started_at = time.time() + self.node_info.finished_at = None + cli.node.set_power_state.side_effect = iter([Exception('BadaBoom')]) + + introspect.abort(self.uuid) + + get_mock.assert_called_once_with(self.uuid, ironic=cli, + locked=False) + self.node_info.acquire_lock.assert_called_once_with(blocking=False) + filters_mock.assert_called_once_with(cli) + cli.node.set_power_state.assert_called_once_with(self.uuid, 'off') + self.node_info.finished.assert_called_once_with(error='Canceled ' + 'by operator') diff --git a/ironic_inspector/test/test_main.py b/ironic_inspector/test/test_main.py index 9b9c924b0..a6728f0a5 100644 --- a/ironic_inspector/test/test_main.py +++ b/ironic_inspector/test/test_main.py @@ -135,6 +135,50 @@ class TestApiContinue(BaseAPITest): self.assertFalse(process_mock.called) +@mock.patch.object(introspect, 'abort', autospec=True) +class TestApiAbort(BaseAPITest): + def test_ok(self, abort_mock): + abort_mock.return_value = '', 202 + + res = self.app.post('/v1/introspection/%s/abort' % self.uuid, + headers={'X-Auth-Token': 'token'}) + + abort_mock.assert_called_once_with(self.uuid, token='token') + self.assertEqual(202, res.status_code) + self.assertEqual(b'', res.data) + + def test_no_authentication(self, abort_mock): + abort_mock.return_value = b'', 202 + + res = self.app.post('/v1/introspection/%s/abort' % self.uuid) + + abort_mock.assert_called_once_with(self.uuid, token=None) + self.assertEqual(202, res.status_code) + self.assertEqual(b'', res.data) + + def test_node_not_found(self, abort_mock): + exc = utils.Error("Not Found.", code=404) + abort_mock.side_effect = iter([exc]) + + res = self.app.post('/v1/introspection/%s/abort' % self.uuid) + + abort_mock.assert_called_once_with(self.uuid, token=None) + self.assertEqual(404, res.status_code) + data = json.loads(str(res.data.decode())) + self.assertEqual(str(exc), data['error']['message']) + + def test_abort_failed(self, abort_mock): + exc = utils.Error("Locked.", code=409) + abort_mock.side_effect = iter([exc]) + + res = self.app.post('/v1/introspection/%s/abort' % self.uuid) + + abort_mock.assert_called_once_with(self.uuid, token=None) + self.assertEqual(409, res.status_code) + data = json.loads(res.data.decode()) + self.assertEqual(str(exc), data['error']['message']) + + class TestApiGetStatus(BaseAPITest): @mock.patch.object(node_cache, 'get_node', autospec=True) def test_get_introspection_in_progress(self, get_mock): diff --git a/ironic_inspector/test/test_process.py b/ironic_inspector/test/test_process.py index 245a25547..163c65789 100644 --- a/ironic_inspector/test/test_process.py +++ b/ironic_inspector/test/test_process.py @@ -127,6 +127,18 @@ class TestProcess(BaseTest): self.assertFalse(process_mock.called) pop_mock.return_value.finished.assert_called_once_with(error=mock.ANY) + @prepare_mocks + def test_already_finished(self, cli, pop_mock, process_mock): + old_finished_at = pop_mock.return_value.finished_at + pop_mock.return_value.finished_at = time.time() + try: + self.assertRaisesRegexp(utils.Error, 'already finished', + process.process, self.data) + self.assertFalse(process_mock.called) + self.assertFalse(pop_mock.return_value.finished.called) + finally: + pop_mock.return_value.finished_at = old_finished_at + @prepare_mocks def test_expected_exception(self, cli, pop_mock, process_mock): process_mock.side_effect = iter([utils.Error('boom')]) diff --git a/releasenotes/notes/abort-introspection-ae5cb5a9fbacd2ac.yaml b/releasenotes/notes/abort-introspection-ae5cb5a9fbacd2ac.yaml new file mode 100644 index 000000000..8800269fa --- /dev/null +++ b/releasenotes/notes/abort-introspection-ae5cb5a9fbacd2ac.yaml @@ -0,0 +1,4 @@ +--- +features: + - Introduced API "POST /v1/introspection//abort" for aborting + the introspection process.