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

diff --git a/cinder/tests/test_glusterfs.py b/cinder/tests/test_glusterfs.py
index b8a604d2647b171f6e20810da6df1206ce5cfeec..8214850e34d8692b1f7ba893f868d5d95ad4b1f5 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):