openstack/deb-manila
Code Issues Proposed changes

Bring remote and local executors into accord

Browse Source 
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
This commit is contained in:
Alexey Ovchinnikov 2016-09-19 13:40:44 +03:00
parent 16c522bf5e
commit 49856dc9c8
4 changed files with 63 additions and 37 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

16
manila/share/drivers/generic.py
View File

@ -160,6 +160,11 @@ class GenericShareDriver(driver.ExecuteMixin, driver.ShareDriver):
ssh_pool.remove(ssh) ssh_pool.remove(ssh)
ssh = ssh_pool.create() ssh = ssh_pool.create()
self.ssh_connections[server['instance_id']] = (ssh_pool, ssh) 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), return processutils.ssh_execute(ssh, ' '.join(command),
check_exit_code=check_exit_code) check_exit_code=check_exit_code)
@ -335,10 +340,11 @@ class GenericShareDriver(driver.ExecuteMixin, driver.ShareDriver):
volume): volume):
LOG.debug("Mounting '%(dev)s' to path '%(path)s' on " LOG.debug("Mounting '%(dev)s' to path '%(path)s' on "
"server '%(server)s'.", log_data) "server '%(server)s'.", log_data)
mount_cmd = ['sudo mkdir -p', mount_path, '&&'] mount_cmd = ['sudo', 'mkdir', '-p', mount_path, '&&']
mount_cmd.extend(['sudo mount', volume['mountpoint'], mount_cmd.extend(['sudo', 'mount', volume['mountpoint'],
mount_path])
mount_cmd.extend(['&&', 'sudo', 'chmod', '777',
mount_path]) mount_path])
mount_cmd.extend(['&& sudo chmod 777', mount_path])
self._ssh_exec(server_details, mount_cmd) self._ssh_exec(server_details, mount_cmd)
# Add mount permanently # Add mount permanently
@ -365,8 +371,8 @@ class GenericShareDriver(driver.ExecuteMixin, driver.ShareDriver):
if self._is_device_mounted(mount_path, server_details): if self._is_device_mounted(mount_path, server_details):
LOG.debug("Unmounting path '%(path)s' on server " LOG.debug("Unmounting path '%(path)s' on server "
"'%(server)s'.", log_data) "'%(server)s'.", log_data)
unmount_cmd = ['sudo umount', mount_path, '&& sudo rmdir', unmount_cmd = ['sudo', 'umount', mount_path, '&&', 'sudo',
mount_path] 'rmdir', mount_path]
self._ssh_exec(server_details, unmount_cmd) self._ssh_exec(server_details, unmount_cmd)
# Remove mount permanently # Remove mount permanently
self._sync_mount_temp_and_perm_files(server_details) self._sync_mount_temp_and_perm_files(server_details)

31
manila/share/drivers/helpers.py
View File

