Use abstract class for the backup driver interface

Instead of using NotImplementedError exceptions to define the interface
it's better to use python abc class [1]. The advantage that it fails
faster if a class doesn't implement the needed interface.
See also [2].

[1]: http://legacy.python.org/dev/peps/pep-3119/
[2]: http://dbader.org/blog/abstract-base-classes-in-python

Change-Id: I625a73f70ac5c6ca68ee38a70c98d999454a817e
Partial-Bug: #1346797

diff --git a/cinder/backup/driver.py b/cinder/backup/driver.py
index c831d1df7e83141bdf94ce8516600b3436580da8..674f59e5a03ec8eed8ebbb3e246da16140176a23 100644 (file)
--- a/cinder/backup/driver.py
+++ b/cinder/backup/driver.py
@@ -15,7 +15,10 @@
 
 """Base class for all backup drivers."""
 
+import abc
+
 from oslo.config import cfg
+import six
 
 from cinder.db import base
 from cinder import exception
@@ -241,6 +244,7 @@ class BackupMetadataAPI(base.Base):
                 LOG.debug(msg)
 
 
+@six.add_metaclass(abc.ABCMeta)
 class BackupDriver(base.Base):
 
     def __init__(self, context, db_driver=None):
@@ -254,17 +258,20 @@ class BackupDriver(base.Base):
     def put_metadata(self, volume_id, json_metadata):
         self.backup_meta_api.put(volume_id, json_metadata)
 
+    @abc.abstractmethod
     def backup(self, backup, volume_file, backup_metadata=False):
         """Start a backup of a specified volume."""
-        raise NotImplementedError()
+        return
 
+    @abc.abstractmethod
     def restore(self, backup, volume_id, volume_file):
         """Restore a saved backup."""
-        raise NotImplementedError()
+        return
 
+    @abc.abstractmethod
     def delete(self, backup):
         """Delete a saved backup."""
-        raise NotImplementedError()
+        return
 
     def export_record(self, backup):
         """Export backup record.
@@ -290,6 +297,10 @@ class BackupDriver(base.Base):
         """
         return jsonutils.loads(backup_url.decode("base64"))
 
+
+@six.add_metaclass(abc.ABCMeta)
+class BackupDriverWithVerify(BackupDriver):
+    @abc.abstractmethod
     def verify(self, backup):
         """Verify that the backup exists on the backend.
 
@@ -299,4 +310,4 @@ class BackupDriver(base.Base):
         :param backup: backup id of the backup to verify
         :raises: InvalidBackup, NotImplementedError
         """
-        raise NotImplementedError()
+        return

diff --git a/cinder/backup/manager.py b/cinder/backup/manager.py
index e6ac5446ee1d37526eaf8d305ccd833224ba768d..abbea1844621d159a8a3344d46d276aadcf8c902 100644 (file)
--- a/cinder/backup/manager.py
+++ b/cinder/backup/manager.py
@@ -36,6 +36,7 @@ Volume backups can be created, restored, deleted and listed.
 from oslo.config import cfg
 from oslo import messaging
 
+from cinder.backup import driver
 from cinder.backup import rpcapi as backup_rpcapi
 from cinder import context
 from cinder import exception
@@ -560,12 +561,14 @@ class BackupManager(manager.SchedulerDependentManager):
 
             # Verify backup
             try:
