From 49856dc9c8df62fd9a672aa1808cffb4ebe0cbb0 Mon Sep 17 00:00:00 2001 From: Alexey Ovchinnikov Date: Mon, 19 Sep 2016 13:40:44 +0300 Subject: [PATCH] Bring remote and local executors into accord It appears that remote executor (processutils.ssh_execute) and local executor (processutils.execute) have different expectations for command keywords containing spaces. This difference manifests itself in helper failures either for one executor type or for another. This change adds command preprocessing to generic driver which seems to be the only active user of both remote executor and failing helpers. Change-Id: I26eaca3ca652171fbf20d7580f90eef4f935332e Closes-Bug: 1621033 --- manila/share/drivers/generic.py | 16 +++++++---- manila/share/drivers/helpers.py | 31 +++++++++++----------- manila/tests/share/drivers/test_generic.py | 30 ++++++++++++++++----- manila/tests/share/drivers/test_helpers.py | 23 ++++++++-------- 4 files changed, 63 insertions(+), 37 deletions(-) diff --git a/manila/share/drivers/generic.py b/manila/share/drivers/generic.py index b5c12ac0..b3e48461 100644 --- a/manila/share/drivers/generic.py +++ b/manila/share/drivers/generic.py @@ -160,6 +160,11 @@ class GenericShareDriver(driver.ExecuteMixin, driver.ShareDriver): ssh_pool.remove(ssh) ssh = ssh_pool.create() self.ssh_connections[server['instance_id']] = (ssh_pool, ssh) + + # (aovchinnikov): ssh_execute does not behave well when passed + # parameters with spaces. + wrap = lambda token: "\"" + token + "\"" + command = [wrap(tkn) if tkn.count(' ') else tkn for tkn in command] return processutils.ssh_execute(ssh, ' '.join(command), check_exit_code=check_exit_code) @@ -335,10 +340,11 @@ class GenericShareDriver(driver.ExecuteMixin, driver.ShareDriver): volume): LOG.debug("Mounting '%(dev)s' to path '%(path)s' on " "server '%(server)s'.", log_data) - mount_cmd = ['sudo mkdir -p', mount_path, '&&'] - mount_cmd.extend(['sudo mount', volume['mountpoint'], + mount_cmd = ['sudo', 'mkdir', '-p', mount_path, '&&'] + mount_cmd.extend(['sudo', 'mount', volume['mountpoint'], + mount_path]) + mount_cmd.extend(['&&', 'sudo', 'chmod', '777', mount_path]) - mount_cmd.extend(['&& sudo chmod 777', mount_path]) self._ssh_exec(server_details, mount_cmd) # Add mount permanently @@ -365,8 +371,8 @@ class GenericShareDriver(driver.ExecuteMixin, driver.ShareDriver): if self._is_device_mounted(mount_path, server_details): LOG.debug("Unmounting path '%(path)s' on server " "'%(server)s'.", log_data) - unmount_cmd = ['sudo umount', mount_path, '&& sudo rmdir', - mount_path] + unmount_cmd = ['sudo', 'umount', mount_path, '&&', 'sudo', + 'rmdir', mount_path] self._ssh_exec(server_details, unmount_cmd) # Remove mount permanently self._sync_mount_temp_and_perm_files(server_details) diff --git a/manila/share/drivers/helpers.py b/manila/share/drivers/helpers.py index 47894829..068f990f 100644 --- a/manila/share/drivers/helpers.py +++ b/manila/share/drivers/helpers.py @@ -289,8 +289,8 @@ class NFSHelper(NASHelperBase): maintenance_file = self._get_maintenance_file_path(share_name) backup_exports = [ 'cat', const.NFS_EXPORTS_FILE, - '| grep', share_name, - '| sudo tee', maintenance_file + '|', 'grep', share_name, + '|', 'sudo', 'tee', maintenance_file ] self._ssh_exec(server, backup_exports) @@ -304,9 +304,9 @@ class NFSHelper(NASHelperBase): maintenance_file = self._get_maintenance_file_path(share_name) restore_exports = [ 'cat', maintenance_file, - '| sudo tee -a', const.NFS_EXPORTS_FILE, - '&& sudo exportfs -r', - '&& sudo rm -f', maintenance_file + '|', 'sudo', 'tee', '-a', const.NFS_EXPORTS_FILE, + '&&', 'sudo', 'exportfs', '-r', + '&&', 'sudo', 'rm', '-f', maintenance_file ] self._ssh_exec(server, restore_exports) @@ -324,10 +324,10 @@ class CIFSHelperIPAccess(NASHelperBase): self.export_format = '\\\\%s\\%s' self.parameters = { 'browseable': 'yes', - '\"create mask\"': '0755', - '\"hosts deny\"': '0.0.0.0/0', # deny all by default - '\"hosts allow\"': '127.0.0.1', - '\"read only\"': 'no', + 'create mask': '0755', + 'hosts deny': '0.0.0.0/0', # deny all by default + 'hosts allow': '127.0.0.1', + 'read only': 'no', } def init_helper(self, server): @@ -406,13 +406,13 @@ class CIFSHelperIPAccess(NASHelperBase): def _get_allow_hosts(self, server, share_name): (out, _) = self._ssh_exec(server, ['sudo', 'net', 'conf', 'getparm', - share_name, '\"hosts allow\"']) + share_name, 'hosts allow']) return out.split() def _set_allow_hosts(self, server, hosts, share_name): - value = "\"" + ' '.join(hosts) + "\"" + value = ' '.join(hosts) or ' ' self._ssh_exec(server, ['sudo', 'net', 'conf', 'setparm', share_name, - '\"hosts allow\"', value]) + 'hosts allow', value]) @staticmethod def _get_share_group_name_from_export_location(export_location): @@ -450,7 +450,8 @@ class CIFSHelperIPAccess(NASHelperBase): allowed_hosts = " ".join(self._get_allow_hosts(server, share_name)) backup_exports = [ - 'echo', "'%s'" % allowed_hosts, '| sudo tee', maintenance_file + 'echo', "'%s'" % allowed_hosts, '|', 'sudo', 'tee', + maintenance_file ] self._ssh_exec(server, backup_exports) self._set_allow_hosts(server, [], share_name) @@ -459,7 +460,7 @@ class CIFSHelperIPAccess(NASHelperBase): maintenance_file = self._get_maintenance_file_path(share_name) (exports, __) = self._ssh_exec(server, ['cat', maintenance_file]) self._set_allow_hosts(server, exports.split(), share_name) - self._ssh_exec(server, ['sudo rm -f', maintenance_file]) + self._ssh_exec(server, ['sudo', 'rm', '-f', maintenance_file]) class CIFSHelperUserAccess(CIFSHelperIPAccess): @@ -512,7 +513,7 @@ class CIFSHelperUserAccess(CIFSHelperIPAccess): return 'read list' def _set_valid_users(self, server, users, share_name, access_level): - value = "\"" + ' '.join(users) + "\"" + value = ' '.join(users) param = self._get_conf_param(access_level) self._ssh_exec(server, ['sudo', 'net', 'conf', 'setparm', share_name, param, value]) diff --git a/manila/tests/share/drivers/test_generic.py b/manila/tests/share/drivers/test_generic.py index bc679325..82d46640 100644 --- a/manila/tests/share/drivers/test_generic.py +++ b/manila/tests/share/drivers/test_generic.py @@ -397,9 +397,9 @@ class GenericShareDriverTestCase(test.TestCase): server) self._driver._ssh_exec.assert_called_once_with( server, - ['sudo mkdir -p', mount_path, - '&&', 'sudo mount', volume['mountpoint'], mount_path, - '&& sudo chmod 777', mount_path], + ['sudo', 'mkdir', '-p', mount_path, + '&&', 'sudo', 'mount', volume['mountpoint'], mount_path, + '&&', 'sudo', 'chmod', '777', mount_path], ) def test_mount_device_present(self): @@ -454,7 +454,7 @@ class GenericShareDriverTestCase(test.TestCase): self.server) self._driver._ssh_exec.assert_called_once_with( self.server, - ['sudo umount', mount_path, '&& sudo rmdir', mount_path], + ['sudo', 'umount', mount_path, '&&', 'sudo', 'rmdir', mount_path], ) def test_unmount_device_retry_once(self): @@ -485,8 +485,8 @@ class GenericShareDriverTestCase(test.TestCase): self._driver._sync_mount_temp_and_perm_files.assert_called_once_with( self.server) self.assertEqual( - [mock.call(self.server, ['sudo umount', mount_path, - '&& sudo rmdir', mount_path]) + [mock.call(self.server, ['sudo', 'umount', mount_path, + '&&', 'sudo', 'rmdir', mount_path]) for i in moves.range(2)], self._driver._ssh_exec.mock_calls, ) @@ -1366,6 +1366,24 @@ class GenericShareDriverTestCase(test.TestCase): ) self.assertEqual(ssh_output, result) + def test__ssh_exec_check_list_comprehensions_still_work(self): + ssh_output = 'fake_ssh_output' + cmd = ['fake', 'command spaced'] + ssh = mock.Mock() + ssh_pool = mock.Mock() + ssh_pool.create = mock.Mock(side_effect=lambda: ssh) + ssh_pool.remove = mock.Mock() + self.mock_object(processutils, 'ssh_execute', + mock.Mock(return_value=ssh_output)) + self._driver.ssh_connections = { + self.server['instance_id']: (ssh_pool, ssh) + } + + self._driver._ssh_exec(self.server, cmd) + + processutils.ssh_execute.assert_called_once_with( + ssh, 'fake "command spaced"', check_exit_code=True) + def test_get_share_stats_refresh_false(self): self._driver._stats = {'fake_key': 'fake_value'} diff --git a/manila/tests/share/drivers/test_helpers.py b/manila/tests/share/drivers/test_helpers.py index 087312e6..e0a4b0f9 100644 --- a/manila/tests/share/drivers/test_helpers.py +++ b/manila/tests/share/drivers/test_helpers.py @@ -241,8 +241,8 @@ class NFSHelperTestCase(test.TestCase): self._helper._ssh_exec.assert_any_call( self.server, ['cat', const.NFS_EXPORTS_FILE, - '| grep', self.share_name, - '| sudo tee', fake_maintenance_path] + '|', 'grep', self.share_name, + '|', 'sudo', 'tee', fake_maintenance_path] ) self._helper._ssh_exec.assert_any_call( self.server, @@ -264,8 +264,8 @@ class NFSHelperTestCase(test.TestCase): self._helper._ssh_exec.assert_called_once_with( self.server, ['cat', fake_maintenance_path, - '| sudo tee -a', const.NFS_EXPORTS_FILE, - '&& sudo exportfs -r', '&& sudo rm -f', + '|', 'sudo', 'tee', '-a', const.NFS_EXPORTS_FILE, + '&&', 'sudo', 'exportfs', '-r', '&&', 'sudo', 'rm', '-f', fake_maintenance_path] ) @@ -448,8 +448,8 @@ class CIFSHelperIPAccessTestCase(test.TestCase): access_rules, [], []) self._helper._ssh_exec.assert_called_once_with( self.server_details, ['sudo', 'net', 'conf', 'setparm', - self.share_name, '"hosts allow"', - '"1.1.1.1"']) + self.share_name, 'hosts allow', + '1.1.1.1']) def test_get_allow_hosts(self): self.mock_object(self._helper, '_ssh_exec', @@ -460,7 +460,7 @@ class CIFSHelperIPAccessTestCase(test.TestCase): self.server_details, self.share_name) self.assertEqual(expected, result) cmd = ['sudo', 'net', 'conf', 'getparm', self.share_name, - '\"hosts allow\"'] + 'hosts allow'] self._helper._ssh_exec.assert_called_once_with( self.server_details, cmd) @@ -537,7 +537,8 @@ class CIFSHelperIPAccessTestCase(test.TestCase): self.server_details, self.share_name) self._helper._set_allow_hosts.assert_called_once_with( self.server_details, [], self.share_name) - valid_cmd = ['echo', "'test test2'", '| sudo tee', maintenance_path] + valid_cmd = ['echo', "'test test2'", '|', 'sudo', 'tee', + maintenance_path] self._helper._ssh_exec.assert_called_once_with( self.server_details, valid_cmd) @@ -557,7 +558,7 @@ class CIFSHelperIPAccessTestCase(test.TestCase): self._helper._ssh_exec.assert_any_call( self.server_details, ['cat', fake_maintenance_path]) self._helper._ssh_exec.assert_any_call( - self.server_details, ['sudo rm -f', fake_maintenance_path]) + self.server_details, ['sudo', 'rm', '-f', fake_maintenance_path]) @ddt.ddt @@ -601,10 +602,10 @@ class CIFSHelperUserAccessTestCase(test.TestCase): self._helper._ssh_exec.assert_has_calls([ mock.call(self.server_details, ['sudo', 'net', 'conf', 'setparm', self.share_name, - 'valid users', '"user1"']), + 'valid users', 'user1']), mock.call(self.server_details, ['sudo', 'net', 'conf', 'setparm', self.share_name, - 'read list', '"user2"']) + 'read list', 'user2']) ]) def test_update_access_exception_level(self):