@ -289,8 +289,8 @@ class NFSHelper(NASHelperBase):
maintenance_file = self._get_maintenance_file_path(share_name) maintenance_file = self._get_maintenance_file_path(share_name)
backup_exports = [ backup_exports = [
'cat', const.NFS_EXPORTS_FILE, 'cat', const.NFS_EXPORTS_FILE,
'| grep', share_name, '|', 'grep', share_name,
'| sudo tee', maintenance_file '|', 'sudo', 'tee', maintenance_file
] ]
self._ssh_exec(server, backup_exports) self._ssh_exec(server, backup_exports)
@ -304,9 +304,9 @@ class NFSHelper(NASHelperBase):
maintenance_file = self._get_maintenance_file_path(share_name) maintenance_file = self._get_maintenance_file_path(share_name)
restore_exports = [ restore_exports = [
'cat', maintenance_file, 'cat', maintenance_file,
'| sudo tee -a', const.NFS_EXPORTS_FILE, '|', 'sudo', 'tee', '-a', const.NFS_EXPORTS_FILE,
'&& sudo exportfs -r', '&&', 'sudo', 'exportfs', '-r',
'&& sudo rm -f', maintenance_file '&&', 'sudo', 'rm', '-f', maintenance_file
] ]
self._ssh_exec(server, restore_exports) self._ssh_exec(server, restore_exports)
@ -324,10 +324,10 @@ class CIFSHelperIPAccess(NASHelperBase):
self.export_format = '\\\\%s\\%s' self.export_format = '\\\\%s\\%s'
self.parameters = { self.parameters = {
'browseable': 'yes', 'browseable': 'yes',
'\"create mask\"': '0755', 'create mask': '0755',
'\"hosts deny\"': '0.0.0.0/0', # deny all by default 'hosts deny': '0.0.0.0/0', # deny all by default
'\"hosts allow\"': '127.0.0.1', 'hosts allow': '127.0.0.1',
'\"read only\"': 'no', 'read only': 'no',
} }
def init_helper(self, server): def init_helper(self, server):
@ -406,13 +406,13 @@ class CIFSHelperIPAccess(NASHelperBase):
def _get_allow_hosts(self, server, share_name): def _get_allow_hosts(self, server, share_name):
(out, _) = self._ssh_exec(server, ['sudo', 'net', 'conf', 'getparm', (out, _) = self._ssh_exec(server, ['sudo', 'net', 'conf', 'getparm',
share_name, '\"hosts allow\"']) share_name, 'hosts allow'])
return out.split() return out.split()
def _set_allow_hosts(self, server, hosts, share_name): 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, self._ssh_exec(server, ['sudo', 'net', 'conf', 'setparm', share_name,
'\"hosts allow\"', value]) 'hosts allow', value])
@staticmethod @staticmethod
def _get_share_group_name_from_export_location(export_location): 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)) allowed_hosts = " ".join(self._get_allow_hosts(server, share_name))
backup_exports = [ 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._ssh_exec(server, backup_exports)
self._set_allow_hosts(server, [], share_name) self._set_allow_hosts(server, [], share_name)
@ -459,7 +460,7 @@ class CIFSHelperIPAccess(NASHelperBase):
maintenance_file = self._get_maintenance_file_path(share_name) maintenance_file = self._get_maintenance_file_path(share_name)
(exports, __) = self._ssh_exec(server, ['cat', maintenance_file]) (exports, __) = self._ssh_exec(server, ['cat', maintenance_file])
self._set_allow_hosts(server, exports.split(), share_name) 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): class CIFSHelperUserAccess(CIFSHelperIPAccess):
@ -512,7 +513,7 @@ class CIFSHelperUserAccess(CIFSHelperIPAccess):
return 'read list' return 'read list'
def _set_valid_users(self, server, users, share_name, access_level): def _set_valid_users(self, server, users, share_name, access_level):
value = "\"" + ' '.join(users) + "\"" value = ' '.join(users)
param = self._get_conf_param(access_level) param = self._get_conf_param(access_level)
self._ssh_exec(server, ['sudo', 'net', 'conf', 'setparm', share_name, self._ssh_exec(server, ['sudo', 'net', 'conf', 'setparm', share_name,
param, value]) param, value])

30
manila/tests/share/drivers/test_generic.py
View File

@ -397,9 +397,9 @@ class GenericShareDriverTestCase(test.TestCase):
server) server)
self._driver._ssh_exec.assert_called_once_with( self._driver._ssh_exec.assert_called_once_with(
server, server,
['sudo mkdir -p', mount_path, ['sudo', 'mkdir', '-p', mount_path,
'&&', 'sudo mount', volume['mountpoint'], mount_path, '&&', 'sudo', 'mount', volume['mountpoint'], mount_path,
'&& sudo chmod 777', mount_path], '&&', 'sudo', 'chmod', '777', mount_path],
) )
def test_mount_device_present(self): def test_mount_device_present(self):
@ -454,7 +454,7 @@ class GenericShareDriverTestCase(test.TestCase):
self.server) self.server)
self._driver._ssh_exec.assert_called_once_with( self._driver._ssh_exec.assert_called_once_with(
self.server, 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): 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._driver._sync_mount_temp_and_perm_files.assert_called_once_with(
self.server) self.server)
self.assertEqual( self.assertEqual(
[mock.call(self.server, ['sudo umount', mount_path, [mock.call(self.server, ['sudo', 'umount', mount_path,
'&& sudo rmdir', mount_path]) '&&', 'sudo', 'rmdir', mount_path])
for i in moves.range(2)], for i in moves.range(2)],
self._driver._ssh_exec.mock_calls, self._driver._ssh_exec.mock_calls,
) )
@ -1366,6 +1366,24 @@ class GenericShareDriverTestCase(test.TestCase):
) )
self.assertEqual(ssh_output, result) 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): def test_get_share_stats_refresh_false(self):
self._driver._stats = {'fake_key': 'fake_value'} self._driver._stats = {'fake_key': 'fake_value'}

