Merge "Adding traceback to async faults"

5 files changed, 157 insertions, 91 deletions

diff --git a/nova/api/openstack/compute/views/servers.py b/nova/api/openstack/compute/views/servers.py
index f3fbee11f..d20cd26d3 100644
--- a/nova/api/openstack/compute/views/servers.py
+++ b/nova/api/openstack/compute/views/servers.py
@@ -187,9 +187,19 @@ class ViewBuilder(common.ViewBuilder):
         if not fault:
             return None
 
-        return {
+        fault_dict = {
             "code": fault["code"],
             "created": utils.isotime(fault["created_at"]),
             "message": fault["message"],
-            "details": fault["details"],
         }
+
+        if fault.get('details', None):
+            is_admin = False
+            context = getattr(request, 'context', None)
+            if context:
+                is_admin = getattr(request.context, 'is_admin', False)
+
+            if is_admin or fault['code'] != 500:
+                fault_dict['details'] = fault["details"]
+
+        return fault_dict
diff --git a/nova/compute/manager.py b/nova/compute/manager.py
index 861444e1a..74f9af3f9 100644
--- a/nova/compute/manager.py
+++ b/nova/compute/manager.py
@@ -40,6 +40,7 @@ import socket
 import sys
 import tempfile
 import time
+import traceback
 
 from eventlet import greenthread
 
@@ -170,7 +171,8 @@ def wrap_instance_fault(function):
             raise
         except Exception, e:
             with utils.save_and_reraise_exception():
-                self.add_instance_fault_from_exc(context, instance_uuid, e)
+                self.add_instance_fault_from_exc(context, instance_uuid, e,
+                                                 sys.exc_info())
 
     return decorated_function
 
@@ -441,7 +443,8 @@ class ComputeManager(manager.SchedulerDependentManager):
     def _check_instance_not_already_created(self, context, instance):
         """Ensure an instance with the same name is not already present."""
         if self.driver.instance_exists(instance['name']):
-            raise exception.Error(_("Instance has already been created"))
+            _msg = _("Instance has already been created")
+            raise exception.Invalid(_msg)
 
     def _check_image_size(self, context, instance):
         """Ensure image is smaller than the maximum size allowed by the
@@ -638,8 +641,9 @@ class ComputeManager(manager.SchedulerDependentManager):
 
         if current_power_state == power_state.SHUTOFF:
             self.db.instance_destroy(context, instance_id)
-            raise exception.Error(_('trying to destroy already destroyed'
-                                    ' instance: %s') % instance_uuid)
+            _msg = _('trying to destroy already destroyed instance: %s')
+            raise exception.Invalid(_msg % instance_uuid)
+
         # NOTE(vish) get bdms before destroying the instance
         bdms = self._get_instance_volume_bdms(context, instance_id)
         block_device_info = self._get_instance_volume_block_device_info(
@@ -981,9 +985,9 @@ class ComputeManager(manager.SchedulerDependentManager):
 
             if current_power_state != expected_state:
                 self._instance_update(context, instance_id, task_state=None)
-                raise exception.Error(_('Failed to set admin password. '
-                                      'Instance %s is not running') %
-                                      instance_ref["uuid"])
+                _msg = _('Failed to set admin password. Instance %s is not'
+                         ' running') % instance_ref["uuid"]
+                raise exception.Invalid(_msg)
             else:
                 try:
                     self.driver.set_admin_password(instance_ref, new_pass)
@@ -1011,7 +1015,7 @@ class ComputeManager(manager.SchedulerDependentManager):
                         # potentially reveal password information to the
                         # API caller.  The real exception is logged above
                         _msg = _('Error setting admin password')
-                        raise exception.Error(_msg)
+                        raise exception.NovaException(_msg)
                     time.sleep(1)
                     continue
 
@@ -2192,18 +2196,24 @@ class ComputeManager(manager.SchedulerDependentManager):
         """
         self.driver.update_available_resource(context, self.host)
 
-    def add_instance_fault_from_exc(self, context, instance_uuid, fault):
+    def add_instance_fault_from_exc(self, context, instance_uuid, fault,
+                                    exc_info=None):
         """Adds the specified fault to the database."""
-        if hasattr(fault, "code"):
-            code = fault.code
-        else:
-            code = 500
+
+        code = 500
+        if hasattr(fault, "kwargs"):
+            code = fault.kwargs.get('code', 500)
+
+        details = unicode(fault)
+        if exc_info and code == 500:
+            tb = exc_info[2]
+            details += '\n' + ''.join(traceback.format_tb(tb))
 
         values = {
             'instance_uuid': instance_uuid,
             'code': code,
             'message': fault.__class__.__name__,
-            'details': unicode(fault),
+            'details': unicode(details),
         }
         self.db.instance_fault_create(context, values)
 
diff --git a/nova/exception.py b/nova/exception.py
index dc8f7e19d..4f7cc5f33 100644
--- a/nova/exception.py
+++ b/nova/exception.py
@@ -154,6 +154,13 @@ class NovaException(Exception):
 
     def __init__(self, message=None, **kwargs):
         self.kwargs = kwargs
+
+        if 'code' not in self.kwargs:
+            try:
+                self.kwargs['code'] = self.code
+            except AttributeError:
+                pass
+
         if not message:
             try:
                 message = self.message % kwargs
@@ -192,6 +199,7 @@ class MelangeConnectionFailed(NovaException):
 
 class NotAuthorized(NovaException):
     message = _("Not authorized.")
+    code = 401
 
 
 class AdminRequired(NotAuthorized):
@@ -204,6 +212,7 @@ class PolicyNotAuthorized(NotAuthorized):
 
 class Invalid(NovaException):
     message = _("Unacceptable parameters.")
+    code = 400
 
 
 class InvalidSnapshot(Invalid):
@@ -375,11 +384,11 @@ class InvalidDiskFormat(Invalid):
 
 
 class ImageUnacceptable(Invalid):
-    message = _("Image %(image_id)s is unacceptable") + ": %(reason)s"
+    message = _("Image %(image_id)s is unacceptable: %(reason)s")
 
 
 class InstanceUnacceptable(Invalid):
-    message = _("Instance %(instance_id)s is unacceptable") + ": %(reason)s"
+    message = _("Instance %(instance_id)s is unacceptable: %(reason)s")
 
 
 class InvalidEc2Id(Invalid):
@@ -388,6 +397,7 @@ class InvalidEc2Id(Invalid):
 
 class NotFound(NovaException):
     message = _("Resource could not be found.")
+    code = 404
 
 
 class FlagNotSet(NotFound):
@@ -668,12 +678,12 @@ class SecurityGroupNotFoundForRule(SecurityGroupNotFound):
 
 class SecurityGroupExistsForInstance(Invalid):
     message = _("Security group %(security_group_id)s is already associated"
-                 " with the instance %(instance_id)s")
+                " with the instance %(instance_id)s")
 
 
 class SecurityGroupNotExistsForInstance(Invalid):
     message = _("Security group %(security_group_id)s is not associated with"
-                 " the instance %(instance_id)s")
+                " the instance %(instance_id)s")
 
 
 class MigrationNotFound(NotFound):
