From 0a360207112f5d21eb482feddfd88d5b6e10fdaa Mon Sep 17 00:00:00 2001 From: Lucian Petrut Date: Sun, 21 Jun 2015 13:59:59 +0300 Subject: [PATCH] RemoteFS: Fix the offline snapshot delete operation The RemoteFS snapshot driver commits the wrong differencing disk when performing an offline snapshot delete. Because of this issue, the same snapshot will represent a different point in time if the previous one is deleted. More information about this situation is provided in the bug description. This patch fixes the issue by commiting the right image in this case. Change-Id: Ifb5e840e5a918d727275a2c0be783e1fe045e798 Closes-bug: #1468445 --- cinder/tests/unit/test_remotefs.py | 45 +++--------- cinder/volume/drivers/remotefs.py | 107 ++++++++++------------------- 2 files changed, 45 insertions(+), 107 deletions(-) diff --git a/cinder/tests/unit/test_remotefs.py b/cinder/tests/unit/test_remotefs.py index a17723d2d..df077b87e 100644 --- a/cinder/tests/unit/test_remotefs.py +++ b/cinder/tests/unit/test_remotefs.py @@ -53,12 +53,9 @@ class RemoteFsSnapDriverTestCase(test.TestCase): def _test_delete_snapshot(self, volume_in_use=False, stale_snapshot=False, - is_active_image=True, - highest_file_exists=False): + is_active_image=True): # If the snapshot is not the active image, it is guaranteed that # another snapshot exists having it as backing file. - # If yet another file is backed by the file from the next level, - # it means that the 'highest file' exists and it needs to be rebased. fake_snapshot_name = os.path.basename(self._FAKE_SNAPSHOT_PATH) fake_info = {'active': fake_snapshot_name, @@ -71,6 +68,7 @@ class RemoteFsSnapDriverTestCase(test.TestCase): fake_snap_img_info.backing_file = self._FAKE_VOLUME_NAME fake_snap_img_info.file_format = 'qcow2' fake_base_img_info.backing_file = None + fake_base_img_info.file_format = 'raw' self._driver._local_path_volume_info = mock.Mock( return_value=mock.sentinel.fake_info_path) @@ -121,6 +119,7 @@ class RemoteFsSnapDriverTestCase(test.TestCase): self._driver._img_commit.assert_called_once_with( self._FAKE_SNAPSHOT_PATH) + self.assertNotIn(self._FAKE_SNAPSHOT_ID, fake_info) self._driver._write_info_file.assert_called_once_with( mock.sentinel.fake_info_path, fake_info) else: @@ -139,31 +138,10 @@ class RemoteFsSnapDriverTestCase(test.TestCase): fake_info[fake_upper_snap_id] = fake_upper_snap_name fake_info[self._FAKE_SNAPSHOT_ID] = fake_snapshot_name - - if highest_file_exists: - fake_highest_snap_id = 'fake_highest_snap_id' - fake_highest_snap_path = ( - self._FAKE_VOLUME_PATH + '-snapshot' + - fake_highest_snap_id) - fake_highest_snap_name = os.path.basename( - fake_highest_snap_path) - - fake_highest_snap_info = { - 'filename': fake_highest_snap_name, - 'backing-filename': fake_upper_snap_name, - } - fake_backing_chain.insert(0, fake_highest_snap_info) - - fake_info['active'] = fake_highest_snap_name - fake_info[fake_highest_snap_id] = fake_highest_snap_name - else: - fake_info['active'] = fake_upper_snap_name + fake_info['active'] = fake_upper_snap_name expected_info = copy.deepcopy(fake_info) - expected_info[fake_upper_snap_id] = fake_snapshot_name del expected_info[self._FAKE_SNAPSHOT_ID] - if not highest_file_exists: - expected_info['active'] = fake_snapshot_name self._driver._read_info_file.return_value = fake_info self._driver._get_backing_chain_for_path = mock.Mock( @@ -171,12 +149,11 @@ class RemoteFsSnapDriverTestCase(test.TestCase): self._driver._delete_snapshot(self._FAKE_SNAPSHOT) - self._driver._img_commit.assert_any_call( - fake_upper_snap_path) - if highest_file_exists: - self._driver._rebase_img.assert_called_once_with( - fake_highest_snap_path, fake_snapshot_name, 'qcow2') - + self._driver._img_commit.assert_called_once_with( + self._FAKE_SNAPSHOT_PATH) + self._driver._rebase_img.assert_called_once_with( + fake_upper_snap_path, self._FAKE_VOLUME_NAME, + fake_base_img_info.file_format) self._driver._write_info_file.assert_called_once_with( mock.sentinel.fake_info_path, expected_info) @@ -193,10 +170,6 @@ class RemoteFsSnapDriverTestCase(test.TestCase): def test_delete_snapshot_with_one_upper_file(self): self._test_delete_snapshot(is_active_image=False) - def test_delete_snapshot_with_two_or_more_upper_files(self): - self._test_delete_snapshot(is_active_image=False, - highest_file_exists=True) - def test_delete_stale_snapshot(self): fake_snapshot_name = os.path.basename(self._FAKE_SNAPSHOT_PATH) fake_snap_info = { diff --git a/cinder/volume/drivers/remotefs.py b/cinder/volume/drivers/remotefs.py index 3cadd7c4a..620e86e09 100644 --- a/cinder/volume/drivers/remotefs.py +++ b/cinder/volume/drivers/remotefs.py @@ -919,8 +919,8 @@ class RemoteFSSnapDriver(RemoteFSDriver, driver.SnapshotVD): msg = _('Volume status must be "available" or "in-use".') raise exception.InvalidVolume(msg) - self._ensure_share_writable( - self._local_volume_dir(snapshot['volume'])) + vol_path = self._local_volume_dir(snapshot['volume']) + self._ensure_share_writable(vol_path) # Determine the true snapshot file for this snapshot # based on the .info file @@ -946,7 +946,20 @@ class RemoteFSSnapDriver(RemoteFSDriver, driver.SnapshotVD): snapshot_path, snapshot['volume']['name']) - vol_path = self._local_volume_dir(snapshot['volume']) + base_file = snapshot_path_img_info.backing_file + if base_file is None: + # There should always be at least the original volume + # file as base. + LOG.warning(_LW('No backing file found for %s, allowing ' + 'snapshot to be deleted.'), snapshot_path) + + # 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(vol_path, base_file) + base_file_img_info = self._qemu_img_info(base_path, + snapshot['volume']['name']) # Find what file has this as its backing file active_file = self.get_active_image_from_info(snapshot['volume']) @@ -956,22 +969,6 @@ class RemoteFSSnapDriver(RemoteFSDriver, driver.SnapshotVD): # Online delete context = snapshot['context'] - base_file = snapshot_path_img_info.backing_file - if base_file is None: - # There should always be at least the original volume - # file as base. - LOG.warning(_LW('No backing file found for %s, allowing ' - 'snapshot to be deleted.'), snapshot_path) - - # 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) - base_file_img_info = self._qemu_img_info( - base_path, - snapshot['volume']['name']) new_base_file = base_file_img_info.backing_file base_id = None @@ -997,28 +994,21 @@ class RemoteFSSnapDriver(RemoteFSDriver, driver.SnapshotVD): online_delete_info) if snapshot_file == active_file: - # Need to merge snapshot_file into its backing file # There is no top file # T0 | T1 | # base | snapshot_file | None - # (guaranteed to| (being deleted) | - # exist) | | - - base_file = snapshot_path_img_info.backing_file + # (guaranteed to| (being deleted, | + # exist) | commited down) | self._img_commit(snapshot_path) - - # Remove snapshot_file from info - del(snap_info[snapshot['id']]) # Active file has changed snap_info['active'] = base_file - self._write_info_file(info_path, snap_info) else: - # T0 | T1 | T2 | T3 - # base | snapshot_file | higher_file | highest_file - # (guaranteed to | (being deleted)|(guaranteed to | (may exist, - # exist, not | | exist, being | needs ptr - # used here) | | committed down)| update if so) + # T0 | T1 | T2 | T3 + # base | snapshot_file | higher_file | highest_file + # (guaranteed to | (being deleted, | (guaranteed to | (may exist) + # exist, not | commited down) | exist, needs | + # used here) | | ptr update) | backing_chain = self._get_backing_chain_for_path( snapshot['volume'], active_file_path) @@ -1043,37 +1033,15 @@ class RemoteFSSnapDriver(RemoteFSDriver, driver.SnapshotVD): higher_file raise exception.RemoteFSException(msg) - # Is there a file depending on higher_file? - highest_file = next((os.path.basename(f['filename']) - for f in backing_chain - if f.get('backing-filename', '') == - higher_file), - None) - if highest_file is None: - LOG.debug('No file depends on %s.', higher_file) + self._img_commit(snapshot_path) - # Committing higher_file into snapshot_file - # And update pointer in highest_file higher_file_path = os.path.join(vol_path, higher_file) - self._img_commit(higher_file_path) - if highest_file is not None: - highest_file_path = os.path.join(vol_path, highest_file) - snapshot_file_fmt = snapshot_path_img_info.file_format - self._rebase_img(highest_file_path, snapshot_file, - snapshot_file_fmt) - - # Remove snapshot_file from info - del(snap_info[snapshot['id']]) - snap_info[higher_id] = snapshot_file - if higher_file == active_file: - if highest_file is not None: - msg = _('Check condition failed: ' - '%s expected to be None.') % 'highest_file' - raise exception.RemoteFSException(msg) - # Active file has changed - snap_info['active'] = snapshot_file + base_file_fmt = base_file_img_info.file_format + self._rebase_img(higher_file_path, base_file, base_file_fmt) - self._write_info_file(info_path, snap_info) + # Remove snapshot_file from info + del(snap_info[snapshot['id']]) + self._write_info_file(info_path, snap_info) def _create_volume_from_snapshot(self, volume, snapshot): """Creates a volume from a snapshot. @@ -1212,19 +1180,16 @@ class RemoteFSSnapDriver(RemoteFSDriver, driver.SnapshotVD): 5. Snapshot deletion when volume is detached ('available' state): * When first snapshot is deleted, Cinder does the snapshot - deletion. volume-1234.aaaa is removed (logically) from the - snapshot chain. The data from volume-1234.bbbb is merged into - it. + deletion. volume-1234.aaaa is removed from the snapshot chain. + The data from it is merged into its parent. - Since bbbb's data was committed into the aaaa file, we have - "removed" aaaa's snapshot point but the .aaaa file now - represents snapshot with id "bbbb". Also .aaaa file becomes the - "active" disk image as it represent snapshot with id "bbbb". + volume-1234.bbbb is rebased, having volume-1234 as its new + parent. - volume-1234 <- volume-1234.aaaa(* now with bbbb's data) + volume-1234 <- volume-1234.bbbb - info file: { 'active': 'volume-1234.aaaa', (* changed!) - 'bbbb': 'volume-1234.aaaa' (* changed!) + info file: { 'active': 'volume-1234.bbbb', + 'bbbb': 'volume-1234.bbbb' } * When second snapshot is deleted, Cinder does the snapshot -- 2.45.2