Merge "Expose instance action event details out of the API"

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"
     }

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"
         }

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

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,
+    @staticmethod
-                      show_host=False, show_hostid=False):
+    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}

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``.

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,

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

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'])

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)

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)

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):

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": "",

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,

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 = (

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``.