From 64f6bebc6623a0e48dde3bef38390460728ea91e Mon Sep 17 00:00:00 2001 From: Marc Koderer Date: Tue, 22 Jul 2014 13:53:45 +0200 Subject: [PATCH] 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 --- cinder/backup/driver.py | 19 +++++++++++++---- cinder/backup/manager.py | 15 +++++++------ cinder/tests/backup/fake_service.py | 2 +- .../tests/backup/fake_service_with_verify.py | 4 +++- cinder/tests/test_backup.py | 17 +++++---------- cinder/tests/test_backup_driver_base.py | 21 ++----------------- 6 files changed, 35 insertions(+), 43 deletions(-) diff --git a/cinder/backup/driver.py b/cinder/backup/driver.py index c831d1df7..674f59e5a 100644 --- 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 e6ac5446e..abbea1844 100644 --- 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 fa8fac4da..3f2091160 100644 --- 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 32695c127..f88069fb3 100644 --- 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 4e2131eb5..d656cc351 100644 --- 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 3ad234889..a77ae7fa3 100644 --- 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): -- 2.45.2