From 33f6d78c3a0f3e3c34e8b91a8dffb1391f7b46b6 Mon Sep 17 00:00:00 2001 From: Sergey Vilgelm Date: Mon, 24 Jun 2013 15:19:50 +0400 Subject: [PATCH] Do not raise NEW exceptions Raising NEW exception is bad practice, because we lose TraceBack. So all places like: except SomeException as e: raise e should be replaced by except SomeException: raise If we are doing some other actions before reraising we should store information about exception then do all actions and then reraise it. This is caused by eventlet bug. It lost information about exception if it switch threads. fixes bug 1191730 Change-Id: Ic2be96e9f03d2ca46d060caf6f6f7f713a1d6b82 --- HACKING.rst | 9 +++++++++ cinder/exception.py | 12 +++++++----- cinder/tests/test_netapp_nfs.py | 2 +- cinder/tests/test_scality.py | 4 ++-- cinder/tests/test_storwize_svc.py | 3 ++- cinder/transfer/api.py | 13 +++++++------ cinder/volume/drivers/hds/hds.py | 5 +++-- cinder/volume/drivers/netapp/iscsi.py | 7 +++++-- cinder/volume/drivers/netapp/nfs.py | 2 +- cinder/volume/drivers/san/hp/hp_3par_common.py | 10 +++++++--- cinder/volume/drivers/san/san.py | 7 ++++--- cinder/volume/drivers/scality.py | 4 +++- 12 files changed, 51 insertions(+), 27 deletions(-) diff --git a/HACKING.rst b/HACKING.rst index f124e0095..9d5e995e0 100644 --- a/HACKING.rst +++ b/HACKING.rst @@ -55,6 +55,15 @@ General {'vol_name': vol_name, 'vol_size': vol_size}) # OKAY +- Use 'raise' instead of 'raise e' to preserve original traceback or exception being reraised:: + + except Exception as e: + ... + raise e # BAD + + except Exception: + ... + raise # OKAY Imports ------- diff --git a/cinder/exception.py b/cinder/exception.py index a028a5686..ccfc95c19 100644 --- a/cinder/exception.py +++ b/cinder/exception.py @@ -24,6 +24,8 @@ SHOULD include dedicated exception logging. """ +import sys + from oslo.config import cfg import webob.exc @@ -105,17 +107,17 @@ class CinderException(Exception): try: message = self.message % kwargs - except Exception as e: + except Exception: + exc_info = sys.exc_info() # kwargs doesn't match a variable in the message # log the issue and the kwargs LOG.exception(_('Exception in string format operation')) for name, value in kwargs.iteritems(): LOG.error("%s: %s" % (name, value)) if CONF.fatal_exception_format_errors: - raise e - else: - # at least get the core message out if something happened - message = self.message + raise exc_info[0], exc_info[1], exc_info[2] + # at least get the core message out if something happened + message = self.message super(CinderException, self).__init__(message) diff --git a/cinder/tests/test_netapp_nfs.py b/cinder/tests/test_netapp_nfs.py index 5a4b16f96..b087156f5 100644 --- a/cinder/tests/test_netapp_nfs.py +++ b/cinder/tests/test_netapp_nfs.py @@ -386,6 +386,6 @@ class NetappDirect7modeNfsDriverTestCase(NetappDirectCmodeNfsDriverTestCase): if isinstance(e, api.NaApiError): pass else: - raise e + raise mox.VerifyAll() diff --git a/cinder/tests/test_scality.py b/cinder/tests/test_scality.py index edaadd480..77b1c4946 100644 --- a/cinder/tests/test_scality.py +++ b/cinder/tests/test_scality.py @@ -61,7 +61,7 @@ class ScalityDriverTestCase(test.TestCase): os.makedirs(path) except OSError as e: if e.errno != errno.EEXIST: - raise e + raise def _create_fake_config(self): open(self.TEST_CONFIG, "w+").close() @@ -75,7 +75,7 @@ class ScalityDriverTestCase(test.TestCase): os.unlink(self.TEST_CONFIG) except OSError as e: if e.errno != errno.ENOENT: - raise e + raise def _configure_driver(self): scality.CONF.scality_sofs_config = self.TEST_CONFIG diff --git a/cinder/tests/test_storwize_svc.py b/cinder/tests/test_storwize_svc.py index 04171739d..ad083a4a2 100644 --- a/cinder/tests/test_storwize_svc.py +++ b/cinder/tests/test_storwize_svc.py @@ -24,6 +24,7 @@ Tests for the IBM Storwize family and SVC volume driver. """ + import random import re import socket @@ -1610,7 +1611,7 @@ class StorwizeSVCDriverTestCase(test.TestCase): self.assertEqual(attrs[k], v) except exception.ProcessExecutionError as e: if 'CMMVC7050E' not in e.stderr: - raise e + raise def test_storwize_svc_unicode_host_and_volume_names(self): # We'll check with iSCSI only - nothing protocol-dependednt here diff --git a/cinder/transfer/api.py b/cinder/transfer/api.py index c8cd7cf87..b89aa5990 100644 --- a/cinder/transfer/api.py +++ b/cinder/transfer/api.py @@ -27,6 +27,7 @@ from oslo.config import cfg from cinder.db import base from cinder import exception +from cinder.openstack.common import excutils from cinder.openstack.common import log as logging from cinder import quota from cinder.volume import api as volume_api @@ -192,12 +193,12 @@ class API(base.Base): if donor_reservations: QUOTAS.commit(context, donor_reservations, project_id=donor_id) LOG.info(_("Volume %s has been transferred.") % volume_id) - except Exception as exc: - QUOTAS.rollback(context, reservations) - if donor_reservations: - QUOTAS.rollback(context, donor_reservations, - project_id=donor_id) - raise exc + except Exception: + with excutils.save_and_reraise_exception(): + QUOTAS.rollback(context, reservations) + if donor_reservations: + QUOTAS.rollback(context, donor_reservations, + project_id=donor_id) vol_ref = self.db.volume_get(context, volume_id) return {'id': transfer_id, diff --git a/cinder/volume/drivers/hds/hds.py b/cinder/volume/drivers/hds/hds.py index a85fd1c6d..b023a43a8 100644 --- a/cinder/volume/drivers/hds/hds.py +++ b/cinder/volume/drivers/hds/hds.py @@ -24,6 +24,7 @@ from oslo.config import cfg from xml.etree import ElementTree as ETree from cinder import exception +from cinder.openstack.common import excutils from cinder.openstack.common import log as logging from cinder import utils from cinder.volume import driver @@ -86,8 +87,8 @@ def _xml_read(root, element, check=None): return None except ETree.ParseError as e: if check: - LOG.error(_("XML exception reading parameter: %s") % element) - raise e + with excutils.save_and_reraise_exception(): + LOG.error(_("XML exception reading parameter: %s") % element) else: LOG.info(_("XML exception reading parameter: %s") % element) return None diff --git a/cinder/volume/drivers/netapp/iscsi.py b/cinder/volume/drivers/netapp/iscsi.py index 17f9decb0..a83e274a1 100644 --- a/cinder/volume/drivers/netapp/iscsi.py +++ b/cinder/volume/drivers/netapp/iscsi.py @@ -22,6 +22,7 @@ This driver requires NetApp Clustered Data ONTAP or 7-mode storage systems with installed iSCSI licenses. """ +import sys import time import uuid @@ -402,12 +403,13 @@ class NetAppDirectISCSIDriver(driver.ISCSIDriver): message = e.message msg = _('Error mapping lun. Code :%(code)s, Message:%(message)s') msg_fmt = {'code': code, 'message': message} + exc_info = sys.exc_info() LOG.warn(msg % msg_fmt) (igroup, lun_id) = self._find_mapped_lun_igroup(path, initiator) if lun_id is not None: return lun_id else: - raise e + raise exc_info[0], exc_info[1], exc_info[2] def _unmap_lun(self, path, initiator): """Unmaps a lun from given initiator.""" @@ -422,12 +424,13 @@ class NetAppDirectISCSIDriver(driver.ISCSIDriver): msg = _("Error unmapping lun. Code :%(code)s," " Message:%(message)s") msg_fmt = {'code': e.code, 'message': e.message} + exc_info = sys.exc_info() LOG.warn(msg % msg_fmt) # if the lun is already unmapped if e.code == '13115' or e.code == '9016': pass else: - raise e + raise exc_info[0], exc_info[1], exc_info[2] def _find_mapped_lun_igroup(self, path, initiator, os=None): """Find the igroup for mapped lun with initiator.""" diff --git a/cinder/volume/drivers/netapp/nfs.py b/cinder/volume/drivers/netapp/nfs.py index 79b3edfbe..c15787584 100644 --- a/cinder/volume/drivers/netapp/nfs.py +++ b/cinder/volume/drivers/netapp/nfs.py @@ -397,7 +397,7 @@ class NetAppDirect7modeNfsDriver (NetAppDirectNfsDriver): except NaApiError as e: if e.code != 'UnknownCloneId': self._clear_clone(clone_id) - raise e + raise def _get_actual_path_for_export(self, export_path): """Gets the actual path on the filer for export path.""" diff --git a/cinder/volume/drivers/san/hp/hp_3par_common.py b/cinder/volume/drivers/san/hp/hp_3par_common.py index 4c0ab2de4..919742e09 100644 --- a/cinder/volume/drivers/san/hp/hp_3par_common.py +++ b/cinder/volume/drivers/san/hp/hp_3par_common.py @@ -36,6 +36,8 @@ hp3par_api_url, hp3par_username, hp3par_password for credentials to talk to the REST service on the 3PAR array. """ + + import base64 import json import paramiko @@ -52,10 +54,12 @@ from oslo.config import cfg from cinder import context from cinder import exception +from cinder.openstack.common import excutils from cinder.openstack.common import log as logging from cinder import utils from cinder.volume import volume_types + LOG = logging.getLogger(__name__) hp3par_opts = [ @@ -313,9 +317,9 @@ exit "attempts : '%(command)s'") % {'total_attempts': total_attempts, 'command': command}) raise paramiko.SSHException(msg) - except Exception as e: - LOG.error(_("Error running ssh command: %s") % command) - raise e + except Exception: + with excutils.save_and_reraise_exception(): + LOG.error(_("Error running ssh command: %s") % command) def _delete_3par_host(self, hostname): self._cli_run('removehost %s' % hostname, None) diff --git a/cinder/volume/drivers/san/san.py b/cinder/volume/drivers/san/san.py index 8d407c27e..09ea3e29b 100644 --- a/cinder/volume/drivers/san/san.py +++ b/cinder/volume/drivers/san/san.py @@ -27,6 +27,7 @@ from eventlet import greenthread from oslo.config import cfg from cinder import exception +from cinder.openstack.common import excutils from cinder.openstack.common import log as logging from cinder import utils from cinder.volume.driver import ISCSIDriver @@ -143,9 +144,9 @@ class SanISCSIDriver(ISCSIDriver): stderr="Error running SSH command", cmd=command) - except Exception as e: - LOG.error(_("Error running SSH command: %s") % command) - raise e + except Exception: + with excutils.save_and_reraise_exception(): + LOG.error(_("Error running SSH command: %s") % command) def ensure_export(self, context, volume): """Synchronously recreates an export for a logical volume.""" diff --git a/cinder/volume/drivers/scality.py b/cinder/volume/drivers/scality.py index 0cad54ba0..f75262346 100644 --- a/cinder/volume/drivers/scality.py +++ b/cinder/volume/drivers/scality.py @@ -16,6 +16,7 @@ Scality SOFS Volume Driver. """ + import errno import os import urllib2 @@ -28,6 +29,7 @@ from cinder.image import image_utils from cinder.openstack.common import log as logging from cinder.volume import driver + LOG = logging.getLogger(__name__) volume_opts = [ @@ -85,7 +87,7 @@ class ScalityDriver(driver.VolumeDriver): os.makedirs(path) except OSError as e: if e.errno != errno.EEXIST: - raise e + raise def _mount_sofs(self): config = CONF.scality_sofs_config -- 2.45.2