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 <sxmatch1986@gmail.com>
Co-Authored-By: Sofia Enriquez <lsofia.enriquez@gmail.com>
(cherry picked from commit 5018727f8e)

Conflicts:
	cinder/tests/unit/backup/drivers/test_backup_ceph.py
Change-Id: I516b7c82b05b26e81195f7f106d43a9e0804082d

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)

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,
+    def _format_base_name(self, service_metadata):
-                              diff_format=False):
+        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:
+        if backup.service_metadata:
-                msg = _("Backup id required")
+            return self._format_base_name(backup.service_metadata)
-                raise exception.InvalidParameterValue(msg)
+
-            return utils.convert_str("volume-%s.backup.%s"
+        # 'parent' field will only be present in incremental backups. This is
-                                     % (volume_id, backup_id))
+        # 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 = None
-        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
 
-                # Create new base image
+        # If backup.parent_id is None performs full RBD backup
-                self._create_base_image(base_name, length, client)
+        if backup.parent_id is None:
-                image_created = True
+            base_name = self._get_backup_base_name(volume_id, backup=backup)
-            else:
+            from_snap, image_created = self._full_rbd_backup(backup.container,
-                # If a from_snap is defined and is present in the source volume
+                                                             base_name,
-                # image but does not exist in the backup base then we look down
+                                                             length)
-                # the list of source volume snapshots and find the latest one
+        # Otherwise performs incremental rbd backup
-                # for which a backup snapshot exist in the backup base. Until
+        else:
-                # that snapshot is reached, we delete all the other snapshots
+            # Find the base name from the parent backup's service_metadata
-                # for which backup snapshot does not exist.
+            base_name = self._get_backup_base_name(volume_id, backup=backup)
-                from_snap = self._get_most_recent_snap(source_rbd_image,
+            rbd_img = source_rbd_image
-                                                       base_name, client)
+            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
+        return {'service_metadata': '{"base": "%s"}' % base_name}
-        # 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
 
     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 backup.snapshot_id:
-        if self._file_is_rbd(volume_file):
+            do_full_backup = True
-            # If volume an RBD, attempt incremental backup.
+        elif self._file_is_rbd(volume_file):
-            LOG.debug("Volume file is RBD: attempting incremental backup.")
+            # 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,
+                updates = self._backup_rbd(backup, volume_file, volume.name,
-                                           volume.name, length)
+                                           length)
             except exception.BackupRBDOperationFailed:
-                LOG.debug("Forcing full backup of volume %s.", volume.id)
+                with excutils.save_and_reraise_exception():
-                do_full_backup = True
+                    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_name = self._get_backup_base_name(backup.volume_id,
-                                                     backup_id=backup.id,
-                                                     diff_format=diff_format)
 
             # 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,
+        if backup.service_metadata:
-                                               diff_format=True)
+            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:

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()

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):

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):
+    def test_get_backup_base_name_without_backup_param(self):
-        name = self.service._get_backup_base_name(self.volume_id,
+        """Test _get_backup_base_name without backup."""
-                                                  diff_format=True)
+        name = self.service._get_backup_base_name(self.volume_id)
         self.assertEqual("volume-%s.backup.base" % (self.volume_id), name)
 
-        self.assertRaises(exception.InvalidParameterValue,
+    @common_mocks
-                          self.service._get_backup_base_name,
+    def test_get_backup_base_name_w_backup_and_no_parent(self):
-                          self.volume_id)
+        """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')
+    @common_mocks
-        self.assertEqual("volume-%s.backup.%s" % (self.volume_id, '1234'),
+    def test_get_backup_base_name_w_backup_and_parent(self):
-                         name)
+        """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)
+                            output = self.service.backup(self.alt_backup,
-                            self.assertDictEqual({}, output)
+                                                         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):
+    def test_backup_volume_from_rbd_got_exception(self):
-        backup_name = self.service._get_backup_base_name(
+        base_name = self.service._get_backup_base_name(self.volume_id,
-            self.volume_id, diff_format=True)
+                                                       self.alt_backup)
 