23
manila/tests/share/drivers/test_helpers.py
View File

@ -241,8 +241,8 @@ class NFSHelperTestCase(test.TestCase):
self._helper._ssh_exec.assert_any_call( self._helper._ssh_exec.assert_any_call(
self.server, self.server,
['cat', const.NFS_EXPORTS_FILE, ['cat', const.NFS_EXPORTS_FILE,
'| grep', self.share_name, '|', 'grep', self.share_name,
'| sudo tee', fake_maintenance_path] '|', 'sudo', 'tee', fake_maintenance_path]
) )
self._helper._ssh_exec.assert_any_call( self._helper._ssh_exec.assert_any_call(
self.server, self.server,
@ -264,8 +264,8 @@ class NFSHelperTestCase(test.TestCase):
self._helper._ssh_exec.assert_called_once_with( self._helper._ssh_exec.assert_called_once_with(
self.server, self.server,
['cat', fake_maintenance_path, ['cat', fake_maintenance_path,
'| sudo tee -a', const.NFS_EXPORTS_FILE, '|', 'sudo', 'tee', '-a', const.NFS_EXPORTS_FILE,
'&& sudo exportfs -r', '&& sudo rm -f', '&&', 'sudo', 'exportfs', '-r', '&&', 'sudo', 'rm', '-f',
fake_maintenance_path] fake_maintenance_path]
) )
@ -448,8 +448,8 @@ class CIFSHelperIPAccessTestCase(test.TestCase):
access_rules, [], []) access_rules, [], [])
self._helper._ssh_exec.assert_called_once_with( self._helper._ssh_exec.assert_called_once_with(
self.server_details, ['sudo', 'net', 'conf', 'setparm', self.server_details, ['sudo', 'net', 'conf', 'setparm',
self.share_name, '"hosts allow"', self.share_name, 'hosts allow',
'"1.1.1.1"']) '1.1.1.1'])
def test_get_allow_hosts(self): def test_get_allow_hosts(self):
self.mock_object(self._helper, '_ssh_exec', self.mock_object(self._helper, '_ssh_exec',
@ -460,7 +460,7 @@ class CIFSHelperIPAccessTestCase(test.TestCase):
self.server_details, self.share_name) self.server_details, self.share_name)
self.assertEqual(expected, result) self.assertEqual(expected, result)
cmd = ['sudo', 'net', 'conf', 'getparm', self.share_name, cmd = ['sudo', 'net', 'conf', 'getparm', self.share_name,
'\"hosts allow\"'] 'hosts allow']
self._helper._ssh_exec.assert_called_once_with( self._helper._ssh_exec.assert_called_once_with(
self.server_details, cmd) self.server_details, cmd)
@ -537,7 +537,8 @@ class CIFSHelperIPAccessTestCase(test.TestCase):
self.server_details, self.share_name) self.server_details, self.share_name)
self._helper._set_allow_hosts.assert_called_once_with( self._helper._set_allow_hosts.assert_called_once_with(
self.server_details, [], self.share_name) 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._helper._ssh_exec.assert_called_once_with(
self.server_details, valid_cmd) self.server_details, valid_cmd)
@ -557,7 +558,7 @@ class CIFSHelperIPAccessTestCase(test.TestCase):
self._helper._ssh_exec.assert_any_call( self._helper._ssh_exec.assert_any_call(
self.server_details, ['cat', fake_maintenance_path]) self.server_details, ['cat', fake_maintenance_path])
self._helper._ssh_exec.assert_any_call( 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 @ddt.ddt
@ -601,10 +602,10 @@ class CIFSHelperUserAccessTestCase(test.TestCase):
self._helper._ssh_exec.assert_has_calls([ self._helper._ssh_exec.assert_has_calls([
mock.call(self.server_details, mock.call(self.server_details,
['sudo', 'net', 'conf', 'setparm', self.share_name, ['sudo', 'net', 'conf', 'setparm', self.share_name,
'valid users', '"user1"']), 'valid users', 'user1']),
mock.call(self.server_details, mock.call(self.server_details,
['sudo', 'net', 'conf', 'setparm', self.share_name, ['sudo', 'net', 'conf', 'setparm', self.share_name,
'read list', '"user2"']) 'read list', 'user2'])
]) ])
def test_update_access_exception_level(self): def test_update_access_exception_level(self):