GlusterFS: Handle deletion of snapshot with no backing file

If a glusterfs volume is in-use, nova is called to delete a
volume snapshot.  It is possible for a timeout to occur, since
cinder is polling for nova task progress.  In such case, a
GlusterfsException is thrown, but nova continues to process the
snapshot delete and commits the changes.  Cinder is not aware of
this, and the actual snapshot entry in cinder is prevented from
being deleted, since no backing file exists for the out-of-sync
snapshot.  This patch allows a snapshot with no backing file to
be deleted from cinder and updates the volume info file.

Change-Id: I83c8a7242199edbfd1897297589ac7deb42c5865
Closes-Bug: #1311182

diff --git a/cinder/tests/test_glusterfs.py b/cinder/tests/test_glusterfs.py
index 8c2eb2eb1a738128f53497dcbadbac02982dfdfa..a05d455a351610b2808bf862e292f377dd70065b 100644 (file)
--- a/cinder/tests/test_glusterfs.py
+++ b/cinder/tests/test_glusterfs.py
@@ -713,12 +713,14 @@ class GlusterFsDriverTestCase(test.TestCase):
                 mock.patch.object(self._driver, '_local_volume_dir'),
                 mock.patch.object(self._driver, 'get_active_image_from_info'),
                 mock.patch.object(self._driver, '_execute'),
+                mock.patch.object(self._driver, '_local_path_volume'),
                 mock.patch.object(self._driver, '_local_path_volume_info')
         ) as (mock_ensure_share_mounted, mock_local_volume_dir,
               mock_active_image_from_info, mock_execute,
-              mock_local_path_volume_info):
+              mock_local_path_volume, mock_local_path_volume_info):
             mock_local_volume_dir.return_value = self.TEST_MNT_POINT
             mock_active_image_from_info.return_value = volume_filename
+            mock_local_path_volume.return_value = volume_path
             mock_local_path_volume_info.return_value = info_file
 
             self._driver.delete_volume(volume)
@@ -730,7 +732,9 @@ class GlusterFsDriverTestCase(test.TestCase):
             mock_execute.assert_called_once_with('rm', '-f', volume_path,
                                                  run_as_root=True)
             mock_local_path_volume_info.assert_called_once_with(volume)
