From: John Griffith Date: Fri, 11 Sep 2015 19:57:32 +0000 (+0000) Subject: Fix volume lookups in SolidFire template caching X-Git-Url: https://review.fuel-infra.org/gitweb?a=commitdiff_plain;h=809567661d6732eaf4a5e77d2f2df07621ee5114;p=openstack-build%2Fcinder-build.git Fix volume lookups in SolidFire template caching There's a number of issues with the SolidFire template caching code. There are a number of metadata items that aren't consistent between Glance API V1 and V2, there's also some problems with how the code is dealing with it's internal volume and just flat out wrong account checking. This patch focuses mainly on fixing up the account and volumeID issues. The _do_clone method currently expects to find the src volume under the same account as the dest volume. That's fine for most cases, but doesn't work for either the SolidFire template caching, or for the use of generic image caching in Cinder. We fix that easily by modifying the _get_sf_volume call to have an option to search all active volumes on the Cluster instead of just those for a specified account. For now we only change the clone methods, as they're the only case where we fall into the case of cross-ownership of volumes in the same context. The other issue is around the cleanup/delete in the case of a failed template-volume create. In this case we were passing in the Cinder UUID to a direct SolidFire API delete cmd, which is wrong. The direct call for the SolidFire API call requires that the actual SolidFire ID be used. So we leverage the modified _get_sf_volume call here as well. Another issue that was introduced was the introduction of a connector object to the intialize_connection call. We got around this once already by just passing in a fake connector that was actually a string. That was fine, but the initialize_connection routine also now checks the connector for multi_attach, so it fails the _get call on a string. We fix that by just building a dict and using that instead of a fake string. Finally, the method of relying on the Glance metadata virt size has been problematic, it's not set by default, it's not accurate and it's a bit inefficient. There have been a number of issues with this method of size determination, so we'll dump that and just leverage the image_utils methods directly. Change-Id: Icfd6668389049d3d5686acdb832847aa2d2fce52 Closes-Bug: #1494830 Closes-Bug: #1494927 --- diff --git a/cinder/tests/unit/test_solidfire.py b/cinder/tests/unit/test_solidfire.py index c3cda4d28..a8cbdb15f 100644 --- a/cinder/tests/unit/test_solidfire.py +++ b/cinder/tests/unit/test_solidfire.py @@ -386,6 +386,10 @@ class SolidFireVolumeTestCase(test.TestCase): _mock_issue_api_request.return_value = self.mock_stats_data _mock_create_template_account.return_value = 1 _fake_get_snaps = [{'snapshotID': 5, 'name': 'testvol'}] + _fake_get_volume = ( + {'volumeID': 99, + 'name': 'UUID-a720b3c0-d1f0-11e1-9b23-0800200c9a66', + 'attributes': {}}) testvol = {'project_id': 'testprjid', 'name': 'testvol', @@ -405,6 +409,9 @@ class SolidFireVolumeTestCase(test.TestCase): with mock.patch.object(sfv, '_get_sf_snapshots', return_value=_fake_get_snaps), \ + mock.patch.object(sfv, + '_get_sf_volume', + return_value=_fake_get_volume), \ mock.patch.object(sfv, '_issue_api_request', side_effect=self.fake_issue_api_request), \ @@ -985,33 +992,14 @@ class SolidFireVolumeTestCase(test.TestCase): 'fake', _fake_image_meta, 'fake')) - - @mock.patch.object(solidfire.SolidFireDriver, '_issue_api_request') - @mock.patch.object(solidfire.SolidFireDriver, '_create_template_account') - def test_clone_image_virt_size_not_set(self, - _mock_create_template_account, - _mock_issue_api_request): - _mock_issue_api_request.return_value = self.mock_stats_data - _mock_create_template_account.return_value = 1 - - self.configuration.sf_allow_template_caching = True - sfv = solidfire.SolidFireDriver(configuration=self.configuration) - - # Don't run clone_image if virtual_size property not on image - _fake_image_meta = {'id': '17c550bb-a411-44c0-9aaf-0d96dd47f501', - 'updated_at': datetime.datetime(2013, 9, - 28, 15, - 27, 36, - 325355), - 'is_public': True, - 'owner': 'testprjid'} - - self.assertEqual((None, False), - sfv.clone_image(self.ctxt, - self.mock_volume, - 'fake', - _fake_image_meta, - 'fake')) + # And using the new V2 visibility tag + _fake_image_meta['visibility'] = 'public' + _fake_image_meta['owner'] = 'wrong-owner' + self.assertEqual(('fo', True), sfv.clone_image(self.ctxt, + self.mock_volume, + 'fake', + _fake_image_meta, + 'fake')) def test_create_template_no_account(self): sfv = solidfire.SolidFireDriver(configuration=self.configuration) diff --git a/cinder/volume/drivers/solidfire.py b/cinder/volume/drivers/solidfire.py index 97b8dd56b..63ee271e9 100644 --- a/cinder/volume/drivers/solidfire.py +++ b/cinder/volume/drivers/solidfire.py @@ -32,7 +32,7 @@ import six from cinder import context from cinder import exception -from cinder.i18n import _, _LE, _LI, _LW +from cinder.i18n import _, _LE, _LW from cinder.image import image_utils from cinder.volume.drivers.san import san from cinder.volume import qos_specs @@ -439,6 +439,7 @@ class SolidFireDriver(san.SanISCSIDriver): src_project_id, vref): """Create a clone of an existing volume or snapshot.""" + attributes = {} qos = {} @@ -470,8 +471,7 @@ class SolidFireDriver(san.SanISCSIDriver): params['volumeID'] = int(snap['volumeID']) params['newSize'] = int(vref['size'] * units.Gi) else: - sf_vol = self._get_sf_volume( - src_uuid, {'accountID': sf_account['accountID']}) + sf_vol = self._get_sf_volume(src_uuid) if sf_vol is None: raise exception.VolumeNotFound(volume_id=src_uuid) params['volumeID'] = int(sf_vol['volumeID']) @@ -591,9 +591,13 @@ class SolidFireDriver(san.SanISCSIDriver): qos[key] = int(value) return qos - def _get_sf_volume(self, uuid, params): - vols = self._issue_api_request( - 'ListVolumesForAccount', params)['result']['volumes'] + def _get_sf_volume(self, uuid, params=None): + if params: + vols = self._issue_api_request( + 'ListVolumesForAccount', params)['result']['volumes'] + else: + vols = self._issue_api_request( + 'ListActiveVolumes', params)['result']['volumes'] found_count = 0 sf_volref = None @@ -635,63 +639,79 @@ class SolidFireDriver(san.SanISCSIDriver): def _create_image_volume(self, context, image_meta, image_service, image_id): - # NOTE(jdg): It's callers responsibility to ensure that - # the optional properties.virtual_size is set on the image - # before we get here - virt_size = int(image_meta['properties'].get('virtual_size')) - min_sz_in_bytes = ( - math.ceil(virt_size / float(units.Gi)) * float(units.Gi)) - min_sz_in_gig = math.ceil(min_sz_in_bytes / float(units.Gi)) - - attributes = {} - attributes['image_info'] = {} - attributes['image_info']['image_updated_at'] = ( - image_meta['updated_at'].isoformat()) - attributes['image_info']['image_name'] = ( - image_meta['name']) - attributes['image_info']['image_created_at'] = ( - image_meta['created_at'].isoformat()) - attributes['image_info']['image_id'] = image_meta['id'] - params = {'name': 'OpenStackIMG-%s' % image_id, - 'accountID': self.template_account_id, - 'sliceCount': 1, - 'totalSize': int(min_sz_in_bytes), - 'enable512e': self.configuration.sf_emulate_512, - 'attributes': attributes, - 'qos': {}} - - sf_account = self._issue_api_request( - 'GetAccountByID', - {'accountID': self.template_account_id})['result']['account'] - - template_vol = self._do_volume_create(sf_account, params) - tvol = {} - tvol['id'] = image_id - tvol['provider_location'] = template_vol['provider_location'] - tvol['provider_auth'] = template_vol['provider_auth'] - - connector = 'na' - conn = self.initialize_connection(tvol, connector) - attach_info = super(SolidFireDriver, self)._connect_device(conn) - properties = 'na' - - try: - image_utils.fetch_to_raw(context, - image_service, - image_id, - attach_info['device']['path'], - self.configuration.volume_dd_blocksize, - size=min_sz_in_gig) - except Exception as exc: - params['volumeID'] = template_vol['volumeID'] - LOG.error(_LE('Failed image conversion during cache creation: %s'), - exc) - LOG.debug('Removing SolidFire Cache Volume (SF ID): %s', - template_vol['volumeID']) - - self._detach_volume(context, attach_info, tvol, properties) - self._issue_api_request('DeleteVolume', params) - return + with image_utils.TemporaryImages.fetch(image_service, + context, + image_id) as tmp_image: + data = image_utils.qemu_img_info(tmp_image) + fmt = data.file_format + if fmt is None: + raise exception.ImageUnacceptable( + reason=_("'qemu-img info' parsing failed."), + image_id=image_id) + + backing_file = data.backing_file + if backing_file is not None: + raise exception.ImageUnacceptable( + image_id=image_id, + reason=_("fmt=%(fmt)s backed by:%(backing_file)s") + % {'fmt': fmt, 'backing_file': backing_file, }) + + virtual_size = int(math.ceil(float(data.virtual_size) / units.Gi)) + attributes = {} + attributes['image_info'] = {} + attributes['image_info']['image_updated_at'] = ( + image_meta['updated_at'].isoformat()) + attributes['image_info']['image_name'] = ( + image_meta['name']) + attributes['image_info']['image_created_at'] = ( + image_meta['created_at'].isoformat()) + attributes['image_info']['image_id'] = image_meta['id'] + params = {'name': 'OpenStackIMG-%s' % image_id, + 'accountID': self.template_account_id, + 'sliceCount': 1, + 'totalSize': int(virtual_size * units.Gi), + 'enable512e': self.configuration.sf_emulate_512, + 'attributes': attributes, + 'qos': {}} + + sf_account = self._issue_api_request( + 'GetAccountByID', + {'accountID': self.template_account_id})['result']['account'] + template_vol = self._do_volume_create(sf_account, params) + + tvol = {} + tvol['id'] = image_id + tvol['provider_location'] = template_vol['provider_location'] + tvol['provider_auth'] = template_vol['provider_auth'] + + connector = {'multipath': False} + conn = self.initialize_connection(tvol, connector) + attach_info = super(SolidFireDriver, self)._connect_device(conn) + properties = 'na' + try: + image_utils.convert_image(tmp_image, + attach_info['device']['path'], + 'raw', + run_as_root=True) + data = image_utils.qemu_img_info(attach_info['device']['path'], + run_as_root=True) + if data.file_format != 'raw': + raise exception.ImageUnacceptable( + image_id=image_id, + reason=_("Converted to %(vol_format)s, but format is " + "now %(file_format)s") % {'vol_format': 'raw', + 'file_format': data. + file_format}) + except Exception as exc: + vol = self._get_sf_volume(image_id) + LOG.error(_LE('Failed image conversion during ' + 'cache creation: %s'), + exc) + LOG.debug('Removing SolidFire Cache Volume (SF ID): %s', + vol['volumeID']) + self._detach_volume(context, attach_info, tvol, properties) + self._issue_api_request('DeleteVolume', params) + return self._detach_volume(context, attach_info, tvol, properties) sf_vol = self._get_sf_volume(image_id, params) @@ -793,25 +813,28 @@ class SolidFireDriver(san.SanISCSIDriver): def clone_image(self, context, volume, image_location, image_meta, image_service): - + public = False # Check out pre-requisites: # Is template caching enabled? if not self.configuration.sf_allow_template_caching: return None, False - # Is the image owned by this tenant or public? - if ((not image_meta.get('is_public', False)) and - (image_meta['owner'] != volume['project_id'])): - LOG.warning(_LW("Requested image is not " - "accessible by current Tenant.")) - return None, False - - # Is virtual_size property set on the image? - if ((not image_meta.get('properties', None)) or - (not image_meta['properties'].get('virtual_size', None))): - LOG.info(_LI('Unable to create cache volume because image: %s ' - 'does not include properties.virtual_size'), - image_meta['id']) + # NOTE(jdg): Glance V2 moved from is_public to visibility + # so we check both, as we don't necessarily know or want + # to care which we're using. Will need to look at + # future handling of things like shared and community + # but for now, it's owner or public and that's it + visibility = image_meta.get('visibility', None) + if visibility and visibility == 'public': + public = True + elif image_meta.get('is_public', False): + public = True + else: + if image_meta['owner'] == volume['project_id']: + public = True + if not public: + LOG.warning(_LW("Requested image is not " + "accessible by current Tenant.")) return None, False try: