From 0273dc5b6d4e95ab3d1c945378637221cd421989 Mon Sep 17 00:00:00 2001 From: John Griffith Date: Thu, 21 Jul 2016 02:29:57 +0000 Subject: [PATCH] Add lock decorator to SolidFire clone_image method When using the SolidFire template-caching feature, if a template volume doesn't exist and multiple calls are made at the same time to create a volume from image, we can run in to a couple of problems. 1. It's possible to clone the template volume before the image conversion is actually completed (so the clone is a corrupt volume). 2. Hit a race where a subsequent call doesn't get the in progress template create and creates a duplicate. We can fix this by using a lock decorator around the clone_image method in the driver. Change-Id: I0000c5bf29629fc1ded7b9e3ae26989fbf9dc286 Closes-Bug: #1605027 --- cinder/tests/unit/test_solidfire.py | 82 ++++++++++++++++++++--------- cinder/volume/drivers/solidfire.py | 67 +++++++++++++---------- 2 files changed, 94 insertions(+), 55 deletions(-) diff --git a/cinder/tests/unit/test_solidfire.py b/cinder/tests/unit/test_solidfire.py index 00ca1fa189c..4486ce4a65f 100644 --- a/cinder/tests/unit/test_solidfire.py +++ b/cinder/tests/unit/test_solidfire.py @@ -24,6 +24,7 @@ from cinder import context from cinder import exception from cinder.objects import fields from cinder import test +from cinder.tests.unit.image import fake as fake_image from cinder.volume import configuration as conf from cinder.volume.drivers import solidfire from cinder.volume import qos_specs @@ -79,7 +80,7 @@ class SolidFireVolumeTestCase(test.TestCase): 325355), 'is_public': True, 'owner': 'testprjid'} - self.fake_image_service = 'null' + self.fake_image_service = fake_image.FakeImageService() def fake_init_cluster_pairs(*args, **kwargs): return None @@ -960,7 +961,19 @@ class SolidFireVolumeTestCase(test.TestCase): 'fake')) @mock.patch.object(solidfire.SolidFireDriver, '_create_template_account') - def test_clone_image_authorization(self, _mock_create_template_account): + @mock.patch.object(solidfire.SolidFireDriver, '_create_image_volume') + def test_clone_image_authorization(self, + _mock_create_image_volume, + _mock_create_template_account): + fake_sf_vref = { + 'status': 'active', 'volumeID': 1, + 'attributes': { + 'image_info': + {'image_updated_at': '2014-12-17T00:16:23+00:00', + 'image_id': '155d900f-4e14-4e4c-a73d-069cbf4541e6', + 'image_name': 'fake-image', + 'image_created_at': '2014-12-17T00:16:23+00:00'}}} + _mock_create_image_volume.return_value = fake_sf_vref _mock_create_template_account.return_value = 1 self.configuration.sf_allow_template_caching = True @@ -968,14 +981,24 @@ class SolidFireVolumeTestCase(test.TestCase): # Make sure if it's NOT public and we're NOT the owner it # doesn't try and cache - _fake_image_meta = {'id': '17c550bb-a411-44c0-9aaf-0d96dd47f501', - 'updated_at': datetime.datetime(2013, 9, - 28, 15, - 27, 36, - 325355), - 'properties': {'virtual_size': 1}, - 'is_public': False, - 'owner': 'wrong-owner'} + timestamp = datetime.datetime(2011, 1, 1, 1, 2, 3) + _fake_image_meta = { + 'id': '155d900f-4e14-4e4c-a73d-069cbf4541e6', + 'name': 'fakeimage123456', + 'created_at': timestamp, + 'updated_at': timestamp, + 'deleted_at': None, + 'deleted': False, + 'status': 'active', + 'visibility': 'private', + 'protected': False, + 'container_format': 'raw', + 'disk_format': 'raw', + 'owner': 'wrong-owner', + 'properties': {'kernel_id': 'nokernel', + 'ramdisk_id': 'nokernel', + 'architecture': 'x86_64'}} + with mock.patch.object(sfv, '_do_clone_volume', return_value=('fe', 'fi', 'fo')): self.assertEqual((None, False), @@ -983,32 +1006,39 @@ class SolidFireVolumeTestCase(test.TestCase): self.mock_volume, 'fake', _fake_image_meta, - 'fake')) + self.fake_image_service)) # And is_public False, but the correct owner does work _fake_image_meta['owner'] = 'testprjid' - self.assertEqual(('fo', True), sfv.clone_image(self.ctxt, - self.mock_volume, - 'fake', - _fake_image_meta, - 'fake')) + self.assertEqual( + ('fo', True), + sfv.clone_image( + self.ctxt, + self.mock_volume, + 'fake', + _fake_image_meta, + self.fake_image_service)) # And is_public True, even if not the correct owner _fake_image_meta['is_public'] = True _fake_image_meta['owner'] = 'wrong-owner' - self.assertEqual(('fo', True), sfv.clone_image(self.ctxt, - self.mock_volume, - 'fake', - _fake_image_meta, - 'fake')) + self.assertEqual( + ('fo', True), + sfv.clone_image(self.ctxt, + self.mock_volume, + 'fake', + _fake_image_meta, + self.fake_image_service)) # 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')) + self.assertEqual( + ('fo', True), + sfv.clone_image(self.ctxt, + self.mock_volume, + 'fake', + _fake_image_meta, + self.fake_image_service)) 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 cbb7fc6fb35..014492ebc0f 100644 --- a/cinder/volume/drivers/solidfire.py +++ b/cinder/volume/drivers/solidfire.py @@ -1,6 +1,6 @@ # All Rights Reserved. # Copyright 2013 SolidFire Inc -# + # Licensed under the Apache License, Version 2.0 (the "License"); you may # not use this file except in compliance with the License. You may obtain # a copy of the License at @@ -13,6 +13,7 @@ # License for the specific language governing permissions and limitations # under the License. +import inspect import json import math import random @@ -37,6 +38,7 @@ from cinder.i18n import _, _LE, _LW from cinder.image import image_utils from cinder import interface from cinder.objects import fields +from cinder import utils from cinder.volume.drivers.san import san from cinder.volume import qos_specs from cinder.volume.targets import iscsi as iscsi_driver @@ -151,9 +153,11 @@ class SolidFireDriver(san.SanISCSIDriver): 2.0.4 - Implement volume replication 2.0.5 - Try and deal with the stupid retry/clear issues from objects and tflow + 2.0.6 - Add a lock decorator around the clone_image method """ - VERSION = '2.0.2' + VERSION = '2.0.6' + driver_prefix = 'solidfire' sf_qos_dict = {'slow': {'minIOPS': 100, 'maxIOPS': 200, @@ -746,6 +750,24 @@ class SolidFireDriver(san.SanISCSIDriver): return self._issue_api_request( 'ListSnapshots', params, version='6.0')['result']['snapshots'] + def locked_image_id_operation(f, external=False): + def lvo_inner1(inst, *args, **kwargs): + lock_tag = inst.driver_prefix + call_args = inspect.getcallargs(f, inst, *args, **kwargs) + + if call_args.get('image_meta'): + image_id = call_args['image_meta']['id'] + else: + err_msg = _('The decorated method must accept image_meta.') + raise exception.VolumeBackendAPIException(data=err_msg) + + @utils.synchronized('%s-%s' % (lock_tag, image_id), + external=external) + def lvo_inner2(): + return f(inst, *args, **kwargs) + return lvo_inner2() + return lvo_inner1 + def _create_image_volume(self, context, image_meta, image_service, image_id): @@ -839,24 +861,22 @@ class SolidFireDriver(san.SanISCSIDriver): params = {'accountID': self.template_account_id} sf_vol = self._get_sf_volume(image_meta['id'], params) - if sf_vol is None: + if not sf_vol: + self._create_image_volume(context, + image_meta, + image_service, + image_meta['id']) return - # Check updated_at field, delete copy and update if needed - if sf_vol['attributes']['image_info']['image_updated_at'] == ( + if sf_vol['attributes']['image_info']['image_updated_at'] != ( image_meta['updated_at'].isoformat()): - return - else: - # Bummer, it's been updated, delete it params = {'accountID': self.template_account_id} params['volumeID'] = sf_vol['volumeID'] self._issue_api_request('DeleteVolume', params) - if not self._create_image_volume(context, - image_meta, - image_service, - image_meta['id']): - msg = _("Failed to create SolidFire Image-Volume") - raise exception.SolidFireAPIException(msg) + self._create_image_volume(context, + image_meta, + image_service, + image_meta['id']) def _get_sfaccounts_for_tenant(self, cinder_project_id): accounts = self._issue_api_request( @@ -1082,6 +1102,7 @@ class SolidFireDriver(san.SanISCSIDriver): for vag in sorted_targets[:limit]: self._remove_vag(vag['volumeAccessGroupID']) + @locked_image_id_operation def clone_image(self, context, volume, image_location, image_meta, image_service): @@ -1116,21 +1137,9 @@ class SolidFireDriver(san.SanISCSIDriver): except exception.SolidFireAPIException: return None, False - try: - (data, sfaccount, model) = self._do_clone_volume(image_meta['id'], - volume) - except exception.VolumeNotFound: - if self._create_image_volume(context, - image_meta, - image_service, - image_meta['id']) is None: - # We failed, dump out - return None, False - - # Ok, should be good to go now, try it again - (data, sfaccount, model) = self._do_clone_volume(image_meta['id'], - volume) - + # Ok, should be good to go now, try it again + (data, sfaccount, model) = self._do_clone_volume(image_meta['id'], + volume) return model, True def _retrieve_qos_setting(self, volume):