diff --git a/nova/tests/api/openstack/compute/test_servers.py b/nova/tests/api/openstack/compute/test_servers.py
index 281ba88e6..97add65b4 100644
--- a/nova/tests/api/openstack/compute/test_servers.py
+++ b/nova/tests/api/openstack/compute/test_servers.py
@@ -2868,9 +2868,65 @@ class ServersViewBuilderTest(test.TestCase):
             }
         }
 
+        self.request.context = nova.context.RequestContext('fake', 'fake')
         output = self.view_builder.show(self.request, self.instance)
         self.assertDictMatch(output, expected_server)
 
+    def test_build_server_detail_with_fault_no_details_not_admin(self):
+        self.instance['vm_state'] = vm_states.ERROR
+        self.instance['fault'] = {
+            'code': 500,
+            'instance_uuid': self.uuid,
+            'message': "Error",
+            'details': 'Stock details for test',
+            'created_at': datetime.datetime(2010, 10, 10, 12, 0, 0),
+        }
+
+        expected_fault = {"code": 500,
+                          "created": "2010-10-10T12:00:00Z",
+                          "message": "Error"}
+
+        self.request.context = nova.context.RequestContext('fake', 'fake')
+        output = self.view_builder.show(self.request, self.instance)
+        self.assertDictMatch(output['server']['fault'], expected_fault)
+
+    def test_build_server_detail_with_fault_admin(self):
+        self.instance['vm_state'] = vm_states.ERROR
+        self.instance['fault'] = {
+            'code': 500,
+            'instance_uuid': self.uuid,
+            'message': "Error",
+            'details': 'Stock details for test',
+            'created_at': datetime.datetime(2010, 10, 10, 12, 0, 0),
+        }
+
+        expected_fault = {"code": 500,
+                          "created": "2010-10-10T12:00:00Z",
+                          "message": "Error",
+                          'details': 'Stock details for test'}
+
+        self.request.context = nova.context.get_admin_context()
+        output = self.view_builder.show(self.request, self.instance)
+        self.assertDictMatch(output['server']['fault'], expected_fault)
+
+    def test_build_server_detail_with_fault_no_details_admin(self):
+        self.instance['vm_state'] = vm_states.ERROR
+        self.instance['fault'] = {
+            'code': 500,
+            'instance_uuid': self.uuid,
+            'message': "Error",
+            'details': '',
+            'created_at': datetime.datetime(2010, 10, 10, 12, 0, 0),
+        }
+
+        expected_fault = {"code": 500,
+                          "created": "2010-10-10T12:00:00Z",
+                          "message": "Error"}
+
+        self.request.context = nova.context.get_admin_context()
+        output = self.view_builder.show(self.request, self.instance)
+        self.assertDictMatch(output['server']['fault'], expected_fault)
+
     def test_build_server_detail_with_fault_but_active(self):
         self.instance['vm_state'] = vm_states.ACTIVE
         self.instance['progress'] = 100
