Fix restore point if backup base is diff-format in ceph

In Ceph when backup base is diff-format and use this backup
to restore volumes not rbd, need to find out the correct restore
point.

This is fixed in cinder/backup/drivers/ceph.py _diff_restore_allowed.

Before the change: when backup is diff-format, and destination volume
is not rbd, the function returns restore_point as None. As a result,
backup restore uses wrong image/snapshot to do restore.

With the change, we always get the right restore point, regardless
of the kind of restore we'll do differential or not.

Change-Id: I006c05d246f59fc6aff597543bedc68589d37576
Closes-Bug: #1472088

diff --git a/cinder/backup/drivers/ceph.py b/cinder/backup/drivers/ceph.py
index 089a5add79570f6df5987921a8c63b664033eec6..e197eb3fa727a56be84e73cd279aaefb8dc1a3b1 100644 (file)
--- a/cinder/backup/drivers/ceph.py
+++ b/cinder/backup/drivers/ceph.py
@@ -1033,41 +1033,45 @@ class CephBackupDriver(driver.BackupDriver):
 
     def _diff_restore_allowed(self, base_name, backup, volume, volume_file,
                               rados_client):
-        """Determine whether a differential restore is possible/allowed.
+        """Determine if differential restore is possible and restore point.
+
+        Determine whether a differential restore is possible/allowed,
+        and find out the restore point if backup base is diff-format.
 
         In order for a differential restore to be performed we need:
             * destination volume must be RBD
             * destination volume must have zero extents
             * backup base image must exist
             * backup must have a restore point
+            * target volume is different from source volume of backup
 
         Returns True if differential restore is allowed, False otherwise.
+        Return the restore point if back base is diff-format.
         """
-        not_allowed = (False, None)
-
-        if self._file_is_rbd(volume_file):
-            # NOTE(dosaboy): base_name here must be diff format.
-            rbd_exists, base_name = self._rbd_image_exists(base_name,
-                                                           backup['volume_id'],
-                                                           rados_client)
+        # NOTE(dosaboy): base_name here must be diff format.
+        rbd_exists, base_name = self._rbd_image_exists(base_name,
+                                                       backup['volume_id'],
+                                                       rados_client)
+
+        if not rbd_exists:
+            return False, None
+
+        # Get the restore point. If no restore point is found, we assume
+        # that the backup was not performed using diff/incremental methods
+        # so we enforce full copy.
+        restore_point = self._get_restore_point(base_name, backup['id'])
+
+        if restore_point:
+            if self._file_is_rbd(volume_file):
+
+                # If the volume we are restoring to is the volume the backup
+                # was made from, force a full restore since a diff will not
+                # work in this case.
+                if volume['id'] == backup['volume_id']:
+                    LOG.debug("Destination volume is same as backup source "
+                              "volume %s - forcing full copy.", volume['id'])
+                    return False, restore_point
 
-            if not rbd_exists:
-                return not_allowed
-
-            # Get the restore point. If no restore point is found, we assume
-            # that the backup was not performed using diff/incremental methods
-            # so we enforce full copy.
-            restore_point = self._get_restore_point(base_name, backup['id'])
-
-            # If the volume we are restoring to is the volume the backup was
-            # made from, force a full restore since a diff will not work in
-            # this case.
-            if volume['id'] == backup['volume_id']:
-                LOG.debug("Destination volume is same as backup source volume "
-                          "%s - forcing full copy.", volume['id'])
-                return False, restore_point
-
-            if restore_point:
                 # If the destination volume has extents we cannot allow a diff
                 # restore.
                 if self._rbd_has_extents(volume_file.rbd_image):
@@ -1077,14 +1081,14 @@ class CephBackupDriver(driver.BackupDriver):
                     return False, restore_point
 
                 return True, restore_point
-            else:
-                LOG.info(_LI("No restore point found for "
-                             "backup='%(backup)s' of "
-                             "volume %(volume)s - forcing full copy."),
-                         {'backup': backup['id'],
-                          'volume': backup['volume_id']})
-
-        return not_allowed
+        else:
+            LOG.info(_LI("No restore point found for backup="
+                         "'%(backup)s' of volume %(volume)s "
+                         "although base image is found - "
+                         "forcing full copy."),
+                     {'backup': backup['id'],
+                      'volume': backup['volume_id']})
+        return False, restore_point
 
     def _restore_volume(self, backup, volume, volume_file):
         """Restore volume from backup using diff transfer if possible.

