From 5cb87d5dc3cc6ac11de07ed9971f5eff747c6e44 Mon Sep 17 00:00:00 2001 From: Ben Swartzlander Date: Fri, 28 Jul 2017 14:50:45 -0400 Subject: [PATCH] Fix multiple issues with revert to snapshot in LVM Reverts with the LVM driver were happening asynchronously which resulted in unreliable behavior. This change makes the reverts synchronous. Also, the driver now unexports and unmounts both the share and the snapshot during the revert (required to make the operation synchronous) which requires changing the driver interface to include the snapshot access rules in addition to the share access rules. Closes-bug: #1707943 Change-Id: Ifea1799e1d2e3963fec7e90ce3f9cb47b9f02f4f --- etc/manila/rootwrap.d/share.filters | 4 - manila/share/driver.py | 16 ++-- manila/share/drivers/hitachi/hnas/driver.py | 10 ++- manila/share/drivers/lvm.py | 79 +++++++++--------- .../dataontap/cluster_mode/drv_multi_svm.py | 6 +- .../dataontap/cluster_mode/drv_single_svm.py | 6 +- manila/share/manager.py | 28 ++++--- manila/share/snapshot_access.py | 5 ++ manila/tests/share/drivers/dummy.py | 7 +- .../share/drivers/hitachi/hnas/test_driver.py | 2 +- manila/tests/share/drivers/test_lvm.py | 80 ++++++++----------- manila/tests/share/test_manager.py | 71 ++++++++++------ ...m-revert-synchronous-0ef5baee3367fd27.yaml | 4 + 13 files changed, 172 insertions(+), 146 deletions(-) create mode 100644 releasenotes/notes/bug-1707943-make-lvm-revert-synchronous-0ef5baee3367fd27.yaml diff --git a/etc/manila/rootwrap.d/share.filters b/etc/manila/rootwrap.d/share.filters index 58cc84336c..bbe8f30a57 100644 --- a/etc/manila/rootwrap.d/share.filters +++ b/etc/manila/rootwrap.d/share.filters @@ -177,9 +177,5 @@ e2fsck: CommandFilter, e2fsck, root # manila/share/drivers/lvm.py: lvconvert --merge %s lvconvert: CommandFilter, lvconvert, root -# manila/share/drivers/lvm.py: lvchange -an %s -# manila/share/drivers/lvm.py: lvchange -ay %s -lvchange: CommandFilter, lvchange, root - # manila/data/utils.py: 'sha256sum', '%s' sha256sum: CommandFilter, sha256sum, root diff --git a/manila/share/driver.py b/manila/share/driver.py index 49bad69d49..775f99e985 100644 --- a/manila/share/driver.py +++ b/manila/share/driver.py @@ -991,8 +991,8 @@ class ShareDriver(object): the failure. """ - def revert_to_snapshot(self, context, snapshot, access_rules, - share_server=None): + def revert_to_snapshot(self, context, snapshot, share_access_rules, + snapshot_access_rules, share_server=None): """Reverts a share (in place) to the specified snapshot. Does not delete the share snapshot. The share and snapshot must both @@ -1006,7 +1006,10 @@ class ShareDriver(object): :param context: Current context :param snapshot: The snapshot to be restored - :param access_rules: List of all access rules for the affected share + :param share_access_rules: List of all access rules for the affected + share + :param snapshot_access_rules: List of all access rules for the affected + snapshot :param share_server: Optional -- Share server model or None """ raise NotImplementedError() @@ -2103,7 +2106,8 @@ class ShareDriver(object): def revert_to_replicated_snapshot(self, context, active_replica, replica_list, active_replica_snapshot, - replica_snapshots, access_rules, + replica_snapshots, share_access_rules, + snapshot_access_rules, share_server=None): """Reverts a replicated share (in place) to the specified snapshot. @@ -2130,7 +2134,9 @@ class ShareDriver(object): These snapshot instances track the snapshot across the replicas. The snapshot of the active replica to be restored with have its status attribute set to 'restoring'. - :param access_rules: List of access rules for the affected share. + :param share_access_rules: List of access rules for the affected share. + :param snapshot_access_rules: List of access rules for the affected + snapshot. :param share_server: Optional -- Share server model """ raise NotImplementedError() diff --git a/manila/share/drivers/hitachi/hnas/driver.py b/manila/share/drivers/hitachi/hnas/driver.py index 9c49ec519b..8a15b45b50 100644 --- a/manila/share/drivers/hitachi/hnas/driver.py +++ b/manila/share/drivers/hitachi/hnas/driver.py @@ -807,14 +807,16 @@ class HitachiHNASDriver(driver.ShareDriver): {'shr_id': share['id'], 'shr_size': six.text_type(new_size)}) - def revert_to_snapshot(self, context, snapshot, access_rules, - share_server=None): + def revert_to_snapshot(self, context, snapshot, share_access_rules, + snapshot_access_rules, share_server=None): """Reverts a share to a given snapshot. :param context: The `context.RequestContext` object for the request :param snapshot: The snapshot to which the share is to be reverted to. - :param access_rules: List of all access rules for the affected share. - Not used by this driver. + :param share_access_rules: List of all access rules for the affected + share. Not used by this driver. + :param snapshot_access_rules: List of all access rules for the affected + snapshot. Not used by this driver. :param share_server: Data structure with share server information. Not used by this driver. """ diff --git a/manila/share/drivers/lvm.py b/manila/share/drivers/lvm.py index d74c4ba38d..a2d7e0b08c 100644 --- a/manila/share/drivers/lvm.py +++ b/manila/share/drivers/lvm.py @@ -260,27 +260,26 @@ class LVMShareDriver(LVMMixin, driver.ShareDriver): return location def delete_share(self, context, share, share_server=None): - self._remove_export(context, share) + self._unmount_device(share) self._delete_share(context, share) self._deallocate_container(share['name']) - def _remove_export(self, ctx, share): - """Removes an access rules for a share.""" - mount_path = self._get_mount_path(share) + def _unmount_device(self, share_or_snapshot): + """Unmount the filesystem of a share or snapshot LV.""" + mount_path = self._get_mount_path(share_or_snapshot) if os.path.exists(mount_path): # umount, may be busy try: self._execute('umount', '-f', mount_path, run_as_root=True) except exception.ProcessExecutionError as exc: if 'device is busy' in six.text_type(exc): - raise exception.ShareBusyException(reason=share['name']) + raise exception.ShareBusyException( + reason=share_or_snapshot['name']) else: - LOG.info('Unable to umount: %s', exc) + LOG.error('Unable to umount: %s', exc) + raise # remove dir - try: - os.rmdir(mount_path) - except OSError: - LOG.warning('Unable to delete %s', mount_path) + self._execute('rmdir', mount_path, run_as_root=True) def ensure_share(self, ctx, share, share_server=None): """Ensure that storage are mounted and exported.""" @@ -336,9 +335,9 @@ class LVMShareDriver(LVMMixin, driver.ShareDriver): else: raise exception.InvalidShare(reason='Wrong share protocol') - def _mount_device(self, share, device_name): - """Mount LVM share and ignore if already mounted.""" - mount_path = self._get_mount_path(share) + def _mount_device(self, share_or_snapshot, device_name): + """Mount LV for share or snapshot and ignore if already mounted.""" + mount_path = self._get_mount_path(share_or_snapshot) self._execute('mkdir', '-p', mount_path) try: self._execute('mount', device_name, mount_path, @@ -353,15 +352,10 @@ class LVMShareDriver(LVMMixin, driver.ShareDriver): raise return mount_path - def _unmount_device(self, share): - mount_path = self._get_mount_path(share) - self._execute('umount', mount_path, run_as_root=True) - self._execute('rmdir', mount_path, run_as_root=True) - - def _get_mount_path(self, share): - """Returns path where share is mounted.""" + def _get_mount_path(self, share_or_snapshot): + """Returns path where share or snapshot is mounted.""" return os.path.join(self.configuration.share_mount_path, - share['name']) + share_or_snapshot['name']) def _copy_volume(self, srcstr, deststr, size_in_g): # Use O_DIRECT to avoid thrashing the system buffer cache @@ -384,53 +378,56 @@ class LVMShareDriver(LVMMixin, driver.ShareDriver): self._extend_container(share, device_name, new_size) self._execute('resize2fs', device_name, run_as_root=True) - def revert_to_snapshot(self, context, snapshot, access_rules, - share_server=None): + def revert_to_snapshot(self, context, snapshot, share_access_rules, + snapshot_access_rules, share_server=None): share = snapshot['share'] # Temporarily remove all access rules + self._get_helper(share).update_access(self.share_server, + snapshot['name'], [], [], []) self._get_helper(share).update_access(self.share_server, share['name'], [], [], []) - # Unmount the filesystem - self._remove_export(context, snapshot) - # First we merge the snapshot LV and the share LV - # This won't actually do anything until the LV is reactivated + # Unmount the snapshot filesystem + self._unmount_device(snapshot) + # Unmount the share filesystem + self._unmount_device(share) + # Merge the snapshot LV back into the share, reverting it snap_lv_name = "%s/%s" % (self.configuration.lvm_share_volume_group, snapshot['name']) self._execute('lvconvert', '--merge', snap_lv_name, run_as_root=True) - # Unmount the share so we can deactivate it - self._unmount_device(share) - # Deactivate the share LV - share_lv_name = "%s/%s" % (self.configuration.lvm_share_volume_group, - share['name']) - self._execute('lvchange', '-an', share_lv_name, run_as_root=True) - # Reactivate the share LV. This will trigger the merge and delete the - # snapshot. - self._execute('lvchange', '-ay', share_lv_name, run_as_root=True) + # Now recreate the snapshot that was destroyed by the merge self._create_snapshot(context, snapshot) # At this point we can mount the share again device_name = self._get_local_path(share) self._mount_device(share, device_name) + # Also remount the snapshot device_name = self._get_local_path(snapshot) self._mount_device(snapshot, device_name) # Lastly we add all the access rules back self._get_helper(share).update_access(self.share_server, - share['name'], access_rules, + share['name'], + share_access_rules, + [], []) + snapshot_access_rules, __, __ = utils.change_rules_to_readonly( + snapshot_access_rules, [], []) + self._get_helper(share).update_access(self.share_server, + snapshot['name'], + snapshot_access_rules, [], []) def create_snapshot(self, context, snapshot, share_server=None): self._create_snapshot(context, snapshot) - helper = self._get_helper(snapshot['share']) - exports = helper.create_exports(self.share_server, snapshot['name']) - device_name = self._get_local_path(snapshot) self._mount_device(snapshot, device_name) + helper = self._get_helper(snapshot['share']) + exports = helper.create_exports(self.share_server, snapshot['name']) + return {'export_locations': exports} def delete_snapshot(self, context, snapshot, share_server=None): - self._remove_export(context, snapshot) + self._unmount_device(snapshot) super(LVMShareDriver, self).delete_snapshot(context, snapshot, share_server) diff --git a/manila/share/drivers/netapp/dataontap/cluster_mode/drv_multi_svm.py b/manila/share/drivers/netapp/dataontap/cluster_mode/drv_multi_svm.py index a3ba9e7a21..9f7701cec7 100644 --- a/manila/share/drivers/netapp/dataontap/cluster_mode/drv_multi_svm.py +++ b/manila/share/drivers/netapp/dataontap/cluster_mode/drv_multi_svm.py @@ -55,7 +55,8 @@ class NetAppCmodeMultiSvmShareDriver(driver.ShareDriver): def create_snapshot(self, context, snapshot, **kwargs): return self.library.create_snapshot(context, snapshot, **kwargs) - def revert_to_snapshot(self, context, snapshot, access_rules, **kwargs): + def revert_to_snapshot(self, context, snapshot, share_access_rules, + snapshot_access_rules, **kwargs): return self.library.revert_to_snapshot(context, snapshot, **kwargs) def delete_share(self, context, share, **kwargs): @@ -170,7 +171,8 @@ class NetAppCmodeMultiSvmShareDriver(driver.ShareDriver): def revert_to_replicated_snapshot(self, context, active_replica, replica_list, active_replica_snapshot, - replica_snapshots, access_rules, + replica_snapshots, share_access_rules, + snapshot_access_rules, share_server=None): raise NotImplementedError() diff --git a/manila/share/drivers/netapp/dataontap/cluster_mode/drv_single_svm.py b/manila/share/drivers/netapp/dataontap/cluster_mode/drv_single_svm.py index 904a31194a..2433afdba6 100644 --- a/manila/share/drivers/netapp/dataontap/cluster_mode/drv_single_svm.py +++ b/manila/share/drivers/netapp/dataontap/cluster_mode/drv_single_svm.py @@ -55,7 +55,8 @@ class NetAppCmodeSingleSvmShareDriver(driver.ShareDriver): def create_snapshot(self, context, snapshot, **kwargs): return self.library.create_snapshot(context, snapshot, **kwargs) - def revert_to_snapshot(self, context, snapshot, access_rules, **kwargs): + def revert_to_snapshot(self, context, snapshot, share_access_rules, + snapshot_access_rules, **kwargs): return self.library.revert_to_snapshot(context, snapshot, **kwargs) def delete_share(self, context, share, **kwargs): @@ -185,7 +186,8 @@ class NetAppCmodeSingleSvmShareDriver(driver.ShareDriver): def revert_to_replicated_snapshot(self, context, active_replica, replica_list, active_replica_snapshot, - replica_snapshots, access_rules, + replica_snapshots, share_access_rules, + snapshot_access_rules, **kwargs): return self.library.revert_to_replicated_snapshot( context, active_replica, replica_list, active_replica_snapshot, diff --git a/manila/share/manager.py b/manila/share/manager.py index 333dc5aab2..cec9a1a5b1 100644 --- a/manila/share/manager.py +++ b/manila/share/manager.py @@ -2597,20 +2597,24 @@ class ShareManager(manager.SchedulerDependentManager): share = snapshot['share'] share_id = share['id'] share_instance_id = snapshot.instance.share_instance_id - access_rules = self.access_helper.get_share_instance_access_rules( - context, filters={'state': constants.STATUS_ACTIVE}, - share_instance_id=share_instance_id) + share_access_rules = ( + self.access_helper.get_share_instance_access_rules( + context, filters={'state': constants.STATUS_ACTIVE}, + share_instance_id=share_instance_id)) + snapshot_access_rules = ( + self.snapshot_access_helper.get_snapshot_instance_access_rules( + context, snapshot.instance['id'])) if share.get('has_replicas'): self._revert_to_replicated_snapshot( - context, share, snapshot, reservations, access_rules, - share_id=share_id) + context, share, snapshot, reservations, share_access_rules, + snapshot_access_rules, share_id=share_id) else: self._revert_to_snapshot(context, share, snapshot, reservations, - access_rules) + share_access_rules, snapshot_access_rules) def _revert_to_snapshot(self, context, share, snapshot, reservations, - access_rules): + share_access_rules, snapshot_access_rules): share_server = self._get_share_server(context, share) share_id = share['id'] @@ -2629,7 +2633,8 @@ class ShareManager(manager.SchedulerDependentManager): try: self.driver.revert_to_snapshot(context, snapshot_instance_dict, - access_rules, + share_access_rules, + snapshot_access_rules, share_server=share_server) except Exception as excep: with excutils.save_and_reraise_exception(): @@ -2975,8 +2980,8 @@ class ShareManager(manager.SchedulerDependentManager): @locked_share_replica_operation def _revert_to_replicated_snapshot(self, context, share, snapshot, - reservations, access_rules, - share_id=None): + reservations, share_access_rules, + snapshot_access_rules, share_id=None): share_server = self._get_share_server(context, share) snapshot_id = snapshot['id'] @@ -3013,7 +3018,8 @@ class ShareManager(manager.SchedulerDependentManager): try: self.driver.revert_to_replicated_snapshot( context, active_replica, replica_list, active_replica_snapshot, - replica_snapshots, access_rules, share_server=share_server) + replica_snapshots, share_access_rules, + snapshot_access_rules, share_server=share_server) except Exception: with excutils.save_and_reraise_exception(): diff --git a/manila/share/snapshot_access.py b/manila/share/snapshot_access.py index c5bdef799d..51beb0397a 100644 --- a/manila/share/snapshot_access.py +++ b/manila/share/snapshot_access.py @@ -166,3 +166,8 @@ class ShareSnapshotInstanceAccess(object): for rule in rules: self.db.share_snapshot_instance_access_delete( context, rule['access_id'], snapshot_instance_id) + + def get_snapshot_instance_access_rules(self, context, + snapshot_instance_id): + return self.db.share_snapshot_access_get_all_for_snapshot_instance( + context, snapshot_instance_id) diff --git a/manila/tests/share/drivers/dummy.py b/manila/tests/share/drivers/dummy.py index cfdb1ad161..5d77bdc628 100644 --- a/manila/tests/share/drivers/dummy.py +++ b/manila/tests/share/drivers/dummy.py @@ -320,8 +320,8 @@ class DummyDriver(driver.ShareDriver): """Removes the specified snapshot from Manila management.""" @slow_me_down - def revert_to_snapshot(self, context, snapshot, access_rules, - share_server=None): + def revert_to_snapshot(self, context, snapshot, share_access_rules, + snapshot_access_rules, share_server=None): """Reverts a share (in place) to the specified snapshot.""" @slow_me_down @@ -496,7 +496,8 @@ class DummyDriver(driver.ShareDriver): @slow_me_down def revert_to_replicated_snapshot(self, context, active_replica, replica_list, active_replica_snapshot, - replica_snapshots, access_rules, + replica_snapshots, share_access_rules, + snapshot_access_rules, share_server=None): """Reverts a replicated share (in place) to the specified snapshot.""" diff --git a/manila/tests/share/drivers/hitachi/hnas/test_driver.py b/manila/tests/share/drivers/hitachi/hnas/test_driver.py index 2e8efcccae..b5a524cdd0 100644 --- a/manila/tests/share/drivers/hitachi/hnas/test_driver.py +++ b/manila/tests/share/drivers/hitachi/hnas/test_driver.py @@ -1135,7 +1135,7 @@ class HitachiHNASTestCase(test.TestCase): self.mock_object(ssh.HNASSSHBackend, 'tree_clone', mock.Mock(side_effect=exc)) - self._driver.revert_to_snapshot('context', snap, None) + self._driver.revert_to_snapshot('context', snap, None, None) driver.HitachiHNASDriver._check_fs_mounted.assert_called_once_with() ssh.HNASSSHBackend.tree_delete.assert_called_once_with( diff --git a/manila/tests/share/drivers/test_lvm.py b/manila/tests/share/drivers/test_lvm.py index f10fe91fbc..4dc2fc6f30 100644 --- a/manila/tests/share/drivers/test_lvm.py +++ b/manila/tests/share/drivers/test_lvm.py @@ -18,6 +18,7 @@ import os import ddt import mock +from oslo_concurrency import processutils from oslo_config import cfg from oslo_utils import timeutils @@ -286,21 +287,7 @@ class LVMShareDriverTestCase(test.TestCase): self._driver.get_share_stats(refresh=True)) self._driver._update_share_stats.assert_called_once_with() - def test_remove_export(self): - mount_path = self._get_mount_path(self.share) - self._os.path.exists.return_value = True - - self._driver._remove_export(self._context, self.share) - - expected_exec = [ - "umount -f %s" % (mount_path,), - ] - - self._os.path.exists.assert_called_with(mount_path) - self._os.rmdir.assert_called_with(mount_path) - self.assertEqual(expected_exec, fake_utils.fake_execute_get_log()) - - def test_remove_export_is_busy_error(self): + def test__unmount_device_is_busy_error(self): def exec_runner(*ignore_args, **ignore_kwargs): raise exception.ProcessExecutionError(stderr='device is busy') self._os.path.exists.return_value = True @@ -311,37 +298,33 @@ class LVMShareDriverTestCase(test.TestCase): fake_utils.fake_execute_set_repliers([(expected_exec[0], exec_runner)]) self.assertRaises(exception.ShareBusyException, - self._driver._remove_export, self._context, + self._driver._unmount_device, self.share) self.assertEqual(expected_exec, fake_utils.fake_execute_get_log()) - def test_remove_export_error(self): + def test__unmount_device_error(self): def exec_runner(*ignore_args, **ignore_kwargs): raise exception.ProcessExecutionError(stderr='fake error') - - mount_path = self._get_mount_path(self.share) - expected_exec = [ - "umount -f %s" % (mount_path), - ] - fake_utils.fake_execute_set_repliers([(expected_exec[0], exec_runner)]) - self._os.path.exists.return_value = True - self._driver._remove_export(self._context, self.share) - self.assertEqual(expected_exec, fake_utils.fake_execute_get_log()) - - def test_remove_export_rmdir_error(self): mount_path = self._get_mount_path(self.share) self._os.path.exists.return_value = True - self.mock_object(self._os, 'rmdir', mock.Mock(side_effect=OSError)) - - self._driver._remove_export(self._context, self.share) - - expected_exec = [ - "umount -f %s" % (mount_path,), - ] - + cmd = "umount -f %s" % (mount_path) + fake_utils.fake_execute_set_repliers([(cmd, exec_runner)]) + self.assertRaises(processutils.ProcessExecutionError, + self._driver._unmount_device, + self.share) + self._os.path.exists.assert_called_with(mount_path) + + def test__unmount_device_rmdir_error(self): + def exec_runner(*ignore_args, **ignore_kwargs): + raise exception.ProcessExecutionError(stderr='fake error') + mount_path = self._get_mount_path(self.share) + self._os.path.exists.return_value = True + cmd = "rmdir %s" % (mount_path) + fake_utils.fake_execute_set_repliers([(cmd, exec_runner)]) + self.assertRaises(processutils.ProcessExecutionError, + self._driver._unmount_device, + self.share) self._os.path.exists.assert_called_with(mount_path) - self._os.rmdir.assert_called_with(mount_path) - self.assertEqual(expected_exec, fake_utils.fake_execute_get_log()) def test_create_snapshot(self): self._driver.create_snapshot(self._context, self.snapshot, @@ -375,8 +358,10 @@ class LVMShareDriverTestCase(test.TestCase): self._driver._delete_share(self._context, self.share) def test_delete_snapshot(self): + mount_path = self._get_mount_path(self.snapshot) expected_exec = [ - 'umount -f ' + self._get_mount_path(self.snapshot), + 'umount -f %s' % mount_path, + 'rmdir %s' % mount_path, 'lvremove -f fakevg/fakesnapshotname', ] self._driver.delete_snapshot(self._context, self.snapshot, @@ -482,14 +467,16 @@ class LVMShareDriverTestCase(test.TestCase): def _get_mount_path(self, share): return os.path.join(CONF.lvm_share_export_root, share['name']) - def test_unmount_device(self): + def test__unmount_device(self): mount_path = self._get_mount_path(self.share) + self._os.path.exists.return_value = True self.mock_object(self._driver, '_execute') self._driver._unmount_device(self.share) - self._driver._execute.assert_any_call('umount', mount_path, + self._driver._execute.assert_any_call('umount', '-f', mount_path, run_as_root=True) self._driver._execute.assert_any_call('rmdir', mount_path, run_as_root=True) + self._os.path.exists.assert_called_with(mount_path) def test_extend_share(self): local_path = self._driver._get_local_path(self.share) @@ -579,18 +566,17 @@ class LVMShareDriverTestCase(test.TestCase): mock_update_access = self.mock_object(self._helper_nfs, 'update_access') self._driver.revert_to_snapshot(self._context, self.snapshot, - [], self.share_server) + [], [], self.share_server) snap_lv = "%s/fakesnapshotname" % (CONF.lvm_share_volume_group) share_lv = "%s/fakename" % (CONF.lvm_share_volume_group) share_mount_path = self._get_mount_path(self.snapshot['share']) snapshot_mount_path = self._get_mount_path(self.snapshot) expected_exec = [ ('umount -f %s' % snapshot_mount_path), - ("lvconvert --merge %s" % snap_lv), - ("umount %s" % share_mount_path), + ("rmdir %s" % snapshot_mount_path), + ("umount -f %s" % share_mount_path), ("rmdir %s" % share_mount_path), - ("lvchange -an %s" % share_lv), - ("lvchange -ay %s" % share_lv), + ("lvconvert --merge %s" % snap_lv), ("lvcreate -L 1G --name fakesnapshotname --snapshot %s" % share_lv), ('tune2fs -U random /dev/mapper/%s-fakesnapshotname' % @@ -605,7 +591,7 @@ class LVMShareDriverTestCase(test.TestCase): ("chmod 777 %s" % snapshot_mount_path), ] self.assertEqual(expected_exec, fake_utils.fake_execute_get_log()) - self.assertEqual(2, mock_update_access.call_count) + self.assertEqual(4, mock_update_access.call_count) def test_snapshot_update_access(self): access_rules = [{ diff --git a/manila/tests/share/test_manager.py b/manila/tests/share/test_manager.py index b2629107db..5a6f46b93c 100644 --- a/manila/tests/share/test_manager.py +++ b/manila/tests/share/test_manager.py @@ -5565,51 +5565,61 @@ class ShareManagerTestCase(test.TestCase): reservations = 'fake_reservations' share_id = 'fake_share_id' snapshot_id = 'fake_snapshot_id' - instance_id = 'fake_instance_id' + snapshot_instance_id = 'fake_snapshot_instance_id' + share_instance_id = 'fake_share_instance_id' share_instance = fakes.fake_share_instance( - id=instance_id, share_id=share_id) + id=share_instance_id, share_id=share_id) share = fakes.fake_share( id=share_id, instance=share_instance, project_id='fake_project', user_id='fake_user', size=2, has_replicas=has_replicas) snapshot_instance = fakes.fake_snapshot_instance( - share_id=instance_id, share=share, name='fake_snapshot', - share_instance=share_instance, share_instance_id=instance_id) + id=snapshot_instance_id, share_id=share_instance_id, share=share, + name='fake_snapshot', share_instance=share_instance, + share_instance_id=share_instance_id) snapshot = fakes.fake_snapshot( id=snapshot_id, share_id=share_id, share=share, instance=snapshot_instance, project_id='fake_project', user_id='fake_user', size=1) - access_rules = ['fake_access_rule'] + share_access_rules = ['fake_share_access_rule'] + snapshot_access_rules = ['fake_snapshot_access_rule'] mock_share_snapshot_get = self.mock_object( self.share_manager.db, 'share_snapshot_get', mock.Mock(return_value=snapshot)) - mock_access_get = self.mock_object( + mock_share_access_get = self.mock_object( self.share_manager.access_helper, 'get_share_instance_access_rules', - mock.Mock(return_value=access_rules)) + mock.Mock(return_value=share_access_rules)) + mock_snapshot_access_get = self.mock_object( + self.share_manager.snapshot_access_helper, + 'get_snapshot_instance_access_rules', + mock.Mock(return_value=snapshot_access_rules)) mock_revert_to_snapshot = self.mock_object( self.share_manager, '_revert_to_snapshot') mock_revert_to_replicated_snapshot = self.mock_object( self.share_manager, '_revert_to_replicated_snapshot') - self.share_manager.revert_to_snapshot( - self.context, snapshot_id, reservations) + self.share_manager.revert_to_snapshot(self.context, snapshot_id, + reservations) mock_share_snapshot_get.assert_called_once_with(mock.ANY, snapshot_id) - mock_access_get.assert_called_once_with( + mock_share_access_get.assert_called_once_with( mock.ANY, filters={'state': constants.STATUS_ACTIVE}, - share_instance_id=instance_id) + share_instance_id=share_instance_id) + mock_snapshot_access_get.assert_called_once_with( + mock.ANY, snapshot_instance_id) if not has_replicas: mock_revert_to_snapshot.assert_called_once_with( - mock.ANY, share, snapshot, reservations, access_rules) + mock.ANY, share, snapshot, reservations, share_access_rules, + snapshot_access_rules) self.assertFalse(mock_revert_to_replicated_snapshot.called) else: self.assertFalse(mock_revert_to_snapshot.called) mock_revert_to_replicated_snapshot.assert_called_once_with( - mock.ANY, share, snapshot, reservations, access_rules, - share_id=share_id) + mock.ANY, share, snapshot, reservations, share_access_rules, + snapshot_access_rules, share_id=share_id) @ddt.data(None, 'fake_reservations') def test__revert_to_snapshot(self, reservations): @@ -5633,7 +5643,8 @@ class ShareManagerTestCase(test.TestCase): id='fake_snapshot_id', share_id=share_id, share=share, instance=snapshot_instance, project_id='fake_project', user_id='fake_user', size=1) - access_rules = [] + share_access_rules = [] + snapshot_access_rules = [] self.mock_object( self.share_manager.db, 'share_snapshot_get', @@ -5646,14 +5657,16 @@ class ShareManagerTestCase(test.TestCase): mock_share_snapshot_update = self.mock_object( self.share_manager.db, 'share_snapshot_update') - self.share_manager._revert_to_snapshot( - self.context, share, snapshot, reservations, access_rules) + self.share_manager._revert_to_snapshot(self.context, share, snapshot, + reservations, + share_access_rules, + snapshot_access_rules) mock_driver.revert_to_snapshot.assert_called_once_with( mock.ANY, self._get_snapshot_instance_dict( snapshot_instance, share, snapshot=snapshot), - access_rules, + share_access_rules, snapshot_access_rules, share_server=None) self.assertFalse(mock_quotas_rollback.called) @@ -5696,7 +5709,8 @@ class ShareManagerTestCase(test.TestCase): id='fake_snapshot_id', share_id=share_id, share=share, instance=snapshot_instance, project_id='fake_project', user_id='fake_user', size=1) - access_rules = [] + share_access_rules = [] + snapshot_access_rules = [] self.mock_object( self.share_manager.db, 'share_snapshot_get', @@ -5715,13 +5729,15 @@ class ShareManagerTestCase(test.TestCase): share, snapshot, reservations, - access_rules) + share_access_rules, + snapshot_access_rules) mock_driver.revert_to_snapshot.assert_called_once_with( mock.ANY, self._get_snapshot_instance_dict( snapshot_instance, share, snapshot=snapshot), - access_rules, + share_access_rules, + snapshot_access_rules, share_server=None) self.assertFalse(mock_quotas_commit.called) @@ -5915,7 +5931,8 @@ class ShareManagerTestCase(test.TestCase): id='rid2', share_id=share_id, host='secondary', replica_state=constants.REPLICA_STATE_IN_SYNC, as_primitive=False) replicas = [active_replica, replica] - access_rules = [] + share_access_rules = [] + snapshot_access_rules = [] self.mock_object( db, 'share_snapshot_get', mock.Mock(return_value=snapshot)) self.mock_object( @@ -5937,8 +5954,8 @@ class ShareManagerTestCase(test.TestCase): self.share_manager.db, 'share_snapshot_instance_update') self.share_manager._revert_to_replicated_snapshot( - self.context, share, snapshot, reservations, access_rules, - share_id=share_id) + self.context, share, snapshot, reservations, share_access_rules, + snapshot_access_rules, share_id=share_id) self.assertTrue(mock_driver.revert_to_replicated_snapshot.called) self.assertFalse(mock_quotas_rollback.called) @@ -5981,7 +5998,8 @@ class ShareManagerTestCase(test.TestCase): replica_state=constants.REPLICA_STATE_IN_SYNC, as_primitive=False, share_type_id='fake_share_type_id') replicas = [active_replica, replica] - access_rules = [] + share_access_rules = [] + snapshot_access_rules = [] self.mock_object( db, 'share_snapshot_get', mock.Mock(return_value=snapshot)) self.mock_object( @@ -6010,7 +6028,8 @@ class ShareManagerTestCase(test.TestCase): share, snapshot, reservations, - access_rules, + share_access_rules, + snapshot_access_rules, share_id=share_id) self.assertTrue(mock_driver.revert_to_replicated_snapshot.called) diff --git a/releasenotes/notes/bug-1707943-make-lvm-revert-synchronous-0ef5baee3367fd27.yaml b/releasenotes/notes/bug-1707943-make-lvm-revert-synchronous-0ef5baee3367fd27.yaml new file mode 100644 index 0000000000..d065e6a78a --- /dev/null +++ b/releasenotes/notes/bug-1707943-make-lvm-revert-synchronous-0ef5baee3367fd27.yaml @@ -0,0 +1,4 @@ +--- +fixes: + - Changed implementation of revert-to-snapshot in LVM driver to be + synchronous, preventing failure of subsequent operations.