@@ -2881,66 +2937,8 @@ class ServersViewBuilderTest(test.TestCase):
             'details': "Stock details for test",
             'created_at': datetime.datetime(2010, 10, 10, 12, 0, 0),
         }
-
-        image_bookmark = "http://localhost/fake/images/5"
-        flavor_bookmark = "http://localhost/fake/flavors/1"
-        self_link = "http://localhost/v2/fake/servers/%s" % self.uuid
-        bookmark_link = "http://localhost/fake/servers/%s" % self.uuid
-        expected_server = {
-            "server": {
-                "id": self.uuid,
-                "user_id": "fake",
-                "tenant_id": "fake",
-                "updated": "2010-11-11T11:00:00Z",
-                "created": "2010-10-10T12:00:00Z",
-                "progress": 100,
-                "name": "test_server",
-                "status": "ACTIVE",
-                "accessIPv4": "",
-                "accessIPv6": "",
-                "hostId": '',
-                "key_name": '',
-                "image": {
-                    "id": "5",
-                    "links": [
-                        {
-                            "rel": "bookmark",
-                            "href": image_bookmark,
-                        },
-                    ],
-                },
-                "flavor": {
-                    "id": "1",
-                  "links": [
-                                            {
-                          "rel": "bookmark",
-                          "href": flavor_bookmark,
-                      },
-                  ],
-                },
-                "addresses": {
-                    'test1': [
-                        {'version': 4, 'addr': '192.168.1.100'},
-                        {'version': 6, 'addr': '2001:db8:0:1::1'}
-                    ]
-                },
-                "metadata": {},
-                "config_drive": None,
-                "links": [
-                    {
-                        "rel": "self",
-                        "href": self_link,
-                    },
-                    {
-                        "rel": "bookmark",
-                        "href": bookmark_link,
-                    },
-                ],
-            }
-        }
-
         output = self.view_builder.show(self.request, self.instance)
-        self.assertDictMatch(output, expected_server)
+        self.assertFalse('fault' in output['server'])
 
     def test_build_server_detail_active_status(self):
         #set the power state of the instance to running
