From f702fe7e30e4021895dac8e7ab243e5192f8182d Mon Sep 17 00:00:00 2001 From: rackerjoe Date: Wed, 4 Sep 2013 15:31:42 -0500 Subject: [PATCH] Fix secure delete for thick LVM snapshots This change modifies the behaviour of the secure delete for thick LVM snapshots to wipe the underlying COW of the snapshot LV instead of the snapshot LV itself. This change is necessary because the snapshot LV does not contain exactly the same number of writable blocks as the original LV. The COW includes header information per COW block that identifies the device as a COW device as well as the source and destination blocks for the changed item. The amount of metadata contained in the COW is variable based on I/O performed on the snapshot. This does not change the behavior of secure deletes on thin LVs or secure deletes on the thick LV snapshot origin. Closes-Bug: #1191812 Change-Id: I20e02b6c20d5ac539b5b5469e665fc986180f2e9 --- .../tests/api/contrib/test_admin_actions.py | 2 + cinder/tests/test_volume.py | 37 +++++++++++++++++++ cinder/volume/drivers/lvm.py | 30 +++++++++------ 3 files changed, 58 insertions(+), 11 deletions(-) diff --git a/cinder/tests/api/contrib/test_admin_actions.py b/cinder/tests/api/contrib/test_admin_actions.py index 65e003cb5..099708c8f 100644 --- a/cinder/tests/api/contrib/test_admin_actions.py +++ b/cinder/tests/api/contrib/test_admin_actions.py @@ -11,6 +11,7 @@ # under the License. import ast +import os import shutil import tempfile import webob @@ -258,6 +259,7 @@ class AdminActionsTest(test.TestCase): self.assertRaises(exception.NotFound, db.volume_get, ctx, volume['id']) def test_force_delete_snapshot(self): + self.stubs.Set(os.path, 'exists', lambda x: True) # admin context ctx = context.RequestContext('admin', 'fake', True) # current status is creating diff --git a/cinder/tests/test_volume.py b/cinder/tests/test_volume.py index 86d3f3827..0c0af7db5 100644 --- a/cinder/tests/test_volume.py +++ b/cinder/tests/test_volume.py @@ -104,6 +104,7 @@ class BaseVolumeTestCase(test.TestCase): fake_image.stub_out_image_service(self.stubs) test_notifier.NOTIFICATIONS = [] self.stubs.Set(brick_lvm.LVM, '_vg_exists', lambda x: True) + self.stubs.Set(os.path, 'exists', lambda x: True) def tearDown(self): try: @@ -1133,6 +1134,41 @@ class VolumeTestCase(BaseVolumeTestCase): self.volume.delete_snapshot(self.context, snapshot_id) self.volume.delete_volume(self.context, volume_id) + def test_delete_no_dev_fails(self): + """Test delete snapshot with no dev file fails.""" + self.stubs.Set(os.path, 'exists', lambda x: False) + self.volume.driver.vg = FakeBrickLVM('cinder-volumes', + False, + None, + 'default') + + volume = tests_utils.create_volume(self.context, **self.volume_params) + volume_id = volume['id'] + self.volume.create_volume(self.context, volume_id) + snapshot_id = self._create_snapshot(volume_id)['id'] + self.volume.create_snapshot(self.context, volume_id, snapshot_id) + + self.mox.StubOutWithMock(self.volume.driver, 'delete_snapshot') + + self.volume.driver.delete_snapshot( + mox.IgnoreArg()).AndRaise( + exception.SnapshotIsBusy(snapshot_name='fake')) + self.mox.ReplayAll() + self.volume.delete_snapshot(self.context, snapshot_id) + snapshot_ref = db.snapshot_get(self.context, snapshot_id) + self.assertEqual(snapshot_id, snapshot_ref.id) + self.assertEqual("available", snapshot_ref.status) + + self.mox.UnsetStubs() + self.assertRaises(exception.VolumeBackendAPIException, + self.volume.delete_snapshot, + self.context, + snapshot_id) + self.assertRaises(exception.VolumeBackendAPIException, + self.volume.delete_volume, + self.context, + volume_id) + def _create_volume_from_image(self, fakeout_copy_image_to_volume=False, fakeout_clone_image=False): """Test function of create_volume_from_image. @@ -2099,6 +2135,7 @@ class LVMVolumeDriverTestCase(DriverTestCase): lvm_driver = lvm.LVMVolumeDriver(configuration=configuration) self.stubs.Set(volutils, 'copy_volume', lambda x, y, z, sync=False, execute='foo': True) + self.stubs.Set(os.path, 'exists', lambda x: True) fake_volume = {'name': 'test1', 'volume_name': 'test1', diff --git a/cinder/volume/drivers/lvm.py b/cinder/volume/drivers/lvm.py index afd9ad5a3..438bcb7c3 100644 --- a/cinder/volume/drivers/lvm.py +++ b/cinder/volume/drivers/lvm.py @@ -131,13 +131,7 @@ class LVMVolumeDriver(driver.VolumeDriver): # zero out old volumes to prevent data leaking between users # TODO(ja): reclaiming space should be done lazy and low priority - dev_path = self.local_path(volume) - - # TODO(jdg): Maybe we could optimize this for snaps by looking at - # the cow table and only overwriting what's necessary? - # for now we're still skipping on snaps due to hang issue - if os.path.exists(dev_path) and not is_snapshot: - self.clear_volume(volume) + self.clear_volume(volume, is_snapshot) name = volume['name'] if is_snapshot: name = self._escape_snapshot(volume['name']) @@ -191,13 +185,27 @@ class LVMVolumeDriver(driver.VolumeDriver): self._delete_volume(volume) - def clear_volume(self, volume): + def clear_volume(self, volume, is_snapshot=False): """unprovision old volumes to prevent data leaking between users.""" if self.configuration.volume_clear == 'none': return - vol_path = self.local_path(volume) + if is_snapshot and not self.configuration.lvm_type == 'thin': + # if the volume to be cleared is a snapshot of another volume + # we need to clear out the volume using the -cow instead of the + # directly volume path. We need to skip this if we are using + # thin provisioned LVs. + # bug# lp1191812 + dev_path = self.local_path(volume) + "-cow" + else: + dev_path = self.local_path(volume) + + if not os.path.exists(dev_path): + msg = (_('Volume device file path %s does not exist.') % dev_path) + LOG.error(msg) + raise exception.VolumeBackendAPIException(data=msg) + size_in_g = volume.get('size', volume.get('volume_size', None)) if size_in_g is None: LOG.warning(_("Size for volume: %s not found, " @@ -210,7 +218,7 @@ class LVMVolumeDriver(driver.VolumeDriver): if self.configuration.volume_clear == 'zero': if size_in_m == 0: return volutils.copy_volume('/dev/zero', - vol_path, size_in_g * 1024, + dev_path, size_in_g * 1024, sync=True, execute=self._execute) else: @@ -224,7 +232,7 @@ class LVMVolumeDriver(driver.VolumeDriver): self.configuration.volume_clear) return - clear_cmd.append(vol_path) + clear_cmd.append(dev_path) self._execute(*clear_cmd, run_as_root=True) def create_snapshot(self, snapshot): -- 2.45.2