GlusterFS: Delete active snapshot file on volume delete.

If a snapshot is taken of a volume that is attached to an active
instance, the volume file used by the instance will be switched to
the new snapshot file that is created.  When you delete the
snapshot, the base volume file will be merged with the snapshot
file and the base volume is deleted.  Upon a deleting the active
volume, the active snapshot file is not deleted because it does not
have the expected name that cinder is looking for, i.e.
volume-<uuid>.  Instead, the snapshot file has the name
volume-<uuid>.<snapshot-uuid>.  This patch looks at the volume info
file to find any active snapshot file and properly delete it when
the volume is deleted.

Change-Id: Ib0af4401d839ec3bd1eb3a81e1671811e0d4a288
Closes-Bug: #1300303
(cherry picked from commit 5f00cad02da1093d71f636add0810a538cbd444f)

diff --git a/cinder/tests/test_glusterfs.py b/cinder/tests/test_glusterfs.py
index 7f1f7d1e35b2ebd961cb765107b20af06b3e0bcd..ca145f0f4fcb1f4c7031dcb7379e159eebeefaf2 100644 (file)
--- a/cinder/tests/test_glusterfs.py
+++ b/cinder/tests/test_glusterfs.py
@@ -685,28 +685,36 @@ class GlusterFsDriverTestCase(test.TestCase):
 
         drv.create_cloned_volume(volume, src_vref)
 
-    def test_delete_volume(self):
-        """delete_volume simple test case."""
-        mox = self._mox
-        drv = self._driver
-
-        self.stub_out_not_replaying(drv, '_ensure_share_mounted')
-
-        volume = DumbVolume()
-        volume['name'] = 'volume-123'
-        volume['provider_location'] = self.TEST_EXPORT1
-
-        mox.StubOutWithMock(drv, 'local_path')
-        drv.local_path(volume).AndReturn(self.TEST_LOCAL_PATH)
-
-        mox.StubOutWithMock(drv, '_execute')
-        drv._execute('rm', '-f', self.TEST_LOCAL_PATH, run_as_root=True)
+    @mock.patch('cinder.openstack.common.fileutils.delete_if_exists')
+    def test_delete_volume(self, mock_delete_if_exists):
+        volume = self._simple_volume()
+        volume_filename = 'volume-%s' % self.VOLUME_UUID
+        volume_path = '%s/%s' % (self.TEST_MNT_POINT, volume_filename)
+        info_file = volume_path + '.info'
 
-        mox.ReplayAll()
+        with contextlib.nested(
+                mock.patch.object(self._driver, '_ensure_share_mounted'),
+                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_info')
+        ) as (mock_ensure_share_mounted, mock_local_volume_dir,
+              mock_active_image_from_info, mock_execute,
+              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_info.return_value = info_file
 
-        drv.delete_volume(volume)
+            self._driver.delete_volume(volume)
 
-        mox.VerifyAll()
+            mock_ensure_share_mounted.assert_called_once_with(
+                volume['provider_location'])
+            mock_local_volume_dir.assert_called_once_with(volume)
+            mock_active_image_from_info.assert_called_once_with(volume)
+            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)
 
     def test_delete_should_ensure_share_mounted(self):
         """delete_volume should ensure that corresponding share is mounted."""
@@ -747,31 +755,6 @@ class GlusterFsDriverTestCase(test.TestCase):
 
         mox.VerifyAll()
 
-    @mock.patch('os.remove')
-    @mock.patch('os.path.exists')
-    def test_delete_volume_with_info_file(self, mock_path_exists, mock_remove):
-        mock_path_exists.return_value = True
-        info_file = self.TEST_LOCAL_PATH + '.info'
-        volume = self._simple_volume()
-
-        with contextlib.nested(
-                mock.patch.object(self._driver, '_ensure_share_mounted'),
-                mock.patch.object(self._driver, 'local_path'),
-                mock.patch.object(self._driver, '_execute')
-        ) as (mock_ensure_share_mounted, mock_local_path, mock_execute):
-            mock_local_path.return_value = self.TEST_LOCAL_PATH
-
-            self._driver.delete_volume(volume)
-
-            mock_ensure_share_mounted.assert_called_once_with(
-                volume['provider_location'])
-            mock_local_path.assert_called_once_with(volume)
-            mock_execute.assert_called_once_with('rm', '-f',
-                                                 self.TEST_LOCAL_PATH,
-                                                 run_as_root=True)
-            mock_path_exists.assert_called_once_with(info_file)
-            mock_remove.assert_called_once_with(info_file)
-
     def test_create_snapshot(self):
         (mox, drv) = self._mox, self._driver
 

diff --git a/cinder/volume/drivers/glusterfs.py b/cinder/volume/drivers/glusterfs.py
index 25000cfba63d3df7b96b2f561f8b3d260f0f0207..9df0fee262fc204cbb012492c1eda56d6ebdabed 100644 (file)
--- a/cinder/volume/drivers/glusterfs.py
+++ b/cinder/volume/drivers/glusterfs.py
@@ -28,6 +28,7 @@ from cinder import compute
 from cinder import db
 from cinder import exception
 from cinder.image import image_utils
+from cinder.openstack.common import fileutils
 from cinder.openstack.common import log as logging
 from cinder.openstack.common import processutils
 from cinder import units
@@ -293,13 +294,14 @@ class GlusterfsDriver(nfs.RemoteFsDriver):
 
         self._ensure_share_mounted(volume['provider_location'])
 
-        mounted_path = self.local_path(volume)
+        volume_dir = self._local_volume_dir(volume)
+        mounted_path = os.path.join(volume_dir,
+                                    self.get_active_image_from_info(volume))
 
         self._execute('rm', '-f', mounted_path, run_as_root=True)
 
-        info_path = mounted_path + '.info'
-        if os.path.exists(info_path):
-            os.remove(info_path)
+        info_path = self._local_path_volume_info(volume)
+        fileutils.delete_if_exists(info_path)
 
     @utils.synchronized('glusterfs', external=False)
     def create_snapshot(self, snapshot):