diff --git a/nova/tests/test_compute.py b/nova/tests/test_compute.py
index 07abd62c4..48f16d7b4 100644
--- a/nova/tests/test_compute.py
+++ b/nova/tests/test_compute.py
@@ -21,6 +21,7 @@ Tests For Compute
 """
 from copy import copy
 import datetime
+import sys
 import time
 from webob import exc
 
@@ -205,7 +206,7 @@ class ComputeTestCase(BaseTestCase):
 
         called = {'fault_added': False}
 
-        def did_it_add_fault(_ctxt, _inst_uuid, _e):
+        def did_it_add_fault(*args):
             called['fault_added'] = True
 
         self.stubs.Set(self.compute, 'add_instance_fault_from_exc',
@@ -225,7 +226,7 @@ class ComputeTestCase(BaseTestCase):
 
         called = {'fault_added': False}
 
-        def did_it_add_fault(_ctxt, _inst_uuid, _e):
+        def did_it_add_fault(*args):
             called['fault_added'] = True
 
         self.stubs.Set(self.compute, 'add_instance_fault_from_exc',
@@ -563,7 +564,7 @@ class ComputeTestCase(BaseTestCase):
         self.stubs.Set(nova.virt.fake.FakeConnection, 'get_info',
                        fake_driver_get_info)
 
-        self.assertRaises(exception.Error,
+        self.assertRaises(exception.Invalid,
                           self.compute.set_admin_password,
                           self.context,
                           instance['uuid'])
@@ -595,7 +596,7 @@ class ComputeTestCase(BaseTestCase):
 
         #error raised from the driver should not reveal internal information
         #so a new error is raised
-        self.assertRaises(exception.Error,
+        self.assertRaises(exception.NovaException,
                           self.compute.set_admin_password,
                           self.context, instance_uuid)
 
@@ -884,7 +885,7 @@ class ComputeTestCase(BaseTestCase):
         """Ensure failure when running an instance that already exists"""
         instance = self._create_fake_instance()
         self.compute.run_instance(self.context, instance['uuid'])
-        self.assertRaises(exception.Error,
+        self.assertRaises(exception.Invalid,
                           self.compute.run_instance,
                           self.context,
                           instance['uuid'])
@@ -1472,31 +1473,68 @@ class ComputeTestCase(BaseTestCase):
         self.assertEqual(power_state.NOSTATE, instances[0]['power_state'])
 
     def test_add_instance_fault(self):
+        exc_info = None
         instance_uuid = str(utils.gen_uuid())
 
         def fake_db_fault_create(ctxt, values):
+            self.assertTrue(values['details'].startswith('test'))
+            self.assertTrue('raise NotImplementedError' in values['details'])
+            del values['details']
+
             expected = {
                 'code': 500,
                 'message': 'NotImplementedError',
-                'details': '',
                 'instance_uuid': instance_uuid,
             }
             self.assertEquals(expected, values)
 
+        try:
+            raise NotImplementedError('test')
+        except:
+            exc_info = sys.exc_info()
+
         self.stubs.Set(nova.db, 'instance_fault_create', fake_db_fault_create)
 
         ctxt = context.get_admin_context()
         self.compute.add_instance_fault_from_exc(ctxt, instance_uuid,
-                                                 NotImplementedError())
+                                                 NotImplementedError('test'),
+                                                 exc_info)
 
-    def test_add_instance_fault_http_exception(self):
+    def test_add_instance_fault_user_error(self):
+        exc_info = None
         instance_uuid = str(utils.gen_uuid())
 
         def fake_db_fault_create(ctxt, values):
+
             expected = {
-                'code': 404,
-                'message': 'HTTPNotFound',
-                'details': 'Error Details',
+                'code': 400,
+                'message': 'Invalid',
+                'details': 'fake details',
+                'instance_uuid': instance_uuid,
+            }
+            self.assertEquals(expected, values)
+
+        user_exc = exception.Invalid('fake details', code=400)
+
+        try:
+            raise user_exc
+        except:
+            exc_info = sys.exc_info()
+
+        self.stubs.Set(nova.db, 'instance_fault_create', fake_db_fault_create)
+
+        ctxt = context.get_admin_context()
+        self.compute.add_instance_fault_from_exc(ctxt, instance_uuid,
+            user_exc, exc_info)
+
+    def test_add_instance_fault_no_exc_info(self):
+        instance_uuid = str(utils.gen_uuid())
+
+        def fake_db_fault_create(ctxt, values):
+            expected = {
+                'code': 500,
+                'message': 'NotImplementedError',
+                'details': 'test',
                 'instance_uuid': instance_uuid,
             }
             self.assertEquals(expected, values)
@@ -1505,7 +1543,7 @@ class ComputeTestCase(BaseTestCase):
 
         ctxt = context.get_admin_context()
         self.compute.add_instance_fault_from_exc(ctxt, instance_uuid,
-                                        exc.HTTPNotFound("Error Details"))
+                                                 NotImplementedError('test'))
 
 
 class ComputeAPITestCase(BaseTestCase):