diff --git a/cinder/tests/unit/test_backup_ceph.py b/cinder/tests/unit/test_backup_ceph.py
index b5e9d4399173963678eab6abdc30b220abbc62f8..4e51cce325343d5ecd8fc42a6953ac0312931e84 100644 (file)
--- a/cinder/tests/unit/test_backup_ceph.py
+++ b/cinder/tests/unit/test_backup_ceph.py
@@ -772,94 +772,149 @@ class BackupCephTestCase(test.TestCase):
             self.assertEqual(RAISED_EXCEPTIONS, [MockImageNotFoundException])
 
     @common_mocks
-    def test_diff_restore_allowed(self):
+    def test_diff_restore_allowed_with_image_not_exists(self):
+        '''Test diff restore not allowed when backup not diff-format.'''
         not_allowed = (False, None)
         backup_base = 'backup.base'
-        restore_point = 'backup.snap.1'
         rbd_io = self._get_wrapped_rbd_io(self.service.rbd.Image())
         args_vols_different = [backup_base, self.backup, self.alt_volume,
                                rbd_io, self.mock_rados]
-        args_vols_same = [backup_base, self.backup, self.volume, rbd_io,
-                          self.mock_rados]
 
-        # 1. destination volume is not an rbd
-        with mock.patch.object(self.service, '_file_is_rbd') as \
-                mock_file_is_rbd:
-            mock_file_is_rbd.return_value = False
+        with mock.patch.object(self.service, '_rbd_image_exists') as \
+                mock_rbd_image_exists:
+            mock_rbd_image_exists.return_value = (False, backup_base)
+
             resp = self.service._diff_restore_allowed(*args_vols_different)
-            self.assertEqual(resp, not_allowed)
-            self.assertTrue(mock_file_is_rbd.called)
 
-        # 1. destination volume is an rbd
-        # 2. backup base is not diff-format
-        with mock.patch.object(self.service, '_file_is_rbd') as \
-                mock_file_is_rbd:
-            mock_file_is_rbd.return_value = True
-            with mock.patch.object(self.service, '_rbd_image_exists') as \
-                    mock_rbd_image_exists:
-                mock_rbd_image_exists.return_value = (False, backup_base)
-                resp = self.service._diff_restore_allowed(*args_vols_different)
-                self.assertEqual(resp, not_allowed)
-                self.assertTrue(mock_file_is_rbd.called)
+            self.assertEqual(not_allowed, resp)
+            mock_rbd_image_exists.assert_called_once_with(
+                backup_base,
+                self.backup['volume_id'],
+                self.mock_rados)
+
+    @common_mocks
+    def test_diff_restore_allowed_with_no_restore_point(self):
+        '''Test diff restore not allowed when no restore point found.
+
+        Detail conditions:
+          1. backup base is diff-format
+          2. restore point does not exist
+        '''
+        not_allowed = (False, None)
+        backup_base = 'backup.base'
+        rbd_io = self._get_wrapped_rbd_io(self.service.rbd.Image())
+        args_vols_different = [backup_base, self.backup, self.alt_volume,
+                               rbd_io, self.mock_rados]
+
+        with mock.patch.object(self.service, '_rbd_image_exists') as \
+                mock_rbd_image_exists:
+            mock_rbd_image_exists.return_value = (True, backup_base)
+            with mock.patch.object(self.service, '_get_restore_point') as \
+                    mock_get_restore_point:
+                mock_get_restore_point.return_value = None
+
+                args = args_vols_different
+                resp = self.service._diff_restore_allowed(*args)
+
+                self.assertEqual(not_allowed, resp)
                 self.assertTrue(mock_rbd_image_exists.called)
+                mock_get_restore_point.assert_called_once_with(
+                    backup_base,
+                    self.backup['id'])
 
