diff --git a/cinder/tests/unit/volume/drivers/test_remotefs.py b/cinder/tests/unit/volume/drivers/test_remotefs.py index 6f86e9c946b..38c5645905c 100644 --- a/cinder/tests/unit/volume/drivers/test_remotefs.py +++ b/cinder/tests/unit/volume/drivers/test_remotefs.py @@ -615,6 +615,35 @@ class RemoteFsSnapDriverTestCase(test.TestCase): 'count=1024', run_as_root=True) + @mock.patch.object(remotefs.RemoteFSSnapDriver, '_local_path_volume_info') + @mock.patch.object(remotefs.RemoteFSSnapDriver, '_read_info_file') + @mock.patch.object(remotefs.RemoteFSSnapDriver, '_local_volume_dir') + @mock.patch.object(remotefs.RemoteFSSnapDriver, '_qemu_img_info') + def test_get_snapshot_backing_file( + self, mock_qemu_img_info, mock_local_vol_dir, + mock_read_info_file, mock_local_path_vol_info): + + fake_snapshot_file_name = os.path.basename(self._fake_snapshot_path) + fake_snapshot_info = {self._fake_snapshot.id: fake_snapshot_file_name} + + fake_snap_img_info = mock.Mock() + fake_snap_img_info.backing_file = self._fake_volume.name + + mock_read_info_file.return_value = fake_snapshot_info + mock_qemu_img_info.return_value = fake_snap_img_info + mock_local_vol_dir.return_value = self._FAKE_MNT_POINT + + snap_backing_file = self._driver._get_snapshot_backing_file( + self._fake_snapshot) + self.assertEqual(os.path.basename(self._fake_volume_path), + snap_backing_file) + + mock_local_path_vol_info.assert_called_once_with(self._fake_volume) + mock_read_info_file.assert_called_once_with( + mock_local_path_vol_info.return_value) + mock_local_vol_dir.assert_called_once_with(self._fake_volume) + mock_qemu_img_info.assert_called_once_with(self._fake_snapshot_path) + class RemoteFSPoolMixinTestCase(test.TestCase): def setUp(self): diff --git a/cinder/tests/unit/windows/test_smbfs.py b/cinder/tests/unit/windows/test_smbfs.py index 63778b45958..e49ff7eaec1 100644 --- a/cinder/tests/unit/windows/test_smbfs.py +++ b/cinder/tests/unit/windows/test_smbfs.py @@ -24,8 +24,8 @@ from cinder import exception from cinder.image import image_utils from cinder.objects import fields from cinder import test -from cinder.tests.unit import fake_snapshot from cinder.tests.unit import fake_volume +from cinder.tests.unit import utils as test_utils from cinder.volume.drivers import remotefs from cinder.volume.drivers.windows import smbfs @@ -83,17 +83,16 @@ class WindowsSmbFsTestCase(test.TestCase): 'provider_location': self._FAKE_SHARE} updates.update(kwargs) ctxt = context.get_admin_context() - return fake_volume.fake_volume_obj(ctxt, **updates) + volume = test_utils.create_volume(ctxt, **updates) + return volume def _simple_snapshot(self, **kwargs): volume = kwargs.pop('volume', None) or self._simple_volume() ctxt = context.get_admin_context() updates = {'id': self._FAKE_SNAPSHOT_ID, - 'volume_size': volume.size, 'volume_id': volume.id} updates.update(kwargs) - snapshot = fake_snapshot.fake_snapshot_obj(ctxt, **updates) - snapshot.volume = volume + snapshot = test_utils.create_snapshot(ctxt, **updates) return snapshot @mock.patch.object(smbfs.WindowsSmbfsDriver, '_check_os_platform') @@ -385,17 +384,16 @@ class WindowsSmbFsTestCase(test.TestCase): self._smbfs_driver.get_volume_format = mock.Mock( return_value=volume_format) - volume = self._simple_volume() if volume_exists or volume_format not in ('vhd', 'vhdx'): self.assertRaises(exception.InvalidVolume, self._smbfs_driver._do_create_volume, - volume) + self.volume) else: fake_vol_path = self._FAKE_VOLUME_PATH - self._smbfs_driver._do_create_volume(volume) + self._smbfs_driver._do_create_volume(self.volume) fake_create.assert_called_once_with( fake_vol_path, mock_get_vhd_type.return_value, - max_internal_size=volume.size << 30) + max_internal_size=self.volume.size << 30) def test_create_volume(self): self._test_create_volume() @@ -475,8 +473,8 @@ class WindowsSmbFsTestCase(test.TestCase): @ddt.data('attached', 'detached') def test_create_snapshot(self, attach_status): - snapshot = self._simple_snapshot() - snapshot.volume.attach_status = attach_status + self.snapshot.volume.attach_status = attach_status + self.snapshot.volume.save() self._smbfs_driver._vhdutils.create_differencing_vhd = ( mock.Mock()) @@ -487,7 +485,7 @@ class WindowsSmbFsTestCase(test.TestCase): self._smbfs_driver._vhdutils.create_differencing_vhd) self._smbfs_driver._do_create_snapshot( - snapshot, + self.snapshot, os.path.basename(self._FAKE_VOLUME_PATH), self._FAKE_SNAPSHOT_PATH) @@ -497,6 +495,11 @@ class WindowsSmbFsTestCase(test.TestCase): else: fake_create_diff.assert_not_called() + self.assertEqual(os.path.basename(self._FAKE_VOLUME_PATH), + self.snapshot.metadata['backing_file']) + # Ensure that the changes have been saved. + self.assertFalse(bool(self.snapshot.obj_what_changed())) + @mock.patch.object(smbfs.WindowsSmbfsDriver, '_check_extend_volume_support') @mock.patch.object(smbfs.WindowsSmbfsDriver, @@ -561,7 +564,10 @@ class WindowsSmbFsTestCase(test.TestCase): @mock.patch.object(smbfs.WindowsSmbfsDriver, '_read_info_file') @mock.patch.object(smbfs.WindowsSmbfsDriver, '_nova_assisted_vol_snap_delete') - def test_delete_snapshot(self, mock_nova_assisted_snap_del, + @mock.patch.object(smbfs.WindowsSmbfsDriver, + '_get_snapshot_by_backing_file') + def test_delete_snapshot(self, mock_get_snap_by_backing_file, + mock_nova_assisted_snap_del, mock_read_info_file, mock_write_info_file, mock_local_path_volume_info, mock_get_local_dir, @@ -569,8 +575,11 @@ class WindowsSmbFsTestCase(test.TestCase): attach_status='attached', snap_info_contains_snap_id=True, delete_latest=False): - snapshot = self._simple_snapshot() - snapshot.volume.attach_status = attach_status + self.snapshot.volume.attach_status = attach_status + self.snapshot.metadata['backing_file'] = os.path.basename( + self._FAKE_VOLUME_PATH) + higher_snapshot = self._simple_snapshot(id=None, + volume=self.volume) fake_snap_file = 'snap_file' fake_snap_parent_path = os.path.join(self._FAKE_MNT_POINT, @@ -579,8 +588,10 @@ class WindowsSmbFsTestCase(test.TestCase): snap_info = dict(active=active_img) if snap_info_contains_snap_id: - snap_info[snapshot.id] = fake_snap_file + snap_info[self.snapshot.id] = fake_snap_file + mock_get_snap_by_backing_file.return_value = ( + higher_snapshot if not delete_latest else None) mock_info_path = mock_local_path_volume_info.return_value mock_read_info_file.return_value = snap_info mock_get_local_dir.return_value = self._FAKE_MNT_POINT @@ -588,19 +599,19 @@ class WindowsSmbFsTestCase(test.TestCase): fake_snap_parent_path) expected_delete_info = {'file_to_merge': fake_snap_file, - 'volume_id': snapshot.volume.id} + 'volume_id': self.snapshot.volume.id} - self._smbfs_driver._delete_snapshot(snapshot) + self._smbfs_driver._delete_snapshot(self.snapshot) if attach_status != 'attached': - mock_remotefs_snap_delete.assert_called_once_with(snapshot) + mock_remotefs_snap_delete.assert_called_once_with(self.snapshot) elif snap_info_contains_snap_id: mock_local_path_volume_info.assert_called_once_with( - snapshot.volume) + self.snapshot.volume) mock_read_info_file.assert_called_once_with( mock_info_path, empty_if_missing=True) mock_nova_assisted_snap_del.assert_called_once_with( - snapshot._context, snapshot, expected_delete_info) + self.snapshot._context, self.snapshot, expected_delete_info) exp_merged_img_path = os.path.join(self._FAKE_MNT_POINT, fake_snap_file) @@ -615,7 +626,7 @@ class WindowsSmbFsTestCase(test.TestCase): exp_active = active_img self.assertEqual(exp_active, snap_info['active']) - self.assertNotIn(snap_info, snapshot.id) + self.assertNotIn(snap_info, self.snapshot.id) mock_write_info_file.assert_called_once_with(mock_info_path, snap_info) @@ -623,6 +634,53 @@ class WindowsSmbFsTestCase(test.TestCase): mock_nova_assisted_snap_del.assert_not_called() mock_write_info_file.assert_not_called() + if not delete_latest and snap_info_contains_snap_id: + self.assertEqual(os.path.basename(self._FAKE_VOLUME_PATH), + higher_snapshot.metadata['backing_file']) + self.assertFalse(bool(higher_snapshot.obj_what_changed())) + + @ddt.data(True, False) + def test_get_snapshot_by_backing_file(self, metadata_set): + backing_file = 'fake_backing_file' + if metadata_set: + self.snapshot.metadata['backing_file'] = backing_file + self.snapshot.save() + + for idx in range(2): + # We're adding a few other snapshots. + self._simple_snapshot(id=None, + volume=self.volume) + + snapshot = self._smbfs_driver._get_snapshot_by_backing_file( + self.volume, backing_file) + + if metadata_set: + self.assertEqual(self.snapshot.id, snapshot.id) + else: + self.assertIsNone(snapshot) + + @ddt.data(True, False) + @mock.patch.object(remotefs.RemoteFSSnapDriverDistributed, + '_get_snapshot_backing_file') + def test_get_snapshot_backing_file_md_set(self, md_set, + remotefs_get_backing_file): + backing_file = 'fake_backing_file' + if md_set: + self.snapshot.metadata['backing_file'] = backing_file + + ret_val = self._smbfs_driver._get_snapshot_backing_file( + self.snapshot) + + # If the metadata is not set, we expect the super class method to + # be used, which is supposed to query the image. + if md_set: + self.assertEqual(backing_file, ret_val) + else: + self.assertEqual(remotefs_get_backing_file.return_value, + ret_val) + remotefs_get_backing_file.assert_called_once_with( + self.snapshot) + def test_create_volume_from_unavailable_snapshot(self): self.snapshot.status = fields.SnapshotStatus.ERROR self.assertRaises( @@ -657,14 +715,13 @@ class WindowsSmbFsTestCase(test.TestCase): with mock.patch.object(image_utils, 'upload_volume') as ( fake_upload_volume): - volume = self._simple_volume() drv.copy_volume_to_image( - mock.sentinel.context, volume, + mock.sentinel.context, self.volume, mock.sentinel.image_service, fake_image_meta) if has_parent: fake_temp_image_name = '%s.temp_image.%s.%s' % ( - volume.id, + self.volume.id, fake_image_meta['id'], fake_img_format) fake_temp_image_path = os.path.join( @@ -700,9 +757,8 @@ class WindowsSmbFsTestCase(test.TestCase): with mock.patch.object(image_utils, 'fetch_to_volume_format') as fake_fetch: - volume = self._simple_volume() drv.copy_image_to_volume( - mock.sentinel.context, volume, + mock.sentinel.context, self.volume, mock.sentinel.image_service, mock.sentinel.image_id) fake_fetch.assert_called_once_with( @@ -714,33 +770,25 @@ class WindowsSmbFsTestCase(test.TestCase): mock_get_vhd_type.return_value) drv._vhdutils.resize_vhd.assert_called_once_with( self._FAKE_VOLUME_PATH, - volume.size * units.Gi, + self.volume.size * units.Gi, is_file_max_size=False) @mock.patch.object(smbfs.WindowsSmbfsDriver, '_get_vhd_type') def test_copy_volume_from_snapshot(self, mock_get_vhd_type): drv = self._smbfs_driver - snapshot = self._simple_snapshot() - fake_volume_info = { - snapshot.id: 'fake_snapshot_file_name'} - fake_img_info = mock.MagicMock() - fake_img_info.backing_file = self._FAKE_VOLUME_NAME - drv._local_path_volume_info = mock.Mock( - return_value=self._FAKE_VOLUME_PATH + '.info') + drv._get_snapshot_backing_file = mock.Mock( + return_value=self._FAKE_VOLUME_NAME) drv._local_volume_dir = mock.Mock( return_value=self._FAKE_MNT_POINT) - drv._read_info_file = mock.Mock( - return_value=fake_volume_info) - drv._qemu_img_info = mock.Mock( - return_value=fake_img_info) drv.local_path = mock.Mock( return_value=mock.sentinel.new_volume_path) - volume = self._simple_volume() - drv._copy_volume_from_snapshot(snapshot, - volume, volume.size) + drv._copy_volume_from_snapshot(self.snapshot, + self.volume, self.volume.size) + drv._get_snapshot_backing_file.assert_called_once_with( + self.snapshot) drv._delete.assert_called_once_with(mock.sentinel.new_volume_path) drv._vhdutils.convert_vhd.assert_called_once_with( self._FAKE_VOLUME_PATH, @@ -748,7 +796,7 @@ class WindowsSmbFsTestCase(test.TestCase): vhd_type=mock_get_vhd_type.return_value) drv._vhdutils.resize_vhd.assert_called_once_with( mock.sentinel.new_volume_path, - volume.size * units.Gi, + self.volume.size * units.Gi, is_file_max_size=False) def test_rebase_img(self): diff --git a/cinder/volume/drivers/remotefs.py b/cinder/volume/drivers/remotefs.py index 1f3271e6438..3ce72b0be27 100644 --- a/cinder/volume/drivers/remotefs.py +++ b/cinder/volume/drivers/remotefs.py @@ -985,6 +985,19 @@ class RemoteFSSnapDriverBase(RemoteFSDriver): active_fpath = os.path.join(vol_dir, active_fname) return active_fpath + def _get_snapshot_backing_file(self, snapshot): + info_path = self._local_path_volume_info(snapshot.volume) + snap_info = self._read_info_file(info_path) + vol_dir = self._local_volume_dir(snapshot.volume) + + forward_file = snap_info[snapshot.id] + forward_path = os.path.join(vol_dir, forward_file) + + # Find the file which backs this file, which represents the point + # in which this snapshot was created. + img_info = self._qemu_img_info(forward_path) + return img_info.backing_file + def _snapshots_exist(self, volume): if not volume.provider_location: return False diff --git a/cinder/volume/drivers/windows/smbfs.py b/cinder/volume/drivers/windows/smbfs.py index 4c63a63bc2c..eab5801d461 100644 --- a/cinder/volume/drivers/windows/smbfs.py +++ b/cinder/volume/drivers/windows/smbfs.py @@ -31,6 +31,7 @@ from cinder import exception from cinder.i18n import _ from cinder.image import image_utils from cinder import interface +from cinder import objects from cinder import utils from cinder.volume import configuration from cinder.volume.drivers import remotefs as remotefs_drv @@ -382,13 +383,29 @@ class WindowsSmbfsDriver(remotefs_drv.RevertToSnapshotMixin, if self._is_volume_attached(snapshot.volume): LOG.debug("Snapshot is in-use. Performing Nova " "assisted creation.") - return + else: + backing_file_full_path = os.path.join( + self._local_volume_dir(snapshot.volume), + backing_file) + self._vhdutils.create_differencing_vhd(new_snap_path, + backing_file_full_path) - backing_file_full_path = os.path.join( - self._local_volume_dir(snapshot.volume), - backing_file) - self._vhdutils.create_differencing_vhd(new_snap_path, - backing_file_full_path) + # We're setting the backing file information in the DB as we may not + # be able to query the image while it's in use due to file locks. + # + # When dealing with temporary snapshots created by the driver, we + # may not receive an actual snapshot VO. We currently need this check + # in order to avoid breaking the volume clone operation. + # + # TODO(lpetrut): remove this check once we'll start using db entries + # for such temporary snapshots, most probably when we'll add support + # for cloning in-use volumes. + if isinstance(snapshot, objects.Snapshot): + snapshot.metadata['backing_file'] = backing_file + snapshot.save() + else: + LOG.debug("Received a '%s' object, skipping setting the backing " + "file in the DB.", type(snapshot)) def _extend_volume(self, volume, size_gb): self._check_extend_volume_support(volume, size_gb) @@ -405,9 +422,6 @@ class WindowsSmbfsDriver(remotefs_drv.RevertToSnapshotMixin, # NOTE(lpetrut): We're slightly diverging from the super class # workflow. The reason is that we cannot query in-use vhd/x images, # nor can we add or remove images from a vhd/x chain in this case. - if not self._is_volume_attached(snapshot.volume): - return super(WindowsSmbfsDriver, self)._delete_snapshot(snapshot) - info_path = self._local_path_volume_info(snapshot.volume) snap_info = self._read_info_file(info_path, empty_if_missing=True) @@ -417,27 +431,81 @@ class WindowsSmbfsDriver(remotefs_drv.RevertToSnapshotMixin, return file_to_merge = snap_info[snapshot.id] - delete_info = {'file_to_merge': file_to_merge, - 'volume_id': snapshot.volume.id} - self._nova_assisted_vol_snap_delete( - snapshot._context, snapshot, delete_info) + deleting_latest_snap = utils.paths_normcase_equal(snap_info['active'], + file_to_merge) - # At this point, the image file should no longer be in use, so we - # may safely query it so that we can update the 'active' image - # reference, if needed. - merged_img_path = os.path.join( - self._local_volume_dir(snapshot.volume), - file_to_merge) - if utils.paths_normcase_equal(snap_info['active'], file_to_merge): - new_active_file_path = self._vhdutils.get_vhd_parent_path( - merged_img_path).lower() - snap_info['active'] = os.path.basename(new_active_file_path) + if not self._is_volume_attached(snapshot.volume): + super(WindowsSmbfsDriver, self)._delete_snapshot(snapshot) + else: + delete_info = {'file_to_merge': file_to_merge, + 'volume_id': snapshot.volume.id} + self._nova_assisted_vol_snap_delete( + snapshot._context, snapshot, delete_info) - self._delete(merged_img_path) + # At this point, the image file should no longer be in use, so we + # may safely query it so that we can update the 'active' image + # reference, if needed. + merged_img_path = os.path.join( + self._local_volume_dir(snapshot.volume), + file_to_merge) + if deleting_latest_snap: + new_active_file_path = self._vhdutils.get_vhd_parent_path( + merged_img_path).lower() + snap_info['active'] = os.path.basename(new_active_file_path) - # TODO(lpetrut): drop snapshot info file usage. - del(snap_info[snapshot.id]) - self._write_info_file(info_path, snap_info) + self._delete(merged_img_path) + + # TODO(lpetrut): drop snapshot info file usage. + del(snap_info[snapshot.id]) + self._write_info_file(info_path, snap_info) + + if not isinstance(snapshot, objects.Snapshot): + LOG.debug("Received a '%s' object, skipping setting the backing " + "file in the DB.", type(snapshot)) + elif not deleting_latest_snap: + backing_file = snapshot['metadata'].get('backing_file') + higher_snapshot = self._get_snapshot_by_backing_file( + snapshot.volume, file_to_merge) + # The snapshot objects should have a backing file set, unless + # created before an upgrade. If the snapshot we're deleting + # does not have a backing file set yet there is a newer one that + # does, we're clearing it out so that it won't provide wrong info. + if higher_snapshot: + LOG.debug("Updating backing file reference (%(backing_file)s) " + "for higher snapshot: %(higher_snapshot_id)s.", + dict(backing_file=snapshot.metadata['backing_file'], + higher_snapshot_id=higher_snapshot.id)) + + higher_snapshot.metadata['backing_file'] = ( + snapshot.metadata['backing_file']) + higher_snapshot.save() + if not (higher_snapshot and backing_file): + LOG.info( + "The deleted snapshot is not latest one, yet we could not " + "find snapshot backing file information in the DB. This " + "may happen after an upgrade. Certain operations against " + "this volume may be unavailable while it's in-use.") + + def _get_snapshot_by_backing_file(self, volume, backing_file): + all_snapshots = objects.SnapshotList.get_all_for_volume( + context.get_admin_context(), volume.id) + for snapshot in all_snapshots: + snap_backing_file = snapshot.metadata.get('backing_file') + if utils.paths_normcase_equal(snap_backing_file or '', + backing_file): + return snapshot + + def _get_snapshot_backing_file(self, snapshot): + backing_file = snapshot.metadata.get('backing_file') + if not backing_file: + LOG.info("Could not find the snapshot backing file in the DB. " + "This may happen after an upgrade. Attempting to " + "query the image as a fallback. This may fail if " + "the image is in-use.") + backing_file = super( + WindowsSmbfsDriver, self)._get_snapshot_backing_file(snapshot) + + return backing_file def _check_extend_volume_support(self, volume, size_gb): snapshots_exist = self._snapshots_exist(volume) @@ -511,17 +579,12 @@ class WindowsSmbfsDriver(remotefs_drv.RevertToSnapshotMixin, 'vol': volume.id, 'size': snapshot.volume_size}) - info_path = self._local_path_volume_info(snapshot.volume) - snap_info = self._read_info_file(info_path) vol_dir = self._local_volume_dir(snapshot.volume) - forward_file = snap_info[snapshot.id] - forward_path = os.path.join(vol_dir, forward_file) - # Find the file which backs this file, which represents the point # when this snapshot was created. - img_info = self._qemu_img_info(forward_path) - snapshot_path = os.path.join(vol_dir, img_info.backing_file) + backing_file = self._get_snapshot_backing_file(snapshot) + snapshot_path = os.path.join(vol_dir, backing_file) volume_path = self.local_path(volume) vhd_type = self._get_vhd_type()