diff --git a/doc/api_samples/versions/v21-version-get-resp.json b/doc/api_samples/versions/v21-version-get-resp.json index a17f60f5ea4a..5e5c77561fbc 100644 --- a/doc/api_samples/versions/v21-version-get-resp.json +++ b/doc/api_samples/versions/v21-version-get-resp.json @@ -19,7 +19,7 @@ } ], "status": "CURRENT", - "version": "2.83", + "version": "2.84", "min_version": "2.1", "updated": "2013-07-23T11:33:21Z" } diff --git a/doc/api_samples/versions/versions-get-resp.json b/doc/api_samples/versions/versions-get-resp.json index 4d05fabc5734..8eb238e31930 100644 --- a/doc/api_samples/versions/versions-get-resp.json +++ b/doc/api_samples/versions/versions-get-resp.json @@ -22,7 +22,7 @@ } ], "status": "CURRENT", - "version": "2.83", + "version": "2.84", "min_version": "2.1", "updated": "2013-07-23T11:33:21Z" } diff --git a/nova/api/openstack/api_version_request.py b/nova/api/openstack/api_version_request.py index a20ef3c13872..3d2f0549ee88 100644 --- a/nova/api/openstack/api_version_request.py +++ b/nova/api/openstack/api_version_request.py @@ -226,6 +226,7 @@ REST_API_VERSION_HISTORY = """REST API Version History: from Cyborg with ``GET /v2/accelerator_requests?instance={uuid}`` * 2.83 - Allow more filter parameters for ``GET /servers/detail`` and ``GET /servers`` for non-admin. + * 2.84 - Adds ``details`` field to instance action events. """ # The minimum and maximum versions of the API supported @@ -234,7 +235,7 @@ REST_API_VERSION_HISTORY = """REST API Version History: # Note(cyeoh): This only applies for the v2.1 API once microversions # support is fully merged. It does not affect the V2 API. _MIN_API_VERSION = "2.1" -_MAX_API_VERSION = "2.83" +_MAX_API_VERSION = "2.84" DEFAULT_API_VERSION = _MIN_API_VERSION # Almost all proxy APIs which are related to network, images and baremetal diff --git a/nova/api/openstack/compute/instance_actions.py b/nova/api/openstack/compute/instance_actions.py index be9f571e4c62..d0b58fff6f46 100644 --- a/nova/api/openstack/compute/instance_actions.py +++ b/nova/api/openstack/compute/instance_actions.py @@ -53,8 +53,9 @@ class InstanceActionsController(wsgi.Controller): action[key] = action_raw.get(key) return action - def _format_event(self, event_raw, project_id, show_traceback=False, - show_host=False, show_hostid=False): + @staticmethod + def _format_event(event_raw, project_id, show_traceback=False, + show_host=False, show_hostid=False, show_details=False): event = {} for key in EVENT_KEYS: # By default, non-admins are not allowed to see traceback details. @@ -67,6 +68,8 @@ class InstanceActionsController(wsgi.Controller): if show_hostid: event['hostId'] = utils.generate_hostid(event_raw['host'], project_id) + if show_details: + event['details'] = event_raw['details'] return event @wsgi.Controller.api_version("2.1", "2.20") @@ -178,6 +181,15 @@ class InstanceActionsController(wsgi.Controller): show_hostid = api_version_request.is_supported(req, '2.62') if show_events: + # NOTE(brinzhang): Event details are shown since microversion + # 2.84. + show_details = False + support_v284 = api_version_request.is_supported(req, '2.84') + if support_v284: + show_details = context.can( + ia_policies.BASE_POLICY_NAME % 'events:details', + target={'project_id': instance.project_id}, fatal=False) + events_raw = self.action_api.action_events_get(context, instance, action_id) # NOTE(takashin): The project IDs of instance action events @@ -189,6 +201,7 @@ class InstanceActionsController(wsgi.Controller): action['events'] = [self._format_event( evt, action['project_id'] or instance.project_id, show_traceback=show_traceback, - show_host=show_host, show_hostid=show_hostid + show_host=show_host, show_hostid=show_hostid, + show_details=show_details ) for evt in events_raw] return {'instanceAction': action} diff --git a/nova/api/openstack/compute/rest_api_version_history.rst b/nova/api/openstack/compute/rest_api_version_history.rst index 817106efb4c3..28c567e652c2 100644 --- a/nova/api/openstack/compute/rest_api_version_history.rst +++ b/nova/api/openstack/compute/rest_api_version_history.rst @@ -1105,3 +1105,11 @@ and ``GET /servers`` for non-admin : * ``vm_state`` * ``progress`` * ``user_id`` + +2.84 +---- + +The ``GET /servers/{server_id}/os-instance-actions/{request_id}`` API returns +a ``details`` parameter for each failed event with a fault message, similar to +the server ``fault.message`` parameter in ``GET /servers/{server_id}`` for a +server with status ``ERROR``. diff --git a/nova/policies/instance_actions.py b/nova/policies/instance_actions.py index 5880e138bbe0..04dc9753e7fc 100644 --- a/nova/policies/instance_actions.py +++ b/nova/policies/instance_actions.py @@ -33,6 +33,27 @@ in nova 23.0.0 release. """ instance_actions_policies = [ + policy.DocumentedRuleDefault( + name=BASE_POLICY_NAME % 'events:details', + check_str=base.SYSTEM_READER, + description="""Add "details" key in action events for a server. + +This check is performed only after the check +os_compute_api:os-instance-actions:show passes. Beginning with Microversion +2.84, new field 'details' is exposed via API which can have more details about +event failure. That field is controlled by this policy which is system reader +by default. Making the 'details' field visible to the non-admin user helps to +understand the nature of the problem (i.e. if the action can be retried), +but in the other hand it might leak information about the deployment +(e.g. the type of the hypervisor). +""", + operations=[ + { + 'method': 'GET', + 'path': '/servers/{server_id}/os-instance-actions/{request_id}' + } + ], + scope_types=['system']), policy.DocumentedRuleDefault( name=BASE_POLICY_NAME % 'events', check_str=base.SYSTEM_READER, diff --git a/nova/tests/functional/integrated_helpers.py b/nova/tests/functional/integrated_helpers.py index cb56e9e2690e..0782dab202cf 100644 --- a/nova/tests/functional/integrated_helpers.py +++ b/nova/tests/functional/integrated_helpers.py @@ -162,10 +162,12 @@ class InstanceHelperMixin(object): :param server: API response dict of the server being resized/migrated :param action: Either "resize" or "migrate" instance action. :param error_in_tb: Some expected part of the error event traceback. + :returns: The instance action event dict from the API response """ event = self._wait_for_action_fail_completion( server, action, 'conductor_migrate_server') self.assertIn(error_in_tb, event['traceback']) + return event def _wait_for_migration_status(self, server, expected_statuses): """Waits for a migration record with the given statuses to be found diff --git a/nova/tests/functional/test_instance_actions.py b/nova/tests/functional/test_instance_actions.py index 34d9f5cc4535..9236fa3160ef 100644 --- a/nova/tests/functional/test_instance_actions.py +++ b/nova/tests/functional/test_instance_actions.py @@ -12,8 +12,19 @@ # License for the specific language governing permissions and limitations # under the License. +import mock +from oslo_policy import policy as oslo_policy + +from nova import exception +from nova import policy +from nova import test +from nova.tests import fixtures as nova_fixtures from nova.tests.functional.api import client +from nova.tests.functional import fixtures as func_fixtures +from nova.tests.functional import integrated_helpers from nova.tests.functional import test_servers +from nova.tests.unit.image import fake as fake_image +from nova.tests.unit import policy_fixture class InstanceActionsTestV2(test_servers.ServersTestBase): @@ -49,3 +60,214 @@ class InstanceActionsTestV221(InstanceActionsTestV21): actions = self.api.get_instance_actions(server['id']) self.assertEqual('delete', actions[0]['action']) self.assertEqual('create', actions[1]['action']) + + +class HypervisorError(Exception): + """This is just used to make sure the exception type is in the events.""" + pass + + +class InstanceActionEventFaultsTestCase( + test.TestCase, integrated_helpers.InstanceHelperMixin): + """Tests for the instance action event details reporting from the API""" + + def setUp(self): + super(InstanceActionEventFaultsTestCase, self).setUp() + # Setup the standard fixtures. + fake_image.stub_out_image_service(self) + self.addCleanup(fake_image.FakeImageService_reset) + self.useFixture(nova_fixtures.NeutronFixture(self)) + self.useFixture(func_fixtures.PlacementFixture()) + self.useFixture(policy_fixture.RealPolicyFixture()) + + # Start the compute services. + self.start_service('conductor') + self.start_service('scheduler') + self.compute = self.start_service('compute') + api_fixture = self.useFixture(nova_fixtures.OSAPIFixture( + api_version='v2.1')) + self.api = api_fixture.api + self.admin_api = api_fixture.admin_api + + def _set_policy_rules(self, overwrite=True): + rules = {'os_compute_api:os-instance-actions:show': '', + 'os_compute_api:os-instance-actions:events:details': + 'project_id:%(project_id)s'} + policy.set_rules(oslo_policy.Rules.from_dict(rules), + overwrite=overwrite) + + def test_instance_action_event_details_non_nova_exception(self): + """Creates a server using the non-admin user, then reboot it which + will generate a non-NovaException fault and put the instance into + ERROR status. Then checks that fault details are visible. + """ + + # Create the server with the non-admin user. + server = self._build_server( + networks=[{'port': nova_fixtures.NeutronFixture.port_1['id']}]) + server = self.api.post_server({'server': server}) + server = self._wait_for_state_change(server, 'ACTIVE') + + # Stop the server before rebooting it so that after the driver.reboot + # method raises an exception, the fake driver does not report the + # instance power state as running - that will make the compute manager + # set the instance vm_state to error. + self.api.post_server_action(server['id'], {'os-stop': None}) + server = self._wait_for_state_change(server, 'SHUTOFF') + + # Stub out the compute driver reboot method to raise a non-nova + # exception to simulate some error from the underlying hypervisor + # which in this case we are going to say has sensitive content. + error_msg = 'sensitive info' + with mock.patch.object( + self.compute.manager.driver, 'reboot', + side_effect=HypervisorError(error_msg)) as mock_reboot: + reboot_request = {'reboot': {'type': 'HARD'}} + self.api.post_server_action(server['id'], reboot_request) + # In this case we wait for the status to change to ERROR using + # the non-admin user so we can assert the fault details. We also + # wait for the task_state to be None since the wrap_instance_fault + # decorator runs before the reverts_task_state decorator so we will + # be sure the fault is set on the server. + server = self._wait_for_server_parameter( + server, {'status': 'ERROR', 'OS-EXT-STS:task_state': None}, + api=self.api) + mock_reboot.assert_called_once() + + self._set_policy_rules(overwrite=False) + + server_id = server['id'] + # Calls GET on the server actions and verifies that the reboot + # action expected in the response. + response = self.api.api_get('/servers/%s/os-instance-actions' % + server_id) + server_actions = response.body['instanceActions'] + for actions in server_actions: + if actions['action'] == 'reboot': + reboot_request_id = actions['request_id'] + # non admin shows instance actions details and verifies the 'details' + # in the action events via 'request_id', since microversion 2.51 that + # we can show events, but in microversion 2.84 that we can show + # 'details' for non-admin. + self.api.microversion = '2.84' + action_events_response = self.api.api_get( + '/servers/%s/os-instance-actions/%s' % (server_id, + reboot_request_id)) + reboot_action = action_events_response.body['instanceAction'] + # Since reboot action failed, the 'message' property in reboot action + # should be 'Error', otherwise it's None. + self.assertEqual('Error', reboot_action['message']) + reboot_action_events = reboot_action['events'] + # The instance action events from the non-admin user API response + # should not have 'traceback' in it. + self.assertNotIn('traceback', reboot_action_events[0]) + # And the sensitive details from the non-nova exception should not be + # in the details. + self.assertIn('details', reboot_action_events[0]) + self.assertNotIn(error_msg, reboot_action_events[0]['details']) + # The exception type class name should be in the details. + self.assertIn('HypervisorError', reboot_action_events[0]['details']) + + # Get the server fault details for the admin user. + self.admin_api.microversion = '2.84' + action_events_response = self.admin_api.api_get( + '/servers/%s/os-instance-actions/%s' % (server_id, + reboot_request_id)) + reboot_action = action_events_response.body['instanceAction'] + self.assertEqual('Error', reboot_action['message']) + reboot_action_events = reboot_action['events'] + # The admin can see the fault details which includes the traceback, + # and make sure the traceback is there by looking for part of it. + self.assertIn('traceback', reboot_action_events[0]) + self.assertIn('in reboot_instance', + reboot_action_events[0]['traceback']) + # The exception type class name should be in the details for the admin + # user as well since the fault handling code cannot distinguish who + # is going to see the message so it only sets class name. + self.assertIn('HypervisorError', reboot_action_events[0]['details']) + + def test_instance_action_event_details_with_nova_exception(self): + """Creates a server using the non-admin user, then reboot it which + will generate a nova exception fault and put the instance into + ERROR status. Then checks that fault details are visible. + """ + + # Create the server with the non-admin user. + server = self._build_server( + networks=[{'port': nova_fixtures.NeutronFixture.port_1['id']}]) + server = self.api.post_server({'server': server}) + server = self._wait_for_state_change(server, 'ACTIVE') + + # Stop the server before rebooting it so that after the driver.reboot + # method raises an exception, the fake driver does not report the + # instance power state as running - that will make the compute manager + # set the instance vm_state to error. + self.api.post_server_action(server['id'], {'os-stop': None}) + server = self._wait_for_state_change(server, 'SHUTOFF') + + # Stub out the compute driver reboot method to raise a nova + # exception 'InstanceRebootFailure' to simulate some error. + exc_reason = 'reboot failure' + with mock.patch.object( + self.compute.manager.driver, 'reboot', + side_effect=exception.InstanceRebootFailure(reason=exc_reason) + ) as mock_reboot: + reboot_request = {'reboot': {'type': 'HARD'}} + self.api.post_server_action(server['id'], reboot_request) + # In this case we wait for the status to change to ERROR using + # the non-admin user so we can assert the fault details. We also + # wait for the task_state to be None since the wrap_instance_fault + # decorator runs before the reverts_task_state decorator so we will + # be sure the fault is set on the server. + server = self._wait_for_server_parameter( + server, {'status': 'ERROR', 'OS-EXT-STS:task_state': None}, + api=self.api) + mock_reboot.assert_called_once() + + self._set_policy_rules(overwrite=False) + + server_id = server['id'] + # Calls GET on the server actions and verifies that the reboot + # action expected in the response. + response = self.api.api_get('/servers/%s/os-instance-actions' % + server_id) + server_actions = response.body['instanceActions'] + for actions in server_actions: + if actions['action'] == 'reboot': + reboot_request_id = actions['request_id'] + + # non admin shows instance actions details and verifies the 'details' + # in the action events via 'request_id', since microversion 2.51 that + # we can show events, but in microversion 2.84 that we can show + # 'details' for non-admin. + self.api.microversion = '2.84' + action_events_response = self.api.api_get( + '/servers/%s/os-instance-actions/%s' % (server_id, + reboot_request_id)) + reboot_action = action_events_response.body['instanceAction'] + # Since reboot action failed, the 'message' property in reboot action + # should be 'Error', otherwise it's None. + self.assertEqual('Error', reboot_action['message']) + reboot_action_events = reboot_action['events'] + # The instance action events from the non-admin user API response + # should not have 'traceback' in it. + self.assertNotIn('traceback', reboot_action_events[0]) + # The nova exception format message should be in the details. + self.assertIn('details', reboot_action_events[0]) + self.assertIn(exc_reason, reboot_action_events[0]['details']) + + # Get the server fault details for the admin user. + self.admin_api.microversion = '2.84' + action_events_response = self.admin_api.api_get( + '/servers/%s/os-instance-actions/%s' % (server_id, + reboot_request_id)) + reboot_action = action_events_response.body['instanceAction'] + self.assertEqual('Error', reboot_action['message']) + reboot_action_events = reboot_action['events'] + # The admin can see the fault details which includes the traceback, + # and make sure the traceback is there by looking for part of it. + self.assertIn('traceback', reboot_action_events[0]) + self.assertIn('in reboot_instance', + reboot_action_events[0]['traceback']) + # The nova exception format message should be in the details. + self.assertIn(exc_reason, reboot_action_events[0]['details']) diff --git a/nova/tests/functional/test_servers.py b/nova/tests/functional/test_servers.py index c35232f66b65..ab3fc2cf7246 100644 --- a/nova/tests/functional/test_servers.py +++ b/nova/tests/functional/test_servers.py @@ -1931,10 +1931,16 @@ class ServerMovingTests(integrated_helpers.ProviderUsageBaseTestCase): self.api.post_server_action( server['id'], resize_req, check_response_status=[202]) - self._assert_resize_migrate_action_fail( + event = self._assert_resize_migrate_action_fail( server, instance_actions.RESIZE, 'NoValidHost') + self.assertIn('details', event) + # This test case works in microversion 2.84. + self.assertIn('No valid host was found', event['details']) server = self.admin_api.get_server(server['id']) self.assertEqual(source_hostname, server['OS-EXT-SRV-ATTR:host']) + # The server is still ACTIVE and thus there is no fault message. + self.assertEqual('ACTIVE', server['status']) + self.assertNotIn('fault', server) # only the source host shall have usages after the failed resize self.assertFlavorMatchesUsage(source_rp_uuid, self.flavor1) diff --git a/nova/tests/unit/api/openstack/compute/test_instance_actions.py b/nova/tests/unit/api/openstack/compute/test_instance_actions.py index 1dcabdf61be9..9a1423369239 100644 --- a/nova/tests/unit/api/openstack/compute/test_instance_actions.py +++ b/nova/tests/unit/api/openstack/compute/test_instance_actions.py @@ -415,3 +415,47 @@ class InstanceActionsTestV266(InstanceActionsTestV258): self.controller.index, req) detail = 'Additional properties are not allowed' self.assertIn(detail, six.text_type(ex)) + + +class InstanceActionsTestV284(InstanceActionsTestV266): + wsgi_api_version = "2.84" + + def _set_policy_rules(self, overwrite=True): + rules = {'os_compute_api:os-instance-actions:show': '', + 'os_compute_api:os-instance-actions:events:details': + 'project_id:%(project_id)s'} + policy.set_rules(oslo_policy.Rules.from_dict(rules), + overwrite=overwrite) + + def test_show_action_with_details(self): + def fake_get_action(context, uuid, request_id): + return self.fake_actions[uuid][request_id] + + def fake_get_events(context, action_id): + return self.fake_events[action_id] + + self.stub_out('nova.db.api.action_get_by_request_id', fake_get_action) + self.stub_out('nova.db.api.action_events_get', fake_get_events) + + self._set_policy_rules(overwrite=False) + req = self._get_http_req('os-instance-actions/1') + res_dict = self.controller.show(req, FAKE_UUID, FAKE_REQUEST_ID) + for event in res_dict['instanceAction']['events']: + self.assertIn('details', event) + + def test_show_action_with_details_old_microversion(self): + """Before microversion 2.84, we cannot get the details in events.""" + def fake_get_action(context, uuid, request_id): + return self.fake_actions[uuid][request_id] + + def fake_get_events(context, action_id): + return self.fake_events[action_id] + + self.stub_out('nova.db.api.action_get_by_request_id', fake_get_action) + self.stub_out('nova.db.api.action_events_get', fake_get_events) + + req = self._get_http_req_with_version('os-instance-actions/1', + version="2.83") + res_dict = self.controller.show(req, FAKE_UUID, FAKE_REQUEST_ID) + for event in res_dict['instanceAction']['events']: + self.assertNotIn('details', event) diff --git a/nova/tests/unit/cmd/test_policy.py b/nova/tests/unit/cmd/test_policy.py index 8b51fe81d0d3..81c19624e518 100644 --- a/nova/tests/unit/cmd/test_policy.py +++ b/nova/tests/unit/cmd/test_policy.py @@ -122,6 +122,8 @@ class TestPolicyCheck(test.NoDBTestCase): context, 'os-instance-actions:show', target) passing_rules += self.cmd._filter_rules( context, 'os-instance-actions:events', target) + passing_rules += self.cmd._filter_rules( + context, 'os-instance-actions:events:details', target) self.assertEqual(set(expected_rules), set(passing_rules)) def test_filter_rules_non_admin(self): diff --git a/nova/tests/unit/fake_policy.py b/nova/tests/unit/fake_policy.py index c86ba7b3ab85..d16c7499c07b 100644 --- a/nova/tests/unit/fake_policy.py +++ b/nova/tests/unit/fake_policy.py @@ -52,6 +52,7 @@ policy_data = """ "os_compute_api:os-instance-actions:list": "", "os_compute_api:os-instance-actions:show": "", "os_compute_api:os-instance-actions:events": "", + "os_compute_api:os-instance-actions:events:details": "", "os_compute_api:os-instance-usage-audit-log": "", "os_compute_api:os-lock-server:lock": "", diff --git a/nova/tests/unit/policies/test_instance_actions.py b/nova/tests/unit/policies/test_instance_actions.py index c2c8e8520151..5102aa15782b 100644 --- a/nova/tests/unit/policies/test_instance_actions.py +++ b/nova/tests/unit/policies/test_instance_actions.py @@ -14,6 +14,7 @@ import copy import fixtures import mock +from nova.api.openstack import api_version_request from oslo_policy import policy as oslo_policy from oslo_utils.fixture import uuidsentinel as uuids from oslo_utils import timeutils @@ -232,6 +233,50 @@ class InstanceActionsScopeTypePolicyTest(InstanceActionsPolicyTest): self.project_foo_context, self.project_reader_context ] + @mock.patch('nova.objects.InstanceActionEventList.get_by_action') + @mock.patch('nova.objects.InstanceAction.get_by_request_id') + def test_show_instance_action_policy_with_show_details( + self, mock_get_action, mock_get_events): + """Test to ensure skip checking policy rule + 'os_compute_api:os-instance-actions:show'. + """ + self.req.api_version_request = api_version_request.APIVersionRequest( + '2.84') + fake_action = self.fake_actions[FAKE_UUID][FAKE_REQUEST_ID] + mock_get_action.return_value = fake_action + fake_events = self.fake_events[fake_action['id']] + fake_action['events'] = fake_events + mock_get_events.return_value = fake_events + fake_action_fmt = test_instance_actions.format_action( + copy.deepcopy(fake_action)) + + self._set_policy_rules(overwrite=False) + rule_name = ia_policies.BASE_POLICY_NAME % "events:details" + authorize_res, unauthorize_res = self.common_policy_check( + self.system_reader_authorized_contexts, + self.system_reader_unauthorized_contexts, + rule_name, self.controller.show, + self.req, self.instance['uuid'], + fake_action['request_id'], fatal=False) + + for action in authorize_res: + # Ensure the 'details' field in the action events + for event in action['instanceAction']['events']: + self.assertIn('details', event) + # In order to unify the display forms of 'start_time' and + # 'finish_time', format the results returned by the show api. + res_fmt = test_instance_actions.format_action( + action['instanceAction']) + self.assertEqual(fake_action_fmt['events'], res_fmt['events']) + + # Because of the microversion > '2.51', that will be contain + # 'events' in the os-instance-actions show api response, but the + # 'details' should not contain in the action events. + for action in unauthorize_res: + # Ensure the 'details' field not in the action events + for event in action['instanceAction']['events']: + self.assertNotIn('details', event) + class InstanceActionsNoLegacyPolicyTest(InstanceActionsPolicyTest): """Test os-instance-actions APIs policies with system scope enabled, diff --git a/nova/tests/unit/test_policy.py b/nova/tests/unit/test_policy.py index 32cbb4014d1f..2c2ddbc1e484 100644 --- a/nova/tests/unit/test_policy.py +++ b/nova/tests/unit/test_policy.py @@ -456,6 +456,7 @@ class RealRolePolicyTestCase(test.NoDBTestCase): self.system_reader_rules = ( "os_compute_api:os-services:list", +"os_compute_api:os-instance-actions:events:details", ) self.system_reader_or_owner_rules = ( diff --git a/releasenotes/notes/bp-action-event-fault-details-8bfabc6e7390446a.yaml b/releasenotes/notes/bp-action-event-fault-details-8bfabc6e7390446a.yaml new file mode 100644 index 000000000000..9f1faba02930 --- /dev/null +++ b/releasenotes/notes/bp-action-event-fault-details-8bfabc6e7390446a.yaml @@ -0,0 +1,8 @@ +--- +features: + - | + With microversion 2.84 the + ``GET /servers/{server_id}/os-instance-actions/{request_id}`` API returns + a ``details`` parameter for each failed event with a fault message, similar + to the server ``fault.message`` parameter in ``GET /servers/{server_id}`` + for a server with status ``ERROR``.