-        self.mock_rbd.RBD().list.return_value = [backup_name]
+        self.mock_rbd.RBD().list.return_value = [base_name]
-        self.backup.parent_id = 'mock_parent_id'
 
         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.153464362.12',
-                             {'name': 'backup.mock.snap.199994362.10'}])
+                              'backup_id': 'mock_parent_id'},
-                        output = self.service.backup(self.backup, rbdio)
+                             {'name': 'backup.mock.snap.199994362.10',
-                        self.assertIsNone(output['parent_id'])
+                              '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(
+        base_name = self.service._get_backup_base_name(self.volume_id,
-            self.volume_id, diff_format=True)
+                                                       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.patch.object(self.service, '_get_backup_snap_name') as \
-                mock_get_backup_base_name, mock.patch.object(
+                mock_get_backup_snap_name, \
-                self.service, '_get_most_recent_snap') as mock_get_most_recent_snap, \
                 mock.patch.object(self.service, '_rbd_diff_transfer'):
-            mock_get_backup_base_name.return_value = backup_name
+            image = self.service.rbd.Image()
-            mock_get_most_recent_snap.return_value = (
+            mock_get_backup_snap_name.return_value = 'mock_snap_name'
-                'backup.mock.snap.153464362.12')
+            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,
+        backup_name = self.service._get_backup_base_name(self.volume_id)
-                                                         diff_format=True)
         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,
+        base_name = self.service._get_backup_base_name(self.volume_id,
-                                                         diff_format=True)
+                                                       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 \
+        with mock.patch.object(self.service, '_get_backup_base_name') as \
-                mock_get_most_recent_snap:
+                mock_get_backup_base_name:
-            with mock.patch.object(self.service, '_get_backup_base_name') as \
+            with mock.patch.object(self.service, '_rbd_diff_transfer') as \
-                    mock_get_backup_base_name:
+                    mock_rbd_diff_transfer:
-                with mock.patch.object(self.service, '_rbd_diff_transfer') as \
+                with mock.patch.object(self.service, '_get_new_snap_name') as \
-                        mock_rbd_diff_transfer:
+                        mock_get_new_snap_name:
-                    with mock.patch.object(self.service, '_get_new_snap_name') as \
+                    mock_get_backup_base_name.return_value = base_name
-                            mock_get_new_snap_name:
+                    mock_get_new_snap_name.return_value = 'new_snap'
-                        mock_get_backup_base_name.return_value = (
+                    image = self.service.rbd.Image()
-                            backup_name)
+                    meta = linuxrbd.RBDImageMetadata(image, 'pool_foo',
-                        mock_get_most_recent_snap.return_value = (
+                                                     'user_foo', 'conf_foo')
-                            'backup.mock.snap.153464362.12')
+                    rbdio = linuxrbd.RBDVolumeIOWrapper(meta)
-                        mock_get_new_snap_name.return_value = 'new_snap'
+                    rbdio.seek(0)
-                        image = self.service.rbd.Image()
+                    self.service._backup_rbd(self.alt_backup, rbdio, vol_name,
-                        meta = linuxrbd.RBDImageMetadata(image,
+                                             vol_length)
-                                                         'pool_foo',
+                    mock_rbd_diff_transfer.assert_called_with(
-                                                         'user_foo',
+                        vol_name, 'pool_foo', base_name,
-                                                         'conf_foo')
+                        self.backup.container, src_user='user_foo',
-                        rbdio = linuxrbd.RBDVolumeIOWrapper(meta)
+                        src_conf='conf_foo',
-                        rbdio.seek(0)
+                        dest_conf='/etc/ceph/ceph.conf',
-                        self.service._backup_rbd(self.backup, rbdio,
+                        dest_user='cinder', src_snap='new_snap',
-                                                 vol_name, vol_length)
+                        from_snap=None)
-                        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')
 
     @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,
+        base_name = self.service._get_backup_base_name(self.volume_id)
-                                                       diff_format=True)
         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)

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',

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.