From: Thang Pham Date: Thu, 29 May 2014 03:07:49 +0000 (-0400) Subject: GlusterFS: Handle deletion of snapshot with no backing file X-Git-Url: https://review.fuel-infra.org/gitweb?a=commitdiff_plain;h=3fd0a403b2688a52603accb9cc530a6b0ccad066;p=openstack-build%2Fcinder-build.git 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 8c2eb2eb1..a05d455a3 100644 --- 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 8d75d3486..246b5501c 100644 --- 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.