-        # 1. destination volume is an rbd
-        # 2. backup base is diff-format
-        # 3. restore point does not exist
-        # 4. source and destination volumes are different
-        with mock.patch.object(self.service, '_file_is_rbd') as \
-                mock_file_is_rbd:
-            mock_file_is_rbd.return_value = True
-            with mock.patch.object(self.service, '_rbd_image_exists') as \
-                    mock_rbd_image_exists:
-                mock_rbd_image_exists.return_value = (True, backup_base)
-                with mock.patch.object(self.service, '_get_restore_point') as \
-                        mock_get_restore_point:
-                    mock_get_restore_point.return_value = None
+    @common_mocks
+    def test_diff_restore_allowed_with_not_rbd(self):
+        '''Test diff restore not allowed when destination volume is not rbd.
+
+        Detail conditions:
+          1. backup base is diff-format
+          2. restore point exists
+          3. destination volume is not an rbd.
+        '''
+        backup_base = 'backup.base'
+        restore_point = 'backup.snap.1'
+        rbd_io = self._get_wrapped_rbd_io(self.service.rbd.Image())
+        args_vols_different = [backup_base, self.backup, self.alt_volume,
+                               rbd_io, self.mock_rados]
+
+        with mock.patch.object(self.service, '_rbd_image_exists') as \
+                mock_rbd_image_exists:
+            mock_rbd_image_exists.return_value = (True, backup_base)
+            with mock.patch.object(self.service, '_get_restore_point') as \
+                    mock_get_restore_point:
+                mock_get_restore_point.return_value = restore_point
+                with mock.patch.object(self.service, '_file_is_rbd') as \
+                        mock_file_is_rbd:
+                    mock_file_is_rbd.return_value = False
 
                     args = args_vols_different
                     resp = self.service._diff_restore_allowed(*args)
 
-                    self.assertEqual(resp, not_allowed)
-                    self.assertTrue(mock_file_is_rbd.called)
+                    self.assertEqual((False, restore_point), resp)
                     self.assertTrue(mock_rbd_image_exists.called)
                     self.assertTrue(mock_get_restore_point.called)
+                    mock_file_is_rbd.assert_called_once_with(
+                        rbd_io)
 
-        # 1. destination volume is an rbd
-        # 2. backup base is diff-format
-        # 3. restore point does not exist
-        # 4. source and destination volumes are the same
-        with mock.patch.object(self.service, '_file_is_rbd') as \
-                mock_file_is_rbd:
-            mock_file_is_rbd.return_value = True
-            with mock.patch.object(self.service, '_rbd_image_exists') as \
-                    mock_rbd_image_exists:
-                mock_rbd_image_exists.return_value = (True, backup_base)
-                with mock.patch.object(self.service, '_get_restore_point') as \
-                        mock_get_restore_point:
-                    mock_get_restore_point.return_value = None
+    @common_mocks
+    def test_diff_restore_allowed_with_same_volume(self):
+        '''Test diff restore not allowed when volumes are same.
+
+        Detail conditions:
+          1. backup base is diff-format
+          2. restore point exists
+          3. destination volume is an rbd
+          4. source and destination volumes are the same
+        '''
+        backup_base = 'backup.base'
+        restore_point = 'backup.snap.1'
+        rbd_io = self._get_wrapped_rbd_io(self.service.rbd.Image())
+        args_vols_same = [backup_base, self.backup, self.volume, rbd_io,
+                          self.mock_rados]
+
+        with mock.patch.object(self.service, '_rbd_image_exists') as \
+                mock_rbd_image_exists:
+            mock_rbd_image_exists.return_value = (True, backup_base)
+            with mock.patch.object(self.service, '_get_restore_point') as \
+                    mock_get_restore_point:
+                mock_get_restore_point.return_value = restore_point
+                with mock.patch.object(self.service, '_file_is_rbd') as \
+                        mock_file_is_rbd:
+                    mock_file_is_rbd.return_value = True
 
                     resp = self.service._diff_restore_allowed(*args_vols_same)
 
-                    self.assertEqual(resp, not_allowed)
-                    self.assertTrue(mock_file_is_rbd.called)
+                    self.assertEqual((False, restore_point), resp)
                     self.assertTrue(mock_rbd_image_exists.called)
                     self.assertTrue(mock_get_restore_point.called)
+                    self.assertTrue(mock_file_is_rbd.called)
 
