SMBFS: fix creating volume from snapshot

In some cases, attempting to create a volume from an in-use snapshot
fails because of Hyper-V locks.

The reason is that in order to locate the backing image that needs
to be *cloned*, we may need to query the currently attached image,
which is not allowed.

In order to avoid this, we're going to store the backing file
information in the DB using the snapshot object metadata field,
at the same time preserving backwards compatibility.

Note that we cannot rely on model changes passed to the manager as
snapshots are used internally in a few cases in which the manager
would not expect snapshot object changes. Also, when deleting
snapshots, we're updating the metadata for more recent snapshots
as the backing file changes.

Fake volume/snapshot object usage throughout the smbfs driver unit
tests had to be updated due to the object changes performed by it.

Change-Id: I88e9dad5cf36621c4b86acc7b7e972850e061119
Closes-Bug: #1694635
This commit is contained in:
Lucian Petrut 2017-12-13 11:25:23 +02:00
parent f41e556521
commit 8aa2f5a5b6
4 changed files with 230 additions and 77 deletions

View File

@ -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):

View File

@ -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):

View File

@ -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

View File

@ -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()