From 820638ccef471eb00f2a233e64b05a9c377e170c Mon Sep 17 00:00:00 2001 From: Goutham Pacha Ravi Date: Fri, 3 Aug 2018 14:46:08 -0700 Subject: [PATCH] [ZFSOnLinux] Retry unmounting old datasets during manage Add a retry loop to ensure the dataset being renamed is cleanly unmounted before the rename operation. Change-Id: Ie506f237010c415ee9f0d64abbefd5854f776a5f Closes-Bug: #1785180 (cherry picked from commit d7c01efb444b7bf02c1729be3554abebaee473a3) (cherry picked from commit 6d8f47d3c367aa0247dca713146825cfa66b6848) --- manila/share/drivers/zfsonlinux/driver.py | 54 +++++++++---- .../share/drivers/zfsonlinux/test_driver.py | 78 +++++++++++++++++-- ...unting-during-manage-872cf46313c5a4ff.yaml | 6 ++ 3 files changed, 116 insertions(+), 22 deletions(-) create mode 100644 releasenotes/notes/bug-1785180-zfsonlinux-retry-unmounting-during-manage-872cf46313c5a4ff.yaml diff --git a/manila/share/drivers/zfsonlinux/driver.py b/manila/share/drivers/zfsonlinux/driver.py index 8a9141caf9..06d9f18417 100644 --- a/manila/share/drivers/zfsonlinux/driver.py +++ b/manila/share/drivers/zfsonlinux/driver.py @@ -724,15 +724,6 @@ class ZFSonLinuxShareDriver(zfs_utils.ExecuteMixin, driver.ShareDriver): "username": self.configuration.zfs_ssh_username, "host": self.service_ip, } - self.private_storage.update( - share["id"], { - "entity_type": "share", - "dataset_name": new_dataset_name, - "ssh_cmd": ssh_cmd, # used in replication - "pool_name": actual_pool_name, # used in replication - "used_options": " ".join(options), - } - ) # Perform checks on requested dataset if actual_pool_name != scheduled_pool_name: @@ -760,13 +751,25 @@ class ZFSonLinuxShareDriver(zfs_utils.ExecuteMixin, driver.ShareDriver): "dataset": old_dataset_name, "zpool": actual_pool_name}) - # Rename dataset - out, err = self.execute("sudo", "mount") - if "%s " % old_dataset_name in out: - self.zfs_with_retry("umount", "-f", old_dataset_name) - time.sleep(1) + # Unmount the dataset before attempting to rename and mount + try: + self._unmount_share_with_retry(old_dataset_name) + except exception.ZFSonLinuxException: + msg = _("Unable to unmount share before renaming and re-mounting.") + raise exception.ZFSonLinuxException(message=msg) + + # Rename the dataset and mount with new name self.zfs_with_retry("rename", old_dataset_name, new_dataset_name) - self.zfs("mount", new_dataset_name) + + try: + self.zfs("mount", new_dataset_name) + except exception.ProcessExecutionError: + # Workaround for bug/1785180 + out, err = self.zfs("mount") + mounted = any([new_dataset_name in mountedfs + for mountedfs in out.splitlines()]) + if not mounted: + raise # Apply options to dataset for option in options: @@ -776,6 +779,16 @@ class ZFSonLinuxShareDriver(zfs_utils.ExecuteMixin, driver.ShareDriver): export_locations = self._get_share_helper( share["share_proto"]).get_exports(new_dataset_name) + self.private_storage.update( + share["id"], { + "entity_type": "share", + "dataset_name": new_dataset_name, + "ssh_cmd": ssh_cmd, # used in replication + "pool_name": actual_pool_name, # used in replication + "used_options": " ".join(options), + } + ) + return {"size": share["size"], "export_locations": export_locations} def unmanage(self, share): @@ -836,6 +849,17 @@ class ZFSonLinuxShareDriver(zfs_utils.ExecuteMixin, driver.ShareDriver): """Unmanage dataset snapshot.""" self.private_storage.delete(snapshot_instance["snapshot_id"]) + @utils.retry(exception.ZFSonLinuxException) + def _unmount_share_with_retry(self, share_name): + out, err = self.execute("sudo", "mount") + if "%s " % share_name not in out: + return + self.zfs_with_retry("umount", "-f", share_name) + out, err = self.execute("sudo", "mount") + if "%s " % share_name in out: + raise exception.ZFSonLinuxException( + _("Unable to unmount dataset %s"), share_name) + def _get_replication_snapshot_prefix(self, replica): """Returns replica-based snapshot prefix.""" replication_snapshot_prefix = "%s_%s" % ( diff --git a/manila/tests/share/drivers/zfsonlinux/test_driver.py b/manila/tests/share/drivers/zfsonlinux/test_driver.py index a91c970b42..36ee0a54de 100644 --- a/manila/tests/share/drivers/zfsonlinux/test_driver.py +++ b/manila/tests/share/drivers/zfsonlinux/test_driver.py @@ -1138,7 +1138,7 @@ class ZFSonLinuxShareDriverTestCase(test.TestCase): zfs_driver.share_types, 'get_extra_specs_from_share', mock.Mock(return_value={})) - mock_sleep = self.mock_object(zfs_driver.time, "sleep") + self.mock_object(zfs_driver.time, "sleep") mock__get_dataset_name = self.mock_object( self.driver, "_get_dataset_name", mock.Mock(return_value=new_dataset_name)) @@ -1148,11 +1148,17 @@ class ZFSonLinuxShareDriverTestCase(test.TestCase): mock.Mock(return_value=("fake_out", "fake_error"))) mock_zfs_with_retry = self.mock_object(self.driver, "zfs_with_retry") - mock_execute = self.mock_object(self.driver, "execute") + mock_execute_side_effects = [ + ("%s " % old_dataset_name, "fake_err") + if mount_exists else ("foo", "bar") + ] * 3 if mount_exists: - mock_execute.return_value = "%s " % old_dataset_name, "fake_err" - else: - mock_execute.return_value = ("foo", "bar") + # After three retries, assume the mount goes away + mock_execute_side_effects.append((("foo", "bar"))) + mock_execute = self.mock_object( + self.driver, "execute", + mock.Mock(side_effect=iter(mock_execute_side_effects))) + mock_parse_zfs_answer = self.mock_object( self.driver, "parse_zfs_answer", @@ -1175,9 +1181,11 @@ class ZFSonLinuxShareDriverTestCase(test.TestCase): self.assertEqual( mock_helper.return_value.get_exports.return_value, result["export_locations"]) + mock_execute.assert_called_with("sudo", "mount") if mount_exists: - mock_sleep.assert_called_once_with(1) - mock_execute.assert_called_once_with("sudo", "mount") + self.assertEqual(4, mock_execute.call_count) + else: + self.assertEqual(1, mock_execute.call_count) mock_parse_zfs_answer.assert_called_once_with(mock_zfs.return_value[0]) if driver_options.get("size"): self.assertFalse(mock_get_zfs_option.called) @@ -1259,6 +1267,62 @@ class ZFSonLinuxShareDriverTestCase(test.TestCase): mock_parse_zfs_answer.assert_called_once_with(mock_zfs.return_value[0]) mock_get_extra_specs_from_share.assert_called_once_with(share) + def test_manage_unmount_exception(self): + old_ds_name = "foopool/path/to/old/dataset/name" + new_ds_name = "foopool/path/to/new/dataset/name" + share = { + "id": "fake_share_instance_id", + "share_id": "fake_share_id", + "export_locations": [{"path": "1.1.1.1:/%s" % old_ds_name}], + "host": "foobackend@foohost#foopool", + "share_proto": "NFS", + } + + mock_get_extra_specs_from_share = self.mock_object( + zfs_driver.share_types, + 'get_extra_specs_from_share', + mock.Mock(return_value={})) + self.mock_object(zfs_driver.time, "sleep") + mock__get_dataset_name = self.mock_object( + self.driver, "_get_dataset_name", + mock.Mock(return_value=new_ds_name)) + mock_helper = self.mock_object(self.driver, "_get_share_helper") + mock_zfs = self.mock_object( + self.driver, "zfs", + mock.Mock(return_value=("fake_out", "fake_error"))) + mock_zfs_with_retry = self.mock_object(self.driver, "zfs_with_retry") + + # 10 Retries, would mean 20 calls to check the mount still exists + mock_execute_side_effects = [("%s " % old_ds_name, "fake_err")] * 21 + mock_execute = self.mock_object( + self.driver, "execute", + mock.Mock(side_effect=mock_execute_side_effects)) + + mock_parse_zfs_answer = self.mock_object( + self.driver, + "parse_zfs_answer", + mock.Mock(return_value=[ + {"NAME": "some_other_dataset_1"}, + {"NAME": old_ds_name}, + {"NAME": "some_other_dataset_2"}, + ])) + mock_get_zfs_option = self.mock_object( + self.driver, 'get_zfs_option', mock.Mock(return_value="4G")) + + self.assertRaises(exception.ZFSonLinuxException, + self.driver.manage_existing, + share, {'size': 10}) + + self.assertFalse(mock_helper.return_value.get_exports.called) + mock_zfs_with_retry.assert_called_with("umount", "-f", old_ds_name) + mock_execute.assert_called_with("sudo", "mount") + self.assertEqual(10, mock_zfs_with_retry.call_count) + self.assertEqual(20, mock_execute.call_count) + mock_parse_zfs_answer.assert_called_once_with(mock_zfs.return_value[0]) + self.assertFalse(mock_get_zfs_option.called) + mock__get_dataset_name.assert_called_once_with(share) + mock_get_extra_specs_from_share.assert_called_once_with(share) + def test_unmanage(self): share = {'id': 'fake_share_id'} self.mock_object(self.driver.private_storage, 'delete') diff --git a/releasenotes/notes/bug-1785180-zfsonlinux-retry-unmounting-during-manage-872cf46313c5a4ff.yaml b/releasenotes/notes/bug-1785180-zfsonlinux-retry-unmounting-during-manage-872cf46313c5a4ff.yaml new file mode 100644 index 0000000000..bcaf21dda6 --- /dev/null +++ b/releasenotes/notes/bug-1785180-zfsonlinux-retry-unmounting-during-manage-872cf46313c5a4ff.yaml @@ -0,0 +1,6 @@ +--- +fixes: + - | + The ZFSOnLinux driver now retries unmounting zfs shares + to perform the manage operation. See `Launchpad bug 1785180 + `_ for details.