From beecd769af02ba5915be827d28a7b46d970e41b0 Mon Sep 17 00:00:00 2001 From: John Griffith Date: Fri, 31 Jan 2014 03:53:37 +0000 Subject: [PATCH] Remove create_export from volume create Currently the LVM driver creates iscsi targets on volume creation, even though it's only used at attach time. This isn't necessary and in fact causes issues if a volume is extended because the target information isn't currently updated after the extend. In addition a number of other drivers inclduing the LIO iscsi driver require the target be created with initiator info, so the tgt that's created initially again isn't valid. This change removes the create_export call from the volume create process and makes it part of the managers intialize_connection routine which is more appropriate. This change also removes the target on detach since it's not used any longer and again as the volume may be modified or extended. Change-Id: I0b7dfdaf7e49a069da22f22f459861b0b49430a4 Closes-Bug: 1248947 --- cinder/brick/iscsi/iscsi.py | 10 ---- cinder/exception.py | 4 ++ cinder/tests/test_volume.py | 22 +++++--- cinder/volume/driver.py | 2 + cinder/volume/drivers/lvm.py | 1 + cinder/volume/flows/manager/create_volume.py | 22 -------- cinder/volume/manager.py | 57 ++++++++++++++++---- cinder/volume/rpcapi.py | 9 ++++ 8 files changed, 79 insertions(+), 48 deletions(-) diff --git a/cinder/brick/iscsi/iscsi.py b/cinder/brick/iscsi/iscsi.py index cb042ba832a..236e4d0a8fd 100644 --- a/cinder/brick/iscsi/iscsi.py +++ b/cinder/brick/iscsi/iscsi.py @@ -53,10 +53,6 @@ class TargetAdmin(executor.Executor): """Create a iSCSI target and logical unit.""" raise NotImplementedError() - def update_iscsi_target(self, name): - """Update an iSCSI target.""" - raise NotImplementedError() - def remove_iscsi_target(self, tid, lun, vol_id, vol_name, **kwargs): """Remove a iSCSI target and logical unit.""" raise NotImplementedError() @@ -368,9 +364,6 @@ class IetAdm(TargetAdmin): raise exception.ISCSITargetCreateFailed(volume_id=vol_id) return tid - def update_iscsi_target(self, name): - pass - def remove_iscsi_target(self, tid, lun, vol_id, vol_name, **kwargs): LOG.info(_('Removing iscsi_target for volume: %s') % vol_id) self._delete_logicalunit(tid, lun, **kwargs) @@ -525,9 +518,6 @@ class LioAdm(TargetAdmin): return tid - def update_iscsi_target(self, name): - pass - def remove_iscsi_target(self, tid, lun, vol_id, vol_name, **kwargs): LOG.info(_('Removing iscsi_target: %s') % vol_id) vol_uuid_name = vol_name diff --git a/cinder/exception.py b/cinder/exception.py index dfbb0524698..6e7c4f10afd 100644 --- a/cinder/exception.py +++ b/cinder/exception.py @@ -660,3 +660,7 @@ class GlusterfsNoSharesMounted(VolumeDriverException): class GlusterfsNoSuitableShareFound(VolumeDriverException): message = _("There is no share which can host %(volume_size)sG") + + +class RemoveExportException(VolumeDriverException): + message = _("Failed to remove export for volume %(volume)s: %(reason)s") diff --git a/cinder/tests/test_volume.py b/cinder/tests/test_volume.py index 476a9f3ff62..9f7aa760ac3 100644 --- a/cinder/tests/test_volume.py +++ b/cinder/tests/test_volume.py @@ -931,15 +931,25 @@ class VolumeTestCase(BaseVolumeTestCase): image_id='fake_id', source_volume='fake_id') - @mock.patch.object(db, 'volume_get') @mock.patch.object(db, 'volume_admin_metadata_get') + @mock.patch.object(db, 'volume_get') + @mock.patch.object(db, 'volume_update') def test_initialize_connection_fetchqos(self, - _mock_volume_admin_metadata_get, - _mock_volume_get): + _mock_volume_update, + _mock_volume_get, + _mock_volume_admin_metadata_get): """Make sure initialize_connection returns correct information.""" - _mock_volume_get.return_value = {'volume_type_id': 'fake_type_id', - 'volume_admin_metadata': {}} - _mock_volume_admin_metadata_get.return_value = {} + _fake_admin_meta = {'fake-key': 'fake-value'} + _fake_volume = {'volume_type_id': 'fake_type_id', + 'name': 'fake_name', + 'host': 'fake_host', + 'id': 'fake_volume_id', + 'volume_admin_metadata': _fake_admin_meta} + + _mock_volume_get.return_value = _fake_volume + _mock_volume_update.return_value = _fake_volume + _mock_volume_admin_metadata_get.return_value = _fake_admin_meta + connector = {'ip': 'IP', 'initiator': 'INITIATOR'} qos_values = {'consumer': 'front-end', 'specs': { diff --git a/cinder/volume/driver.py b/cinder/volume/driver.py index 50987551228..3bcc59b5829 100644 --- a/cinder/volume/driver.py +++ b/cinder/volume/driver.py @@ -373,8 +373,10 @@ class VolumeDriver(object): """Attach the volume.""" if remote: rpcapi = volume_rpcapi.VolumeAPI() + rpcapi.create_export(context, volume) conn = rpcapi.initialize_connection(context, volume, properties) else: + self.create_export(context, volume) conn = self.initialize_connection(volume, properties) # Use Brick's code to do attach/detach diff --git a/cinder/volume/drivers/lvm.py b/cinder/volume/drivers/lvm.py index c1c2b4edece..fb13a588db5 100644 --- a/cinder/volume/drivers/lvm.py +++ b/cinder/volume/drivers/lvm.py @@ -669,6 +669,7 @@ class LVMISCSIDriver(LVMVolumeDriver, driver.ISCSIDriver): LOG.info(_("Skipping remove_export. No iscsi_target " "provisioned for volume: %s"), volume['id']) return + else: iscsi_target = 0 diff --git a/cinder/volume/flows/manager/create_volume.py b/cinder/volume/flows/manager/create_volume.py index 080c16fbb37..30b7f2e1029 100644 --- a/cinder/volume/flows/manager/create_volume.py +++ b/cinder/volume/flows/manager/create_volume.py @@ -613,28 +613,6 @@ class CreateVolumeFromSpecTask(flow_utils.CinderTask): {'volume_id': volume_id, 'model': model_update}) raise exception.ExportFailure(reason=ex) - # Persist any driver exported model information. - model_update = None - try: - LOG.debug(_("Volume %s: creating export"), volume_ref['id']) - model_update = self.driver.create_export(context, volume_ref) - if model_update: - self.db.volume_update(context, volume_ref['id'], model_update) - except exception.CinderException as ex: - # If somehow the read *or* create export failed we want to ensure - # that the failure is logged (but not try rescheduling since - # the volume at this point has been created). - # - # NOTE(harlowja): Notice that since the model_update is initially - # empty, the only way it will still be empty is if there is no - # model_update (which we don't care about) or there was an - # model_update and updating failed. - if model_update: - LOG.exception(_("Failed updating model of volume %(volume_id)s" - " with driver provided model %(model)s") % - {'volume_id': volume_id, 'model': model_update}) - raise exception.ExportFailure(reason=ex) - return volume_ref diff --git a/cinder/volume/manager.py b/cinder/volume/manager.py index 100cc422c92..0b4e7a9d37b 100644 --- a/cinder/volume/manager.py +++ b/cinder/volume/manager.py @@ -182,7 +182,7 @@ def locked_snapshot_operation(f): class VolumeManager(manager.SchedulerDependentManager): """Manages attachable block storage devices.""" - RPC_API_VERSION = '1.12' + RPC_API_VERSION = '1.13' def __init__(self, volume_driver=None, service_name=None, *args, **kwargs): @@ -247,7 +247,7 @@ class VolumeManager(manager.SchedulerDependentManager): sum = 0 self.stats.update({'allocated_capacity_gb': sum}) for volume in volumes: - if volume['status'] in ['available', 'in-use']: + if volume['status'] in ['in-use']: # calculate allocated capacity for driver sum += volume['size'] self.stats['allocated_capacity_gb'] = sum @@ -393,7 +393,6 @@ class VolumeManager(manager.SchedulerDependentManager): except exception.VolumeIsBusy: LOG.error(_("Cannot delete volume %s: volume is busy"), volume_ref['id']) - self.driver.ensure_export(context, volume_ref) self.db.volume_update(context, volume_ref['id'], {'status': 'available'}) return True @@ -641,7 +640,6 @@ class VolumeManager(manager.SchedulerDependentManager): def detach_volume(self, context, volume_id): """Updates db to show volume is detached.""" # TODO(vish): refactor this into a more general "unreserve" - # TODO(sleepsonthefloor): Is this 'elevated' appropriate? volume = self.db.volume_get(context, volume_id) self._notify_about_volume_usage(context, volume, "detach.start") @@ -662,11 +660,29 @@ class VolumeManager(manager.SchedulerDependentManager): self.db.volume_admin_metadata_delete(context.elevated(), volume_id, 'attached_mode') - # Check for https://bugs.launchpad.net/cinder/+bug/1065702 + # NOTE(jdg): We used to do an ensure export here to + # catch upgrades while volumes were attached (E->F) + # this was necessary to convert in-use volumes from + # int ID's to UUID's. Don't need this any longer + + # We're going to remove the export here + # (delete the iscsi target) volume = self.db.volume_get(context, volume_id) - if (volume['provider_location'] and - volume['name'] not in volume['provider_location']): - self.driver.ensure_export(context, volume) + try: + utils.require_driver_initialized(self.driver) + LOG.debug(_("volume %s: removing export"), volume_id) + self.driver.remove_export(context, volume) + except exception.DriverNotInitialized as ex: + with excutils.save_and_reraise_exception(): + LOG.exception(_("Error detaching volume %(volume)s, " + "due to uninitialized driver."), + {"volume": volume_id}) + except Exception as ex: + LOG.exception(_("Error detaching volume %(volume)s, " + "due to remove export failure."), + {"volume": volume_id}) + raise exception.RemoveExportException(volume=volume_id, reason=ex) + self._notify_about_volume_usage(context, volume, "detach.end") def copy_volume_to_image(self, context, volume_id, image_meta): @@ -684,7 +700,6 @@ class VolumeManager(manager.SchedulerDependentManager): utils.require_driver_initialized(self.driver) volume = self.db.volume_get(context, volume_id) - self.driver.ensure_export(context.elevated(), volume) image_service, image_id = \ glance.get_remote_image_service(context, image_meta['id']) self.driver.copy_volume_to_image(context, volume, image_service, @@ -745,12 +760,34 @@ class VolumeManager(manager.SchedulerDependentManager): # before going forward. The exception will be caught # and the volume status updated. utils.require_driver_initialized(self.driver) + try: + self.driver.validate_connector(connector) + except Exception as err: + err_msg = (_('Unable to fetch connection information from ' + 'backend: %(err)s') % {'err': str(err)}) + LOG.error(err_msg) + raise exception.VolumeBackendAPIException(data=err_msg) volume = self.db.volume_get(context, volume_id) - self.driver.validate_connector(connector) + model_update = None + try: + LOG.debug(_("Volume %s: creating export"), volume_id) + model_update = self.driver.create_export(context, volume) + if model_update: + volume = self.db.volume_update(context, + volume_id, + model_update) + except exception.CinderException as ex: + if model_update: + LOG.exception(_("Failed updating model of volume %(volume_id)s" + " with driver provided model %(model)s") % + {'volume_id': volume_id, 'model': model_update}) + raise exception.ExportFailure(reason=ex) + try: conn_info = self.driver.initialize_connection(volume, connector) except Exception as err: + self.driver.remove_export(context, volume) err_msg = (_('Unable to fetch connection information from ' 'backend: %(err)s') % {'err': str(err)}) LOG.error(err_msg) diff --git a/cinder/volume/rpcapi.py b/cinder/volume/rpcapi.py index aaca9a1fbf0..01011527d69 100644 --- a/cinder/volume/rpcapi.py +++ b/cinder/volume/rpcapi.py @@ -47,6 +47,7 @@ class VolumeAPI(cinder.openstack.common.rpc.proxy.RpcProxy): 1.11 - Adds mode parameter to attach_volume() to support volume read-only attaching. 1.12 - Adds retype. + 1.13 - Adds create_export. ''' BASE_RPC_API_VERSION = '1.0' @@ -195,3 +196,11 @@ class VolumeAPI(cinder.openstack.common.rpc.proxy.RpcProxy): reservations=reservations), topic=rpc.queue_get_for(ctxt, self.topic, volume['host']), version='1.12') + + def create_export(self, ctxt, volume): + return self.call(ctxt, self.make_msg('create_export', + volume_id=volume['id']), + topic=rpc.queue_get_for(ctxt, + self.topic, + volume['host']), + version='1.13')