From 80f47322982355ff6ec6eac87e09926432bf1f3e Mon Sep 17 00:00:00 2001 From: John Griffith Date: Wed, 7 Jan 2015 16:25:48 -0700 Subject: [PATCH] Use cinder.utils.execute directly When we were in the process of working on brick libs we did quite a bit of funky stuff with setting a member variable for each class to point to an executor that was passed in during init. There's no longer any reason to do this with the target drivers, so we should simplify our lives a bit and go back to using the good old cinder.utils wrapper. It's also believed that the use of the member variable is susceptible to some concurrency issue that was causing the wrong cmd string to be executed. We'll mark this as closing that bug and reopen if we still see the signature in Kibana. Change-Id: I7a5648d496d15eec5d7ac3643411198226f1ffdf Closes-Bug: #1398078 --- .../tests/targets/test_base_iscsi_driver.py | 4 ++-- cinder/tests/targets/test_tgt_driver.py | 24 +++++++++---------- cinder/tests/test_volume.py | 3 ++- cinder/volume/targets/driver.py | 9 ++++--- cinder/volume/targets/iscsi.py | 4 ++-- cinder/volume/targets/lio.py | 13 +++++----- cinder/volume/targets/tgt.py | 15 ++++++------ 7 files changed, 39 insertions(+), 33 deletions(-) diff --git a/cinder/tests/targets/test_base_iscsi_driver.py b/cinder/tests/targets/test_base_iscsi_driver.py index 2608e5be3..1414ed5be 100644 --- a/cinder/tests/targets/test_base_iscsi_driver.py +++ b/cinder/tests/targets/test_base_iscsi_driver.py @@ -116,8 +116,8 @@ class TestBaseISCSITargetDriver(test.TestCase): 'safe_get', _fake_safe_get) - self.stubs.Set(self.target, - '_execute', + self.stubs.Set(utils, + 'execute', _fake_execute) self.assertEqual(target_string, diff --git a/cinder/tests/targets/test_tgt_driver.py b/cinder/tests/targets/test_tgt_driver.py index 9682b4891..72bee11ab 100644 --- a/cinder/tests/targets/test_tgt_driver.py +++ b/cinder/tests/targets/test_tgt_driver.py @@ -115,8 +115,8 @@ class TestTgtAdmDriver(test.TestCase): def _fake_execute(*args, **kwargs): return self.fake_iscsi_scan, None - self.stubs.Set(self.target, - '_execute', + self.stubs.Set(utils, + 'execute', _fake_execute) self.assertEqual('1', @@ -129,8 +129,8 @@ class TestTgtAdmDriver(test.TestCase): def _fake_execute(*args, **kwargs): return self.fake_iscsi_scan, None - self.stubs.Set(self.target, - '_execute', + self.stubs.Set(utils, + 'execute', _fake_execute) self.assertTrue(self.target._verify_backing_lun( @@ -144,8 +144,8 @@ class TestTgtAdmDriver(test.TestCase): def _fake_execute_bad_lun(*args, **kwargs): return bad_scan, None - self.stubs.Set(self.target, - '_execute', + self.stubs.Set(utils, + 'execute', _fake_execute_bad_lun) self.assertFalse(self.target._verify_backing_lun( @@ -176,8 +176,8 @@ class TestTgtAdmDriver(test.TestCase): def _fake_execute(*args, **kwargs): return '', '' - self.stubs.Set(self.target, - '_execute', + self.stubs.Set(utils, + 'execute', _fake_execute) self.stubs.Set(self.target, @@ -206,8 +206,8 @@ class TestTgtAdmDriver(test.TestCase): stderr='target already exists', cmd='tgtad --lld iscsi --op show --mode target') - self.stubs.Set(self.target, - '_execute', + self.stubs.Set(utils, + 'execute', _fake_execute) self.stubs.Set(self.target, @@ -233,8 +233,8 @@ class TestTgtAdmDriver(test.TestCase): def _fake_execute(*args, **kwargs): return '', '' - self.stubs.Set(self.target, - '_execute', + self.stubs.Set(utils, + 'execute', _fake_execute) self.stubs.Set(self.target, diff --git a/cinder/tests/test_volume.py b/cinder/tests/test_volume.py index c5dc25aeb..54e99cde2 100644 --- a/cinder/tests/test_volume.py +++ b/cinder/tests/test_volume.py @@ -4027,7 +4027,8 @@ class ISCSITestCase(DriverTestCase): iscsi_driver = \ cinder.volume.targets.tgt.TgtAdm( configuration=self.configuration) - iscsi_driver._execute = lambda *a, **kw: \ + + utils.execute = lambda *a, **kw: \ ("%s dummy" % CONF.iscsi_ip_address, '') volume = {"name": "dummy", "host": "0.0.0.0", diff --git a/cinder/volume/targets/driver.py b/cinder/volume/targets/driver.py index bfa636d55..9fabfd8df 100644 --- a/cinder/volume/targets/driver.py +++ b/cinder/volume/targets/driver.py @@ -12,9 +12,11 @@ import abc -from oslo_concurrency import processutils as putils +from oslo.config import cfg import six +CONF = cfg.CONF + @six.add_metaclass(abc.ABCMeta) class Target(object): @@ -33,8 +35,9 @@ class Target(object): def __init__(self, *args, **kwargs): self.db = kwargs.get('db') self.configuration = kwargs.get('configuration') - self._execute = kwargs.get('executor', putils.execute) - self._root_helper = kwargs.get('root_helper') + self._root_helper = kwargs.get('root_helper', + 'sudo cinder-rootwrap %s' % + CONF.rootwrap_config) @abc.abstractmethod def ensure_export(self, context, volume, diff --git a/cinder/volume/targets/iscsi.py b/cinder/volume/targets/iscsi.py index 0ae0a4512..f8eae6ddd 100644 --- a/cinder/volume/targets/iscsi.py +++ b/cinder/volume/targets/iscsi.py @@ -15,6 +15,7 @@ from oslo.concurrency import processutils from cinder import exception from cinder.i18n import _, _LW, _LE from cinder.openstack.common import log as logging +from cinder import utils from cinder.volume.targets import driver LOG = logging.getLogger(__name__) @@ -135,10 +136,9 @@ class ISCSITarget(driver.Target): # NOTE(griff) We're doing the split straight away which should be # safe since using '@' in hostname is considered invalid - (out, _err) = self._execute('iscsiadm', '-m', 'discovery', + (out, _err) = utils.execute('iscsiadm', '-m', 'discovery', '-t', 'sendtargets', '-p', volume['host'].split('@')[0], - root_helper=self._root_helper, run_as_root=True) except processutils.ProcessExecutionError as ex: LOG.error(_LE("ISCSI discovery attempt failed for:%s") % diff --git a/cinder/volume/targets/lio.py b/cinder/volume/targets/lio.py index 3f38dacd2..4c66e2bcb 100644 --- a/cinder/volume/targets/lio.py +++ b/cinder/volume/targets/lio.py @@ -15,6 +15,7 @@ from oslo_concurrency import processutils as putils from cinder import exception from cinder.i18n import _, _LE, _LI, _LW from cinder.openstack.common import log as logging +from cinder import utils from cinder.volume.targets.tgt import TgtAdm LOG = logging.getLogger(__name__) @@ -71,13 +72,13 @@ class LioAdm(TgtAdm): def _verify_rtstool(self): try: - self._execute('cinder-rtstool', 'verify') + utils.execute('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', + (out, err) = utils.execute('cinder-rtstool', 'get-targets', run_as_root=True) lines = out.split('\n') @@ -105,7 +106,7 @@ class LioAdm(TgtAdm): name, chap_auth_userid, chap_auth_password] - self._execute(*command_args, run_as_root=True) + utils.execute(*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) @@ -128,7 +129,7 @@ class LioAdm(TgtAdm): iqn = '%s%s' % (self.iscsi_target_prefix, vol_uuid_name) try: - self._execute('cinder-rtstool', + utils.execute('cinder-rtstool', 'delete', iqn, run_as_root=True) @@ -155,7 +156,7 @@ class LioAdm(TgtAdm): # Add initiator iqns to target ACL try: - self._execute('cinder-rtstool', 'add-initiator', + utils.execute('cinder-rtstool', 'add-initiator', volume_iqn, auth_user, auth_pass, @@ -183,7 +184,7 @@ class LioAdm(TgtAdm): # Delete initiator iqns from target ACL try: - self._execute('cinder-rtstool', 'delete-initiator', + utils.execute('cinder-rtstool', 'delete-initiator', volume_iqn, connector['initiator'], run_as_root=True) diff --git a/cinder/volume/targets/tgt.py b/cinder/volume/targets/tgt.py index f5cf3b846..02ff1d385 100644 --- a/cinder/volume/targets/tgt.py +++ b/cinder/volume/targets/tgt.py @@ -22,6 +22,7 @@ from cinder import exception from cinder.openstack.common import fileutils from cinder.i18n import _, _LI, _LW, _LE from cinder.openstack.common import log as logging +from cinder import utils from cinder.volume.targets import iscsi from cinder.volume import utils as vutils @@ -58,7 +59,7 @@ class TgtAdm(iscsi.ISCSITarget): self.volumes_dir = self.configuration.safe_get('volumes_dir') def _get_target(self, iqn): - (out, err) = self._execute('tgt-admin', '--show', run_as_root=True) + (out, err) = utils.execute('tgt-admin', '--show', run_as_root=True) lines = out.split('\n') for line in lines: if iqn in line: @@ -73,7 +74,7 @@ class TgtAdm(iscsi.ISCSITarget): capture = False target_info = [] - (out, err) = self._execute('tgt-admin', '--show', run_as_root=True) + (out, err) = utils.execute('tgt-admin', '--show', run_as_root=True) lines = out.split('\n') for line in lines: @@ -99,7 +100,7 @@ class TgtAdm(iscsi.ISCSITarget): # and error on the side of caution time.sleep(10) try: - (out, err) = self._execute('tgtadm', '--lld', 'iscsi', + (out, err) = utils.execute('tgtadm', '--lld', 'iscsi', '--op', 'new', '--mode', 'logicalunit', '--tid', tid, '--lun', '1', '-b', @@ -214,7 +215,7 @@ class TgtAdm(iscsi.ISCSITarget): # 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, + (out, err) = utils.execute('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) @@ -222,7 +223,7 @@ class TgtAdm(iscsi.ISCSITarget): # 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', + (out, err) = utils.execute('tgtadm', '--lld', 'iscsi', '--op', @@ -357,7 +358,7 @@ class TgtAdm(iscsi.ISCSITarget): try: # NOTE(vish): --force is a workaround for bug: # https://bugs.launchpad.net/cinder/+bug/1159948 - self._execute('tgt-admin', + utils.execute('tgt-admin', '--force', '--delete', iqn, @@ -381,7 +382,7 @@ class TgtAdm(iscsi.ISCSITarget): try: LOG.warning(_LW('Silent failure of target removal ' 'detected, retry....')) - self._execute('tgt-admin', + utils.execute('tgt-admin', '--delete', iqn, run_as_root=True) -- 2.45.2