From bef90868971118dcced47a64f3bbffc7f2732db7 Mon Sep 17 00:00:00 2001 From: wanghao Date: Wed, 2 Jan 2019 16:24:26 +0800 Subject: [PATCH] Support Incremental Backup Completion In RBD Ceph RBD backend ignores the `--incremental` option when creating a volume backup. The first backup of a given volume is always a full backup, and each subsequent backup is always an incremental backup. This behavior makes it impossible to remove old backups while keeping at least one recent backup. Since Cinder will not find the latest_backup id as parent_id if '--incremental=False', so we can use the parent_id to ensure whether do the full backup in rbd driver or not. If the incremental flag '--incremental' is not specified, this patch will always create a new full backup for rbd volume. Closes-Bug: #1810270 Closes-Bug: #1790713 Co-Authored-By: Wanghao Co-Authored-By: Sofia Enriquez (cherry picked from commit 5018727f8e18992e64c55b84f0d5b39ae7c70b32) Conflicts: cinder/tests/unit/backup/drivers/test_backup_ceph.py Change-Id: I516b7c82b05b26e81195f7f106d43a9e0804082d --- cinder/backup/api.py | 4 + cinder/backup/drivers/ceph.py | 218 +++++++++------- cinder/objects/backup.py | 11 +- cinder/objects/base.py | 1 + .../unit/backup/drivers/test_backup_ceph.py | 245 ++++++++++-------- cinder/tests/unit/objects/test_objects.py | 4 +- ...up-completion-in-rbd-1f2165fefcc470d1.yaml | 5 + 7 files changed, 285 insertions(+), 203 deletions(-) create mode 100644 releasenotes/notes/support-incremental-backup-completion-in-rbd-1f2165fefcc470d1.yaml diff --git a/cinder/backup/api.py b/cinder/backup/api.py index c11e655bb76..07385386661 100644 --- a/cinder/backup/api.py +++ b/cinder/backup/api.py @@ -278,7 +278,10 @@ class API(base.Base): raise exception.InvalidBackup(reason=msg) parent_id = None + parent = None + if latest_backup: + parent = latest_backup parent_id = latest_backup.id if latest_backup['status'] != fields.BackupStatus.AVAILABLE: msg = _('The parent backup must be available for ' @@ -313,6 +316,7 @@ class API(base.Base): 'availability_zone': availability_zone, 'snapshot_id': snapshot_id, 'data_timestamp': data_timestamp, + 'parent': parent, 'metadata': metadata or {} } backup = objects.Backup(context=context, **kwargs) diff --git a/cinder/backup/drivers/ceph.py b/cinder/backup/drivers/ceph.py index a74119f243e..d64925a7775 100644 --- a/cinder/backup/drivers/ceph.py +++ b/cinder/backup/drivers/ceph.py @@ -43,6 +43,7 @@ restore to a new volume (default). """ import fcntl +import json import os import re import subprocess @@ -314,22 +315,39 @@ class CephBackupDriver(driver.BackupDriver): ioctx.close() client.shutdown() - def _get_backup_base_name(self, volume_id, backup_id=None, - diff_format=False): + def _format_base_name(self, service_metadata): + base_name = json.loads(service_metadata)["base"] + return utils.convert_str(base_name) + + def _get_backup_base_name(self, volume_id, backup=None): """Return name of base image used for backup. Incremental backups use a new base name so we support old and new style format. """ # Ensure no unicode - if diff_format: + if not backup: return utils.convert_str("volume-%s.backup.base" % volume_id) - else: - if backup_id is None: - msg = _("Backup id required") - raise exception.InvalidParameterValue(msg) - return utils.convert_str("volume-%s.backup.%s" - % (volume_id, backup_id)) + + if backup.service_metadata: + return self._format_base_name(backup.service_metadata) + + # 'parent' field will only be present in incremental backups. This is + # filled by cinder-api + if backup.parent: + # Old backups don't have the base name in the service_metadata, + # so we use the default RBD backup base + if backup.parent.service_metadata: + service_metadata = backup.parent.service_metadata + base_name = self._format_base_name(service_metadata) + else: + base_name = utils.convert_str("volume-%s.backup.base" + % volume_id) + + return base_name + + return utils.convert_str("volume-%s.backup.%s" + % (volume_id, backup.id)) def _discard_bytes(self, volume, offset, length): """Trim length bytes from offset. @@ -479,7 +497,7 @@ class CephBackupDriver(driver.BackupDriver): if base_name is None: try_diff_format = True - base_name = self._get_backup_base_name(volume_id, backup.id) + base_name = self._get_backup_base_name(volume_id, backup=backup) LOG.debug("Trying diff format basename='%(basename)s' for " "backup base image of volume %(volume)s.", {'basename': base_name, 'volume': volume_id}) @@ -630,7 +648,7 @@ class CephBackupDriver(driver.BackupDriver): if name not in rbds: LOG.debug("Image '%s' not found - trying diff format name", name) if try_diff_format: - name = self._get_backup_base_name(volume_id, diff_format=True) + name = self._get_backup_base_name(volume_id) if name not in rbds: LOG.debug("Diff format image '%s' not found", name) return False, name @@ -657,50 +675,79 @@ class CephBackupDriver(driver.BackupDriver): return False + def _full_rbd_backup(self, container, base_name, length): + """Create the base_image for a full RBD backup.""" + with eventlet.tpool.Proxy(rbd_driver.RADOSClient(self, + container)) as client: + self._create_base_image(base_name, length, client) + # Now we just need to return from_snap=None and image_created=True, if + # there is some exception in making backup snapshot, will clean up the + # base image. + return None, True + + def _incremental_rbd_backup(self, backup, base_name, length, + source_rbd_image, volume_id): + """Select the last snapshot for a RBD incremental backup.""" + + container = backup.container + last_incr = backup.parent_id + LOG.debug("Trying to perform an incremental backup with container: " + "%(container)s, base_name: %(base)s, source RBD image: " + "%(source)s, volume ID %(volume)s and last incremental " + "backup ID: %(incr)s.", + {'container': container, + 'base': base_name, + 'source': source_rbd_image, + 'volume': volume_id, + 'incr': last_incr, + }) + + with eventlet.tpool.Proxy(rbd_driver.RADOSClient(self, + container)) as client: + base_rbd = eventlet.tpool.Proxy(self.rbd.Image(client.ioctx, + base_name, + read_only=True)) + try: + from_snap = self._get_backup_snap_name(base_rbd, + base_name, + last_incr) + if from_snap is None: + msg = (_( + "Can't find snapshot from parent %(incr)s and " + "base name image %(base)s.") % + {'incr': last_incr, 'base': base_name}) + LOG.error(msg) + raise exception.BackupRBDOperationFailed(msg) + finally: + base_rbd.close() + + return from_snap, False + def _backup_rbd(self, backup, volume_file, volume_name, length): - """Create an incremental backup from an RBD image.""" + """Create an incremental or full backup from an RBD image.""" rbd_user = volume_file.rbd_user rbd_pool = volume_file.rbd_pool rbd_conf = volume_file.rbd_conf source_rbd_image = eventlet.tpool.Proxy(volume_file.rbd_image) volume_id = backup.volume_id - updates = {} - base_name = self._get_backup_base_name(volume_id, diff_format=True) - image_created = False - with eventlet.tpool.Proxy(rbd_driver.RADOSClient(self, - backup.container)) as client: - # If from_snap does not exist at the destination (and the - # destination exists), this implies a previous backup has failed. - # In this case we will force a full backup. - # - # TODO(dosaboy): find a way to repair the broken backup - # - if base_name not in eventlet.tpool.Proxy(self.rbd.RBD()).list( - ioctx=client.ioctx): - src_vol_snapshots = self.get_backup_snaps(source_rbd_image) - if src_vol_snapshots: - # If there are source volume snapshots but base does not - # exist then we delete it and set from_snap to None - LOG.debug("Volume '%(volume)s' has stale source " - "snapshots so deleting them.", - {'volume': volume_id}) - for snap in src_vol_snapshots: - from_snap = snap['name'] - source_rbd_image.remove_snap(from_snap) - from_snap = None + base_name = None - # Create new base image - self._create_base_image(base_name, length, client) - image_created = True - else: - # If a from_snap is defined and is present in the source volume - # image but does not exist in the backup base then we look down - # the list of source volume snapshots and find the latest one - # for which a backup snapshot exist in the backup base. Until - # that snapshot is reached, we delete all the other snapshots - # for which backup snapshot does not exist. - from_snap = self._get_most_recent_snap(source_rbd_image, - base_name, client) + # If backup.parent_id is None performs full RBD backup + if backup.parent_id is None: + base_name = self._get_backup_base_name(volume_id, backup=backup) + from_snap, image_created = self._full_rbd_backup(backup.container, + base_name, + length) + # Otherwise performs incremental rbd backup + else: + # Find the base name from the parent backup's service_metadata + base_name = self._get_backup_base_name(volume_id, backup=backup) + rbd_img = source_rbd_image + from_snap, image_created = self._incremental_rbd_backup(backup, + base_name, + length, + rbd_img, + volume_id) LOG.debug("Using --from-snap '%(snap)s' for incremental backup of " "volume %(volume)s.", @@ -744,14 +791,8 @@ class CephBackupDriver(driver.BackupDriver): "source volume='%(volume)s'.", {'snapshot': new_snap, 'volume': volume_id}) source_rbd_image.remove_snap(new_snap) - # We update the parent_id here. The from_snap is of the format: - # backup.BACKUP_ID.snap.TIMESTAMP. So we need to extract the - # backup_id of the parent only from from_snap and set it as - # parent_id - if from_snap: - parent_id = from_snap.split('.') - updates = {'parent_id': parent_id[1]} - return updates + + return {'service_metadata': '{"base": "%s"}' % base_name} def _file_is_rbd(self, volume_file): """Returns True if the volume_file is actually an RBD image.""" @@ -765,7 +806,7 @@ class CephBackupDriver(driver.BackupDriver): image. """ volume_id = backup.volume_id - backup_name = self._get_backup_base_name(volume_id, backup.id) + backup_name = self._get_backup_base_name(volume_id, backup=backup) with eventlet.tpool.Proxy(rbd_driver.RADOSClient(self, backup.container)) as client: @@ -868,23 +909,6 @@ class CephBackupDriver(driver.BackupDriver): LOG.debug("Found snapshot '%s'", snaps[0]) return snaps[0] - def _get_most_recent_snap(self, rbd_image, base_name, client): - """Get the most recent backup snapshot of the provided image. - - Returns name of most recent backup snapshot or None if there are no - backup snapshots. - """ - src_vol_backup_snaps = self.get_backup_snaps(rbd_image, sort=True) - from_snap = None - - for snap in src_vol_backup_snaps: - if self._snap_exists(base_name, snap['name'], client): - from_snap = snap['name'] - break - rbd_image.remove_snap(snap['name']) - - return from_snap - def _get_volume_size_gb(self, volume): """Return the size in gigabytes of the given volume. @@ -938,17 +962,23 @@ class CephBackupDriver(driver.BackupDriver): volume_file.seek(0) length = self._get_volume_size_gb(volume) - do_full_backup = False - if self._file_is_rbd(volume_file): - # If volume an RBD, attempt incremental backup. - LOG.debug("Volume file is RBD: attempting incremental backup.") + if backup.snapshot_id: + do_full_backup = True + elif self._file_is_rbd(volume_file): + # If volume an RBD, attempt incremental or full backup. + do_full_backup = False + LOG.debug("Volume file is RBD: attempting optimized backup") try: - updates = self._backup_rbd(backup, volume_file, - volume.name, length) + updates = self._backup_rbd(backup, volume_file, volume.name, + length) except exception.BackupRBDOperationFailed: - LOG.debug("Forcing full backup of volume %s.", volume.id) - do_full_backup = True + with excutils.save_and_reraise_exception(): + self.delete_backup(backup) else: + if backup.parent_id: + LOG.debug("Volume file is NOT RBD: can't perform" + "incremental backup.") + raise exception.BackupRBDOperationFailed LOG.debug("Volume file is NOT RBD: will do full backup.") do_full_backup = True @@ -970,11 +1000,6 @@ class CephBackupDriver(driver.BackupDriver): LOG.debug("Backup '%(backup_id)s' of volume %(volume_id)s finished.", {'backup_id': backup.id, 'volume_id': volume.id}) - # If updates is empty then set parent_id to None. This will - # take care if --incremental flag is used in CLI but a full - # backup is performed instead - if not updates and backup.parent_id: - updates = {'parent_id': None} return updates def _full_restore(self, backup, dest_file, dest_name, length, @@ -989,13 +1014,10 @@ class CephBackupDriver(driver.BackupDriver): # If a source snapshot is provided we assume the base is diff # format. if src_snap: - diff_format = True + backup_name = self._get_backup_base_name(backup.volume_id, + backup=backup) else: - diff_format = False - - backup_name = self._get_backup_base_name(backup.volume_id, - backup_id=backup.id, - diff_format=diff_format) + backup_name = self._get_backup_base_name(backup.volume_id) # Retrieve backup volume src_rbd = eventlet.tpool.Proxy(self.rbd.Image(client.ioctx, @@ -1022,7 +1044,7 @@ class CephBackupDriver(driver.BackupDriver): post-process and resize it back to its expected size. """ backup_base = self._get_backup_base_name(backup.volume_id, - diff_format=True) + backup=backup) with eventlet.tpool.Proxy(rbd_driver.RADOSClient(self, backup.container)) as client: @@ -1047,7 +1069,7 @@ class CephBackupDriver(driver.BackupDriver): rbd_pool = restore_file.rbd_pool rbd_conf = restore_file.rbd_conf base_name = self._get_backup_base_name(backup.volume_id, - diff_format=True) + backup=backup) LOG.debug("Attempting incremental restore from base='%(base)s' " "snap='%(snap)s'", @@ -1179,8 +1201,10 @@ class CephBackupDriver(driver.BackupDriver): """ length = int(volume.size) * units.Gi - base_name = self._get_backup_base_name(backup.volume_id, - diff_format=True) + if backup.service_metadata: + base_name = self._get_backup_base_name(backup.volume_id, backup) + else: + base_name = self._get_backup_base_name(backup.volume_id) with eventlet.tpool.Proxy(rbd_driver.RADOSClient( self, backup.container)) as client: diff --git a/cinder/objects/backup.py b/cinder/objects/backup.py index e1fa7b48b40..514a25315e2 100644 --- a/cinder/objects/backup.py +++ b/cinder/objects/backup.py @@ -40,7 +40,8 @@ class Backup(base.CinderPersistentObject, base.CinderObject, # Version 1.4: Add restore_volume_id # Version 1.5: Add metadata # Version 1.6: Add encryption_key_id - VERSION = '1.6' + # Version 1.7: Add parent + VERSION = '1.7' OPTIONAL_FIELDS = ('metadata',) @@ -55,6 +56,7 @@ class Backup(base.CinderPersistentObject, base.CinderObject, 'availability_zone': fields.StringField(nullable=True), 'container': fields.StringField(nullable=True), 'parent_id': fields.StringField(nullable=True), + 'parent': fields.ObjectField('Backup', nullable=True), 'status': c_fields.BackupStatusField(nullable=True), 'fail_reason': fields.StringField(nullable=True), 'size': fields.IntegerField(nullable=True), @@ -110,8 +112,14 @@ class Backup(base.CinderPersistentObject, base.CinderObject, def obj_make_compatible(self, primitive, target_version): """Make an object representation compatible with a target version.""" + added_fields = (((1, 7), ('parent',)),) + super(Backup, self).obj_make_compatible(primitive, target_version) target_version = versionutils.convert_version_to_tuple(target_version) + for version, remove_fields in added_fields: + if target_version < version: + for obj_field in remove_fields: + primitive.pop(obj_field, None) @classmethod def _from_db_object(cls, context, backup, db_backup, expected_attrs=None): @@ -174,6 +182,7 @@ class Backup(base.CinderPersistentObject, base.CinderObject, self.metadata = db.backup_metadata_update(self._context, self.id, metadata, True) + updates.pop('parent', None) db.backup_update(self._context, self.id, updates) self.obj_reset_changes() diff --git a/cinder/objects/base.py b/cinder/objects/base.py index f8c02ab32f0..390e6430ec3 100644 --- a/cinder/objects/base.py +++ b/cinder/objects/base.py @@ -146,6 +146,7 @@ OBJ_VERSIONS.add('1.34', {'VolumeAttachment': '1.3'}) OBJ_VERSIONS.add('1.35', {'Backup': '1.6', 'BackupImport': '1.6'}) OBJ_VERSIONS.add('1.36', {'RequestSpec': '1.4'}) OBJ_VERSIONS.add('1.37', {'RequestSpec': '1.5'}) +OBJ_VERSIONS.add('1.38', {'Backup': '1.7', 'BackupImport': '1.7'}) class CinderObjectRegistry(base.VersionedObjectRegistry): diff --git a/cinder/tests/unit/backup/drivers/test_backup_ceph.py b/cinder/tests/unit/backup/drivers/test_backup_ceph.py index 363b9461deb..d1ee3ca27b5 100644 --- a/cinder/tests/unit/backup/drivers/test_backup_ceph.py +++ b/cinder/tests/unit/backup/drivers/test_backup_ceph.py @@ -15,6 +15,7 @@ """ Tests for Ceph backup service.""" import hashlib +import json import os import tempfile import threading @@ -39,6 +40,7 @@ from cinder.i18n import _ from cinder import objects from cinder import test from cinder.tests.unit import fake_constants as fake +import cinder.volume.drivers.rbd as rbd_driver # This is used to collect raised exceptions so that tests may check what was # raised. @@ -119,6 +121,14 @@ class BackupCephTestCase(test.TestCase): 'user_id': userid, 'project_id': projectid} return db.backup_create(self.ctxt, backup)['id'] + def _create_parent_backup_object(self): + tmp_backup_id = fake.BACKUP3_ID + self._create_backup_db_entry(tmp_backup_id, self.volume_id, + self.volume_size) + tmp_backup = objects.Backup.get_by_id(self.ctxt, tmp_backup_id) + tmp_backup.service_metadata = 'mock_base_name' + return tmp_backup + def time_inc(self): self.counter += 1 return self.counter @@ -170,6 +180,22 @@ class BackupCephTestCase(test.TestCase): self.backup = objects.Backup.get_by_id(self.ctxt, self.backup_id) self.backup.container = "backups" + # Create parent backup of volume + self.parent_backup = self._create_parent_backup_object() + + # Create alternate backup with parent + self.alt_backup_id = fake.BACKUP2_ID + self._create_backup_db_entry(self.alt_backup_id, self.volume_id, + self.volume_size) + + self.alt_backup = objects.Backup.get_by_id(self.ctxt, + self.alt_backup_id) + + base_name = "volume-%s.backup.%s" % (self.volume_id, self.backup_id) + self.alt_backup.container = "backups" + self.alt_backup.parent = self.backup + self.alt_backup.parent.service_metadata = '{"base": "%s"}' % base_name + # Create alternate volume. self.alt_volume_id = str(uuid.uuid4()) self._create_volume_db_entry(self.alt_volume_id, self.volume_size) @@ -255,24 +281,6 @@ class BackupCephTestCase(test.TestCase): self.assertFalse(oldformat) self.assertEqual(1 | 2 | 4 | 64, features) - @common_mocks - def test_get_most_recent_snap(self): - last = 'backup.%s.snap.9824923.1212' % (uuid.uuid4()) - - image = self.mock_rbd.Image.return_value - with mock.patch.object(self.service, '_snap_exists') as \ - mock_snap_exists: - mock_snap_exists.return_value = True - image.list_snaps.return_value = \ - [{'name': 'backup.%s.snap.6423868.2342' % (uuid.uuid4())}, - {'name': 'backup.%s.snap.1321319.3235' % (uuid.uuid4())}, - {'name': last}, - {'name': 'backup.%s.snap.3824923.1412' % (uuid.uuid4())}] - base_name = "mock_base" - client = mock.Mock() - snap = self.service._get_most_recent_snap(image, base_name, client) - self.assertEqual(last, snap) - @common_mocks def test_get_backup_snap_name(self): snap_name = 'backup.%s.snap.3824923.1412' % (uuid.uuid4()) @@ -415,7 +423,7 @@ class BackupCephTestCase(test.TestCase): with mock.patch.object(self.service, '_backup_metadata'): with mock.patch.object(self.service, '_discard_bytes'): with tempfile.NamedTemporaryFile() as test_file: - self.service.backup(self.backup, self.volume_file) + self.service.backup(self.alt_backup, self.volume_file) # Ensure the files are equal self.assertEqual(checksum.digest(), self.checksum.digest()) @@ -424,25 +432,34 @@ class BackupCephTestCase(test.TestCase): self.assertNotEqual(threading.current_thread(), thread_dict['thread']) @common_mocks - def test_get_backup_base_name(self): - name = self.service._get_backup_base_name(self.volume_id, - diff_format=True) + def test_get_backup_base_name_without_backup_param(self): + """Test _get_backup_base_name without backup.""" + name = self.service._get_backup_base_name(self.volume_id) self.assertEqual("volume-%s.backup.base" % (self.volume_id), name) - self.assertRaises(exception.InvalidParameterValue, - self.service._get_backup_base_name, - self.volume_id) + @common_mocks + def test_get_backup_base_name_w_backup_and_no_parent(self): + """Test _get_backup_base_name with backup and no parent.""" + name = self.service._get_backup_base_name(self.volume_id, + self.backup) + self.assertEqual("volume-%s.backup.%s" % + (self.volume_id, self.backup.id), name) - name = self.service._get_backup_base_name(self.volume_id, '1234') - self.assertEqual("volume-%s.backup.%s" % (self.volume_id, '1234'), - name) + @common_mocks + def test_get_backup_base_name_w_backup_and_parent(self): + """Test _get_backup_base_name with backup and parent.""" + name = self.service._get_backup_base_name(self.volume_id, + self.alt_backup) + base_name = json.loads(self.alt_backup.parent.service_metadata) + self.assertEqual(base_name["base"], name) @common_mocks @mock.patch('fcntl.fcntl', spec=True) @mock.patch('subprocess.Popen', spec=True) def test_backup_volume_from_rbd(self, mock_popen, mock_fnctl): + """Test full RBD backup generated successfully.""" backup_name = self.service._get_backup_base_name(self.volume_id, - diff_format=True) + self.alt_backup) def mock_write_data(): self.volume_file.seek(0) @@ -483,8 +500,11 @@ class BackupCephTestCase(test.TestCase): {'name': 'backup.mock.snap.15341241.90'}, {'name': 'backup.mock.snap.199994362.10'}]) - output = self.service.backup(self.backup, rbdio) - self.assertDictEqual({}, output) + output = self.service.backup(self.alt_backup, + rbdio) + base_name = '{"base": "%s"}' % backup_name + service_meta = {'service_metadata': base_name} + self.assertDictEqual(service_meta, output) self.assertEqual(['popen_init', 'read', @@ -494,7 +514,7 @@ class BackupCephTestCase(test.TestCase): 'communicate'], self.callstack) self.assertFalse(mock_full_backup.called) - self.assertTrue(mock_get_backup_snaps.called) + self.assertFalse(mock_get_backup_snaps.called) # Ensure the files are equal self.assertEqual(checksum.digest(), @@ -505,7 +525,7 @@ class BackupCephTestCase(test.TestCase): with mock.patch.object(self.service, '_backup_rbd') as \ mock_backup_rbd, mock.patch.object(self.service, '_backup_metadata'): - mock_backup_rbd.return_value = {'parent_id': 'mock'} + mock_backup_rbd.return_value = {'service_metadata': 'base_name'} image = self.service.rbd.Image() meta = linuxrbd.RBDImageMetadata(image, 'pool_foo', @@ -513,15 +533,14 @@ class BackupCephTestCase(test.TestCase): 'conf_foo') rbdio = linuxrbd.RBDVolumeIOWrapper(meta) output = self.service.backup(self.backup, rbdio) - self.assertDictEqual({'parent_id': 'mock'}, output) + self.assertDictEqual({'service_metadata': 'base_name'}, output) @common_mocks - def test_backup_volume_from_rbd_set_parent_id_none(self): - backup_name = self.service._get_backup_base_name( - self.volume_id, diff_format=True) + def test_backup_volume_from_rbd_got_exception(self): + base_name = self.service._get_backup_base_name(self.volume_id, + self.alt_backup) - self.mock_rbd.RBD().list.return_value = [backup_name] - self.backup.parent_id = 'mock_parent_id' + self.mock_rbd.RBD().list.return_value = [base_name] with mock.patch.object(self.service, 'get_backup_snaps'), \ mock.patch.object(self.service, '_rbd_diff_transfer') as \ @@ -550,28 +569,54 @@ class BackupCephTestCase(test.TestCase): 'conf_foo') rbdio = linuxrbd.RBDVolumeIOWrapper(meta) mock_get_backup_snaps.return_value = ( - [{'name': 'backup.mock.snap.153464362.12'}, - {'name': 'backup.mock.snap.199994362.10'}]) - output = self.service.backup(self.backup, rbdio) - self.assertIsNone(output['parent_id']) + [{'name': 'backup.mock.snap.153464362.12', + 'backup_id': 'mock_parent_id'}, + {'name': 'backup.mock.snap.199994362.10', + 'backup_id': 'mock'}]) + self.assertRaises(exception.BackupRBDOperationFailed, + self.service.backup, + self.alt_backup, rbdio) @common_mocks def test_backup_rbd_set_parent_id(self): - backup_name = self.service._get_backup_base_name( - self.volume_id, diff_format=True) + base_name = self.service._get_backup_base_name(self.volume_id, + self.alt_backup) vol_name = self.volume.name vol_length = self.volume.size - self.mock_rbd.RBD().list.return_value = [backup_name] + self.mock_rbd.RBD().list.return_value = [base_name] with mock.patch.object(self.service, '_snap_exists'), \ - mock.patch.object(self.service, '_get_backup_base_name') as \ - mock_get_backup_base_name, mock.patch.object( - self.service, '_get_most_recent_snap') as mock_get_most_recent_snap, \ + mock.patch.object(self.service, '_get_backup_snap_name') as \ + mock_get_backup_snap_name, \ mock.patch.object(self.service, '_rbd_diff_transfer'): - mock_get_backup_base_name.return_value = backup_name - mock_get_most_recent_snap.return_value = ( - 'backup.mock.snap.153464362.12') + image = self.service.rbd.Image() + mock_get_backup_snap_name.return_value = 'mock_snap_name' + meta = linuxrbd.RBDImageMetadata(image, + 'pool_foo', + 'user_foo', + 'conf_foo') + rbdio = linuxrbd.RBDVolumeIOWrapper(meta) + rbdio.seek(0) + output = self.service._backup_rbd(self.alt_backup, rbdio, + vol_name, vol_length) + base_name = '{"base": "%s"}' % base_name + self.assertEqual({'service_metadata': base_name}, output) + self.backup.parent_id = None + + @common_mocks + def test_backup_rbd_without_parent_id(self): + full_backup_name = self.service._get_backup_base_name(self.volume_id, + self.alt_backup) + vol_name = self.volume.name + vol_length = self.volume.size + + with mock.patch.object(self.service, '_rbd_diff_transfer'), \ + mock.patch.object(self.service, '_create_base_image') as \ + mock_create_base_image, mock.patch.object( + rbd_driver, 'RADOSClient') as mock_rados_client: + client = mock.Mock() + mock_rados_client.return_value.__enter__.return_value = client image = self.service.rbd.Image() meta = linuxrbd.RBDImageMetadata(image, 'pool_foo', @@ -579,9 +624,12 @@ class BackupCephTestCase(test.TestCase): 'conf_foo') rbdio = linuxrbd.RBDVolumeIOWrapper(meta) rbdio.seek(0) - output = self.service._backup_rbd(self.backup, rbdio, + output = self.service._backup_rbd(self.alt_backup, rbdio, vol_name, vol_length) - self.assertDictEqual({'parent_id': 'mock'}, output) + mock_create_base_image.assert_called_with(full_backup_name, + vol_length, client) + base_name = '{"base": "%s"}' % full_backup_name + self.assertEqual({'service_metadata': base_name}, output) @common_mocks @mock.patch('fcntl.fcntl', spec=True) @@ -595,7 +643,7 @@ class BackupCephTestCase(test.TestCase): self._try_delete_base_image(). """ backup_name = self.service._get_backup_base_name(self.volume_id, - diff_format=True) + self.alt_backup) def mock_write_data(): self.volume_file.seek(0) @@ -659,7 +707,7 @@ class BackupCephTestCase(test.TestCase): self.assertRaises( self.service.rbd.ImageNotFound, self.service.backup, - self.backup, rbdio) + self.alt_backup, rbdio) @common_mocks @mock.patch('fcntl.fcntl', spec=True) @@ -672,7 +720,7 @@ class BackupCephTestCase(test.TestCase): second exception occurs in self.delete_backup(). """ backup_name = self.service._get_backup_base_name(self.volume_id, - diff_format=True) + self.alt_backup) def mock_write_data(): self.volume_file.seek(0) @@ -730,12 +778,11 @@ class BackupCephTestCase(test.TestCase): self.assertRaises( self.service.rbd.ImageBusy, self.service.backup, - self.backup, rbdio) + self.alt_backup, rbdio) @common_mocks def test_backup_rbd_from_snap(self): - backup_name = self.service._get_backup_base_name(self.volume_id, - diff_format=True) + backup_name = self.service._get_backup_base_name(self.volume_id) vol_name = self.volume['name'] vol_length = self.service._get_volume_size_gb(self.volume) @@ -776,43 +823,36 @@ class BackupCephTestCase(test.TestCase): @common_mocks def test_backup_rbd_from_snap2(self): - backup_name = self.service._get_backup_base_name(self.volume_id, - diff_format=True) + base_name = self.service._get_backup_base_name(self.volume_id, + self.alt_backup) vol_name = self.volume['name'] vol_length = self.service._get_volume_size_gb(self.volume) self.mock_rbd.RBD().list = mock.Mock() - self.mock_rbd.RBD().list.return_value = [backup_name] + self.mock_rbd.RBD().list.return_value = [base_name] - with mock.patch.object(self.service, '_get_most_recent_snap') as \ - mock_get_most_recent_snap: - with mock.patch.object(self.service, '_get_backup_base_name') as \ - mock_get_backup_base_name: - with mock.patch.object(self.service, '_rbd_diff_transfer') as \ - mock_rbd_diff_transfer: - with mock.patch.object(self.service, '_get_new_snap_name') as \ - mock_get_new_snap_name: - mock_get_backup_base_name.return_value = ( - backup_name) - mock_get_most_recent_snap.return_value = ( - 'backup.mock.snap.153464362.12') - mock_get_new_snap_name.return_value = 'new_snap' - image = self.service.rbd.Image() - meta = linuxrbd.RBDImageMetadata(image, - 'pool_foo', - 'user_foo', - 'conf_foo') - rbdio = linuxrbd.RBDVolumeIOWrapper(meta) - rbdio.seek(0) - self.service._backup_rbd(self.backup, rbdio, - vol_name, vol_length) - mock_rbd_diff_transfer.assert_called_with( - vol_name, 'pool_foo', backup_name, - self.backup.container, src_user='user_foo', - src_conf='conf_foo', - dest_conf='/etc/ceph/ceph.conf', - dest_user='cinder', src_snap='new_snap', - from_snap='backup.mock.snap.153464362.12') + with mock.patch.object(self.service, '_get_backup_base_name') as \ + mock_get_backup_base_name: + with mock.patch.object(self.service, '_rbd_diff_transfer') as \ + mock_rbd_diff_transfer: + with mock.patch.object(self.service, '_get_new_snap_name') as \ + mock_get_new_snap_name: + mock_get_backup_base_name.return_value = base_name + mock_get_new_snap_name.return_value = 'new_snap' + image = self.service.rbd.Image() + meta = linuxrbd.RBDImageMetadata(image, 'pool_foo', + 'user_foo', 'conf_foo') + rbdio = linuxrbd.RBDVolumeIOWrapper(meta) + rbdio.seek(0) + self.service._backup_rbd(self.alt_backup, rbdio, vol_name, + vol_length) + mock_rbd_diff_transfer.assert_called_with( + vol_name, 'pool_foo', base_name, + self.backup.container, src_user='user_foo', + src_conf='conf_foo', + dest_conf='/etc/ceph/ceph.conf', + dest_user='cinder', src_snap='new_snap', + from_snap=None) @common_mocks def test_backup_vol_length_0(self): @@ -843,7 +883,7 @@ class BackupCephTestCase(test.TestCase): @common_mocks def test_restore(self): backup_name = self.service._get_backup_base_name(self.volume_id, - diff_format=True) + self.alt_backup) self.mock_rbd.RBD.return_value.list.return_value = [backup_name] @@ -865,7 +905,7 @@ class BackupCephTestCase(test.TestCase): with tempfile.NamedTemporaryFile() as test_file: self.volume_file.seek(0) - self.service.restore(self.backup, self.volume_id, + self.service.restore(self.alt_backup, self.volume_id, test_file) checksum = hashlib.sha256() @@ -960,8 +1000,7 @@ class BackupCephTestCase(test.TestCase): @common_mocks def test_delete_backup_snapshot(self): snap_name = 'backup.%s.snap.3824923.1412' % (uuid.uuid4()) - base_name = self.service._get_backup_base_name(self.volume_id, - diff_format=True) + base_name = self.service._get_backup_base_name(self.volume_id) self.mock_rbd.RBD.remove_snap = mock.Mock() thread_dict = {} @@ -991,16 +1030,16 @@ class BackupCephTestCase(test.TestCase): @mock.patch('cinder.backup.drivers.ceph.VolumeMetadataBackup', spec=True) def test_try_delete_base_image_diff_format(self, mock_meta_backup): backup_name = self.service._get_backup_base_name(self.volume_id, - diff_format=True) + self.alt_backup) self.mock_rbd.RBD.return_value.list.return_value = [backup_name] with mock.patch.object(self.service, '_delete_backup_snapshot') as \ mock_del_backup_snap: - snap_name = self.service._get_new_snap_name(self.backup_id) + snap_name = self.service._get_new_snap_name(self.alt_backup_id) mock_del_backup_snap.return_value = (snap_name, 0) - self.service.delete_backup(self.backup) + self.service.delete_backup(self.alt_backup) self.assertTrue(mock_del_backup_snap.called) self.assertTrue(self.mock_rbd.RBD.return_value.list.called) @@ -1010,7 +1049,7 @@ class BackupCephTestCase(test.TestCase): @mock.patch('cinder.backup.drivers.ceph.VolumeMetadataBackup', spec=True) def test_try_delete_base_image(self, mock_meta_backup): backup_name = self.service._get_backup_base_name(self.volume_id, - self.backup_id) + self.alt_backup) thread_dict = {} def mock_side_effect(ioctx, base_name): @@ -1019,7 +1058,7 @@ class BackupCephTestCase(test.TestCase): self.mock_rbd.RBD.return_value.list.return_value = [backup_name] self.mock_rbd.RBD.return_value.remove.side_effect = mock_side_effect with mock.patch.object(self.service, 'get_backup_snaps'): - self.service.delete_backup(self.backup) + self.service.delete_backup(self.alt_backup) self.assertTrue(self.mock_rbd.RBD.return_value.remove.called) self.assertNotEqual(threading.current_thread(), thread_dict['thread']) @@ -1028,7 +1067,7 @@ class BackupCephTestCase(test.TestCase): def test_try_delete_base_image_busy(self): """This should induce retries then raise rbd.ImageBusy.""" backup_name = self.service._get_backup_base_name(self.volume_id, - self.backup_id) + self.alt_backup) rbd = self.mock_rbd.RBD.return_value rbd.list.return_value = [backup_name] @@ -1038,7 +1077,7 @@ class BackupCephTestCase(test.TestCase): mock_get_backup_snaps: self.assertRaises(self.mock_rbd.ImageBusy, self.service._try_delete_base_image, - self.backup) + self.alt_backup) self.assertTrue(mock_get_backup_snaps.called) self.assertTrue(rbd.list.called) diff --git a/cinder/tests/unit/objects/test_objects.py b/cinder/tests/unit/objects/test_objects.py index 6d5b78c6548..3b18be4deb4 100644 --- a/cinder/tests/unit/objects/test_objects.py +++ b/cinder/tests/unit/objects/test_objects.py @@ -23,9 +23,9 @@ from cinder import test # NOTE: The hashes in this list should only be changed if they come with a # corresponding version bump in the affected objects. object_data = { - 'Backup': '1.6-c7ede487ba6fbcdd2a4711343cd972be', + 'Backup': '1.7-fffdbcd5da3c30750916fa2cc0e8ffb5', 'BackupDeviceInfo': '1.0-74b3950676c690538f4bc6796bd0042e', - 'BackupImport': '1.6-c7ede487ba6fbcdd2a4711343cd972be', + 'BackupImport': '1.7-fffdbcd5da3c30750916fa2cc0e8ffb5', 'BackupList': '1.0-15ecf022a68ddbb8c2a6739cfc9f8f5e', 'CleanupRequest': '1.0-e7c688b893e1d5537ccf65cc3eb10a28', 'Cluster': '1.1-e2c533eb8cdd8d229b6c45c6cf3a9e2c', diff --git a/releasenotes/notes/support-incremental-backup-completion-in-rbd-1f2165fefcc470d1.yaml b/releasenotes/notes/support-incremental-backup-completion-in-rbd-1f2165fefcc470d1.yaml new file mode 100644 index 00000000000..d8be52f1cc4 --- /dev/null +++ b/releasenotes/notes/support-incremental-backup-completion-in-rbd-1f2165fefcc470d1.yaml @@ -0,0 +1,5 @@ +--- +fixes: + - Fixed issue where all Ceph RBD backups would be incremental after the + first one. The driver now honors whether ``--incremental`` is specified or + not.