From 8c38b6a2aaddf28663e1442472540b2e39010e1c Mon Sep 17 00:00:00 2001 From: Gorka Eguileor Date: Mon, 24 Aug 2015 20:31:54 +0200 Subject: [PATCH] Remove API races on extend and volume_upload_image This patch uses compare-and-swap to remove API races from the following actions: - os-volume_upload_image - os-extend In cinder/volume/api.py affects methods: - copy_volume_to_image - extend Specs: https://review.openstack.org/232599/ Implements: blueprint cinder-volume-active-active-support Closes-Bug: #1490946 Closes-Bug: #1493120 Change-Id: I89363879987170a7aa6ce1bcdf24d04ed275bf49 --- cinder/api/contrib/volume_actions.py | 3 +- .../unit/api/contrib/test_volume_actions.py | 491 ++++++++---------- .../tests/unit/api/v1/test_volume_metadata.py | 3 +- .../tests/unit/api/v2/test_volume_metadata.py | 7 +- cinder/tests/unit/test_volume.py | 23 +- cinder/volume/api.py | 147 +++--- 6 files changed, 327 insertions(+), 347 deletions(-) diff --git a/cinder/api/contrib/volume_actions.py b/cinder/api/contrib/volume_actions.py index 684791d4eed..1247e412f43 100644 --- a/cinder/api/contrib/volume_actions.py +++ b/cinder/api/contrib/volume_actions.py @@ -288,12 +288,11 @@ class VolumeActionsController(wsgi.Controller): raise webob.exc.HTTPNotFound(explanation=error.msg) try: - int(body['os-extend']['new_size']) + size = int(body['os-extend']['new_size']) except (KeyError, ValueError, TypeError): msg = _("New volume size must be specified as an integer.") raise webob.exc.HTTPBadRequest(explanation=msg) - size = int(body['os-extend']['new_size']) try: self.volume_api.extend(context, volume, size) except exception.InvalidVolume as error: diff --git a/cinder/tests/unit/api/contrib/test_volume_actions.py b/cinder/tests/unit/api/contrib/test_volume_actions.py index 012cce1adb8..a97d3698812 100644 --- a/cinder/tests/unit/api/contrib/test_volume_actions.py +++ b/cinder/tests/unit/api/contrib/test_volume_actions.py @@ -13,10 +13,10 @@ # under the License. import datetime -import iso8601 import uuid import mock +from oslo_config import cfg import oslo_messaging as messaging from oslo_serialization import jsonutils import webob @@ -24,6 +24,7 @@ import webob from cinder.api.contrib import volume_actions from cinder.api.openstack import api_version_request as api_version from cinder import context +from cinder import db from cinder import exception from cinder.image import glance from cinder import objects @@ -32,11 +33,15 @@ from cinder.tests.unit.api import fakes from cinder.tests.unit.api.v2 import stubs from cinder.tests.unit import fake_constants as fake from cinder.tests.unit import fake_volume +from cinder.tests.unit import utils from cinder import volume from cinder.volume import api as volume_api from cinder.volume import rpcapi as volume_rpcapi +CONF = cfg.CONF + + class VolumeActionsTest(test.TestCase): _actions = ('os-reserve', 'os-unreserve') @@ -659,7 +664,7 @@ class VolumeImageActionsTest(test.TestCase): 'created_at': datetime.datetime(1, 1, 1, 1, 1, 1), 'disk_format': u'raw', 'updated_at': datetime.datetime(1, 1, 1, 1, 1, 1), - 'id': 1, + 'id': fake.IMAGE_ID, 'min_ram': 0, 'checksum': None, 'min_disk': 0, @@ -839,64 +844,59 @@ class VolumeImageActionsTest(test.TestCase): id, body) - def test_copy_volume_to_image_with_protected_prop(self): + def _create_volume_with_type(self, status='available', + display_description='displaydesc', **kwargs): + self.stubs.UnsetAll() + + admin_ctxt = context.get_admin_context() + vol_type = db.volume_type_create(admin_ctxt, {'name': 'vol_name'}) + self.addCleanup(db.volume_type_destroy, admin_ctxt, vol_type.id) + + volume = utils.create_volume(self.context, volume_type_id=vol_type.id, + status=status, + display_description=display_description, + **kwargs) + self.addCleanup(db.volume_destroy, admin_ctxt, volume.id) + + expected = { + 'os-volume_upload_image': { + 'id': volume.id, + 'updated_at': 'DONTCARE', + 'status': 'uploading', + 'display_description': 'displaydesc', + 'size': 1, + 'volume_type': 'DONTCARE', + 'image_id': fake.IMAGE_ID, + 'container_format': 'bare', + 'disk_format': 'raw', + 'image_name': 'image_name' + } + } + return volume, expected + + @mock.patch.object(volume_api.API, "get_volume_image_metadata") + @mock.patch.object(glance.GlanceImageService, "create") + @mock.patch.object(volume_rpcapi.VolumeAPI, "copy_volume_to_image") + def test_copy_volume_to_image_with_protected_prop( + self, mock_copy_to_image, mock_create, mock_get_image_metadata): """Test create image from volume with protected properties.""" - id = fake.VOLUME2_ID + volume, expected = self._create_volume_with_type() + mock_get_image_metadata.return_value = {"volume_id": volume.id, + "key": "x_billing_license", + "value": "246254365"} + mock_create.side_effect = self.fake_image_service_create - def fake_get_volume_image_metadata(*args): - meta_dict = { - "volume_id": fake.VOLUME2_ID, - "key": "x_billing_code_license", - "value": "246254365"} - return meta_dict + req = fakes.HTTPRequest.blank( + '/v2/%s/volumes/%s/action' % (fake.PROJECT_ID, volume.id), + use_admin_context=self.context.is_admin) + body = self._get_os_volume_upload_image() - # Need to mock get_volume_image_metadata, create, - # update and copy_volume_to_image - with mock.patch.object(volume_api.API, "get_volume_image_metadata") \ - as mock_get_volume_image_metadata: - mock_get_volume_image_metadata.side_effect = \ - fake_get_volume_image_metadata + res_dict = self.controller._volume_upload_image(req, volume.id, body) - with mock.patch.object(glance.GlanceImageService, "create") \ - as mock_create: - mock_create.side_effect = self.fake_image_service_create - - with mock.patch.object(volume_api.API, "update") \ - as mock_update: - mock_update.side_effect = stubs.stub_volume_update - - with mock.patch.object(volume_rpcapi.VolumeAPI, - "copy_volume_to_image") \ - as mock_copy_volume_to_image: - mock_copy_volume_to_image.side_effect = \ - self.fake_rpc_copy_volume_to_image - - req = fakes.HTTPRequest.blank( - '/v2/%s/volumes/%s/action' % ( - fake.PROJECT_ID, id)) - body = self._get_os_volume_upload_image() - res_dict = self.controller._volume_upload_image(req, - id, - body) - expected_res = { - 'os-volume_upload_image': { - 'id': id, - 'updated_at': datetime.datetime( - 1900, 1, 1, 1, 1, 1, - tzinfo=iso8601.iso8601.Utc()), - 'status': 'uploading', - 'display_description': 'displaydesc', - 'size': 1, - 'volume_type': fake_volume.fake_db_volume_type( - name='vol_type_name'), - 'image_id': fake.IMAGE_ID, - 'container_format': 'bare', - 'disk_format': 'raw', - 'image_name': 'image_name' - } - } - - self.assertDictMatch(expected_res, res_dict) + self.assertDictMatch(expected, res_dict) + vol_db = objects.Volume.get_by_id(self.context, volume.id) + self.assertEqual('uploading', vol_db.status) + self.assertEqual('available', vol_db.previous_status) def test_copy_volume_to_image_public_not_authorized(self): """Test unauthorized create public image from volume.""" @@ -911,237 +911,213 @@ class VolumeImageActionsTest(test.TestCase): self.controller._volume_upload_image, req, id, body) - def test_copy_volume_to_image_without_glance_metadata(self): + @mock.patch.object(volume_api.API, "get_volume_image_metadata") + @mock.patch.object(glance.GlanceImageService, "create") + @mock.patch.object(volume_rpcapi.VolumeAPI, "copy_volume_to_image") + def test_copy_volume_to_image_without_glance_metadata( + self, mock_copy_to_image, mock_create, mock_get_image_metadata): """Test create image from volume if volume is created without image. In this case volume glance metadata will not be available for this volume. """ - id = fake.VOLUME_ID + volume, expected = self._create_volume_with_type() - def fake_get_volume_image_metadata_raise(*args): - raise exception.GlanceMetadataNotFound(id=id) + mock_get_image_metadata.side_effect = \ + exception.GlanceMetadataNotFound(id=volume.id) + mock_create.side_effect = self.fake_image_service_create - # Need to mock get_volume_image_metadata, create, - # update and copy_volume_to_image - with mock.patch.object(volume_api.API, "get_volume_image_metadata") \ - as mock_get_volume_image_metadata: - mock_get_volume_image_metadata.side_effect = \ - fake_get_volume_image_metadata_raise + req = fakes.HTTPRequest.blank( + '/v2/%s/volumes/%s/action' % (fake.PROJECT_ID, volume.id), + use_admin_context=self.context.is_admin) + body = self._get_os_volume_upload_image() + res_dict = self.controller._volume_upload_image(req, volume.id, body) - with mock.patch.object(glance.GlanceImageService, "create") \ - as mock_create: - mock_create.side_effect = self.fake_image_service_create - - with mock.patch.object(volume_api.API, "update") \ - as mock_update: - mock_update.side_effect = stubs.stub_volume_update - - with mock.patch.object(volume_rpcapi.VolumeAPI, - "copy_volume_to_image") \ - as mock_copy_volume_to_image: - mock_copy_volume_to_image.side_effect = \ - self.fake_rpc_copy_volume_to_image - - req = fakes.HTTPRequest.blank( - '/v2/%s/volumes/%s/action' % (fake.PROJECT_ID, id)) - body = self._get_os_volume_upload_image() - res_dict = self.controller._volume_upload_image(req, - id, - body) - expected_res = { - 'os-volume_upload_image': { - 'id': id, - 'updated_at': datetime.datetime( - 1900, 1, 1, 1, 1, 1, - tzinfo=iso8601.iso8601.Utc()), - 'status': 'uploading', - 'display_description': 'displaydesc', - 'size': 1, - 'volume_type': fake_volume.fake_db_volume_type( - name='vol_type_name'), - 'image_id': fake.IMAGE_ID, - 'container_format': 'bare', - 'disk_format': 'raw', - 'image_name': 'image_name' - } - } - - self.assertDictMatch(expected_res, res_dict) - - def test_copy_volume_to_image_without_protected_prop(self): - """Test protected property is not defined with the root image.""" - id = fake.VOLUME_ID - - def fake_get_volume_image_metadata(*args): - return [] - - # Need to mock get_volume_image_metadata, create, - # update and copy_volume_to_image - with mock.patch.object(volume_api.API, "get_volume_image_metadata") \ - as mock_get_volume_image_metadata: - mock_get_volume_image_metadata.side_effect = \ - fake_get_volume_image_metadata - - with mock.patch.object(glance.GlanceImageService, "create") \ - as mock_create: - mock_create.side_effect = self.fake_image_service_create - - with mock.patch.object(volume_api.API, "update") \ - as mock_update: - mock_update.side_effect = stubs.stub_volume_update - - with mock.patch.object(volume_rpcapi.VolumeAPI, - "copy_volume_to_image") \ - as mock_copy_volume_to_image: - mock_copy_volume_to_image.side_effect = \ - self.fake_rpc_copy_volume_to_image - - req = fakes.HTTPRequest.blank( - '/v2/%s/volumes/%s/action' % ( - fake.PROJECT_ID, id)) - body = self._get_os_volume_upload_image() - res_dict = self.controller._volume_upload_image(req, - id, - body) - expected_res = { - 'os-volume_upload_image': { - 'id': id, - 'updated_at': datetime.datetime( - 1900, 1, 1, 1, 1, 1, - tzinfo=iso8601.iso8601.Utc()), - 'status': 'uploading', - 'display_description': 'displaydesc', - 'size': 1, - 'volume_type': fake_volume.fake_db_volume_type( - name='vol_type_name'), - 'image_id': fake.IMAGE_ID, - 'container_format': 'bare', - 'disk_format': 'raw', - 'image_name': 'image_name' - } - } - - self.assertDictMatch(expected_res, res_dict) - - def test_copy_volume_to_image_without_core_prop(self): - """Test glance_core_properties defined in cinder.conf is empty.""" - id = fake.VOLUME_ID - - # Need to mock create, update, copy_volume_to_image - with mock.patch.object(glance.GlanceImageService, "create") \ - as mock_create: - mock_create.side_effect = self.fake_image_service_create - - with mock.patch.object(volume_api.API, "update") \ - as mock_update: - mock_update.side_effect = stubs.stub_volume_update - - with mock.patch.object(volume_rpcapi.VolumeAPI, - "copy_volume_to_image") \ - as mock_copy_volume_to_image: - mock_copy_volume_to_image.side_effect = \ - self.fake_rpc_copy_volume_to_image - - self.override_config('glance_core_properties', []) - - req = fakes.HTTPRequest.blank( - '/v2/%s/volumes/%s/action' % (fake.PROJECT_ID, id)) - - body = self._get_os_volume_upload_image() - res_dict = self.controller._volume_upload_image(req, - id, - body) - expected_res = { - 'os-volume_upload_image': { - 'id': id, - 'updated_at': datetime.datetime( - 1900, 1, 1, 1, 1, 1, - tzinfo=iso8601.iso8601.Utc()), - 'status': 'uploading', - 'display_description': 'displaydesc', - 'size': 1, - 'volume_type': fake_volume.fake_db_volume_type( - name='vol_type_name'), - 'image_id': fake.IMAGE_ID, - 'container_format': 'bare', - 'disk_format': 'raw', - 'image_name': 'image_name' - } - } - - self.assertDictMatch(expected_res, res_dict) + self.assertDictMatch(expected, res_dict) + vol_db = objects.Volume.get_by_id(self.context, volume.id) + self.assertEqual('uploading', vol_db.status) + self.assertEqual('available', vol_db.previous_status) + + @mock.patch.object(volume_api.API, "get_volume_image_metadata") + @mock.patch.object(glance.GlanceImageService, "create") + @mock.patch.object(volume_rpcapi.VolumeAPI, "copy_volume_to_image") + def test_copy_volume_to_image_fail_image_create( + self, mock_copy_to_image, mock_create, mock_get_image_metadata): + """Test create image from volume if create image fails. + + In this case API will rollback to previous status. + """ + volume = utils.create_volume(self.context) + + mock_get_image_metadata.return_value = {} + mock_create.side_effect = Exception() + + req = fakes.HTTPRequest.blank( + '/v2/fakeproject/volumes/%s/action' % volume.id) + body = self._get_os_volume_upload_image() + self.assertRaises(webob.exc.HTTPBadRequest, + self.controller._volume_upload_image, req, volume.id, + body) + + self.assertFalse(mock_copy_to_image.called) + vol_db = objects.Volume.get_by_id(self.context, volume.id) + self.assertEqual('available', vol_db.status) + self.assertIsNone(vol_db.previous_status) + db.volume_destroy(context.get_admin_context(), volume.id) + + @mock.patch.object(volume_api.API, "get_volume_image_metadata") + @mock.patch.object(glance.GlanceImageService, "create") + @mock.patch.object(volume_rpcapi.VolumeAPI, "copy_volume_to_image") + def test_copy_volume_to_image_in_use_no_force( + self, mock_copy_to_image, mock_create, mock_get_image_metadata): + """Test create image from in-use volume. + + In this case API will fail because we are not passing force. + """ + volume = utils.create_volume(self.context, status='in-use') + + mock_get_image_metadata.return_value = {} + mock_create.side_effect = self.fake_image_service_create + + req = fakes.HTTPRequest.blank( + '/v2/fakeproject/volumes/%s/action' % volume.id) + body = self._get_os_volume_upload_image() + body['os-volume_upload_image']['force'] = False + self.assertRaises(webob.exc.HTTPBadRequest, + self.controller._volume_upload_image, req, volume.id, + body) + + self.assertFalse(mock_copy_to_image.called) + vol_db = objects.Volume.get_by_id(self.context, volume.id) + self.assertEqual('in-use', vol_db.status) + self.assertIsNone(vol_db.previous_status) + db.volume_destroy(context.get_admin_context(), volume.id) + + @mock.patch.object(volume_api.API, "get_volume_image_metadata") + @mock.patch.object(glance.GlanceImageService, "create") + @mock.patch.object(volume_rpcapi.VolumeAPI, "copy_volume_to_image") + def test_copy_volume_to_image_in_use_with_force( + self, mock_copy_to_image, mock_create, mock_get_image_metadata): + """Test create image from in-use volume. + + In this case API will succeed only when CON.enable_force_upload is + enabled. + """ + volume, expected = self._create_volume_with_type(status='in-use') + mock_get_image_metadata.return_value = {} + mock_create.side_effect = self.fake_image_service_create + + req = fakes.HTTPRequest.blank( + '/v2/fakeproject/volumes/%s/action' % volume.id, + use_admin_context=self.context.is_admin) + body = self._get_os_volume_upload_image() + self.assertRaises(webob.exc.HTTPBadRequest, + self.controller._volume_upload_image, req, volume.id, + body) + + self.assertFalse(mock_copy_to_image.called) + vol_db = objects.Volume.get_by_id(self.context, volume.id) + self.assertEqual('in-use', vol_db.status) + self.assertIsNone(vol_db.previous_status) + + CONF.set_default('enable_force_upload', True) + res_dict = self.controller._volume_upload_image(req, volume.id, body) + + self.assertDictMatch(expected, res_dict) + + vol_db = objects.Volume.get_by_id(self.context, volume.id) + self.assertEqual('uploading', vol_db.status) + self.assertEqual('in-use', vol_db.previous_status) + + @mock.patch.object(volume_api.API, "get_volume_image_metadata") + @mock.patch.object(glance.GlanceImageService, "create") + @mock.patch.object(volume_rpcapi.VolumeAPI, "copy_volume_to_image") + def test_copy_volume_to_image_without_protected_prop( + self, mock_volume_to_image, mock_create, mock_get_image_metadata): + """Test protected property is not defined with the root image.""" + volume, expected = self._create_volume_with_type() + + mock_get_image_metadata.return_value = {} + mock_create.side_effect = self.fake_image_service_create + + req = fakes.HTTPRequest.blank( + '/v2/fakeproject/volumes/%s/action' % volume.id, + use_admin_context=self.context.is_admin) + + body = self._get_os_volume_upload_image() + res_dict = self.controller._volume_upload_image(req, volume.id, body) + + self.assertDictMatch(expected, res_dict) + vol_db = objects.Volume.get_by_id(self.context, volume.id) + self.assertEqual('uploading', vol_db.status) + self.assertEqual('available', vol_db.previous_status) + + @mock.patch.object(glance.GlanceImageService, "create") + @mock.patch.object(volume_rpcapi.VolumeAPI, "copy_volume_to_image") + def test_copy_volume_to_image_without_core_prop( + self, mock_copy_to_image, mock_create): + """Test glance_core_properties defined in cinder.conf is empty.""" + volume, expected = self._create_volume_with_type() + mock_create.side_effect = self.fake_image_service_create + + self.override_config('glance_core_properties', []) + + req = fakes.HTTPRequest.blank( + '/v2/fakeproject/volumes/%s/action' % volume.id, + use_admin_context=self.context.is_admin) + + body = self._get_os_volume_upload_image() + res_dict = self.controller._volume_upload_image(req, volume.id, body) + + self.assertDictMatch(expected, res_dict) + vol_db = objects.Volume.get_by_id(self.context, volume.id) + self.assertEqual('uploading', vol_db.status) + self.assertEqual('available', vol_db.previous_status) @mock.patch.object(volume_api.API, "get_volume_image_metadata") @mock.patch.object(glance.GlanceImageService, "create") - @mock.patch.object(volume_api.API, "get") - @mock.patch.object(volume_api.API, "update") @mock.patch.object(volume_rpcapi.VolumeAPI, "copy_volume_to_image") def test_copy_volume_to_image_volume_type_none( self, mock_copy_volume_to_image, - mock_update, - mock_get, mock_create, mock_get_volume_image_metadata): """Test create image from volume with none type volume.""" - - db_volume = fake_volume.fake_db_volume() - volume_obj = objects.Volume._from_db_object(self.context, - objects.Volume(), - db_volume) + volume, expected = self._create_volume_with_type() mock_create.side_effect = self.fake_image_service_create - mock_get.return_value = volume_obj - mock_copy_volume_to_image.side_effect = ( - self.fake_rpc_copy_volume_to_image) - req = fakes.HTTPRequest.blank('/v2/%s/volumes/%s/action' % - (fake.PROJECT_ID, id)) + req = fakes.HTTPRequest.blank( + '/v2/%s/volumes/%s/action' % (fake.PROJECT_ID, volume.id), + use_admin_context=self.context.is_admin) body = self._get_os_volume_upload_image() - res_dict = self.controller._volume_upload_image(req, id, body) - expected_res = { - 'os-volume_upload_image': { - 'id': fake.VOLUME_ID, - 'updated_at': None, - 'status': 'uploading', - 'display_description': None, - 'size': 1, - 'volume_type': None, - 'image_id': fake.IMAGE_ID, - 'container_format': u'bare', - 'disk_format': u'raw', - 'image_name': u'image_name' - } - } - - self.assertDictMatch(expected_res, res_dict) + res_dict = self.controller._volume_upload_image(req, volume.id, body) + self.assertDictMatch(expected, res_dict) @mock.patch.object(volume_api.API, "get_volume_image_metadata") @mock.patch.object(glance.GlanceImageService, "create") - @mock.patch.object(volume_api.API, "update") @mock.patch.object(volume_rpcapi.VolumeAPI, "copy_volume_to_image") def test_copy_volume_to_image_version_3_1( self, mock_copy_volume_to_image, - mock_update, mock_create, mock_get_volume_image_metadata): """Test create image from volume with protected properties.""" - id = 'aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee' + volume, expected = self._create_volume_with_type() mock_get_volume_image_metadata.return_value = { - "volume_id": id, + "volume_id": volume.id, "key": "x_billing_code_license", "value": "246254365"} mock_create.side_effect = self.fake_image_service_create_3_1 - mock_update.side_effect = stubs.stub_volume_update mock_copy_volume_to_image.side_effect = \ self.fake_rpc_copy_volume_to_image self.override_config('glance_api_version', 2) - req = fakes.HTTPRequest.blank('/v3/tenant1/volumes/%s/action' % id) + req = fakes.HTTPRequest.blank( + '/v3/%s/volumes/%s/action' % (fake.PROJECT_ID, volume.id), + use_admin_context=self.context.is_admin) req.environ['cinder.context'].is_admin = True req.headers = {'OpenStack-API-Version': 'volume 3.1'} req.api_version_request = api_version.APIVersionRequest('3.1') @@ -1149,26 +1125,9 @@ class VolumeImageActionsTest(test.TestCase): body['os-volume_upload_image']['visibility'] = 'public' body['os-volume_upload_image']['protected'] = True res_dict = self.controller._volume_upload_image(req, - id, + volume.id, body) - expected_res = { - 'os-volume_upload_image': { - 'id': id, - 'updated_at': datetime.datetime( - 1900, 1, 1, 1, 1, 1, - tzinfo=iso8601.iso8601.Utc()), - 'status': 'uploading', - 'display_description': 'displaydesc', - 'size': 1, - 'visibility': 'public', - 'protected': True, - 'volume_type': fake_volume.fake_db_volume_type( - name='vol_type_name'), - 'image_id': 1, - 'container_format': 'bare', - 'disk_format': 'raw', - 'image_name': 'image_name' - } - } - self.assertDictMatch(expected_res, res_dict) + expected['os-volume_upload_image'].update(visibility='public', + protected=True) + self.assertDictMatch(expected, res_dict) diff --git a/cinder/tests/unit/api/v1/test_volume_metadata.py b/cinder/tests/unit/api/v1/test_volume_metadata.py index 9348c619627..31fcc32a42b 100644 --- a/cinder/tests/unit/api/v1/test_volume_metadata.py +++ b/cinder/tests/unit/api/v1/test_volume_metadata.py @@ -23,6 +23,7 @@ from cinder.api.v1 import volume_metadata from cinder.api.v1 import volumes import cinder.db from cinder import exception as exc +from cinder import objects from cinder import test from cinder.tests.unit.api import fakes from cinder.tests.unit.api.v1 import stubs @@ -211,7 +212,7 @@ class volumeMetaDataTest(test.TestCase): @mock.patch.object(cinder.db, 'volume_metadata_delete') @mock.patch.object(cinder.db, 'volume_metadata_get') def test_delete(self, metadata_get, metadata_delete): - fake_volume = {'id': fake.VOLUME_ID, 'status': 'available'} + fake_volume = objects.Volume(id=fake.VOLUME_ID, status='available') fake_context = mock.Mock() metadata_get.side_effect = return_volume_metadata metadata_delete.side_effect = delete_volume_metadata diff --git a/cinder/tests/unit/api/v2/test_volume_metadata.py b/cinder/tests/unit/api/v2/test_volume_metadata.py index 0c07b3e63fe..80e9c85d346 100644 --- a/cinder/tests/unit/api/v2/test_volume_metadata.py +++ b/cinder/tests/unit/api/v2/test_volume_metadata.py @@ -25,6 +25,7 @@ from cinder.api.v2 import volume_metadata from cinder.api.v2 import volumes from cinder import db from cinder import exception +from cinder import objects from cinder import test from cinder.tests.unit.api import fakes from cinder.tests.unit.api.v2 import stubs @@ -205,7 +206,7 @@ class volumeMetaDataTest(test.TestCase): @mock.patch.object(db, 'volume_metadata_delete') @mock.patch.object(db, 'volume_metadata_get') def test_delete(self, metadata_get, metadata_delete): - fake_volume = {'id': self.req_id, 'status': 'available'} + fake_volume = objects.Volume(id=self.req_id, status='available') fake_context = mock.Mock() metadata_get.side_effect = return_volume_metadata metadata_delete.side_effect = delete_volume_metadata @@ -223,7 +224,7 @@ class volumeMetaDataTest(test.TestCase): @mock.patch.object(db, 'volume_metadata_delete') @mock.patch.object(db, 'volume_metadata_get') def test_delete_volume_maintenance(self, metadata_get, metadata_delete): - fake_volume = {'id': self.req_id, 'status': 'maintenance'} + fake_volume = objects.Volume(id=self.req_id, status='maintenance') fake_context = mock.Mock() metadata_get.side_effect = return_volume_metadata metadata_delete.side_effect = delete_volume_metadata @@ -242,7 +243,7 @@ class volumeMetaDataTest(test.TestCase): @mock.patch.object(db, 'volume_metadata_delete') @mock.patch.object(db, 'volume_metadata_get') def test_delete_nonexistent_volume(self, metadata_get, metadata_delete): - fake_volume = {'id': self.req_id, 'status': 'available'} + fake_volume = objects.Volume(id=self.req_id, status='available') fake_context = mock.Mock() metadata_get.side_effect = return_volume_metadata metadata_delete.side_effect = return_volume_nonexistent diff --git a/cinder/tests/unit/test_volume.py b/cinder/tests/unit/test_volume.py index 36073b9121f..11ef53e1794 100644 --- a/cinder/tests/unit/test_volume.py +++ b/cinder/tests/unit/test_volume.py @@ -3801,10 +3801,7 @@ class VolumeTestCase(BaseVolumeTestCase): """Test volume can be extended at API level.""" # create a volume and assign to host volume = tests_utils.create_volume(self.context, size=2, - status='creating', host=CONF.host) - self.volume.create_volume(self.context, volume['id']) - volume['status'] = 'in-use' - + status='in-use', host=CONF.host) volume_api = cinder.volume.api.API() # Extend fails when status != available @@ -3814,7 +3811,7 @@ class VolumeTestCase(BaseVolumeTestCase): volume, 3) - volume['status'] = 'available' + db.volume_update(self.context, volume.id, {'status': 'available'}) # Extend fails when new_size < orig_size self.assertRaises(exception.InvalidInput, volume_api.extend, @@ -3832,13 +3829,13 @@ class VolumeTestCase(BaseVolumeTestCase): # works when new_size > orig_size reserve.return_value = ["RESERVATION"] volume_api.extend(self.context, volume, 3) - volume = db.volume_get(context.get_admin_context(), volume['id']) - self.assertEqual('extending', volume['status']) + volume_db = db.volume_get(context.get_admin_context(), volume['id']) + self.assertEqual('extending', volume_db['status']) reserve.assert_called_once_with(self.context, gigabytes=1, - project_id=volume['project_id']) + project_id=volume.project_id) # Test the quota exceeded - volume['status'] = 'available' + db.volume_update(self.context, volume.id, {'status': 'available'}) reserve.side_effect = exception.OverQuota(overs=['gigabytes'], quotas={'gigabytes': 20}, usages={'gigabytes': @@ -3922,7 +3919,8 @@ class VolumeTestCase(BaseVolumeTestCase): # clean up self.volume.delete_volume(self.context, volume['id']) - def test_extend_volume_with_volume_type(self): + @mock.patch('cinder.volume.rpcapi.VolumeAPI.extend_volume') + def test_extend_volume_with_volume_type(self, mock_rpc_extend): elevated = context.get_admin_context() project_id = self.context.project_id db.volume_type_create(elevated, {'name': 'type', 'extra_specs': {}}) @@ -3937,11 +3935,10 @@ class VolumeTestCase(BaseVolumeTestCase): except exception.QuotaUsageNotFound: volumes_in_use = 0 self.assertEqual(100, volumes_in_use) - volume['status'] = 'available' - volume['host'] = 'fakehost' - volume['volume_type_id'] = vol_type.get('id') + db.volume_update(self.context, volume.id, {'status': 'available'}) volume_api.extend(self.context, volume, 200) + mock_rpc_extend.called_once_with(self.context, volume, 200, mock.ANY) try: usage = db.quota_usage_get(elevated, project_id, 'gigabytes_type') diff --git a/cinder/volume/api.py b/cinder/volume/api.py index a50d4d4ab17..16b2a09a974 100644 --- a/cinder/volume/api.py +++ b/cinder/volume/api.py @@ -973,13 +973,12 @@ class API(base.Base): def delete_volume_metadata(self, context, volume, key, meta_type=common.METADATA_TYPES.user): """Delete the given metadata item from a volume.""" - if volume['status'] == 'maintenance': - LOG.info(_LI('Unable to delete the volume metadata, ' - 'because it is in maintenance.'), resource=volume) - msg = _("The volume metadata cannot be deleted when the volume " - "is in maintenance mode.") + if volume.status in ('maintenance', 'uploading'): + msg = _('Deleting volume metadata is not allowed for volumes in ' + '%s status.') % volume.status + LOG.info(msg, resource=volume) raise exception.InvalidVolume(reason=msg) - self.db.volume_metadata_delete(context, volume['id'], key, meta_type) + self.db.volume_metadata_delete(context, volume.id, key, meta_type) LOG.info(_LI("Delete volume metadata completed successfully."), resource=volume) @@ -1011,11 +1010,10 @@ class API(base.Base): `metadata` argument will be deleted. """ - if volume['status'] == 'maintenance': - LOG.info(_LI('Unable to update the metadata for volume, ' - 'because it is in maintenance.'), resource=volume) - msg = _("The volume metadata cannot be updated when the volume " - "is in maintenance mode.") + if volume['status'] in ('maintenance', 'uploading'): + msg = _('Updating volume metadata is not allowed for volumes in ' + '%s status.') % volume['status'] + LOG.info(msg, resource=volume) raise exception.InvalidVolume(reason=msg) self._check_metadata_properties(metadata) db_meta = self.db.volume_metadata_update(context, volume['id'], @@ -1132,51 +1130,57 @@ class API(base.Base): meta_entry['value']}) return results - def _check_volume_availability(self, volume, force): - """Check if the volume can be used.""" - if volume['status'] not in ['available', 'in-use']: - msg = _('Volume %(vol_id)s status must be ' - 'available or in-use, but current status is: ' - '%(vol_status)s.') % {'vol_id': volume['id'], - 'vol_status': volume['status']} - raise exception.InvalidVolume(reason=msg) - if not force and 'in-use' == volume['status']: - msg = _('Volume status is in-use.') - raise exception.InvalidVolume(reason=msg) - @wrap_check_policy def copy_volume_to_image(self, context, volume, metadata, force): """Create a new image from the specified volume.""" - if not CONF.enable_force_upload and force: LOG.info(_LI("Force upload to image is disabled, " "Force option will be ignored."), resource={'type': 'volume', 'id': volume['id']}) force = False - self._check_volume_availability(volume, force) - glance_core_properties = CONF.glance_core_properties - if glance_core_properties: - try: - volume_image_metadata = self.get_volume_image_metadata(context, - volume) - custom_property_set = (set(volume_image_metadata).difference - (set(glance_core_properties))) - if custom_property_set: - properties = {custom_property: - volume_image_metadata[custom_property] - for custom_property in custom_property_set} - metadata.update(dict(properties=properties)) - except exception.GlanceMetadataNotFound: - # If volume is not created from image, No glance metadata - # would be available for that volume in - # volume glance metadata table + # Build required conditions for conditional update + expected = {'status': ('available', 'in-use') if force + else 'available'} + values = {'status': 'uploading', + 'previous_status': volume.model.status} - pass + result = volume.conditional_update(values, expected) + if not result: + msg = (_('Volume %(vol_id)s status must be %(statuses)s') % + {'vol_id': volume.id, + 'statuses': utils.build_or_str(expected['status'])}) + raise exception.InvalidVolume(reason=msg) + + try: + glance_core_props = CONF.glance_core_properties + if glance_core_props: + try: + vol_img_metadata = self.get_volume_image_metadata( + context, volume) + custom_property_set = ( + set(vol_img_metadata).difference(glance_core_props)) + if custom_property_set: + metadata['properties'] = { + custom_prop: vol_img_metadata[custom_prop] + for custom_prop in custom_property_set} + except exception.GlanceMetadataNotFound: + # If volume is not created from image, No glance metadata + # would be available for that volume in + # volume glance metadata table + pass + + recv_metadata = self.image_service.create( + context, self.image_service._translate_to_glance(metadata)) + except Exception: + # NOTE(geguileo): To mimic behavior before conditional_update we + # will rollback status if image create fails + with excutils.save_and_reraise_exception(): + volume.conditional_update( + {'status': volume.model.previous_status, + 'previous_status': None}, + {'status': 'uploading'}) - recv_metadata = self.image_service.create( - context, self.image_service._translate_to_glance(metadata)) - self.update(context, volume, {'status': 'uploading'}) self.volume_rpcapi.copy_volume_to_image(context, volume, recv_metadata) @@ -1203,12 +1207,16 @@ class API(base.Base): @wrap_check_policy def extend(self, context, volume, new_size): - if volume.status != 'available': - msg = _('Volume %(vol_id)s status must be available ' - 'to extend, but current status is: ' - '%(vol_status)s.') % {'vol_id': volume.id, - 'vol_status': volume.status} - raise exception.InvalidVolume(reason=msg) + value = {'status': 'extending'} + expected = {'status': 'available'} + + def _roll_back_status(): + msg = _LE('Could not return volume %s to available.') + try: + if not volume.conditional_update(expected, value): + LOG.error(msg, volume.id) + except Exception: + LOG.exception(msg, volume.id) size_increase = (int(new_size)) - volume.size if size_increase <= 0: @@ -1218,16 +1226,30 @@ class API(base.Base): 'size': volume.size}) raise exception.InvalidInput(reason=msg) + result = volume.conditional_update(value, expected) + if not result: + msg = _('Volume %(vol_id)s status must be available to extend.') + raise exception.InvalidVolume(reason=msg) + + rollback = True try: values = {'per_volume_gigabytes': new_size} QUOTAS.limit_check(context, project_id=context.project_id, **values) + rollback = False except exception.OverQuota as e: quotas = e.kwargs['quotas'] raise exception.VolumeSizeExceedsLimit( size=new_size, limit=quotas['per_volume_gigabytes']) + finally: + # NOTE(geguileo): To mimic behavior before conditional_update we + # will rollback status on quota reservation failure regardless of + # the exception that caused the failure. + if rollback: + _roll_back_status() try: + reservations = None reserve_opts = {'gigabytes': size_increase} QUOTAS.add_volume_type_opts(context, reserve_opts, volume.volume_type_id) @@ -1235,25 +1257,26 @@ class API(base.Base): project_id=volume.project_id, **reserve_opts) except exception.OverQuota as exc: - usages = exc.kwargs['usages'] - quotas = exc.kwargs['quotas'] - - def _consumed(name): - return (usages[name]['reserved'] + usages[name]['in_use']) + gigabytes = exc.kwargs['usages']['gigabytes'] + gb_quotas = exc.kwargs['quotas']['gigabytes'] + consumed = gigabytes['reserved'] + gigabytes['in_use'] msg = _LE("Quota exceeded for %(s_pid)s, tried to extend volume " "by %(s_size)sG, (%(d_consumed)dG of %(d_quota)dG " "already consumed).") LOG.error(msg, {'s_pid': context.project_id, 's_size': size_increase, - 'd_consumed': _consumed('gigabytes'), - 'd_quota': quotas['gigabytes']}) + 'd_consumed': consumed, + 'd_quota': gb_quotas}) raise exception.VolumeSizeExceedsAvailableQuota( - requested=size_increase, - consumed=_consumed('gigabytes'), - quota=quotas['gigabytes']) + requested=size_increase, consumed=consumed, quota=gb_quotas) + finally: + # NOTE(geguileo): To mimic behavior before conditional_update we + # will rollback status on quota reservation failure regardless of + # the exception that caused the failure. + if reservations is None: + _roll_back_status() - self.update(context, volume, {'status': 'extending'}) self.volume_rpcapi.extend_volume(context, volume, new_size, reservations) LOG.info(_LI("Extend volume request issued successfully."),