Fix variable scope issue in try-except

Below piece of code in cinder.backup.api.API.create does not handle
the scope of variable 'backup' properly. In general variable used in
except clause has larger scope than what is in try clause.

```
    try:
        ...
        backup = objects.Backup(context=context, **kwargs)
        backup.create()
        ...
    except Exception:
            ...
            backup.destroy()
            ...
```

The first problem: if object.Backup.__init__ raises exception,
variable 'backup' won't be assigned. However the cleanup code invokes
backup.destroy(), which results in an UnboundLocalError. The original
exception of __init__ is dropped, instead UnboundLocalError is raised
up-level. I know that for now objects.Backup.__init__ doesn't have
chance to raise exception, but when writing code we should not rely
on such assumption.

The second problem: backup.create() invokes DB. If an DB exception is
raised, backup.id won't be assigned. Any access to backup.id will
result in an "NotImplementedError: Cannot load 'id' in the base
class". However, when cleaning up, the backup.destroy() does access
backup.id. So we have the similar problem as the first one, and the
original exception is overrode by NotImplementedError.

This patch fixes it by adding a first-assignment outside try clause,
and add a safe check in except clause. The added test cases, if run
on the original code, it will show how the above problem occurs.

Closes-Bug: #1541745
Change-Id: I0e55c342ac489ff1ef27c0597b6dcba588778a04

diff --git a/cinder/backup/api.py b/cinder/backup/api.py
index 258d56d058bf1dec8e80bde64fd3b3b9f6dcdedf..c3900422209370b661cee59f24110123c65554b3 100644 (file)
--- a/cinder/backup/api.py
+++ b/cinder/backup/api.py
@@ -271,6 +271,8 @@ class API(base.Base):
         self.db.volume_update(context, volume_id,
                               {'status': 'backing-up',
                                'previous_status': previous_status})
+
+        backup = None
         try:
             kwargs = {
                 'user_id': context.user_id,
@@ -295,7 +297,8 @@ class API(base.Base):
         except Exception:
             with excutils.save_and_reraise_exception():
                 try:
-                    backup.destroy()
+                    if backup and 'id' in backup:
+                        backup.destroy()
                 finally:
                     QUOTAS.rollback(context, reservations)
 

diff --git a/cinder/tests/unit/test_backup.py b/cinder/tests/unit/test_backup.py
index 18d0c507e5d1f18caac170fedaabc834f251a72f..28e91bac04e3128eb6e943ea7ee178e9e9939c8c 100644 (file)
--- a/cinder/tests/unit/test_backup.py
+++ b/cinder/tests/unit/test_backup.py
@@ -20,6 +20,7 @@ import uuid
 
 import mock
 from oslo_config import cfg
+from oslo_db import exception as db_exc
 from oslo_utils import importutils
 from oslo_utils import timeutils
 
@@ -32,6 +33,7 @@ from cinder import objects
 from cinder.objects import fields
 from cinder import test
 from cinder.tests.unit.backup import fake_service_with_verify as fake_service
+from cinder.tests.unit import utils
 from cinder.volume.drivers import lvm
 
 
@@ -1224,3 +1226,50 @@ class BackupAPITestCase(BaseBackupTest):
         mock_backuplist.get_all_by_project.assert_called_once_with(
             ctxt, ctxt.project_id, {'key': 'value'}, None, None, None, None,
             None)
+
+    @mock.patch.object(api.API, '_is_backup_service_enabled',
+                       return_value=True)
+    @mock.patch.object(db, 'backup_create',
+                       side_effect=db_exc.DBError())
+    def test_create_when_failed_to_create_backup_object(
+            self, mock_create,
+            mock_service_enabled):
+        volume_id = utils.create_volume(self.ctxt)['id']
+        self.ctxt.user_id = 'user_id'
+        self.ctxt.project_id = 'project_id'
+
+        # The opposite side of this test case is a "NotImplementedError:
+        # Cannot load 'id' in the base class" being raised.
+        # More detailed, in the try clause, if backup.create() failed
+        # with DB exception, backup.id won't be assigned. However,
+        # in the except clause, backup.destroy() is invoked to do cleanup,
+        # which internally tries to access backup.id.
+        self.assertRaises(db_exc.DBError, self.api.create,
+                          context=self.ctxt,
+                          name="test_backup",
+                          description="test backup description",
+                          volume_id=volume_id,
+                          container='volumebackups')
+
+    @mock.patch.object(api.API, '_is_backup_service_enabled',
+                       return_value=True)
+    @mock.patch.object(objects.Backup, '__init__',
+                       side_effect=exception.InvalidInput(
+                           reason='Failed to new'))
+    def test_create_when_failed_to_new_backup_object(self, mock_new,
+                                                     mock_service_enabled):
+        volume_id = utils.create_volume(self.ctxt)['id']
+        self.ctxt.user_id = 'user_id'
+        self.ctxt.project_id = 'project_id'
+
+        # The opposite side of this test case is that a "UnboundLocalError:
+        # local variable 'backup' referenced before assignment" is raised.
+        # More detailed, in the try clause, backup = objects.Backup(...)
+        # raises exception, so 'backup' is not assigned. But in the except
+        # clause, 'backup' is referenced to invoke cleanup methods.
+        self.assertRaises(exception.InvalidInput, self.api.create,
+                          context=self.ctxt,
+                          name="test_backup",
+                          description="test backup description",
+                          volume_id=volume_id,
+                          container='volumebackups')