From 78f2101fc5a84e81132e6c2adc89cfe681018d77 Mon Sep 17 00:00:00 2001 From: Silvan Kaiser Date: Tue, 29 May 2018 12:17:43 +0200 Subject: [PATCH] Add context to cloning snapshots in remotefs driver Adds context to _create_cloned_volume method in order to allow taking a snapshot during cloning. Also saves the temp_snapshot in order to enable Nova to update the snapshot state. Destroys the temp_snapshot after usage. This affects the NFS, WindowsSMBFS, VZStorage and Quobyte drivers. Closes-Bug: #1744692 Change-Id: I328a02d0f26e8c3d41ec18a0487da6fd20b39b04 --- cinder/tests/unit/fake_snapshot.py | 2 + .../unit/volume/drivers/test_remotefs.py | 62 +++++++++++++------ .../unit/volume/drivers/test_vzstorage.py | 2 + cinder/volume/drivers/quobyte.py | 3 +- cinder/volume/drivers/remotefs.py | 47 ++++++++------ cinder/volume/drivers/vzstorage.py | 2 +- .../bugfix-1744692-5aebd0c97ae66407.yaml | 6 ++ 7 files changed, 83 insertions(+), 41 deletions(-) create mode 100644 releasenotes/notes/bugfix-1744692-5aebd0c97ae66407.yaml diff --git a/cinder/tests/unit/fake_snapshot.py b/cinder/tests/unit/fake_snapshot.py index d7459c516e7..051809ee000 100644 --- a/cinder/tests/unit/fake_snapshot.py +++ b/cinder/tests/unit/fake_snapshot.py @@ -52,6 +52,8 @@ def fake_snapshot_obj(context, **updates): expected_attrs = updates.pop('expected_attrs', None) or [] if 'volume' in updates and 'volume' not in expected_attrs: expected_attrs.append('volume') + if 'context' in updates and 'context' not in expected_attrs: + expected_attrs.append('context') return snapshot.Snapshot._from_db_object(context, snapshot.Snapshot(), fake_db_snapshot(**updates), expected_attrs=expected_attrs) diff --git a/cinder/tests/unit/volume/drivers/test_remotefs.py b/cinder/tests/unit/volume/drivers/test_remotefs.py index 290e64949f3..b9d0552e2cd 100644 --- a/cinder/tests/unit/volume/drivers/test_remotefs.py +++ b/cinder/tests/unit/volume/drivers/test_remotefs.py @@ -16,6 +16,7 @@ import collections import copy import os import re +import sys import ddt import mock @@ -23,6 +24,7 @@ import mock from cinder import context 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 @@ -327,17 +329,19 @@ class RemoteFsSnapDriverTestCase(test.TestCase): {'status': 'creating', 'progress': '45%'}, {'status': 'deleting'}, ] + fake_snapshot = self._fake_snapshot + fake_snapshot.context = self.context with mock.patch.object(self._driver, '_do_create_snapshot') as \ mock_do_create_snapshot: self.assertRaises(exception.RemoteFSConcurrentRequest, self._driver._create_snapshot_online, - self._fake_snapshot, + fake_snapshot, self._fake_volume.name, self._fake_snapshot_path) mock_do_create_snapshot.assert_called_once_with( - self._fake_snapshot, self._fake_volume.name, + fake_snapshot, self._fake_volume.name, self._fake_snapshot_path) self.assertEqual([mock.call(1), mock.call(1)], mock_sleep.call_args_list) @@ -586,6 +590,7 @@ class RemoteFsSnapDriverTestCase(test.TestCase): {'snapshots_exist': True}, {'force_temp_snap': True}) @ddt.unpack + @mock.patch.object(sys.modules['cinder.objects'], "Snapshot") @mock.patch.object(remotefs.RemoteFSSnapDriver, 'local_path') @mock.patch.object(remotefs.RemoteFSSnapDriver, '_snapshots_exist') @mock.patch.object(remotefs.RemoteFSSnapDriver, '_copy_volume_image') @@ -603,16 +608,19 @@ class RemoteFsSnapDriverTestCase(test.TestCase): mock_copy_volume_image, mock_snapshots_exist, mock_local_path, + mock_obj_snap, snapshots_exist=False, force_temp_snap=False): drv = self._driver + # prepare test volume = fake_volume.fake_volume_obj(self.context) src_vref_id = '375e32b2-804a-49f2-b282-85d1d5a5b9e1' src_vref = fake_volume.fake_volume_obj( self.context, id=src_vref_id, name='volume-%s' % src_vref_id) + src_vref.context = self.context mock_snapshots_exist.return_value = snapshots_exist drv._always_use_temp_snap_when_cloning = force_temp_snap @@ -621,27 +629,38 @@ class RemoteFsSnapDriverTestCase(test.TestCase): 'volume_type', 'metadata'] Volume = collections.namedtuple('Volume', vol_attrs) - snap_attrs = ['volume_name', 'volume_size', 'name', - 'volume_id', 'id', 'volume'] - Snapshot = collections.namedtuple('Snapshot', snap_attrs) - volume_ref = Volume(id=volume.id, + metadata=volume.metadata, name=volume.name, - status=volume.status, provider_location=volume.provider_location, + status=volume.status, size=volume.size, - volume_type=volume.volume_type, - metadata=volume.metadata) + volume_type=volume.volume_type,) - snap_ref = Snapshot(volume_name=volume.name, - name='clone-snap-%s' % src_vref.id, - volume_size=src_vref.size, - volume_id=src_vref.id, - id='tmp-snap-%s' % src_vref.id, - volume=src_vref) + snap_args_creation = { + 'volume_id': src_vref.id, + 'user_id': None, + 'project_id': None, + 'status': fields.SnapshotStatus.CREATING, + 'progress': '0%', + 'volume_size': src_vref.size, + 'display_name': 'tmp-snap-%s' % src_vref['id'], + 'display_description': None, + 'volume_type_id': src_vref.volume_type_id, + 'encryption_key_id': None, + } + snap_args_deletion = snap_args_creation.copy() + snap_args_deletion["status"] = fields.SnapshotStatus.DELETED + snap_args_deletion["deleted"] = True + mock_obj_snap.return_value = mock.Mock() + mock_obj_snap.return_value.create = mock.Mock() + # end of prepare test + + # run test drv.create_cloned_volume(volume, src_vref) + # evaluate test exp_acceptable_states = ['available', 'backing-up', 'downloading'] mock_validate_state.assert_called_once_with( src_vref.status, @@ -649,10 +668,14 @@ class RemoteFsSnapDriverTestCase(test.TestCase): obj_description='source volume') if snapshots_exist or force_temp_snap: - mock_create_snapshot.assert_called_once_with(snap_ref) + mock_obj_snap.return_value.create.assert_called_once_with() + mock_obj_snap.assert_called_once_with( + context=self.context, **snap_args_creation) + mock_create_snapshot.assert_called_once_with( + mock_obj_snap.return_value) mock_copy_volume_from_snapshot.assert_called_once_with( - snap_ref, volume_ref, volume['size']) - self.assertTrue(mock_delete_snapshot.called) + mock_obj_snap.return_value, volume_ref, volume['size']) + mock_delete_snapshot.called_once_with(snap_args_deletion) else: self.assertFalse(mock_create_snapshot.called) @@ -663,8 +686,7 @@ class RemoteFsSnapDriverTestCase(test.TestCase): mock_local_path.return_value) mock_local_path.assert_has_calls( [mock.call(src_vref), mock.call(volume_ref)]) - mock_extend_volume.assert_called_once_with(volume_ref, - volume.size) + mock_extend_volume.assert_called_once_with(volume_ref, volume.size) @mock.patch('shutil.copyfile') @mock.patch.object(remotefs.RemoteFSSnapDriver, '_set_rw_permissions') diff --git a/cinder/tests/unit/volume/drivers/test_vzstorage.py b/cinder/tests/unit/volume/drivers/test_vzstorage.py index 3ea1313e568..0322b2accf1 100644 --- a/cinder/tests/unit/volume/drivers/test_vzstorage.py +++ b/cinder/tests/unit/volume/drivers/test_vzstorage.py @@ -415,6 +415,7 @@ class VZStorageTestCase(test.TestCase): id=src_vref_id, name='volume-%s' % src_vref_id, provider_location=self._FAKE_SHARE) + src_vref.context = self.context mock_remotefs_create_cloned_volume.return_value = { 'provider_location': self._FAKE_SHARE} @@ -446,6 +447,7 @@ class VZStorageTestCase(test.TestCase): id=src_vref_id, name='volume-%s' % src_vref_id, provider_location=self._FAKE_SHARE) + src_vref.context = self.context snap_attrs = ['volume_name', 'size', 'volume_size', 'name', 'volume_id', 'id', 'volume'] diff --git a/cinder/volume/drivers/quobyte.py b/cinder/volume/drivers/quobyte.py index e64b1856877..ede5e3f488d 100644 --- a/cinder/volume/drivers/quobyte.py +++ b/cinder/volume/drivers/quobyte.py @@ -325,7 +325,8 @@ class QuobyteDriver(remotefs_drv.RemoteFSSnapDriverDistributed): @utils.synchronized('quobyte', external=False) def create_cloned_volume(self, volume, src_vref): """Creates a clone of the specified volume.""" - return self._create_cloned_volume(volume, src_vref) + return self._create_cloned_volume(volume, src_vref, + src_vref.obj_context) @coordination.synchronized( '{self.driver_prefix}-{snapshot.id}-{volume.id}') diff --git a/cinder/volume/drivers/remotefs.py b/cinder/volume/drivers/remotefs.py index 7f272e2cb8d..5bd7df5026c 100644 --- a/cinder/volume/drivers/remotefs.py +++ b/cinder/volume/drivers/remotefs.py @@ -37,6 +37,7 @@ from cinder import db from cinder import exception from cinder.i18n import _ from cinder.image import image_utils +from cinder import objects from cinder.objects import fields from cinder import utils from cinder.volume import configuration @@ -1024,7 +1025,7 @@ class RemoteFSSnapDriverBase(RemoteFSDriver): def _is_volume_attached(self, volume): return volume.attach_status == fields.VolumeAttachStatus.ATTACHED - def _create_cloned_volume(self, volume, src_vref): + def _create_cloned_volume(self, volume, src_vref, context): LOG.info('Cloning volume %(src)s to volume %(dst)s', {'src': src_vref.id, 'dst': volume.id}) @@ -1040,7 +1041,6 @@ class RemoteFSSnapDriverBase(RemoteFSDriver): vol_attrs = ['provider_location', 'size', 'id', 'name', 'status', 'volume_type', 'metadata'] Volume = collections.namedtuple('Volume', vol_attrs) - volume_info = Volume(provider_location=src_vref.provider_location, size=src_vref.size, id=volume.id, @@ -1051,25 +1051,34 @@ class RemoteFSSnapDriverBase(RemoteFSDriver): if (self._always_use_temp_snap_when_cloning or self._snapshots_exist(src_vref)): - snap_attrs = ['volume_name', 'volume_size', 'name', - 'volume_id', 'id', 'volume'] - Snapshot = collections.namedtuple('Snapshot', snap_attrs) - - temp_snapshot = Snapshot(volume_name=volume_name, - volume_size=src_vref.size, - name='clone-snap-%s' % src_vref.id, - volume_id=src_vref.id, - id='tmp-snap-%s' % src_vref.id, - volume=src_vref) + kwargs = { + 'volume_id': src_vref.id, + 'user_id': context.user_id, + 'project_id': context.project_id, + 'status': fields.SnapshotStatus.CREATING, + 'progress': '0%', + 'volume_size': src_vref.size, + 'display_name': 'tmp-snap-%s' % src_vref.id, + 'display_description': None, + 'volume_type_id': src_vref.volume_type_id, + 'encryption_key_id': src_vref.encryption_key_id, + } + temp_snapshot = objects.Snapshot(context=context, + **kwargs) + temp_snapshot.create() self._create_snapshot(temp_snapshot) try: self._copy_volume_from_snapshot(temp_snapshot, volume_info, volume.size) - + # remove temp snapshot after the cloning is done + temp_snapshot.status = fields.SnapshotStatus.DELETING + temp_snapshot.context = context.elevated() + temp_snapshot.save() finally: self._delete_snapshot(temp_snapshot) + temp_snapshot.destroy() else: self._copy_volume_image(self.local_path(src_vref), self.local_path(volume_info)) @@ -1459,8 +1468,6 @@ class RemoteFSSnapDriverBase(RemoteFSDriver): def _create_snapshot_online(self, snapshot, backing_filename, new_snap_path): # Perform online snapshot via Nova - context = snapshot._context - self._do_create_snapshot(snapshot, backing_filename, new_snap_path) @@ -1473,7 +1480,7 @@ class RemoteFSSnapDriverBase(RemoteFSDriver): try: result = self._nova.create_volume_snapshot( - context, + snapshot.obj_context, snapshot.volume_id, connection_info) LOG.debug('nova call result: %s', result) @@ -1488,7 +1495,7 @@ class RemoteFSSnapDriverBase(RemoteFSDriver): increment = 1 timeout = 600 while True: - s = db.snapshot_get(context, snapshot.id) + s = db.snapshot_get(snapshot.obj_context, snapshot.id) LOG.debug('Status of snapshot %(id)s is now %(status)s', {'id': snapshot['id'], @@ -1651,7 +1658,8 @@ class RemoteFSSnapDriver(RemoteFSSnapDriverBase): def create_cloned_volume(self, volume, src_vref): """Creates a clone of the specified volume.""" - return self._create_cloned_volume(volume, src_vref) + return self._create_cloned_volume(volume, src_vref, + src_vref.obj_context) @locked_volume_id_operation def copy_volume_to_image(self, context, volume, image_service, image_meta): @@ -1692,7 +1700,8 @@ class RemoteFSSnapDriverDistributed(RemoteFSSnapDriverBase): def create_cloned_volume(self, volume, src_vref): """Creates a clone of the specified volume.""" - return self._create_cloned_volume(volume, src_vref) + return self._create_cloned_volume(volume, src_vref, + src_vref.obj_context) @coordination.synchronized('{self.driver_prefix}-{volume.id}') def copy_volume_to_image(self, context, volume, image_service, image_meta): diff --git a/cinder/volume/drivers/vzstorage.py b/cinder/volume/drivers/vzstorage.py index 29f59e2b8db..6612ba5d83e 100644 --- a/cinder/volume/drivers/vzstorage.py +++ b/cinder/volume/drivers/vzstorage.py @@ -718,7 +718,7 @@ class VZStorageDriver(remotefs_drv.RemoteFSSnapDriver): return {'provider_location': src_vref.provider_location} - def _create_cloned_volume(self, volume, src_vref): + def _create_cloned_volume(self, volume, src_vref, context): """Creates a clone of the specified volume.""" volume_format = self.get_volume_format(src_vref) if volume_format == DISK_FORMAT_PLOOP: diff --git a/releasenotes/notes/bugfix-1744692-5aebd0c97ae66407.yaml b/releasenotes/notes/bugfix-1744692-5aebd0c97ae66407.yaml new file mode 100644 index 00000000000..81d676c2526 --- /dev/null +++ b/releasenotes/notes/bugfix-1744692-5aebd0c97ae66407.yaml @@ -0,0 +1,6 @@ +--- +fixes: + - | + Fixes a bug that prevented distributed file system drivers from creating + snapshots during volume clone operations (NFS, WindowsSMBFS, VZstorage + and Quobyte drivers). Fixing this allows creating snapshot based backups.