-            mock_delete_if_exists.assert_called_once_with(info_file)
+            mock_local_path_volume.assert_called_once_with(volume)
+            mock_delete_if_exists.assert_any_call(volume_path)
+            mock_delete_if_exists.assert_any_call(info_file)
 
     def test_refresh_mounts(self):
         with contextlib.nested(
@@ -1734,6 +1738,136 @@ class GlusterFsDriverTestCase(test.TestCase):
 
         mox.VerifyAll()
 
+    @mock.patch('cinder.volume.drivers.glusterfs.GlusterfsDriver.'
+                '_delete_stale_snapshot')
+    @mock.patch('cinder.volume.drivers.glusterfs.GlusterfsDriver.'
+                'get_active_image_from_info')
+    @mock.patch('cinder.volume.drivers.glusterfs.GlusterfsDriver.'
+                '_qemu_img_info')
+    @mock.patch('cinder.volume.drivers.glusterfs.GlusterfsDriver.'
+                '_read_info_file')
+    @mock.patch('cinder.volume.drivers.glusterfs.GlusterfsDriver.'
+                '_local_path_volume')
+    @mock.patch('cinder.volume.drivers.glusterfs.GlusterfsDriver.'
+                '_local_volume_dir')
+    @mock.patch('cinder.volume.drivers.glusterfs.GlusterfsDriver.'
+                '_ensure_share_writable')
+    def test_delete_snapshot_online_stale_snapshot(self,
+                                                   mock_ensure_share_writable,
+                                                   mock_local_volume_dir,
+                                                   mock_local_path_volume,
+                                                   mock_read_info_file,
+                                                   mock_qemu_img_info,
+                                                   mock_get_active_image,
+                                                   mock_delete_stale_snap):
+        volume = self._simple_volume()
+        ctxt = context.RequestContext('fake_user', 'fake_project')
+        volume['status'] = 'in-use'
+        volume_filename = 'volume-%s' % self.VOLUME_UUID
+        volume_path = '%s/%s' % (self.TEST_MNT_POINT, volume_filename)
+        info_path = volume_path + '.info'
+        stale_snapshot = {'name': 'fake-volume',
+                          'volume_id': self.VOLUME_UUID,
+                          'volume': volume,
+                          'id': self.SNAP_UUID_2,
+                          'context': ctxt}
+        active_snap_file = volume['name'] + '.' + self.SNAP_UUID_2
+        stale_snap_file = volume['name'] + '.' + stale_snapshot['id']
+        stale_snap_path = '%s/%s' % (self.TEST_MNT_POINT, stale_snap_file)
+        snap_info = {'active': active_snap_file,
+                     stale_snapshot['id']: stale_snap_file}
+        qemu_img_info = imageutils.QemuImgInfo()
+        qemu_img_info.file_format = 'qcow2'
+
+        mock_local_path_volume.return_value = volume_path
+        mock_read_info_file.return_value = snap_info
+        mock_local_volume_dir.return_value = self.TEST_MNT_POINT
+        mock_qemu_img_info.return_value = qemu_img_info
+        mock_get_active_image.return_value = active_snap_file
+
+        self._driver.delete_snapshot(stale_snapshot)
+
+        mock_ensure_share_writable.assert_called_once_with(
+            self.TEST_MNT_POINT)
+        mock_local_path_volume.assert_called_once_with(
+            stale_snapshot['volume'])
+        mock_read_info_file.assert_called_once_with(info_path,
+                                                    empty_if_missing=True)
+        mock_qemu_img_info.assert_called_once_with(stale_snap_path)
+        mock_get_active_image.assert_called_once_with(
+            stale_snapshot['volume'])
+        mock_delete_stale_snap.assert_called_once_with(stale_snapshot)
+
+    @mock.patch('cinder.volume.drivers.glusterfs.GlusterfsDriver.'
+                '_write_info_file')
+    @mock.patch('cinder.openstack.common.fileutils.delete_if_exists')
+    @mock.patch('cinder.volume.drivers.glusterfs.GlusterfsDriver.'
+                'get_active_image_from_info')
+    @mock.patch('cinder.volume.drivers.glusterfs.GlusterfsDriver.'
+                '_local_volume_dir')
+    @mock.patch('cinder.volume.drivers.glusterfs.GlusterfsDriver.'
+                '_read_info_file')
+    @mock.patch('cinder.volume.drivers.glusterfs.GlusterfsDriver.'
+                '_local_path_volume')
+    def test_delete_stale_snapshot(self, mock_local_path_volume,
+                                   mock_read_info_file,
+                                   mock_local_volume_dir,
+                                   mock_get_active_image,
+                                   mock_delete_if_exists,
+                                   mock_write_info_file):
+        volume = self._simple_volume()
+        volume['status'] = 'in-use'
+        volume_filename = 'volume-%s' % self.VOLUME_UUID
+        volume_path = '%s/%s' % (self.TEST_MNT_POINT, volume_filename)
+        info_path = volume_path + '.info'
+
+        # Test case where snapshot_file = active_file
+        snapshot = {'name': 'fake-volume',
+                    'volume_id': self.VOLUME_UUID,
+                    'volume': volume,
+                    'id': self.SNAP_UUID_2}
+        active_snap_file = volume['name'] + '.' + self.SNAP_UUID_2
+        stale_snap_file = volume['name'] + '.' + snapshot['id']
+        stale_snap_path = '%s/%s' % (self.TEST_MNT_POINT, stale_snap_file)
+        snap_info = {'active': active_snap_file,
+                     snapshot['id']: stale_snap_file}
+
+        mock_local_path_volume.return_value = volume_path
+        mock_read_info_file.return_value = snap_info
+        mock_get_active_image.return_value = active_snap_file
+        mock_local_volume_dir.return_value = self.TEST_MNT_POINT
+
+        self._driver._delete_stale_snapshot(snapshot)
+
+        mock_local_path_volume.assert_called_with(snapshot['volume'])
+        mock_read_info_file.assert_called_with(info_path)
+        mock_delete_if_exists.assert_not_called()
+        mock_write_info_file.assert_not_called()
+
+        # Test case where snapshot_file != active_file
+        snapshot = {'name': 'fake-volume',
+                    'volume_id': self.VOLUME_UUID,
+                    'volume': volume,
+                    'id': self.SNAP_UUID}
+        active_snap_file = volume['name'] + '.' + self.SNAP_UUID_2
+        stale_snap_file = volume['name'] + '.' + snapshot['id']
+        stale_snap_path = '%s/%s' % (self.TEST_MNT_POINT, stale_snap_file)
+        snap_info = {'active': active_snap_file,
+                     snapshot['id']: stale_snap_file}
+
+        mock_local_path_volume.return_value = volume_path
+        mock_read_info_file.return_value = snap_info
+        mock_get_active_image.return_value = active_snap_file
+        mock_local_volume_dir.return_value = self.TEST_MNT_POINT
+
+        self._driver._delete_stale_snapshot(snapshot)
+
+        mock_local_path_volume.assert_called_with(snapshot['volume'])
+        mock_read_info_file.assert_called_with(info_path)
+        mock_delete_if_exists.assert_called_once_with(stale_snap_path)
+        snap_info.pop(snapshot['id'], None)
+        mock_write_info_file.assert_called_once_with(info_path, snap_info)
+
     def test_get_backing_chain_for_path(self):
         (mox, drv) = self._mox, self._driver
 

diff --git a/cinder/volume/drivers/glusterfs.py b/cinder/volume/drivers/glusterfs.py
index 8d75d3486e99add97400c8f4963e8e2cad0792a9..246b5501c2806025027fa534800cac70e2ec75d3 100644 (file)
--- a/cinder/volume/drivers/glusterfs.py
+++ b/cinder/volume/drivers/glusterfs.py
@@ -333,6 +333,11 @@ class GlusterfsDriver(nfs.RemoteFsDriver):
 
         self._execute('rm', '-f', mounted_path, run_as_root=True)
 
+        # If an exception (e.g. timeout) occurred during delete_snapshot, the
+        # base volume may linger around, so just delete it if it exists
+        base_volume_path = self._local_path_volume(volume)
+        fileutils.delete_if_exists(base_volume_path)
+
         info_path = self._local_path_volume_info(volume)
         fileutils.delete_if_exists(info_path)
 
@@ -661,8 +666,13 @@ class GlusterfsDriver(nfs.RemoteFsDriver):
             if base_file is None:
                 # There should always be at least the original volume
                 # file as base.
-                msg = _('No base file found for %s.') % snapshot_path
-                raise exception.GlusterfsException(msg)
+                msg = _('No backing file found for %s, allowing snapshot '
+                        'to be deleted.') % snapshot_path
+                LOG.warn(msg)
+
+                # Snapshot may be stale, so just delete it and update the
+                # info file instead of blocking
+                return self._delete_stale_snapshot(snapshot)
 
             base_path = os.path.join(
                 self._local_volume_dir(snapshot['volume']), base_file)
@@ -881,6 +891,23 @@ class GlusterfsDriver(nfs.RemoteFsDriver):
             self._local_volume_dir(snapshot['volume']), file_to_delete)
         self._execute('rm', '-f', path_to_delete, run_as_root=True)
 
+    def _delete_stale_snapshot(self, snapshot):
+        info_path = self._local_path_volume(snapshot['volume']) + '.info'
+        snap_info = self._read_info_file(info_path)
+
+        if snapshot['id'] in snap_info:
+            snapshot_file = snap_info[snapshot['id']]
+            active_file = self.get_active_image_from_info(snapshot['volume'])
+            snapshot_path = os.path.join(
+                self._local_volume_dir(snapshot['volume']), snapshot_file)
+            if (snapshot_file == active_file):
+                return
+
+            LOG.info(_('Deleting stale snapshot: %s') % snapshot['id'])
+            fileutils.delete_if_exists(snapshot_path)
+            del(snap_info[snapshot['id']])
+            self._write_info_file(info_path, snap_info)
+
     def _get_backing_chain_for_path(self, volume, path):
         """Returns list of dicts containing backing-chain information.