From 5f00cad02da1093d71f636add0810a538cbd444f Mon Sep 17 00:00:00 2001 From: Thang Pham Date: Tue, 8 Apr 2014 16:45:34 -0400 Subject: [PATCH] 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-. Instead, the snapshot file has the name volume-.. 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 --- cinder/tests/test_glusterfs.py | 71 ++++++++++++------------------ cinder/volume/drivers/glusterfs.py | 10 +++-- 2 files changed, 33 insertions(+), 48 deletions(-) diff --git a/cinder/tests/test_glusterfs.py b/cinder/tests/test_glusterfs.py index b8a604d26..8214850e3 100644 --- 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 25000cfba..9df0fee26 100644 --- 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): -- 2.45.2