From a53198feb550b1bc0976b310a9e24fcf2334dc1f Mon Sep 17 00:00:00 2001 From: John Griffith Date: Mon, 1 Dec 2014 14:01:26 -0700 Subject: [PATCH] Don't use _execute directly in brick/iscsi The brick/iscsi module has a run helper that should be used for executing commands. There are a number of inconsistencies where _execute is called directly. This patch moves everythign to use the run method to keep things consistent and also to fix up some potential issues with variables becoming corrupt under heavy load. Change-Id: Idfc183f2ed1ad73b64fc893efcc07972c95926c6 --- cinder/brick/iscsi/iscsi.py | 119 +++++++++++++++++++----------------- cinder/volume/iscsi.py | 2 + 2 files changed, 65 insertions(+), 56 deletions(-) diff --git a/cinder/brick/iscsi/iscsi.py b/cinder/brick/iscsi/iscsi.py index 14327baab..cecdf9920 100644 --- a/cinder/brick/iscsi/iscsi.py +++ b/cinder/brick/iscsi/iscsi.py @@ -47,10 +47,15 @@ class TargetAdmin(executor.Executor): def __init__(self, cmd, root_helper, execute): super(TargetAdmin, self).__init__(root_helper, execute=execute) + + # NOTE(jdg): cmd is a prefix to the target helper utility we + # use. This can be tgtadm, cinder-rtstool etc self._cmd = cmd - def _run(self, *args, **kwargs): - self._execute(self._cmd, *args, run_as_root=True, **kwargs) + def _run(self, cmd, *args, **kwargs): + return self._execute(cmd, + *args, + **kwargs) def _get_target_chap_auth(self, volume_id): """Get the current chap auth username and password.""" @@ -113,7 +118,7 @@ class TgtAdm(TargetAdmin): self.volumes_dir = volumes_dir def _get_target(self, iqn): - (out, _err) = self._execute('tgt-admin', '--show', run_as_root=True) + (out, _err) = self._run('tgt-admin', '--show', run_as_root=True) lines = out.split('\n') for line in lines: if iqn in line: @@ -128,7 +133,7 @@ class TgtAdm(TargetAdmin): capture = False target_info = [] - (out, _err) = self._execute('tgt-admin', '--show', run_as_root=True) + (out, _err) = self._run('tgt-admin', '--show', run_as_root=True) lines = out.split('\n') for line in lines: @@ -155,11 +160,11 @@ class TgtAdm(TargetAdmin): time.sleep(10) try: - (out, err) = self._execute('tgtadm', '--lld', 'iscsi', - '--op', 'new', '--mode', - 'logicalunit', '--tid', - tid, '--lun', '1', '-b', - path, run_as_root=True) + (out, err) = self._run('tgtadm', '--lld', 'iscsi', + '--op', 'new', '--mode', + 'logicalunit', '--tid', + tid, '--lun', '1', '-b', + path, run_as_root=True) LOG.debug('StdOut from recreate backing lun: %s' % out) LOG.debug('StdErr from recreate backing lun: %s' % err) except putils.ProcessExecutionError as e: @@ -177,6 +182,9 @@ class TgtAdm(TargetAdmin): with open(volume_path, 'r') as f: volume_conf = f.read() except Exception as e: + # NOTE(jdg): Debug is ok here because the caller + # will just generate the CHAP creds and create the + # file based on the None return LOG.debug('Failed to open config for %(vol_id)s: %(e)s' % {'vol_id': vol_id, 'e': six.text_type(e)}) return None @@ -225,22 +233,22 @@ class TgtAdm(TargetAdmin): # by creating the entry in the persist file # and then doing an update to get the target # created. - (out, err) = self._execute('tgt-admin', '--update', name, - run_as_root=True) + (out, err) = self._run('tgt-admin', '--update', name, + run_as_root=True) LOG.debug("StdOut from tgt-admin --update: %s", out) LOG.debug("StdErr from tgt-admin --update: %s", err) # Grab targets list for debug # Consider adding a check for lun 0 and 1 for tgtadm # before considering this as valid - (out, err) = self._execute('tgtadm', - '--lld', - 'iscsi', - '--op', - 'show', - '--mode', - 'target', - run_as_root=True) + (out, err) = self._run('tgtadm', + '--lld', + 'iscsi', + '--op', + 'show', + '--mode', + 'target', + run_as_root=True) LOG.debug("Targets after update: %s" % out) except putils.ProcessExecutionError as e: LOG.warning(_LW("Failed to create iscsi target for volume " @@ -299,12 +307,12 @@ class TgtAdm(TargetAdmin): try: # NOTE(vish): --force is a workaround for bug: # https://bugs.launchpad.net/cinder/+bug/1159948 - self._execute('tgt-admin', - '--force', - '--delete', - iqn, - run_as_root=True, - attempts=CONF.num_shell_tries) + self._run('tgt-admin', + '--force', + '--delete', + iqn, + run_as_root=True, + attempts=CONF.num_shell_tries) except putils.ProcessExecutionError as e: LOG.error(_LE("Failed to remove iscsi target for volume " "id:%(vol_id)s: %(e)s") @@ -325,10 +333,10 @@ class TgtAdm(TargetAdmin): try: LOG.warning(_LW('Silent failure of target removal ' 'detected, retry....')) - self._execute('tgt-admin', - '--delete', - iqn, - run_as_root=True) + self._run('tgt-admin', + '--delete', + iqn, + run_as_root=True) except putils.ProcessExecutionError as e: LOG.error(_LE("Failed to remove iscsi target for volume " "id:%(vol_id)s: %(e)s") @@ -437,36 +445,36 @@ class IetAdm(TargetAdmin): iet_conf_text.close() def _new_target(self, name, tid, **kwargs): - self._run('--op', 'new', + self._run(self._cmd, '--op', 'new', '--tid=%s' % tid, '--params', 'Name=%s' % name, **kwargs) def _delete_target(self, tid, **kwargs): - self._run('--op', 'delete', + self._run(self._cmd, '--op', 'delete', '--tid=%s' % tid, **kwargs) def show_target(self, tid, iqn=None, **kwargs): - self._run('--op', 'show', + self._run(self._cmd, '--op', 'show', '--tid=%s' % tid, **kwargs) def _new_logicalunit(self, tid, lun, path, **kwargs): - self._run('--op', 'new', + self._run(self._cmd, '--op', 'new', '--tid=%s' % tid, '--lun=%d' % lun, '--params', 'Path=%s,Type=%s' % (path, self._iotype(path)), **kwargs) def _delete_logicalunit(self, tid, lun, **kwargs): - self._run('--op', 'delete', + self._run(self._cmd, '--op', 'delete', '--tid=%s' % tid, '--lun=%d' % lun, **kwargs) def _new_auth(self, tid, type, username, password, **kwargs): - self._run('--op', 'new', + self._run(self._cmd, '--op', 'new', '--tid=%s' % tid, '--user', '--params=%s=%s,Password=%s' % (type, username, password), @@ -500,15 +508,15 @@ class LioAdm(TargetAdmin): def _verify_rtstool(self): try: - self._execute('cinder-rtstool', 'verify') + self._run('cinder-rtstool', 'verify') except (OSError, putils.ProcessExecutionError): LOG.error(_LE('cinder-rtstool is not installed correctly')) raise def _get_target(self, iqn): - (out, _err) = self._execute('cinder-rtstool', - 'get-targets', - run_as_root=True) + (out, _err) = self._run('cinder-rtstool', + 'get-targets', + run_as_root=True) lines = out.split('\n') for line in lines: if iqn in line: @@ -532,15 +540,14 @@ class LioAdm(TargetAdmin): extra_args.append(self.lio_initiator_iqns) try: - command_args = ['cinder-rtstool', - 'create', + command_args = ['create', path, name, chap_auth_userid, chap_auth_password] if extra_args: command_args.extend(extra_args) - self._execute(*command_args, run_as_root=True) + self._run('cinder-rtstool', *command_args, run_as_root=True) except putils.ProcessExecutionError as e: LOG.error(_LE("Failed to create iscsi target for volume " "id:%s.") % vol_id) @@ -563,10 +570,10 @@ class LioAdm(TargetAdmin): iqn = '%s%s' % (self.iscsi_target_prefix, vol_uuid_name) try: - self._execute('cinder-rtstool', - 'delete', - iqn, - run_as_root=True) + self._run('cinder-rtstool', + 'delete', + iqn, + run_as_root=True) except putils.ProcessExecutionError as e: LOG.error(_LE("Failed to remove iscsi target for volume " "id:%s.") % vol_id) @@ -590,12 +597,12 @@ class LioAdm(TargetAdmin): # Add initiator iqns to target ACL try: - self._execute('cinder-rtstool', 'add-initiator', - volume_iqn, - auth_user, - auth_pass, - connector['initiator'], - run_as_root=True) + self._run('cinder-rtstool', 'add-initiator', + volume_iqn, + auth_user, + auth_pass, + connector['initiator'], + run_as_root=True) except putils.ProcessExecutionError: LOG.error(_LE("Failed to add initiator iqn %s to target.") % connector['initiator']) @@ -606,10 +613,10 @@ class LioAdm(TargetAdmin): # Delete initiator iqns from target ACL try: - self._execute('cinder-rtstool', 'delete-initiator', - volume_iqn, - connector['initiator'], - run_as_root=True) + self._run('cinder-rtstool', 'delete-initiator', + volume_iqn, + connector['initiator'], + run_as_root=True) except putils.ProcessExecutionError: LOG.error(_LE("Failed to delete initiator iqn %s to target.") % connector['initiator']) diff --git a/cinder/volume/iscsi.py b/cinder/volume/iscsi.py index 143f5fe8c..7198b8324 100644 --- a/cinder/volume/iscsi.py +++ b/cinder/volume/iscsi.py @@ -42,6 +42,8 @@ class _ExportMixin(object): volume, max_targets) + # Verify we haven't setup a CHAP creds file already + # if DNE no big deal, we'll just create it current_chap_auth = self._get_target_chap_auth(iscsi_name) if current_chap_auth: (chap_username, chap_password) = current_chap_auth -- 2.45.2