From: John Griffith Date: Wed, 11 Jun 2014 22:43:41 +0000 (+0000) Subject: Remove global conf settings from iscsi helper X-Git-Url: https://review.fuel-infra.org/gitweb?a=commitdiff_plain;h=1b21d499bb3bb1e94e2815cbada86db8e8e21406;p=openstack-build%2Fcinder-build.git Remove global conf settings from iscsi helper This "intermediate" iscsi helper pulls all of it's config settings from the global config. This is fine if you only have a single backend, but if you do multi-backend it puts things in a bad state where some of the backend specific settings are picked up but others are not (for example iscsi_ip_address). This change modifies methods like create_export in the volume/iscsi helper to take the drivers version of the config settings as a parameter and use those instead of setting off of the global values. Long term there's a lot of cleanup surrounding our inheritance model and especially the iscsi helpers. We can address that going forward but here we just want to fix the bug in the safest way possible. Change-Id: If17ec3ffb3f4ea7f95da65781885dcd613b1a807 Closes-Bug: 1325799 --- diff --git a/cinder/volume/drivers/block_device.py b/cinder/volume/drivers/block_device.py index e2ce8ef2d..15da879e7 100644 --- a/cinder/volume/drivers/block_device.py +++ b/cinder/volume/drivers/block_device.py @@ -77,7 +77,10 @@ class BlockDeviceDriver(driver.ISCSIDriver): def create_export(self, context, volume): """Creates an export for a logical volume.""" volume_path = self.local_path(volume) - data = self.target_helper.create_export(context, volume, volume_path) + data = self.target_helper.create_export(context, + volume, + volume_path, + self.configuration) return { 'provider_location': data['location'] + ' ' + volume_path, 'provider_auth': data['auth'], diff --git a/cinder/volume/drivers/lvm.py b/cinder/volume/drivers/lvm.py index c4ea524a9..7e5018bd6 100644 --- a/cinder/volume/drivers/lvm.py +++ b/cinder/volume/drivers/lvm.py @@ -206,8 +206,8 @@ class LVMVolumeDriver(driver.VolumeDriver): # ThinLVM snapshot LVs. self.vg.activate_lv(snapshot['name'], is_snapshot=True) - # copy_volume expects sizes in MiB, we store integer GiB - # be sure to convert before passing in + # copy_volume expects sizes in MiB, we store integer GiB + # be sure to convert before passing in volutils.copy_volume(self.local_path(snapshot), self.local_path(volume), snapshot['volume_size'] * units.KiB, @@ -298,8 +298,8 @@ class LVMVolumeDriver(driver.VolumeDriver): self.vg.activate_lv(temp_snapshot['name'], is_snapshot=True) - # copy_volume expects sizes in MiB, we store integer GiB - # be sure to convert before passing in + # copy_volume expects sizes in MiB, we store integer GiB + # be sure to convert before passing in try: volutils.copy_volume( self.local_path(temp_snapshot), @@ -488,6 +488,7 @@ class LVMISCSIDriver(LVMVolumeDriver, driver.ISCSIDriver): try: # NOTE(jdg): For TgtAdm case iscsi_name is all we need # should clean this all up at some point in the future + tid = self.target_helper.create_iscsi_target( iscsi_name, iscsi_target, @@ -514,9 +515,11 @@ class LVMISCSIDriver(LVMVolumeDriver, driver.ISCSIDriver): volume_name) # NOTE(jdg): For TgtAdm case iscsi_name is the ONLY param we need # should clean this all up at some point in the future - model_update = self.target_helper.ensure_export(context, volume, - iscsi_name, - volume_path) + model_update = self.target_helper.ensure_export( + context, volume, + iscsi_name, + volume_path, + self.configuration.volume_group) if model_update: self.db.volume_update(context, volume['id'], model_update) @@ -530,7 +533,10 @@ class LVMISCSIDriver(LVMVolumeDriver, driver.ISCSIDriver): volume_path = "/dev/%s/%s" % (vg, volume['name']) - data = self.target_helper.create_export(context, volume, volume_path) + data = self.target_helper.create_export(context, + volume, + volume_path, + self.configuration) return { 'provider_location': data['location'], 'provider_auth': data['auth'], diff --git a/cinder/volume/iscsi.py b/cinder/volume/iscsi.py index 722a9c4e0..22649324b 100644 --- a/cinder/volume/iscsi.py +++ b/cinder/volume/iscsi.py @@ -16,8 +16,6 @@ import os import re -from oslo.config import cfg - from cinder.brick.iscsi import iscsi from cinder import exception from cinder.openstack.common.gettextutils import _ @@ -26,7 +24,6 @@ from cinder.openstack.common import processutils as putils from cinder import utils LOG = logging.getLogger(__name__) -CONF = cfg.CONF class _ExportMixin(object): @@ -35,14 +32,19 @@ class _ExportMixin(object): self.db = kwargs.pop('db', None) super(_ExportMixin, self).__init__(*args, **kwargs) - def create_export(self, context, volume, volume_path): + def create_export(self, context, volume, volume_path, conf): """Creates an export for a logical volume.""" - iscsi_name = "%s%s" % (CONF.iscsi_target_prefix, + iscsi_name = "%s%s" % (conf.iscsi_target_prefix, volume['name']) - iscsi_target, lun = self._get_target_and_lun(context, volume) + max_targets = conf.safe_get('iscsi_num_targets') + (iscsi_target, lun) = self._get_target_and_lun(context, + volume, + max_targets) + chap_username = utils.generate_username() chap_password = utils.generate_password() - chap_auth = self._iscsi_authentication('IncomingUser', chap_username, + chap_auth = self._iscsi_authentication('IncomingUser', + chap_username, chap_password) # NOTE(jdg): For TgtAdm case iscsi_name is the ONLY param we need # should clean this all up at some point in the future @@ -53,7 +55,7 @@ class _ExportMixin(object): chap_auth) data = {} data['location'] = self._iscsi_location( - CONF.iscsi_ip_address, tid, iscsi_name, lun) + conf.iscsi_ip_address, tid, iscsi_name, conf.iscsi_port, lun) data['auth'] = self._iscsi_authentication( 'CHAP', chap_username, chap_password) return data @@ -84,7 +86,7 @@ class _ExportMixin(object): self.remove_iscsi_target(iscsi_target, 0, volume['id'], volume['name']) def ensure_export(self, context, volume, iscsi_name, volume_path, - old_name=None): + vg_name, old_name=None): iscsi_target = self._get_target_for_ensure_export(context, volume['id']) if iscsi_target is None: @@ -106,18 +108,18 @@ class _ExportMixin(object): chap_auth, check_exit_code=False, old_name=old_name) - def _ensure_iscsi_targets(self, context, host): + def _ensure_iscsi_targets(self, context, host, max_targets): """Ensure that target ids have been created in datastore.""" # NOTE(jdg): tgtadm doesn't use the iscsi_targets table # TODO(jdg): In the future move all of the dependent stuff into the # cooresponding target admin class host_iscsi_targets = self.db.iscsi_target_count_by_host(context, host) - if host_iscsi_targets >= CONF.iscsi_num_targets: + if host_iscsi_targets >= max_targets: return # NOTE(vish): Target ids start at 1, not 0. - target_end = CONF.iscsi_num_targets + 1 + target_end = max_targets + 1 for target_num in xrange(1, target_end): target = {'host': host, 'target_num': target_num} self.db.iscsi_target_create_safe(context, target) @@ -130,9 +132,9 @@ class _ExportMixin(object): except exception.NotFound: return None - def _get_target_and_lun(self, context, volume): + def _get_target_and_lun(self, context, volume, max_targets): lun = 0 - self._ensure_iscsi_targets(context, volume['host']) + self._ensure_iscsi_targets(context, volume['host'], max_targets) iscsi_target = self.db.volume_allocate_iscsi_target(context, volume['id'], volume['host']) @@ -144,11 +146,11 @@ class _ExportMixin(object): def _iscsi_authentication(self, chap, name, password): return "%s %s %s" % (chap, name, password) - def _iscsi_location(self, ip, target, iqn, lun=None): - return "%s:%s,%s %s %s" % (ip, CONF.iscsi_port, + def _iscsi_location(self, ip, target, iqn, port, lun=None): + return "%s:%s,%s %s %s" % (ip, port, target, iqn, lun) - def _fix_id_migration(self, context, volume): + def _fix_id_migration(self, context, volume, vg_name): """Fix provider_location and dev files to address bug 1065702. For volumes that the provider_location has NOT been updated @@ -174,12 +176,13 @@ class _ExportMixin(object): self.db.volume_update(context, volume['id'], model_update) start = os.getcwd() - os.chdir('/dev/%s' % CONF.volume_group) + + os.chdir('/dev/%s' % vg_name) try: (out, err) = self._execute('readlink', old_name) except putils.ProcessExecutionError: - link_path = '/dev/%s/%s' % (CONF.volume_group, + link_path = '/dev/%s/%s' % (vg_name, old_name) LOG.debug('Symbolic link %s not found' % link_path) os.chdir(start) @@ -196,7 +199,7 @@ class _ExportMixin(object): class TgtAdm(_ExportMixin, iscsi.TgtAdm): - def _get_target_and_lun(self, context, volume): + def _get_target_and_lun(self, context, volume, max_targets): lun = 1 # For tgtadm the controller is lun 0, dev starts at lun 1 iscsi_target = 0 # NOTE(jdg): Not used by tgtadm return iscsi_target, lun @@ -210,7 +213,7 @@ class TgtAdm(_ExportMixin, iscsi.TgtAdm): class FakeIscsiHelper(_ExportMixin, iscsi.FakeIscsiHelper): - def create_export(self, context, volume, volume_path): + def create_export(self, context, volume, volume_path, conf): return { 'location': "fake_location", 'auth': "fake_auth" @@ -219,8 +222,8 @@ class FakeIscsiHelper(_ExportMixin, iscsi.FakeIscsiHelper): def remove_export(self, context, volume): pass - def ensure_export(self, context, volume_id, iscsi_name, volume_path, - old_name=None): + def ensure_export(self, context, volume, iscsi_name, volume_path, + vg_name, old_name=None): pass @@ -237,10 +240,10 @@ class LioAdm(_ExportMixin, iscsi.LioAdm): self.remove_iscsi_target(iscsi_target, 0, volume['id'], volume['name']) - def ensure_export(self, context, volume_id, iscsi_name, volume_path, - old_name=None): + def ensure_export(self, context, volume, iscsi_name, volume_path, + vg_name, old_name=None): try: - volume_info = self.db.volume_get(context, volume_id) + volume_info = self.db.volume_get(context, volume['id']) (auth_method, auth_user, auth_pass) = volume_info['provider_auth'].split(' ', 3) @@ -250,7 +253,7 @@ class LioAdm(_ExportMixin, iscsi.LioAdm): except exception.NotFound: LOG.debug("volume_info:%s", volume_info) LOG.info(_("Skipping ensure_export. No iscsi_target " - "provision for volume: %s"), volume_id) + "provision for volume: %s"), volume['id']) iscsi_target = 1