From 3bda561e318e0172d6209f1580340ed9a04a161d Mon Sep 17 00:00:00 2001 From: Galyna Zholtkevych Date: Fri, 28 Apr 2017 16:38:33 +0300 Subject: [PATCH] Raise HTTP 400 rather than 500 error Currently conductor method inspecting hardware raises HardwareInspectionFailure with 500 Internal Error if driver.power.validate fail (e.g. , ``driver_info`` is not provided or some fields are missing). Since this is an apparent client error, an HTTP error code 400-Bad request is more appropriate. The validation method actually raises this needed error and catching this is not needed anymore. Change-Id: I080dedeac7ce33135fde8c53494e618ccf07c941 Closes-Bug: #1686457 --- ironic/conductor/manager.py | 16 +++++------- ironic/tests/unit/api/v1/test_nodes.py | 26 +++++++++++++++++++ ironic/tests/unit/conductor/test_manager.py | 13 ++++++++-- ...g-inspection-failure-57d7fd2999cf4ecf.yaml | 4 +++ 4 files changed, 47 insertions(+), 12 deletions(-) create mode 100644 releasenotes/notes/raise-bad-request-exception-on-validating-inspection-failure-57d7fd2999cf4ecf.yaml diff --git a/ironic/conductor/manager.py b/ironic/conductor/manager.py index 8a6769e44c..a4749fe62b 100644 --- a/ironic/conductor/manager.py +++ b/ironic/conductor/manager.py @@ -2323,7 +2323,7 @@ class ConductorManager(base_manager.BaseConductorManager): @METRICS.timer('ConductorManager.inspect_hardware') @messaging.expected_exceptions(exception.NoFreeConductorWorker, exception.NodeLocked, - exception.HardwareInspectionFailure, + exception.InvalidParameterValue, exception.InvalidStateRequested, exception.UnsupportedDriverExtension) def inspect_hardware(self, context, node_id): @@ -2340,8 +2340,10 @@ class ConductorManager(base_manager.BaseConductorManager): support inspect. :raises: NoFreeConductorWorker when there is no free worker to start async task - :raises: HardwareInspectionFailure when unable to get + :raises: InvalidParameterValue when unable to get essential scheduling properties from hardware. + :raises: MissingParameterValue when required + information is not found. :raises: InvalidStateRequested if 'inspect' is not a valid action to do in the current state. @@ -2353,14 +2355,8 @@ class ConductorManager(base_manager.BaseConductorManager): raise exception.UnsupportedDriverExtension( driver=task.node.driver, extension='inspect') - try: - task.driver.power.validate(task) - task.driver.inspect.validate(task) - except exception.InvalidParameterValue as e: - error = (_("Failed to validate inspection or power info. " - "Error: %(msg)s") - % {'msg': e}) - raise exception.HardwareInspectionFailure(error=error) + task.driver.power.validate(task) + task.driver.inspect.validate(task) try: task.process_event( diff --git a/ironic/tests/unit/api/v1/test_nodes.py b/ironic/tests/unit/api/v1/test_nodes.py index f9ce20c900..252a2d1e41 100644 --- a/ironic/tests/unit/api/v1/test_nodes.py +++ b/ironic/tests/unit/api/v1/test_nodes.py @@ -2790,6 +2790,32 @@ class TestPut(test_api_base.BaseApiTest): expect_errors=True) self.assertEqual(http_client.CONFLICT, ret.status_code) # Conflict + def test_inspect_validation_failed_status_code(self): + self.mock_dnih.side_effect = exception.InvalidParameterValue( + err='Failed to validate inspection or power info.') + node = self.node + node.provision_state = states.MANAGEABLE + node.reservation = 'fake-host' + node.save() + ret = self.put_json('/nodes/%s/states/provision' % node.uuid, + {'target': 'inspect'}, + headers={api_base.Version.string: "1.6"}, + expect_errors=True) + self.assertEqual(http_client.BAD_REQUEST, ret.status_code) + + def test_inspect_validation_failed_missing_parameter_value(self): + self.mock_dnih.side_effect = exception.MissingParameterValue( + err='Failed to validate inspection or power info.') + node = self.node + node.provision_state = states.MANAGEABLE + node.reservation = 'fake-host' + node.save() + ret = self.put_json('/nodes/%s/states/provision' % node.uuid, + {'target': 'inspect'}, + headers={api_base.Version.string: "1.6"}, + expect_errors=True) + self.assertEqual(http_client.BAD_REQUEST, ret.status_code) + @mock.patch.object(rpcapi.ConductorAPI, 'do_provisioning_action') def test_manage_from_available(self, mock_dpa): self.node.provision_state = states.AVAILABLE diff --git a/ironic/tests/unit/conductor/test_manager.py b/ironic/tests/unit/conductor/test_manager.py index 78d11f07da..9dfd43c7be 100644 --- a/ironic/tests/unit/conductor/test_manager.py +++ b/ironic/tests/unit/conductor/test_manager.py @@ -5155,13 +5155,22 @@ class NodeInspectHardware(mgr_utils.ServiceSetUpMixin, self.assertIsNone(node.reservation) def _test_inspect_hardware_validate_fail(self, mock_validate): - mock_validate.side_effect = exception.InvalidParameterValue('error') + mock_validate.side_effect = exception.InvalidParameterValue( + 'Fake error message') node = obj_utils.create_test_node(self.context, driver='fake') exc = self.assertRaises(messaging.rpc.ExpectedException, self.service.inspect_hardware, self.context, node.uuid) # Compare true exception hidden by @messaging.expected_exceptions - self.assertEqual(exception.HardwareInspectionFailure, exc.exc_info[0]) + self.assertEqual(exception.InvalidParameterValue, exc.exc_info[0]) + + mock_validate.side_effect = exception.MissingParameterValue( + 'Fake error message') + exc = self.assertRaises(messaging.rpc.ExpectedException, + self.service.inspect_hardware, + self.context, node.uuid) + self.assertEqual(exception.MissingParameterValue, exc.exc_info[0]) + # This is a sync operation last_error should be None. self.assertIsNone(node.last_error) # Verify reservation has been cleared. diff --git a/releasenotes/notes/raise-bad-request-exception-on-validating-inspection-failure-57d7fd2999cf4ecf.yaml b/releasenotes/notes/raise-bad-request-exception-on-validating-inspection-failure-57d7fd2999cf4ecf.yaml new file mode 100644 index 0000000000..265045dc89 --- /dev/null +++ b/releasenotes/notes/raise-bad-request-exception-on-validating-inspection-failure-57d7fd2999cf4ecf.yaml @@ -0,0 +1,4 @@ +--- +fixes: + - Raise HTTP 400 Bad request on failure to validate + power or inspect interface parameters before inspecting. \ No newline at end of file