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

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
 

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.