-                backup_service.verify(backup_id)
-            except NotImplementedError:
-                LOG.warn(_('Backup service %(service)s does not support '
-                           'verify. Backup id %(id)s is not verified. '
-                           'Skipping verify.') % {'service': self.driver_name,
-                                                  'id': backup_id})
+                if isinstance(backup_service, driver.BackupDriverWithVerify):
+                    backup_service.verify(backup_id)
+                else:
+                    LOG.warn(_('Backup service %(service)s does not support '
+                               'verify. Backup id %(id)s is not verified. '
+                               'Skipping verify.') % {'service':
+                                                      self.driver_name,
+                                                      'id': backup_id})
             except exception.InvalidBackup as err:
                 with excutils.save_and_reraise_exception():
                     self.db.backup_update(context, backup_id,

diff --git a/cinder/tests/backup/fake_service.py b/cinder/tests/backup/fake_service.py
index fa8fac4daed3b7f182ecb7ef74bd21b75d32d235..3f20911606ea79bca248e771f28faf6060cef4aa 100644 (file)
--- a/cinder/tests/backup/fake_service.py
+++ b/cinder/tests/backup/fake_service.py
@@ -21,7 +21,7 @@ LOG = logging.getLogger(__name__)
 
 class FakeBackupService(BackupDriver):
     def __init__(self, context, db_driver=None):
-        super(FakeBackupService, self).__init__(db_driver)
+        super(FakeBackupService, self).__init__(context, db_driver)
 
     def backup(self, backup, volume_file):
         pass

diff --git a/cinder/tests/backup/fake_service_with_verify.py b/cinder/tests/backup/fake_service_with_verify.py
index 32695c127efa83a42a4b98725424b446632684af..f88069fb3c11c4178be7fe9d92600b313e8f9760 100644 (file)
--- a/cinder/tests/backup/fake_service_with_verify.py
+++ b/cinder/tests/backup/fake_service_with_verify.py
@@ -13,13 +13,15 @@
 #    License for the specific language governing permissions and limitations
 #    under the License.
 
+from cinder.backup import driver
 from cinder.openstack.common import log as logging
 from cinder.tests.backup import fake_service
 
 LOG = logging.getLogger(__name__)
 
 
-class FakeBackupServiceWithVerify(fake_service.FakeBackupService):
+class FakeBackupServiceWithVerify(driver.BackupDriverWithVerify,
+                                  fake_service.FakeBackupService):
     def verify(self, backup):
         pass
 

diff --git a/cinder/tests/test_backup.py b/cinder/tests/test_backup.py
index 4e2131eb5706f54217844837e73e0ec28a0d207a..d656cc351a0fba45b8b92546a6a5247a485562de 100644 (file)
--- a/cinder/tests/test_backup.py
+++ b/cinder/tests/test_backup.py
@@ -470,18 +470,11 @@ class BackupTestCase(BaseBackupTest):
         export = self._create_exported_record_entry(vol_size=vol_size)
         imported_record = self._create_export_record_db_entry()
         backup_hosts = []
-        backup_driver = self.backup_mgr.service.get_backup_driver(self.ctxt)
-        _mock_backup_verify_class = ('%s.%s.%s' %
-                                     (backup_driver.__module__,
-                                      backup_driver.__class__.__name__,
-                                      'verify'))
-        with mock.patch(_mock_backup_verify_class) as _mock_record_verify:
-            _mock_record_verify.side_effect = NotImplementedError()
-            self.backup_mgr.import_record(self.ctxt,
-                                          imported_record,
-                                          export['backup_service'],
-                                          export['backup_url'],
-                                          backup_hosts)
+        self.backup_mgr.import_record(self.ctxt,
+                                      imported_record,
+                                      export['backup_service'],
+                                      export['backup_url'],
+                                      backup_hosts)
         backup = db.backup_get(self.ctxt, imported_record)
         self.assertEqual(backup['status'], 'available')
         self.assertEqual(backup['size'], vol_size)

diff --git a/cinder/tests/test_backup_driver_base.py b/cinder/tests/test_backup_driver_base.py
index 3ad2348891fda67a68396839a53f41736140d7f4..a77ae7fa39fddc4356941f6bba15ea7772fac60e 100644 (file)
--- a/cinder/tests/test_backup_driver_base.py
+++ b/cinder/tests/test_backup_driver_base.py
@@ -24,7 +24,7 @@ from cinder import db
 from cinder import exception
 from cinder.openstack.common import jsonutils
 from cinder import test
-
+from cinder.tests.backup import fake_service
 
 _backup_db_fields = ['id', 'user_id', 'project_id',
                      'volume_id', 'host', 'availability_zone',
@@ -54,20 +54,7 @@ class BackupBaseDriverTestCase(test.TestCase):
         self._create_backup_db_entry(self.backup_id, self.volume_id, 1)
         self._create_volume_db_entry(self.volume_id, 1)
         self.backup = db.backup_get(self.ctxt, self.backup_id)
-        self.driver = driver.BackupDriver(self.ctxt)
-
-    def test_backup(self):
-        self.assertRaises(NotImplementedError,
-                          self.driver.backup, self.backup, self.volume_id)
-
-    def test_restore(self):
-        self.assertRaises(NotImplementedError,
-                          self.driver.restore, self.backup, self.volume_id,
-                          None)
-
-    def test_delete(self):
-        self.assertRaises(NotImplementedError,
-                          self.driver.delete, self.backup)
+        self.driver = fake_service.FakeBackupService(self.ctxt)
 
     def test_get_metadata(self):
         json_metadata = self.driver.get_metadata(self.volume_id)
@@ -98,10 +85,6 @@ class BackupBaseDriverTestCase(test.TestCase):
             self.assertTrue(key in imported_backup)
             self.assertEqual(imported_backup[key], self.backup[key])
 
-    def test_verify(self):
-        self.assertRaises(NotImplementedError,
-                          self.driver.verify, self.backup)
-
 
 class BackupMetadataAPITestCase(test.TestCase):