-        # 1. destination volume is an rbd
-        # 2. backup base is diff-format
-        # 3. restore point exists
-        # 4. source and destination volumes are different
-        # 5. destination volume has data on it - full copy is mandated
-        with mock.patch.object(self.service, '_file_is_rbd') as \
-                mock_file_is_rbd:
-            mock_file_is_rbd.return_value = True
-            with mock.patch.object(self.service, '_rbd_image_exists') as \
-                    mock_rbd_image_exists:
-                mock_rbd_image_exists.return_value = (True, backup_base)
-                with mock.patch.object(self.service, '_get_restore_point') as \
-                        mock_get_restore_point:
-                    mock_get_restore_point.return_value = restore_point
+    @common_mocks
+    def test_diff_restore_allowed_with_has_extents(self):
+        '''Test diff restore not allowed when destination volume has data.
+
+        Detail conditions:
+          1. backup base is diff-format
+          2. restore point exists
+          3. destination volume is an rbd
+          4. source and destination volumes are different
+          5. destination volume has data on it - full copy is mandated
+        '''
+        backup_base = 'backup.base'
+        restore_point = 'backup.snap.1'
+        rbd_io = self._get_wrapped_rbd_io(self.service.rbd.Image())
+        args_vols_different = [backup_base, self.backup, self.alt_volume,
+                               rbd_io, self.mock_rados]
+
+        with mock.patch.object(self.service, '_rbd_image_exists') as \
+                mock_rbd_image_exists:
+            mock_rbd_image_exists.return_value = (True, backup_base)
+            with mock.patch.object(self.service, '_get_restore_point') as \
+                    mock_get_restore_point:
+                mock_get_restore_point.return_value = restore_point
+                with mock.patch.object(self.service, '_file_is_rbd') as \
+                        mock_file_is_rbd:
+                    mock_file_is_rbd.return_value = True
                     with mock.patch.object(self.service, '_rbd_has_extents') \
                             as mock_rbd_has_extents:
                         mock_rbd_has_extents.return_value = True
@@ -867,26 +922,39 @@ class BackupCephTestCase(test.TestCase):
                         args = args_vols_different
                         resp = self.service._diff_restore_allowed(*args)
 
-                        self.assertEqual(resp, (False, restore_point))
-                        self.assertTrue(mock_file_is_rbd.called)
+                        self.assertEqual((False, restore_point), resp)
                         self.assertTrue(mock_rbd_image_exists.called)
                         self.assertTrue(mock_get_restore_point.called)
-                        self.assertTrue(mock_rbd_has_extents.called)
+                        self.assertTrue(mock_file_is_rbd.called)
+                        mock_rbd_has_extents.assert_called_once_with(
+                            rbd_io.rbd_image)
 
-        # 1. destination volume is an rbd
-        # 2. backup base is diff-format
-        # 3. restore point exists
-        # 4. source and destination volumes are different
-        # 5. destination volume no data on it
-        with mock.patch.object(self.service, '_file_is_rbd') as \
-                mock_file_is_rbd:
-            mock_file_is_rbd.return_value = True
-            with mock.patch.object(self.service, '_rbd_image_exists') as \
-                    mock_rbd_image_exists:
-                mock_rbd_image_exists.return_value = (True, backup_base)
-                with mock.patch.object(self.service, '_get_restore_point') as \
-                        mock_restore_point:
-                    mock_restore_point.return_value = restore_point
+    @common_mocks
+    def test_diff_restore_allowed_with_no_extents(self):
+        '''Test diff restore allowed when no data in destination volume.
+
+        Detail conditions:
+          1. backup base is diff-format
+          2. restore point exists
+          3. destination volume is an rbd
+          4. source and destination volumes are different
+          5. destination volume no data on it
+        '''
+        backup_base = 'backup.base'
+        restore_point = 'backup.snap.1'
+        rbd_io = self._get_wrapped_rbd_io(self.service.rbd.Image())
+        args_vols_different = [backup_base, self.backup, self.alt_volume,
+                               rbd_io, self.mock_rados]
+
+        with mock.patch.object(self.service, '_rbd_image_exists') as \
+                mock_rbd_image_exists:
+            mock_rbd_image_exists.return_value = (True, backup_base)
+            with mock.patch.object(self.service, '_get_restore_point') as \
+                    mock_get_restore_point:
+                mock_get_restore_point.return_value = restore_point
+                with mock.patch.object(self.service, '_file_is_rbd') as \
+                        mock_file_is_rbd:
+                    mock_file_is_rbd.return_value = True
                     with mock.patch.object(self.service, '_rbd_has_extents') \
                             as mock_rbd_has_extents:
                         mock_rbd_has_extents.return_value = False
@@ -894,11 +962,11 @@ class BackupCephTestCase(test.TestCase):
                         args = args_vols_different
                         resp = self.service._diff_restore_allowed(*args)
 
-                        self.assertEqual(resp, (True, restore_point))
-                        self.assertTrue(mock_restore_point.called)
-                        self.assertTrue(mock_rbd_has_extents.called)
+                        self.assertEqual((True, restore_point), resp)
                         self.assertTrue(mock_rbd_image_exists.called)
+                        self.assertTrue(mock_get_restore_point.called)
                         self.assertTrue(mock_file_is_rbd.called)
+                        self.assertTrue(mock_rbd_has_extents.called)
 
     @common_mocks
     @mock.patch('fcntl.fcntl', spec=True)