From 032032f0c329b4eddcbf2185399396ff4fd5f00d Mon Sep 17 00:00:00 2001 From: Thang Pham Date: Tue, 27 Oct 2015 15:38:54 -0700 Subject: [PATCH] Update create_volume API to use versionedobjects The following patch updates create_volume API to use volume versionedobjects. Changes were made to be backwards compatible with older RPC clients. It only includes changes to the core cinder code. Changes in the drivers are left to each driver maintainer to update. Note that this patch DOES NOT try to use object dot notation everywhere, since it would increase the size of the patch. Instead, it will be done in subsequent patches. Co-Authored-By: Michal Dulko Change-Id: Ic1b0f09132f8fc500b29650abbd57f18ea8bd9dd Partial-Implements: blueprint cinder-objects Change-Id: Ief9c63e8bddb2b40bdef4465b9099cff33d7c3bc --- cinder/api/v1/volumes.py | 5 - cinder/api/v2/volumes.py | 4 - cinder/scheduler/driver.py | 12 ++- cinder/scheduler/flows/create_volume.py | 38 +++---- cinder/scheduler/manager.py | 14 ++- cinder/scheduler/rpcapi.py | 28 +++-- cinder/tests/unit/scheduler/test_rpcapi.py | 21 +++- cinder/tests/unit/scheduler/test_scheduler.py | 56 +++++----- cinder/tests/unit/test_rbd.py | 36 +++---- cinder/tests/unit/test_storwize_svc.py | 14 +-- cinder/tests/unit/test_volume.py | 84 +++++++-------- cinder/tests/unit/test_volume_rpcapi.py | 41 ++++++- cinder/tests/unit/test_volume_transfer.py | 54 +++++----- cinder/tests/unit/utils.py | 18 ++-- .../unit/volume/flows/fake_volume_api.py | 4 +- .../volume/flows/test_create_volume_flow.py | 76 ++++++------- cinder/volume/api.py | 5 +- cinder/volume/flows/api/create_volume.py | 40 ++++--- cinder/volume/flows/manager/create_volume.py | 101 +++++++++--------- cinder/volume/manager.py | 27 +++-- cinder/volume/rpcapi.py | 25 +++-- tools/lintstack.py | 2 + 22 files changed, 389 insertions(+), 316 deletions(-) diff --git a/cinder/api/v1/volumes.py b/cinder/api/v1/volumes.py index ba9fdf396..ce594284b 100644 --- a/cinder/api/v1/volumes.py +++ b/cinder/api/v1/volumes.py @@ -398,11 +398,6 @@ class VolumeController(wsgi.Controller): volume.get('display_description'), **kwargs) - # TODO(vish): Instance should be None at db layer instead of - # trying to lazy load, but for now we turn it into - # a dict to avoid an error. - new_volume = dict(new_volume) - retval = _translate_volume_detail_view(context, new_volume, image_uuid) return {'volume': retval} diff --git a/cinder/api/v2/volumes.py b/cinder/api/v2/volumes.py index eec160bff..9f23290c0 100644 --- a/cinder/api/v2/volumes.py +++ b/cinder/api/v2/volumes.py @@ -418,10 +418,6 @@ class VolumeController(wsgi.Controller): volume.get('display_description'), **kwargs) - # TODO(vish): Instance should be None at db layer instead of - # trying to lazy load, but for now we turn it into - # a dict to avoid an error. - new_volume = dict(new_volume) retval = self._view_builder.detail(req, new_volume) return retval diff --git a/cinder/scheduler/driver.py b/cinder/scheduler/driver.py index eb31fa1a0..610808a29 100644 --- a/cinder/scheduler/driver.py +++ b/cinder/scheduler/driver.py @@ -23,8 +23,8 @@ from oslo_config import cfg from oslo_utils import importutils from oslo_utils import timeutils -from cinder import db from cinder.i18n import _ +from cinder import objects from cinder.volume import rpcapi as volume_rpcapi @@ -46,8 +46,14 @@ def volume_update_db(context, volume_id, host): :returns: A Volume with the updated fields set properly. """ - values = {'host': host, 'scheduled_at': timeutils.utcnow()} - return db.volume_update(context, volume_id, values) + volume = objects.Volume.get_by_id(context, volume_id) + volume.host = host + volume.scheduled_at = timeutils.utcnow() + volume.save() + + # A volume object is expected to be returned, as it is used by + # filter_scheduler. + return volume def group_update_db(context, group, host): diff --git a/cinder/scheduler/flows/create_volume.py b/cinder/scheduler/flows/create_volume.py index 70e952761..0f917a37f 100644 --- a/cinder/scheduler/flows/create_volume.py +++ b/cinder/scheduler/flows/create_volume.py @@ -17,7 +17,7 @@ from taskflow.patterns import linear_flow from cinder import exception from cinder import flow_utils -from cinder.i18n import _, _LE +from cinder.i18n import _LE from cinder import rpc from cinder import utils from cinder.volume.flows import common @@ -40,39 +40,33 @@ class ExtractSchedulerSpecTask(flow_utils.CinderTask): **kwargs) self.db_api = db_api - def _populate_request_spec(self, context, volume_id, snapshot_id, + def _populate_request_spec(self, context, volume, snapshot_id, image_id): - # Create the full request spec using the volume_id. + # Create the full request spec using the volume object. # - # NOTE(harlowja): this will fetch the volume from the database, if - # the volume has been deleted before we got here then this should fail. - # - # In the future we might want to have a lock on the volume_id so that - # the volume can not be deleted while its still being created? - if not volume_id: - raise exception.InvalidInput( - reason=_("No volume_id provided to populate a " - "request_spec from")) - volume_ref = self.db_api.volume_get(context, volume_id) - volume_type_id = volume_ref.get('volume_type_id') - vol_type = self.db_api.volume_type_get(context, volume_type_id) + # NOTE(dulek): At this point, a volume can be deleted before it gets + # scheduled. If a delete API call is made, the volume gets instantly + # delete and scheduling will fail when it tries to update the DB entry + # (with the host) in ScheduleCreateVolumeTask below. + volume_type_id = volume.volume_type_id + vol_type = volume.volume_type return { - 'volume_id': volume_id, + 'volume_id': volume.id, 'snapshot_id': snapshot_id, 'image_id': image_id, 'volume_properties': { - 'size': utils.as_int(volume_ref.get('size'), quiet=False), - 'availability_zone': volume_ref.get('availability_zone'), + 'size': utils.as_int(volume.size, quiet=False), + 'availability_zone': volume.availability_zone, 'volume_type_id': volume_type_id, }, 'volume_type': list(dict(vol_type).items()), } - def execute(self, context, request_spec, volume_id, snapshot_id, + def execute(self, context, request_spec, volume, snapshot_id, image_id): # For RPC version < 1.2 backward compatibility if request_spec is None: - request_spec = self._populate_request_spec(context, volume_id, + request_spec = self._populate_request_spec(context, volume.id, snapshot_id, image_id) return { 'request_spec': request_spec, @@ -143,7 +137,7 @@ class ScheduleCreateVolumeTask(flow_utils.CinderTask): def get_flow(context, db_api, driver_api, request_spec=None, filter_properties=None, - volume_id=None, snapshot_id=None, image_id=None): + volume=None, snapshot_id=None, image_id=None): """Constructs and returns the scheduler entrypoint flow. @@ -158,7 +152,7 @@ def get_flow(context, db_api, driver_api, request_spec=None, 'context': context, 'raw_request_spec': request_spec, 'filter_properties': filter_properties, - 'volume_id': volume_id, + 'volume': volume, 'snapshot_id': snapshot_id, 'image_id': image_id, } diff --git a/cinder/scheduler/manager.py b/cinder/scheduler/manager.py index 542c04c0c..f924896b1 100644 --- a/cinder/scheduler/manager.py +++ b/cinder/scheduler/manager.py @@ -33,6 +33,7 @@ from cinder import exception from cinder import flow_utils from cinder.i18n import _, _LE from cinder import manager +from cinder import objects from cinder import quota from cinder import rpc from cinder.scheduler.flows import create_volume @@ -55,7 +56,7 @@ LOG = logging.getLogger(__name__) class SchedulerManager(manager.Manager): """Chooses a host to create volumes.""" - RPC_API_VERSION = '1.8' + RPC_API_VERSION = '1.9' target = messaging.Target(version=RPC_API_VERSION) @@ -116,15 +117,22 @@ class SchedulerManager(manager.Manager): def create_volume(self, context, topic, volume_id, snapshot_id=None, image_id=None, request_spec=None, - filter_properties=None): + filter_properties=None, volume=None): self._wait_for_scheduler() + + # FIXME(thangp): Remove this in v2.0 of RPC API. + if volume is None: + # For older clients, mimic the old behavior and look up the + # volume by its volume_id. + volume = objects.Volume.get_by_id(context, volume_id) + try: flow_engine = create_volume.get_flow(context, db, self.driver, request_spec, filter_properties, - volume_id, + volume, snapshot_id, image_id) except Exception: diff --git a/cinder/scheduler/rpcapi.py b/cinder/scheduler/rpcapi.py index d21080102..eafc46656 100644 --- a/cinder/scheduler/rpcapi.py +++ b/cinder/scheduler/rpcapi.py @@ -42,6 +42,7 @@ class SchedulerAPI(object): 1.6 - Add create_consistencygroup method 1.7 - Add get_active_pools method 1.8 - Add sending object over RPC in create_consistencygroup method + 1.9 - Adds support for sending objects over RPC in create_volume() """ RPC_API_VERSION = '1.0' @@ -51,7 +52,10 @@ class SchedulerAPI(object): target = messaging.Target(topic=CONF.scheduler_topic, version=self.RPC_API_VERSION) serializer = objects_base.CinderObjectSerializer() - self.client = rpc.get_client(target, version_cap='1.8', + + # NOTE(thangp): Until version pinning is impletemented, set the client + # version_cap to None + self.client = rpc.get_client(target, version_cap=None, serializer=serializer) def create_consistencygroup(self, ctxt, topic, group, @@ -72,17 +76,21 @@ class SchedulerAPI(object): def create_volume(self, ctxt, topic, volume_id, snapshot_id=None, image_id=None, request_spec=None, - filter_properties=None): + filter_properties=None, volume=None): - cctxt = self.client.prepare(version='1.2') request_spec_p = jsonutils.to_primitive(request_spec) - return cctxt.cast(ctxt, 'create_volume', - topic=topic, - volume_id=volume_id, - snapshot_id=snapshot_id, - image_id=image_id, - request_spec=request_spec_p, - filter_properties=filter_properties) + msg_args = {'topic': topic, 'volume_id': volume_id, + 'snapshot_id': snapshot_id, 'image_id': image_id, + 'request_spec': request_spec_p, + 'filter_properties': filter_properties} + if self.client.can_send_version('1.9'): + version = '1.9' + msg_args['volume'] = volume + else: + version = '1.2' + + cctxt = self.client.prepare(version=version) + return cctxt.cast(ctxt, 'create_volume', **msg_args) def migrate_volume_to_host(self, ctxt, topic, volume_id, host, force_host_copy=False, request_spec=None, diff --git a/cinder/tests/unit/scheduler/test_rpcapi.py b/cinder/tests/unit/scheduler/test_rpcapi.py index c76617b74..abb68c58c 100644 --- a/cinder/tests/unit/scheduler/test_rpcapi.py +++ b/cinder/tests/unit/scheduler/test_rpcapi.py @@ -87,7 +87,25 @@ class SchedulerRpcAPITestCase(test.TestCase): capabilities='fake_capabilities', fanout=True) - def test_create_volume(self): + @mock.patch('oslo_messaging.RPCClient.can_send_version', + return_value=True) + def test_create_volume(self, can_send_version): + self._test_scheduler_api('create_volume', + rpc_method='cast', + topic='topic', + volume_id='volume_id', + snapshot_id='snapshot_id', + image_id='image_id', + request_spec='fake_request_spec', + filter_properties='filter_properties', + volume='volume', + version='1.9') + can_send_version.assert_called_once_with('1.9') + + @mock.patch('oslo_messaging.RPCClient.can_send_version', + return_value=False) + def test_create_volume_old(self, can_send_version): + # Tests backwards compatibility with older clients self._test_scheduler_api('create_volume', rpc_method='cast', topic='topic', @@ -97,6 +115,7 @@ class SchedulerRpcAPITestCase(test.TestCase): request_spec='fake_request_spec', filter_properties='filter_properties', version='1.2') + can_send_version.assert_called_once_with('1.9') def test_migrate_volume_to_host(self): self._test_scheduler_api('migrate_volume_to_host', diff --git a/cinder/tests/unit/scheduler/test_scheduler.py b/cinder/tests/unit/scheduler/test_scheduler.py index 8e1e8cca7..0a35ce089 100644 --- a/cinder/tests/unit/scheduler/test_scheduler.py +++ b/cinder/tests/unit/scheduler/test_scheduler.py @@ -28,6 +28,7 @@ from cinder.scheduler import filter_scheduler from cinder.scheduler import manager from cinder import test from cinder.tests.unit import fake_consistencygroup +from cinder.tests.unit import fake_volume from cinder.tests.unit import utils as tests_utils CONF = cfg.CONF @@ -100,15 +101,16 @@ class SchedulerManagerTestCase(test.TestCase): # Test NoValidHost exception behavior for create_volume. # Puts the volume in 'error' state and eats the exception. _mock_sched_create.side_effect = exception.NoValidHost(reason="") - fake_volume_id = 1 + volume = fake_volume.fake_volume_obj(self.context) topic = 'fake_topic' - request_spec = {'volume_id': fake_volume_id} + request_spec = {'volume_id': volume.id} - self.manager.create_volume(self.context, topic, fake_volume_id, + self.manager.create_volume(self.context, topic, volume.id, request_spec=request_spec, - filter_properties={}) + filter_properties={}, + volume=volume) _mock_volume_update.assert_called_once_with(self.context, - fake_volume_id, + volume.id, {'status': 'error'}) _mock_sched_create.assert_called_once_with(self.context, request_spec, {}) @@ -116,14 +118,15 @@ class SchedulerManagerTestCase(test.TestCase): @mock.patch('cinder.scheduler.driver.Scheduler.schedule_create_volume') @mock.patch('eventlet.sleep') def test_create_volume_no_delay(self, _mock_sleep, _mock_sched_create): - fake_volume_id = 1 + volume = fake_volume.fake_volume_obj(self.context) topic = 'fake_topic' - request_spec = {'volume_id': fake_volume_id} + request_spec = {'volume_id': volume.id} - self.manager.create_volume(self.context, topic, fake_volume_id, + self.manager.create_volume(self.context, topic, volume.id, request_spec=request_spec, - filter_properties={}) + filter_properties={}, + volume=volume) _mock_sched_create.assert_called_once_with(self.context, request_spec, {}) self.assertFalse(_mock_sleep.called) @@ -135,16 +138,17 @@ class SchedulerManagerTestCase(test.TestCase): _mock_is_ready, _mock_sched_create): self.manager._startup_delay = True - fake_volume_id = 1 + volume = fake_volume.fake_volume_obj(self.context) topic = 'fake_topic' - request_spec = {'volume_id': fake_volume_id} + request_spec = {'volume_id': volume.id} _mock_is_ready.side_effect = [False, False, True] - self.manager.create_volume(self.context, topic, fake_volume_id, + self.manager.create_volume(self.context, topic, volume.id, request_spec=request_spec, - filter_properties={}) + filter_properties={}, + volume=volume) _mock_sched_create.assert_called_once_with(self.context, request_spec, {}) calls = [mock.call(1)] * 2 @@ -158,16 +162,17 @@ class SchedulerManagerTestCase(test.TestCase): _mock_is_ready, _mock_sched_create): self.manager._startup_delay = True - fake_volume_id = 1 + volume = fake_volume.fake_volume_obj(self.context) topic = 'fake_topic' - request_spec = {'volume_id': fake_volume_id} + request_spec = {'volume_id': volume.id} _mock_is_ready.return_value = True - self.manager.create_volume(self.context, topic, fake_volume_id, + self.manager.create_volume(self.context, topic, volume.id, request_spec=request_spec, - filter_properties={}) + filter_properties={}, + volume=volume) _mock_sched_create.assert_called_once_with(self.context, request_spec, {}) self.assertFalse(_mock_sleep.called) @@ -346,10 +351,13 @@ class SchedulerDriverModuleTestCase(test.TestCase): self.context = context.RequestContext('fake_user', 'fake_project') @mock.patch('cinder.db.volume_update') - @mock.patch('oslo_utils.timeutils.utcnow') - def test_volume_host_update_db(self, _mock_utcnow, _mock_vol_update): - _mock_utcnow.return_value = 'fake-now' - driver.volume_update_db(self.context, 31337, 'fake_host') - _mock_vol_update.assert_called_once_with(self.context, 31337, - {'host': 'fake_host', - 'scheduled_at': 'fake-now'}) + @mock.patch('cinder.objects.volume.Volume.get_by_id') + def test_volume_host_update_db(self, _mock_volume_get, _mock_vol_update): + volume = fake_volume.fake_volume_obj(self.context) + _mock_volume_get.return_value = volume + + driver.volume_update_db(self.context, volume.id, 'fake_host') + scheduled_at = volume.scheduled_at.replace(tzinfo=None) + _mock_vol_update.assert_called_once_with( + self.context, volume.id, {'host': 'fake_host', + 'scheduled_at': scheduled_at}) diff --git a/cinder/tests/unit/test_rbd.py b/cinder/tests/unit/test_rbd.py index bb60f4aa8..f0c555673 100644 --- a/cinder/tests/unit/test_rbd.py +++ b/cinder/tests/unit/test_rbd.py @@ -21,13 +21,12 @@ import os import tempfile import mock -from oslo_utils import timeutils from oslo_utils import units -from cinder import db from cinder import exception from cinder.i18n import _ from cinder.image import image_utils +from cinder import objects from cinder import test from cinder.tests.unit.image import fake as fake_image from cinder.tests.unit import test_volume @@ -1090,7 +1089,6 @@ class ManagedRBDTestCase(test_volume.DriverTestCase): NOTE: if clone_error is True we force the image type to raw otherwise clone_image is not called """ - volume_id = 1 # See tests.image.fake for image types. if raw: @@ -1099,32 +1097,34 @@ class ManagedRBDTestCase(test_volume.DriverTestCase): image_id = 'c905cedb-7281-47e4-8a62-f26bc5fc4c77' # creating volume testdata - db.volume_create(self.context, - {'id': volume_id, - 'updated_at': timeutils.utcnow(), - 'display_description': 'Test Desc', - 'size': 20, - 'status': 'creating', - 'instance_uuid': None, - 'host': 'dummy'}) + db_volume = {'display_description': 'Test Desc', + 'size': 20, + 'status': 'creating', + 'availability_zone': 'fake_zone', + 'attach_status': 'detached', + 'host': 'dummy'} + volume = objects.Volume(context=self.context, **db_volume) + volume.create() try: if not clone_error: self.volume.create_volume(self.context, - volume_id, - request_spec={'image_id': image_id}) + volume.id, + request_spec={'image_id': image_id}, + volume=volume) else: self.assertRaises(exception.CinderException, self.volume.create_volume, self.context, - volume_id, - request_spec={'image_id': image_id}) + volume.id, + request_spec={'image_id': image_id}, + volume=volume) - volume = db.volume_get(self.context, volume_id) - self.assertEqual(expected_status, volume['status']) + volume = objects.Volume.get_by_id(self.context, volume.id) + self.assertEqual(expected_status, volume.status) finally: # cleanup - db.volume_destroy(self.context, volume_id) + volume.destroy() def test_create_vol_from_image_status_available(self): """Clone raw image then verify volume is in available state.""" diff --git a/cinder/tests/unit/test_storwize_svc.py b/cinder/tests/unit/test_storwize_svc.py index b85f683aa..aaa90b8b0 100644 --- a/cinder/tests/unit/test_storwize_svc.py +++ b/cinder/tests/unit/test_storwize_svc.py @@ -3024,16 +3024,12 @@ class StorwizeSVCDriverTestCase(test.TestCase): @mock.patch.object(storwize_svc_common.StorwizeHelpers, 'rename_vdisk') def test_storwize_update_migrated_volume(self, rename_vdisk): ctxt = testutils.get_test_admin_context() - current_volume_id = 'fake_volume_id' - original_volume_id = 'fake_original_volume_id' - current_name = 'volume-' + current_volume_id - original_name = 'volume-' + original_volume_id - backend_volume = self._create_volume(id=current_volume_id) - volume = self._create_volume(id=original_volume_id) + backend_volume = self._create_volume() + volume = self._create_volume() model_update = self.driver.update_migrated_volume(ctxt, volume, backend_volume, 'available') - rename_vdisk.assert_called_once_with(current_name, original_name) + rename_vdisk.assert_called_once_with(backend_volume.name, volume.name) self.assertEqual({'_name_id': None}, model_update) rename_vdisk.reset_mock() @@ -3041,14 +3037,14 @@ class StorwizeSVCDriverTestCase(test.TestCase): model_update = self.driver.update_migrated_volume(ctxt, volume, backend_volume, 'available') - self.assertEqual({'_name_id': current_volume_id}, model_update) + self.assertEqual({'_name_id': backend_volume.id}, model_update) rename_vdisk.reset_mock() rename_vdisk.side_effect = exception.VolumeBackendAPIException model_update = self.driver.update_migrated_volume(ctxt, volume, backend_volume, 'attached') - self.assertEqual({'_name_id': current_volume_id}, model_update) + self.assertEqual({'_name_id': backend_volume.id}, model_update) def test_storwize_vdisk_copy_ops(self): ctxt = testutils.get_test_admin_context() diff --git a/cinder/tests/unit/test_volume.py b/cinder/tests/unit/test_volume.py index 222c4a578..e06ae27d8 100644 --- a/cinder/tests/unit/test_volume.py +++ b/cinder/tests/unit/test_volume.py @@ -54,6 +54,7 @@ from cinder.tests.unit.brick import fake_lvm from cinder.tests.unit import conf_fixture from cinder.tests.unit import fake_driver from cinder.tests.unit import fake_snapshot +from cinder.tests.unit import fake_volume from cinder.tests.unit.image import fake as fake_image from cinder.tests.unit.keymgr import fake as fake_keymgr from cinder.tests.unit import utils as tests_utils @@ -516,17 +517,16 @@ class VolumeTestCase(BaseVolumeTestCase): availability_zone=CONF.storage_availability_zone, **self.volume_params) - volume_id = volume['id'] self.assertIsNone(volume['encryption_key_id']) self.assertEqual(0, len(self.notifier.notifications), self.notifier.notifications) self.assertRaises(exception.DriverNotInitialized, self.volume.delete_volume, - self.context, volume_id) + self.context, volume.id) - volume = db.volume_get(context.get_admin_context(), volume_id) + volume = objects.Volume.get_by_id(self.context, volume.id) self.assertEqual("error_deleting", volume.status) - db.volume_destroy(context.get_admin_context(), volume_id) + volume.destroy() @mock.patch('cinder.quota.QUOTAS.rollback', new=mock.Mock()) @mock.patch('cinder.quota.QUOTAS.commit', new=mock.Mock()) @@ -562,7 +562,7 @@ class VolumeTestCase(BaseVolumeTestCase): 'replication_status': 'disabled', 'replication_extended_status': None, 'replication_driver_data': None, - 'metadata': [], + 'metadata': None, 'volume_attachment': [], } self.assertDictMatch(expected, msg['payload']) @@ -580,6 +580,7 @@ class VolumeTestCase(BaseVolumeTestCase): self.assertEqual(4, len(self.notifier.notifications), self.notifier.notifications) msg = self.notifier.notifications[2] + expected['metadata'] = [] self.assertEqual('volume.delete.start', msg['event_type']) self.assertDictMatch(expected, msg['payload']) msg = self.notifier.notifications[3] @@ -597,9 +598,7 @@ class VolumeTestCase(BaseVolumeTestCase): **self.volume_params) volume_id = volume['id'] self.volume.create_volume(self.context, volume_id) - result_meta = { - volume.volume_metadata[0].key: volume.volume_metadata[0].value} - self.assertEqual(test_meta, result_meta) + self.assertEqual(test_meta, volume.metadata) self.volume.delete_volume(self.context, volume_id) self.assertRaises(exception.NotFound, @@ -629,8 +628,7 @@ class VolumeTestCase(BaseVolumeTestCase): FAKE_METADATA_TYPE = enum.Enum('METADATA_TYPES', 'fake_type') volume = tests_utils.create_volume(self.context, metadata=test_meta1, **self.volume_params) - volume_id = volume['id'] - self.volume.create_volume(self.context, volume_id) + self.volume.create_volume(self.context, volume.id, volume=volume) volume_api = cinder.volume.api.API() @@ -1558,7 +1556,6 @@ class VolumeTestCase(BaseVolumeTestCase): dst_vol = tests_utils.create_volume(self.context, source_volid=src_vol_id, **self.volume_params) - dst_vol_id = dst_vol['id'] orig_elevated = self.context.elevated @@ -1571,7 +1568,7 @@ class VolumeTestCase(BaseVolumeTestCase): # we expect this to block and then fail t = eventlet.spawn(self.volume.create_volume, self.context, - volume_id=dst_vol_id, + volume_id=dst_vol.id, request_spec={'source_volid': src_vol_id}) gthreads.append(t) @@ -1747,8 +1744,7 @@ class VolumeTestCase(BaseVolumeTestCase): dst_vol = tests_utils.create_volume(self.context, snapshot_id=snapshot_id, **self.volume_params) - self.volume.create_volume(self.context, - dst_vol['id']) + self.volume.create_volume(self.context, dst_vol.id, volume=dst_vol) self.assertRaises(exception.GlanceMetadataNotFound, db.volume_glance_metadata_copy_to_volume, @@ -3548,8 +3544,7 @@ class VolumeTestCase(BaseVolumeTestCase): spec=tests_utils.get_file_spec()) image_id = 'c905cedb-7281-47e4-8a62-f26bc5fc4c77' - volume_id = tests_utils.create_volume(self.context, - **self.volume_params)['id'] + volume = tests_utils.create_volume(self.context, **self.volume_params) # creating volume testdata try: request_spec = { @@ -3557,12 +3552,13 @@ class VolumeTestCase(BaseVolumeTestCase): 'image_id': image_id, } self.volume.create_volume(self.context, - volume_id, - request_spec) + volume.id, + request_spec, + volume=volume) finally: # cleanup os.unlink(dst_path) - volume = db.volume_get(self.context, volume_id) + volume = objects.Volume.get_by_id(self.context, volume.id) return volume @@ -3600,25 +3596,25 @@ class VolumeTestCase(BaseVolumeTestCase): self.stubs.Set(self.volume.driver, 'local_path', lambda x: dst_path) # creating volume testdata - volume_id = 1 - db.volume_create(self.context, - {'id': volume_id, - 'updated_at': datetime.datetime(1, 1, 1, 1, 1, 1), - 'display_description': 'Test Desc', - 'size': 20, - 'status': 'creating', - 'host': 'dummy'}) + kwargs = {'display_description': 'Test Desc', + 'size': 20, + 'availability_zone': 'fake_availability_zone', + 'status': 'creating', + 'attach_status': 'detached', + 'host': 'dummy'} + volume = objects.Volume(context=self.context, **kwargs) + volume.create() self.assertRaises(exception.ImageNotFound, self.volume.create_volume, self.context, - volume_id, + volume.id, {'image_id': self.FAKE_UUID}) - volume = db.volume_get(self.context, volume_id) + volume = objects.Volume.get_by_id(self.context, volume.id) self.assertEqual("error", volume['status']) self.assertFalse(volume['bootable']) # cleanup - db.volume_destroy(self.context, volume_id) + volume.destroy() os.unlink(dst_path) def test_create_volume_from_image_copy_exception_rescheduling(self): @@ -4389,20 +4385,20 @@ class VolumeMigrationTestCase(VolumeTestCase): nova_api): attached_host = 'some-host' fake_volume_id = 'fake_volume_id' - fake_new_volume = {'status': 'available', 'id': fake_volume_id} + fake_db_new_volume = {'status': 'available', 'id': fake_volume_id} + fake_new_volume = fake_volume.fake_db_volume(**fake_db_new_volume) host_obj = {'host': 'newhost', 'capabilities': {}} fake_uuid = fakes.get_fake_uuid() update_server_volume = nova_api.return_value.update_server_volume volume_get.return_value = fake_new_volume volume = tests_utils.create_volume(self.context, size=1, host=CONF.host) - volume = tests_utils.attach_volume(self.context, volume['id'], - fake_uuid, attached_host, - '/dev/vda') - self.assertIsNotNone(volume['volume_attachment'][0]['id']) - self.assertEqual(fake_uuid, - volume['volume_attachment'][0]['instance_uuid']) - self.assertEqual('in-use', volume['status']) + volume_attach = tests_utils.attach_volume( + self.context, volume['id'], fake_uuid, attached_host, '/dev/vda') + self.assertIsNotNone(volume_attach['volume_attachment'][0]['id']) + self.assertEqual( + fake_uuid, volume_attach['volume_attachment'][0]['instance_uuid']) + self.assertEqual('in-use', volume_attach['status']) self.volume._migrate_volume_generic(self.context, volume, host_obj, None) self.assertFalse(migrate_volume_completion.called) @@ -5118,8 +5114,7 @@ class ConsistencyGroupTestCase(BaseVolumeTestCase): consistencygroup_id=group2.id, snapshot_id=snapshot_id, **self.volume_params) - volume2_id = volume2['id'] - self.volume.create_volume(self.context, volume2_id) + self.volume.create_volume(self.context, volume2.id, volume=volume2) self.volume.create_consistencygroup_from_src( self.context, group2, cgsnapshot=cgsnapshot) cg2 = objects.ConsistencyGroup.get_by_id(self.context, group2.id) @@ -5186,8 +5181,7 @@ class ConsistencyGroupTestCase(BaseVolumeTestCase): consistencygroup_id=group3.id, source_volid=volume_id, **self.volume_params) - volume3_id = volume3['id'] - self.volume.create_volume(self.context, volume3_id) + self.volume.create_volume(self.context, volume3.id, volume=volume3) self.volume.create_consistencygroup_from_src( self.context, group3, source_cg=group) @@ -5444,8 +5438,7 @@ class ConsistencyGroupTestCase(BaseVolumeTestCase): status='creating', size=1) self.volume.host = 'host1@backend1' - volume_id = volume['id'] - self.volume.create_volume(self.context, volume_id) + self.volume.create_volume(self.context, volume.id, volume=volume) self.volume.delete_consistencygroup(self.context, group) cg = objects.ConsistencyGroup.get_by_id( @@ -5480,8 +5473,7 @@ class ConsistencyGroupTestCase(BaseVolumeTestCase): status='creating', size=1) self.volume.host = 'host1@backend2' - volume_id = volume['id'] - self.volume.create_volume(self.context, volume_id) + self.volume.create_volume(self.context, volume.id, volume=volume) self.assertRaises(exception.InvalidVolume, self.volume.delete_consistencygroup, diff --git a/cinder/tests/unit/test_volume_rpcapi.py b/cinder/tests/unit/test_volume_rpcapi.py index 60adbe8b3..72af1a9c9 100644 --- a/cinder/tests/unit/test_volume_rpcapi.py +++ b/cinder/tests/unit/test_volume_rpcapi.py @@ -17,6 +17,7 @@ Unit Tests for cinder.volume.rpcapi """ import copy +import mock from oslo_config import cfg from oslo_serialization import jsonutils @@ -84,6 +85,7 @@ class VolumeRpcAPITestCase(test.TestCase): group2 = objects.ConsistencyGroup.get_by_id(self.context, group2.id) cgsnapshot = objects.CGSnapshot.get_by_id(self.context, cgsnapshot.id) self.fake_volume = jsonutils.to_primitive(volume) + self.fake_volume_obj = fake_volume.fake_volume_obj(self.context, **vol) self.fake_volume_metadata = volume["volume_metadata"] self.fake_snapshot = snapshot self.fake_reservations = ["RESERVATION"] @@ -117,8 +119,13 @@ class VolumeRpcAPITestCase(test.TestCase): expected_msg = copy.deepcopy(kwargs) if 'volume' in expected_msg: volume = expected_msg['volume'] + # NOTE(thangp): copy.deepcopy() is making oslo_versionedobjects + # think that 'metadata' was changed. + if isinstance(volume, objects.Volume): + volume.obj_reset_changes() del expected_msg['volume'] expected_msg['volume_id'] = volume['id'] + expected_msg['volume'] = volume if 'snapshot' in expected_msg: snapshot = expected_msg['snapshot'] del expected_msg['snapshot'] @@ -194,6 +201,10 @@ class VolumeRpcAPITestCase(test.TestCase): expected_cgsnapshot = expected_msg[kwarg].obj_to_primitive() cgsnapshot = value.obj_to_primitive() self.assertEqual(expected_cgsnapshot, cgsnapshot) + elif isinstance(value, objects.Volume): + expected_volume = expected_msg[kwarg].obj_to_primitive() + volume = value.obj_to_primitive() + self.assertEqual(expected_volume, volume) else: self.assertEqual(expected_msg[kwarg], value) @@ -219,26 +230,46 @@ class VolumeRpcAPITestCase(test.TestCase): self._test_volume_api('delete_cgsnapshot', rpc_method='cast', cgsnapshot=self.fake_cgsnap, version='1.31') - def test_create_volume(self): + @mock.patch('oslo_messaging.RPCClient.can_send_version', + return_value=True) + def test_create_volume(self, can_send_version): self._test_volume_api('create_volume', rpc_method='cast', - volume=self.fake_volume, + volume=self.fake_volume_obj, + host='fake_host1', + request_spec='fake_request_spec', + filter_properties='fake_properties', + allow_reschedule=True, + version='1.32') + can_send_version.assert_called_once_with('1.32') + + @mock.patch('oslo_messaging.RPCClient.can_send_version', + return_value=False) + def test_create_volume_old(self, can_send_version): + # Tests backwards compatibility with older clients + self._test_volume_api('create_volume', + rpc_method='cast', + volume=self.fake_volume_obj, host='fake_host1', request_spec='fake_request_spec', filter_properties='fake_properties', allow_reschedule=True, version='1.24') + can_send_version.assert_called_once_with('1.32') - def test_create_volume_serialization(self): + @mock.patch('oslo_messaging.RPCClient.can_send_version', + return_value=True) + def test_create_volume_serialization(self, can_send_version): request_spec = {"metadata": self.fake_volume_metadata} self._test_volume_api('create_volume', rpc_method='cast', - volume=self.fake_volume, + volume=self.fake_volume_obj, host='fake_host1', request_spec=request_spec, filter_properties='fake_properties', allow_reschedule=True, - version='1.24') + version='1.32') + can_send_version.assert_called_once_with('1.32') def test_delete_volume(self): self._test_volume_api('delete_volume', diff --git a/cinder/tests/unit/test_volume_transfer.py b/cinder/tests/unit/test_volume_transfer.py index 54ffc83ee..46be3a9e7 100644 --- a/cinder/tests/unit/test_volume_transfer.py +++ b/cinder/tests/unit/test_volume_transfer.py @@ -17,8 +17,8 @@ import datetime import mock from cinder import context -from cinder import db from cinder import exception +from cinder import objects from cinder import test from cinder.tests.unit import utils from cinder.transfer import api as transfer_api @@ -35,10 +35,9 @@ class VolumeTransferTestCase(test.TestCase): @mock.patch('cinder.volume.utils.notify_about_volume_usage') def test_transfer_volume_create_delete(self, mock_notify): tx_api = transfer_api.API() - utils.create_volume(self.ctxt, id='1', - updated_at=self.updated_at) - response = tx_api.create(self.ctxt, '1', 'Description') - volume = db.volume_get(self.ctxt, '1') + volume = utils.create_volume(self.ctxt, updated_at=self.updated_at) + response = tx_api.create(self.ctxt, volume.id, 'Description') + volume = objects.Volume.get_by_id(self.ctxt, volume.id) self.assertEqual('awaiting-transfer', volume['status'], 'Unexpected state') calls = [mock.call(self.ctxt, mock.ANY, "transfer.create.start"), @@ -47,7 +46,7 @@ class VolumeTransferTestCase(test.TestCase): self.assertEqual(2, mock_notify.call_count) tx_api.delete(self.ctxt, response['id']) - volume = db.volume_get(self.ctxt, '1') + volume = objects.Volume.get_by_id(self.ctxt, volume.id) self.assertEqual('available', volume['status'], 'Unexpected state') calls = [mock.call(self.ctxt, mock.ANY, "transfer.delete.start"), mock.call(self.ctxt, mock.ANY, "transfer.delete.end")] @@ -56,22 +55,21 @@ class VolumeTransferTestCase(test.TestCase): def test_transfer_invalid_volume(self): tx_api = transfer_api.API() - utils.create_volume(self.ctxt, id='1', status='in-use', - updated_at=self.updated_at) + volume = utils.create_volume(self.ctxt, status='in-use', + updated_at=self.updated_at) self.assertRaises(exception.InvalidVolume, tx_api.create, - self.ctxt, '1', 'Description') - volume = db.volume_get(self.ctxt, '1') + self.ctxt, volume.id, 'Description') + volume = objects.Volume.get_by_id(self.ctxt, volume.id) self.assertEqual('in-use', volume['status'], 'Unexpected state') @mock.patch('cinder.volume.utils.notify_about_volume_usage') def test_transfer_accept(self, mock_notify): svc = self.start_service('volume', host='test_host') tx_api = transfer_api.API() - utils.create_volume(self.ctxt, id='1', - updated_at=self.updated_at) - transfer = tx_api.create(self.ctxt, '1', 'Description') - volume = db.volume_get(self.ctxt, '1') + volume = utils.create_volume(self.ctxt, updated_at=self.updated_at) + transfer = tx_api.create(self.ctxt, volume.id, 'Description') + volume = objects.Volume.get_by_id(self.ctxt, volume.id) self.assertEqual('awaiting-transfer', volume['status'], 'Unexpected state') @@ -88,11 +86,13 @@ class VolumeTransferTestCase(test.TestCase): mock_notify.assert_has_calls(calls) self.assertEqual(2, mock_notify.call_count) - db.volume_update(self.ctxt, '1', {'status': 'wrong'}) + volume.status = 'wrong' + volume.save() self.assertRaises(exception.InvalidVolume, tx_api.accept, self.ctxt, transfer['id'], transfer['auth_key']) - db.volume_update(self.ctxt, '1', {'status': 'awaiting-transfer'}) + volume.status = 'awaiting-transfer' + volume.save() # Because the InvalidVolume exception is raised in tx_api, so there is # only transfer.accept.start called and missing transfer.accept.end. @@ -105,15 +105,13 @@ class VolumeTransferTestCase(test.TestCase): response = tx_api.accept(self.ctxt, transfer['id'], transfer['auth_key']) - volume = db.volume_get(self.ctxt, '1') - self.assertEqual('new_project_id', volume['project_id'], - 'Unexpected project id') - self.assertEqual('new_user_id', volume['user_id'], - 'Unexpected user id') + volume = objects.Volume.get_by_id(self.ctxt, volume.id) + self.assertEqual('new_project_id', volume.project_id) + self.assertEqual('new_user_id', volume.user_id) - self.assertEqual(volume['id'], response['volume_id'], + self.assertEqual(response['volume_id'], volume.id, 'Unexpected volume id in response.') - self.assertEqual(transfer['id'], response['id'], + self.assertEqual(response['id'], transfer['id'], 'Unexpected transfer id in response.') calls = [mock.call(self.ctxt, mock.ANY, "transfer.accept.start"), @@ -125,8 +123,7 @@ class VolumeTransferTestCase(test.TestCase): def test_transfer_get(self): tx_api = transfer_api.API() - volume = utils.create_volume(self.ctxt, id='1', - updated_at=self.updated_at) + volume = utils.create_volume(self.ctxt, updated_at=self.updated_at) transfer = tx_api.create(self.ctxt, volume['id'], 'Description') t = tx_api.get(self.ctxt, transfer['id']) self.assertEqual(t['id'], transfer['id'], 'Unexpected transfer id') @@ -136,7 +133,7 @@ class VolumeTransferTestCase(test.TestCase): nctxt = context.RequestContext(user_id='new_user_id', project_id='new_project_id') - utils.create_volume(nctxt, id='2', updated_at=self.updated_at) + utils.create_volume(nctxt, updated_at=self.updated_at) self.assertRaises(exception.TransferNotFound, tx_api.get, nctxt, @@ -148,8 +145,7 @@ class VolumeTransferTestCase(test.TestCase): @mock.patch('cinder.volume.utils.notify_about_volume_usage') def test_delete_transfer_with_deleted_volume(self, mock_notify): # create a volume - volume = utils.create_volume(self.ctxt, id='1', - updated_at=self.updated_at) + volume = utils.create_volume(self.ctxt, updated_at=self.updated_at) # create a transfer tx_api = transfer_api.API() transfer = tx_api.create(self.ctxt, volume['id'], 'Description') @@ -161,7 +157,7 @@ class VolumeTransferTestCase(test.TestCase): mock_notify.assert_has_calls(calls) self.assertEqual(2, mock_notify.call_count) # force delete volume - db.volume_destroy(context.get_admin_context(), volume['id']) + volume.destroy() # Make sure transfer has been deleted. self.assertRaises(exception.TransferNotFound, tx_api.get, diff --git a/cinder/tests/unit/utils.py b/cinder/tests/unit/utils.py index 6b008d9ae..69f85b530 100644 --- a/cinder/tests/unit/utils.py +++ b/cinder/tests/unit/utils.py @@ -52,7 +52,8 @@ def create_volume(ctxt, vol['user_id'] = ctxt.user_id vol['project_id'] = ctxt.project_id vol['status'] = status - vol['migration_status'] = migration_status + if migration_status: + vol['migration_status'] = migration_status vol['display_name'] = display_name vol['display_description'] = display_description vol['attach_status'] = 'detached' @@ -64,11 +65,16 @@ def create_volume(ctxt, for key in kwargs: vol[key] = kwargs[key] vol['replication_status'] = replication_status - vol['replication_extended_status'] = replication_extended_status - vol['replication_driver_data'] = replication_driver_data - vol['previous_status'] = previous_status - - return db.volume_create(ctxt, vol) + if replication_extended_status: + vol['replication_extended_status'] = replication_extended_status + if replication_driver_data: + vol['replication_driver_data'] = replication_driver_data + if previous_status: + vol['previous_status'] = previous_status + + volume = objects.Volume(ctxt, **vol) + volume.create() + return volume def attach_volume(ctxt, volume_id, instance_uuid, attached_host, diff --git a/cinder/tests/unit/volume/flows/fake_volume_api.py b/cinder/tests/unit/volume/flows/fake_volume_api.py index d424758c1..b4c973497 100644 --- a/cinder/tests/unit/volume/flows/fake_volume_api.py +++ b/cinder/tests/unit/volume/flows/fake_volume_api.py @@ -36,9 +36,9 @@ class FakeSchedulerRpcAPI(object): self.expected_spec = expected_spec self.test_inst = test_inst - def create_volume(self, ctxt, volume, volume_ref, snapshot_id=None, + def create_volume(self, ctxt, topic, volume_id, snapshot_id=None, image_id=None, request_spec=None, - filter_properties=None): + filter_properties=None, volume=None): self.test_inst.assertEqual(self.expected_spec, request_spec) diff --git a/cinder/tests/unit/volume/flows/test_create_volume_flow.py b/cinder/tests/unit/volume/flows/test_create_volume_flow.py index 04086c72e..973f5ade2 100644 --- a/cinder/tests/unit/volume/flows/test_create_volume_flow.py +++ b/cinder/tests/unit/volume/flows/test_create_volume_flow.py @@ -50,17 +50,21 @@ class CreateVolumeFlowTestCase(test.TestCase): # called to avoid div by zero errors. self.counter = float(0) + @mock.patch('cinder.objects.Volume.get_by_id') @mock.patch('cinder.volume.utils.extract_host') @mock.patch('time.time', side_effect=time_inc) @mock.patch('cinder.objects.ConsistencyGroup.get_by_id') def test_cast_create_volume(self, consistencygroup_get_by_id, mock_time, - mock_extract_host): + mock_extract_host, volume_get_by_id): + volume = fake_volume.fake_volume_obj(self.ctxt) + volume_get_by_id.return_value = volume props = {} cg_obj = (fake_consistencygroup. fake_consistencyobject_obj(self.ctxt, consistencygroup_id=1, host='host@backend#pool')) consistencygroup_get_by_id.return_value = cg_obj spec = {'volume_id': None, + 'volume': None, 'source_volid': None, 'snapshot_id': None, 'image_id': None, @@ -76,7 +80,8 @@ class CreateVolumeFlowTestCase(test.TestCase): task._cast_create_volume(self.ctxt, spec, props) - spec = {'volume_id': 1, + spec = {'volume_id': volume.id, + 'volume': volume, 'source_volid': 2, 'snapshot_id': 3, 'image_id': 4, @@ -346,26 +351,26 @@ class CreateVolumeFlowManagerTestCase(test.TestCase): @mock.patch('cinder.volume.flows.manager.create_volume.' 'CreateVolumeFromSpecTask.' '_handle_bootable_volume_glance_meta') + @mock.patch('cinder.objects.Volume.get_by_id') @mock.patch('cinder.objects.Snapshot.get_by_id') - def test_create_from_snapshot(self, snapshot_get_by_id, handle_bootable): + def test_create_from_snapshot(self, snapshot_get_by_id, volume_get_by_id, + handle_bootable): fake_db = mock.MagicMock() fake_driver = mock.MagicMock() fake_volume_manager = mock.MagicMock() fake_manager = create_volume_manager.CreateVolumeFromSpecTask( fake_volume_manager, fake_db, fake_driver) - volume = fake_volume.fake_db_volume() - orig_volume_db = mock.MagicMock(id=10, bootable=True) + volume_db = {'bootable': True} + volume_obj = fake_volume.fake_volume_obj(self.ctxt, **volume_db) snapshot_obj = fake_snapshot.fake_snapshot_obj(self.ctxt) snapshot_get_by_id.return_value = snapshot_obj - fake_db.volume_get.return_value = orig_volume_db + volume_get_by_id.return_value = volume_obj - fake_manager._create_from_snapshot(self.ctxt, volume, + fake_manager._create_from_snapshot(self.ctxt, volume_obj, snapshot_obj.id) fake_driver.create_volume_from_snapshot.assert_called_once_with( - volume, snapshot_obj) - fake_db.volume_get.assert_called_once_with(self.ctxt, - snapshot_obj.volume_id) - handle_bootable.assert_called_once_with(self.ctxt, volume['id'], + volume_obj, snapshot_obj) + handle_bootable.assert_called_once_with(self.ctxt, volume_obj.id, snapshot_id=snapshot_obj.id) @mock.patch('cinder.objects.Snapshot.get_by_id') @@ -620,11 +625,13 @@ class CreateVolumeFlowManagerImageCacheTestCase(test.TestCase): image_meta=image_meta ) + @mock.patch('cinder.db.volume_update') + @mock.patch('cinder.objects.Volume.get_by_id') @mock.patch('cinder.image.image_utils.qemu_img_info') def test_create_from_image_cache_miss( - self, mock_qemu_info, mock_get_internal_context, - mock_create_from_img_dl, mock_create_from_src, - mock_handle_bootable, mock_fetch_img): + self, mock_qemu_info, mock_volume_get, mock_volume_update, + mock_get_internal_context, mock_create_from_img_dl, + mock_create_from_src, mock_handle_bootable, mock_fetch_img): mock_get_internal_context.return_value = self.ctxt mock_fetch_img.return_value = mock.MagicMock( spec=utils.get_file_spec()) @@ -636,13 +643,7 @@ class CreateVolumeFlowManagerImageCacheTestCase(test.TestCase): volume = fake_volume.fake_volume_obj(self.ctxt, size=10, host='foo@bar#pool') - image_volume = fake_volume.fake_db_volume(size=2) - self.mock_db.volume_create.return_value = image_volume - - def update_volume(ctxt, id, updates): - volume.update(updates) - return volume - self.mock_db.volume_update.side_effect = update_volume + mock_volume_get.return_value = volume image_location = 'someImageLocationStr' image_id = 'c7a8b8d4-e519-46c7-a0df-ddf1b9b9fff2' @@ -676,12 +677,8 @@ class CreateVolumeFlowManagerImageCacheTestCase(test.TestCase): ) # The volume size should be reduced to virtual_size and then put back - self.mock_db.volume_update.assert_any_call(self.ctxt, - volume['id'], - {'size': 2}) - self.mock_db.volume_update.assert_any_call(self.ctxt, - volume['id'], - {'size': 10}) + mock_volume_update.assert_any_call(self.ctxt, volume.id, {'size': 2}) + mock_volume_update.assert_any_call(self.ctxt, volume.id, {'size': 10}) # Make sure created a new cache entry (self.mock_volume_manager. @@ -695,9 +692,12 @@ class CreateVolumeFlowManagerImageCacheTestCase(test.TestCase): image_meta=image_meta ) + @mock.patch('cinder.db.volume_update') + @mock.patch('cinder.objects.Volume.get_by_id') @mock.patch('cinder.image.image_utils.qemu_img_info') def test_create_from_image_cache_miss_error_downloading( - self, mock_qemu_info, mock_get_internal_context, + self, mock_qemu_info, mock_volume_get, mock_volume_update, + mock_get_internal_context, mock_create_from_img_dl, mock_create_from_src, mock_handle_bootable, mock_fetch_img): mock_fetch_img.return_value = mock.MagicMock() @@ -709,16 +709,10 @@ class CreateVolumeFlowManagerImageCacheTestCase(test.TestCase): volume = fake_volume.fake_volume_obj(self.ctxt, size=10, host='foo@bar#pool') - image_volume = fake_volume.fake_db_volume(size=2) - self.mock_db.volume_create.return_value = image_volume + mock_volume_get.return_value = volume mock_create_from_img_dl.side_effect = exception.CinderException() - def update_volume(ctxt, id, updates): - volume.update(updates) - return volume - self.mock_db.volume_update.side_effect = update_volume - image_location = 'someImageLocationStr' image_id = 'c7a8b8d4-e519-46c7-a0df-ddf1b9b9fff2' image_meta = mock.MagicMock() @@ -756,13 +750,9 @@ class CreateVolumeFlowManagerImageCacheTestCase(test.TestCase): # The volume size should be reduced to virtual_size and then put back, # especially if there is an exception while creating the volume. - self.assertEqual(2, self.mock_db.volume_update.call_count) - self.mock_db.volume_update.assert_any_call(self.ctxt, - volume['id'], - {'size': 2}) - self.mock_db.volume_update.assert_any_call(self.ctxt, - volume['id'], - {'size': 10}) + self.assertEqual(2, mock_volume_update.call_count) + mock_volume_update.assert_any_call(self.ctxt, volume.id, {'size': 2}) + mock_volume_update.assert_any_call(self.ctxt, volume.id, {'size': 10}) # Make sure we didn't try and create a cache entry self.assertFalse(self.mock_cache.ensure_space.called) @@ -773,7 +763,7 @@ class CreateVolumeFlowManagerImageCacheTestCase(test.TestCase): mock_create_from_src, mock_handle_bootable, mock_fetch_img): self.mock_driver.clone_image.return_value = (None, False) mock_get_internal_context.return_value = None - volume = fake_volume.fake_db_volume() + volume = fake_volume.fake_volume_obj(self.ctxt) image_location = 'someImageLocationStr' image_id = 'c7a8b8d4-e519-46c7-a0df-ddf1b9b9fff2' diff --git a/cinder/volume/api.py b/cinder/volume/api.py index 5da39e1a0..1c84175c8 100644 --- a/cinder/volume/api.py +++ b/cinder/volume/api.py @@ -107,7 +107,8 @@ def check_policy(context, action, target_obj=None): if isinstance(target_obj, objects_base.CinderObject): # Turn object into dict so target.update can work - target.update(target_obj.obj_to_primitive() or {}) + target.update( + target_obj.obj_to_primitive()['versioned_object.data'] or {}) else: target.update(target_obj or {}) @@ -310,7 +311,7 @@ class API(base.Base): 'snapshot': snapshot, 'image_id': image_id, 'raw_volume_type': volume_type, - 'metadata': metadata, + 'metadata': metadata or {}, 'raw_availability_zone': availability_zone, 'source_volume': source_volume, 'scheduler_hints': scheduler_hints, diff --git a/cinder/volume/flows/api/create_volume.py b/cinder/volume/flows/api/create_volume.py index 8e1f98ce4..e57565f35 100644 --- a/cinder/volume/flows/api/create_volume.py +++ b/cinder/volume/flows/api/create_volume.py @@ -479,7 +479,8 @@ class EntryCreateTask(flow_utils.CinderTask): # Merge in the other required arguments which should provide the rest # of the volume property fields (if applicable). volume_properties.update(kwargs) - volume = self.db.volume_create(context, volume_properties) + volume = objects.Volume(context=context, **volume_properties) + volume.create() return { 'volume_id': volume['id'], @@ -505,16 +506,16 @@ class EntryCreateTask(flow_utils.CinderTask): # already been created and the quota has already been absorbed. return - vol_id = result['volume_id'] + volume = result['volume'] try: - self.db.volume_destroy(context.elevated(), vol_id) + volume.destroy() except exception.CinderException: # We are already reverting, therefore we should silence this # exception since a second exception being active will be bad. # # NOTE(harlowja): Being unable to destroy a volume is pretty # bad though!! - LOG.exception(_LE("Failed destroying volume entry %s"), vol_id) + LOG.exception(_LE("Failed destroying volume entry %s"), volume.id) class QuotaReserveTask(flow_utils.CinderTask): @@ -678,7 +679,7 @@ class VolumeCastTask(flow_utils.CinderTask): def __init__(self, scheduler_rpcapi, volume_rpcapi, db): requires = ['image_id', 'scheduler_hints', 'snapshot_id', - 'source_volid', 'volume_id', 'volume_type', + 'source_volid', 'volume_id', 'volume', 'volume_type', 'volume_properties', 'source_replicaid', 'consistencygroup_id', 'cgsnapshot_id', ] super(VolumeCastTask, self).__init__(addons=[ACTION], @@ -691,6 +692,7 @@ class VolumeCastTask(flow_utils.CinderTask): source_volid = request_spec['source_volid'] source_replicaid = request_spec['source_replicaid'] volume_id = request_spec['volume_id'] + volume = request_spec['volume'] snapshot_id = request_spec['snapshot_id'] image_id = request_spec['image_id'] cgroup_id = request_spec['consistencygroup_id'] @@ -714,14 +716,17 @@ class VolumeCastTask(flow_utils.CinderTask): # snapshot resides instead of passing it through the scheduler, so # snapshot can be copied to the new volume. snapshot = objects.Snapshot.get_by_id(context, snapshot_id) - source_volume_ref = self.db.volume_get(context, snapshot.volume_id) - host = source_volume_ref['host'] + source_volume_ref = objects.Volume.get_by_id(context, + snapshot.volume_id) + host = source_volume_ref.host elif source_volid: - source_volume_ref = self.db.volume_get(context, source_volid) - host = source_volume_ref['host'] + source_volume_ref = objects.Volume.get_by_id(context, + source_volid) + host = source_volume_ref.host elif source_replicaid: - source_volume_ref = self.db.volume_get(context, source_replicaid) - host = source_volume_ref['host'] + source_volume_ref = objects.Volume.get_by_id(context, + source_replicaid) + host = source_volume_ref.host if not host: # Cast to the scheduler and let it handle whatever is needed @@ -733,18 +738,19 @@ class VolumeCastTask(flow_utils.CinderTask): snapshot_id=snapshot_id, image_id=image_id, request_spec=request_spec, - filter_properties=filter_properties) + filter_properties=filter_properties, + volume=volume) else: # Bypass the scheduler and send the request directly to the volume # manager. - now = timeutils.utcnow() - values = {'host': host, 'scheduled_at': now} - volume_ref = self.db.volume_update(context, volume_id, values) + volume.host = host + volume.scheduled_at = timeutils.utcnow() + volume.save() if not cgsnapshot_id: self.volume_rpcapi.create_volume( context, - volume_ref, - volume_ref['host'], + volume, + volume.host, request_spec, filter_properties, allow_reschedule=False) diff --git a/cinder/volume/flows/manager/create_volume.py b/cinder/volume/flows/manager/create_volume.py index de1d51790..6ae0cc436 100644 --- a/cinder/volume/flows/manager/create_volume.py +++ b/cinder/volume/flows/manager/create_volume.py @@ -62,7 +62,7 @@ class OnFailureRescheduleTask(flow_utils.CinderTask): def __init__(self, reschedule_context, db, scheduler_rpcapi, do_reschedule): - requires = ['filter_properties', 'request_spec', 'volume_id', + requires = ['filter_properties', 'request_spec', 'volume_ref', 'context'] super(OnFailureRescheduleTask, self).__init__(addons=[ACTION], requires=requires) @@ -94,7 +94,7 @@ class OnFailureRescheduleTask(flow_utils.CinderTask): def execute(self, **kwargs): pass - def _pre_reschedule(self, context, volume_id): + def _pre_reschedule(self, context, volume): """Actions that happen before the rescheduling attempt occur here.""" try: @@ -112,15 +112,16 @@ class OnFailureRescheduleTask(flow_utils.CinderTask): 'host': None, } LOG.debug("Updating volume %(volume_id)s with %(update)s.", - {'update': update, 'volume_id': volume_id}) - self.db.volume_update(context, volume_id, update) + {'update': update, 'volume_id': volume.id}) + volume.update(update) + volume.save() except exception.CinderException: # Don't let updating the state cause the rescheduling to fail. LOG.exception(_LE("Volume %s: update volume state failed."), - volume_id) + volume.id) def _reschedule(self, context, cause, request_spec, filter_properties, - volume_id): + volume): """Actions that happen during the rescheduling attempt occur here.""" create_volume = self.scheduler_rpcapi.create_volume @@ -131,11 +132,11 @@ class OnFailureRescheduleTask(flow_utils.CinderTask): retry_info = filter_properties['retry'] num_attempts = retry_info.get('num_attempts', 0) - request_spec['volume_id'] = volume_id + request_spec['volume_id'] = volume.id LOG.debug("Volume %(volume_id)s: re-scheduling %(method)s " "attempt %(num)d due to %(reason)s", - {'volume_id': volume_id, + {'volume_id': volume.id, 'method': common.make_pretty_name(create_volume), 'num': num_attempts, 'reason': cause.exception_str}) @@ -144,16 +145,17 @@ class OnFailureRescheduleTask(flow_utils.CinderTask): # Stringify to avoid circular ref problem in json serialization retry_info['exc'] = traceback.format_exception(*cause.exc_info) - return create_volume(context, CONF.volume_topic, volume_id, + return create_volume(context, CONF.volume_topic, volume.id, request_spec=request_spec, - filter_properties=filter_properties) + filter_properties=filter_properties, + volume=volume) - def _post_reschedule(self, volume_id): + def _post_reschedule(self, volume): """Actions that happen after the rescheduling attempt occur here.""" - LOG.debug("Volume %s: re-scheduled", volume_id) + LOG.debug("Volume %s: re-scheduled", volume.id) - def revert(self, context, result, flow_failures, volume_id, **kwargs): + def revert(self, context, result, flow_failures, volume_ref, **kwargs): # NOTE(dulek): Revert is occurring and manager need to know if # rescheduling happened. We're returning boolean flag that will # indicate that. It which will be available in flow engine store @@ -162,16 +164,16 @@ class OnFailureRescheduleTask(flow_utils.CinderTask): # If do not want to be rescheduled, just set the volume's status to # error and return. if not self.do_reschedule: - common.error_out_volume(context, self.db, volume_id) - LOG.error(_LE("Volume %s: create failed"), volume_id) + common.error_out_volume(context, self.db, volume_ref.id) + LOG.error(_LE("Volume %s: create failed"), volume_ref.id) return False # Check if we have a cause which can tell us not to reschedule and # set the volume's status to error. for failure in flow_failures.values(): if failure.check(*self.no_reschedule_types): - common.error_out_volume(context, self.db, volume_id) - LOG.error(_LE("Volume %s: create failed"), volume_id) + common.error_out_volume(context, self.db, volume_ref.id) + LOG.error(_LE("Volume %s: create failed"), volume_ref.id) return False # Use a different context when rescheduling. @@ -179,12 +181,13 @@ class OnFailureRescheduleTask(flow_utils.CinderTask): cause = list(flow_failures.values())[0] context = self.reschedule_context try: - self._pre_reschedule(context, volume_id) - self._reschedule(context, cause, volume_id=volume_id, **kwargs) - self._post_reschedule(volume_id) + self._pre_reschedule(context, volume_ref) + self._reschedule(context, cause, volume=volume_ref, **kwargs) + self._post_reschedule(volume_ref) return True except exception.CinderException: - LOG.exception(_LE("Volume %s: rescheduling failed"), volume_id) + LOG.exception(_LE("Volume %s: rescheduling failed"), + volume_ref.id) return False @@ -206,8 +209,7 @@ class ExtractVolumeRefTask(flow_utils.CinderTask): # # In the future we might want to have a lock on the volume_id so that # the volume can not be deleted while its still being created? - volume_ref = self.db.volume_get(context, volume_id) - return volume_ref + return objects.Volume.get_by_id(context, volume_id) def revert(self, context, volume_id, result, **kwargs): if isinstance(result, ft.Failure) or not self.set_error: @@ -269,7 +271,8 @@ class ExtractVolumeSpecTask(flow_utils.CinderTask): # NOTE(harlowja): This will likely fail if the source volume # disappeared by the time this call occurred. source_volid = volume_ref.get('source_volid') - source_volume_ref = self.db.volume_get(context, source_volid) + source_volume_ref = objects.Volume.get_by_id(context, + source_volid) specs.update({ 'source_volid': source_volid, # This is captured incase we have to revert and we want to set @@ -284,7 +287,8 @@ class ExtractVolumeSpecTask(flow_utils.CinderTask): # NOTE(harlowja): This will likely fail if the replica # disappeared by the time this call occurred. source_volid = request_spec['source_replicaid'] - source_volume_ref = self.db.volume_get(context, source_volid) + source_volume_ref = objects.Volume.get_by_id(context, + source_volid) specs.update({ 'source_replicaid': source_volid, 'source_replicastatus': source_volume_ref['status'], @@ -443,8 +447,8 @@ class CreateVolumeFromSpecTask(flow_utils.CinderTask): # will not destroy the volume (although they could in the future). make_bootable = False try: - originating_vref = self.db.volume_get(context, - snapshot.volume_id) + originating_vref = objects.Volume.get_by_id(context, + snapshot.volume_id) make_bootable = originating_vref.bootable except exception.CinderException as ex: LOG.exception(_LE("Failed fetching snapshot %(snapshot_id)s " @@ -476,14 +480,14 @@ class CreateVolumeFromSpecTask(flow_utils.CinderTask): # NOTE(harlowja): likely this is not the best place for this to happen # and we should have proper locks on the source volume while actions # that use the source volume are underway. - srcvol_ref = self.db.volume_get(context, source_volid) + srcvol_ref = objects.Volume.get_by_id(context, source_volid) model_update = self.driver.create_cloned_volume(volume_ref, srcvol_ref) # NOTE(harlowja): Subtasks would be useful here since after this # point the volume has already been created and further failures # will not destroy the volume (although they could in the future). if srcvol_ref.bootable: - self._handle_bootable_volume_glance_meta(context, volume_ref['id'], - source_volid=source_volid) + self._handle_bootable_volume_glance_meta( + context, volume_ref.id, source_volid=volume_ref.id) return model_update def _create_from_source_replica(self, context, volume_ref, @@ -494,7 +498,7 @@ class CreateVolumeFromSpecTask(flow_utils.CinderTask): # NOTE(harlowja): likely this is not the best place for this to happen # and we should have proper locks on the source volume while actions # that use the source volume are underway. - srcvol_ref = self.db.volume_get(context, source_replicaid) + srcvol_ref = objects.Volume.get_by_id(context, source_replicaid) model_update = self.driver.create_replica_test_volume(volume_ref, srcvol_ref) # NOTE(harlowja): Subtasks would be useful here since after this @@ -754,12 +758,8 @@ class CreateVolumeFromSpecTask(flow_utils.CinderTask): image_id=image_id, reason=reason) if virtual_size and virtual_size != original_size: - updates = {'size': virtual_size} - volume_ref = self.db.volume_update( - context, - volume_ref['id'], - updates - ) + volume_ref.size = virtual_size + volume_ref.save() model_update = self._create_from_image_download( context, @@ -773,9 +773,8 @@ class CreateVolumeFromSpecTask(flow_utils.CinderTask): # Update the newly created volume db entry before we clone it # for the image-volume creation. if model_update: - volume_ref = self.db.volume_update(context, - volume_ref['id'], - model_update) + volume_ref.update(model_update) + volume_ref.save() self.manager._create_image_cache_volume_entry(internal_context, volume_ref, image_id, @@ -785,12 +784,12 @@ class CreateVolumeFromSpecTask(flow_utils.CinderTask): # what was originally requested. If an exception has occurred we # still need to put this back before letting it be raised further # up the stack. - if volume_ref['size'] != original_size: + if volume_ref.size != original_size: self.driver.extend_volume(volume_ref, original_size) - updates = {'size': original_size} - self.db.volume_update(context, volume_ref['id'], updates) + volume_ref.size = original_size + volume_ref.save() - self._handle_bootable_volume_glance_meta(context, volume_ref['id'], + self._handle_bootable_volume_glance_meta(context, volume_ref.id, image_id=image_id, image_meta=image_meta) return model_update @@ -839,8 +838,8 @@ class CreateVolumeFromSpecTask(flow_utils.CinderTask): # Persist any model information provided on creation. try: if model_update: - volume_ref = self.db.volume_update(context, volume_ref['id'], - model_update) + volume_ref.update(model_update) + volume_ref.save() except exception.CinderException: # If somehow the update failed we want to ensure that the # failure is logged (but not try rescheduling since the volume at @@ -872,7 +871,6 @@ class CreateVolumeOnFinishTask(NotifyVolumeActionTask): } def execute(self, context, volume, volume_spec): - volume_id = volume['id'] new_status = self.status_translation.get(volume_spec.get('status'), 'available') update = { @@ -884,18 +882,19 @@ class CreateVolumeOnFinishTask(NotifyVolumeActionTask): # or are there other side-effects that this will cause if the # status isn't updated correctly (aka it will likely be stuck in # 'creating' if this fails)?? - volume_ref = self.db.volume_update(context, volume_id, update) + volume.update(update) + volume.save() # Now use the parent to notify. - super(CreateVolumeOnFinishTask, self).execute(context, volume_ref) + super(CreateVolumeOnFinishTask, self).execute(context, volume) except exception.CinderException: LOG.exception(_LE("Failed updating volume %(volume_id)s with " - "%(update)s"), {'volume_id': volume_id, + "%(update)s"), {'volume_id': volume.id, 'update': update}) # Even if the update fails, the volume is ready. LOG.info(_LI("Volume %(volume_name)s (%(volume_id)s): " "created successfully"), {'volume_name': volume_spec['volume_name'], - 'volume_id': volume_id}) + 'volume_id': volume.id}) def get_flow(context, manager, db, driver, scheduler_rpcapi, host, volume_id, diff --git a/cinder/volume/manager.py b/cinder/volume/manager.py index 74e3b9af2..6a9b7a85e 100644 --- a/cinder/volume/manager.py +++ b/cinder/volume/manager.py @@ -190,7 +190,7 @@ def locked_snapshot_operation(f): class VolumeManager(manager.SchedulerDependentManager): """Manages attachable block storage devices.""" - RPC_API_VERSION = '1.31' + RPC_API_VERSION = '1.32' target = messaging.Target(version=RPC_API_VERSION) @@ -476,9 +476,16 @@ class VolumeManager(manager.SchedulerDependentManager): return self.driver.initialized def create_volume(self, context, volume_id, request_spec=None, - filter_properties=None, allow_reschedule=True): + filter_properties=None, allow_reschedule=True, + volume=None): """Creates the volume.""" + # FIXME(thangp): Remove this in v2.0 of RPC API. + if volume is None: + # For older clients, mimic the old behavior and look up the volume + # by its volume_id. + volume = objects.Volume.get_by_id(context, volume_id) + context_elevated = context.elevated() if filter_properties is None: filter_properties = {} @@ -496,7 +503,7 @@ class VolumeManager(manager.SchedulerDependentManager): self.driver, self.scheduler_rpcapi, self.host, - volume_id, + volume.id, allow_reschedule, context, request_spec, @@ -505,7 +512,7 @@ class VolumeManager(manager.SchedulerDependentManager): ) except Exception: msg = _("Create manager volume flow failed.") - LOG.exception(msg, resource={'type': 'volume', 'id': volume_id}) + LOG.exception(msg, resource={'type': 'volume', 'id': volume.id}) raise exception.CinderException(msg) snapshot_id = request_spec.get('snapshot_id') @@ -563,13 +570,13 @@ class VolumeManager(manager.SchedulerDependentManager): if not vol_ref: # Flow was reverted and not rescheduled, fetching # volume_ref from the DB, because it will be needed. - vol_ref = self.db.volume_get(context, volume_id) + vol_ref = objects.Volume.get_by_id(context, volume.id) # NOTE(dulek): Volume wasn't rescheduled so we need to update # volume stats as these are decremented on delete. self._update_allocated_capacity(vol_ref) LOG.info(_LI("Created volume successfully."), resource=vol_ref) - return vol_ref['id'] + return vol_ref.id @locked_volume_operation def delete_volume(self, context, volume_id, unmanage_only=False): @@ -1586,9 +1593,10 @@ class VolumeManager(manager.SchedulerDependentManager): new_vol_values = dict(volume) del new_vol_values['id'] del new_vol_values['_name_id'] + new_vol_values.pop('name', None) # We don't copy volume_type because the db sets that according to # volume_type_id, which we do copy - del new_vol_values['volume_type'] + new_vol_values.pop('volume_type', None) if new_type_id: new_vol_values['volume_type_id'] = new_type_id new_vol_values['host'] = host['host'] @@ -1600,8 +1608,9 @@ class VolumeManager(manager.SchedulerDependentManager): # I think new_vol_values['migration_status'] = 'target:%s' % volume['id'] new_vol_values['attach_status'] = 'detached' - new_vol_values['volume_attachment'] = [] - new_volume = self.db.volume_create(ctxt, new_vol_values) + new_vol_values.pop('volume_attachment', None) + new_volume = objects.Volume(context=ctxt, **new_vol_values) + new_volume.create() rpcapi.create_volume(ctxt, new_volume, host['host'], None, None, allow_reschedule=False) diff --git a/cinder/volume/rpcapi.py b/cinder/volume/rpcapi.py index c79ebb57c..91f1a4245 100644 --- a/cinder/volume/rpcapi.py +++ b/cinder/volume/rpcapi.py @@ -79,6 +79,7 @@ class VolumeAPI(object): 1.31 - Updated: create_consistencygroup_from_src(), create_cgsnapshot() and delete_cgsnapshot() to cast method only with necessary args. Forwarding CGSnapshot object instead of CGSnapshot_id. + 1.32 - Adds support for sending objects over RPC in create_volume(). """ BASE_RPC_API_VERSION = '1.0' @@ -88,7 +89,11 @@ class VolumeAPI(object): target = messaging.Target(topic=CONF.volume_topic, version=self.BASE_RPC_API_VERSION) serializer = objects_base.CinderObjectSerializer() - self.client = rpc.get_client(target, '1.31', serializer=serializer) + + # NOTE(thangp): Until version pinning is impletemented, set the client + # version_cap to None + self.client = rpc.get_client(target, version_cap=None, + serializer=serializer) def create_consistencygroup(self, ctxt, group, host): new_host = utils.extract_host(host) @@ -132,14 +137,20 @@ class VolumeAPI(object): def create_volume(self, ctxt, volume, host, request_spec, filter_properties, allow_reschedule=True): + request_spec_p = jsonutils.to_primitive(request_spec) + msg_args = {'volume_id': volume.id, 'request_spec': request_spec_p, + 'filter_properties': filter_properties, + 'allow_reschedule': allow_reschedule} + if self.client.can_send_version('1.32'): + version = '1.32' + msg_args['volume'] = volume + else: + version = '1.24' + new_host = utils.extract_host(host) - cctxt = self.client.prepare(server=new_host, version='1.24') + cctxt = self.client.prepare(server=new_host, version=version) request_spec_p = jsonutils.to_primitive(request_spec) - cctxt.cast(ctxt, 'create_volume', - volume_id=volume['id'], - request_spec=request_spec_p, - filter_properties=filter_properties, - allow_reschedule=allow_reschedule) + cctxt.cast(ctxt, 'create_volume', **msg_args) def delete_volume(self, ctxt, volume, unmanage_only=False): new_host = utils.extract_host(volume['host']) diff --git a/tools/lintstack.py b/tools/lintstack.py index e9a90496c..1901169fa 100755 --- a/tools/lintstack.py +++ b/tools/lintstack.py @@ -76,6 +76,8 @@ objects_ignore_messages = [ "Module 'cinder.objects' has no 'ServiceList' member", "Module 'cinder.objects' has no 'Snapshot' member", "Module 'cinder.objects' has no 'SnapshotList' member", + "Module 'cinder.objects' has no 'Volume' member", + "Module 'cinder.objects' has no 'VolumeList' member", ] objects_ignore_modules = ["cinder/objects/"] -- 2.45.2