From 0c507aa6d08a9471bf896961cc99d40f337f1e4d Mon Sep 17 00:00:00 2001 From: Mike Perez Date: Mon, 3 Dec 2012 10:38:27 -0800 Subject: [PATCH] Make summary and detail view consistent with other projects This introduces the v2 volume view builder for ease of response changing. blueprint vol-api-consistency Change-Id: If061e069d3b09ee5de15f1cbc7a46fa29c95a4cd --- cinder/api/v2/views/__init__.py | 16 +++ cinder/api/v2/views/volumes.py | 122 +++++++++++++++++++++ cinder/api/v2/volumes.py | 108 +++---------------- cinder/tests/api/v2/test_volumes.py | 162 +++++++++++++++++----------- 4 files changed, 255 insertions(+), 153 deletions(-) create mode 100644 cinder/api/v2/views/__init__.py create mode 100644 cinder/api/v2/views/volumes.py diff --git a/cinder/api/v2/views/__init__.py b/cinder/api/v2/views/__init__.py new file mode 100644 index 000000000..cbf4a4506 --- /dev/null +++ b/cinder/api/v2/views/__init__.py @@ -0,0 +1,16 @@ +# vim: tabstop=4 shiftwidth=4 softtabstop=4 + +# Copyright 2012 OpenStack LLC. +# All Rights Reserved. +# +# Licensed under the Apache License, Version 2.0 (the "License"); you may +# not use this file except in compliance with the License. You may obtain +# a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT +# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the +# License for the specific language governing permissions and limitations +# under the License. diff --git a/cinder/api/v2/views/volumes.py b/cinder/api/v2/views/volumes.py new file mode 100644 index 000000000..08f5ca201 --- /dev/null +++ b/cinder/api/v2/views/volumes.py @@ -0,0 +1,122 @@ +# vim: tabstop=4 shiftwidth=4 softtabstop=4 + +# Copyright 2012 OpenStack LLC. +# All Rights Reserved. +# +# Licensed under the Apache License, Version 2.0 (the "License"); you may +# not use this file except in compliance with the License. You may obtain +# a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT +# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the +# License for the specific language governing permissions and limitations +# under the License. + +from cinder.api import common +from cinder.openstack.common import log as logging + + +LOG = logging.getLogger(__name__) + + +class ViewBuilder(common.ViewBuilder): + """Model a server API response as a python dictionary.""" + + _collection_name = "volumes" + + def __init__(self): + """Initialize view builder.""" + super(ViewBuilder, self).__init__() + + def summary_list(self, request, volumes): + """Show a list of volumes without many details.""" + return self._list_view(self.summary, request, volumes) + + def detail_list(self, request, volumes): + """Detailed view of a list of volumes.""" + return self._list_view(self.detail, request, volumes) + + def summary(self, request, volume): + """Generic, non-detailed view of an volume.""" + return { + 'volume': { + 'id': volume['id'], + 'display_name': volume['display_name'], + 'links': self._get_links(request, + volume['id']), + }, + } + + def detail(self, request, volume): + """Detailed view of a single volume.""" + return { + 'volume': { + 'id': volume.get('id'), + 'status': volume.get('status'), + 'size': volume.get('size'), + 'availability_zone': volume.get('availability_zone'), + 'created_at': volume.get('created_at'), + 'attachments': self._get_attachments(volume), + 'display_name': volume.get('display_name'), + 'display_description': volume.get('display_description'), + 'volume_type': self._get_volume_type(volume), + 'snapshot_id': volume.get('snapshot_id'), + 'metadata': self._get_volume_metadata(volume), + 'links': self._get_links(request, volume['id']) + } + } + + def _get_attachments(self, volume): + """Retrieves the attachments of the volume object""" + attachments = [] + + if volume['attach_status'] == 'attached': + d = {} + volume_id = volume['id'] + + # note(justinsb): we use the volume id as the id of the attachments + # object + d['id'] = volume_id + + d['volume_id'] = volume_id + d['server_id'] = volume['instance_uuid'] + if volume.get('mountpoint'): + d['device'] = volume['mountpoint'] + attachments.append(d) + + return attachments + + def _get_volume_metadata(self, volume): + """Retrieves the metadata of the volume object""" + if volume.get('volume_metadata'): + metadata = volume.get('volume_metadata') + return dict((item['key'], item['value']) for item in metadata) + # avoid circular ref when vol is a Volume instance + elif volume.get('metadata') and isinstance(volume.get('metadata'), + dict): + return volume['metadata'] + return {} + + def _get_volume_type(self, volume): + """Retrieves the type the volume object is""" + if volume['volume_type_id'] and volume.get('volume_type'): + return volume['volume_type']['name'] + else: + # TODO(bcwaldon): remove str cast once we use uuids + return str(volume['volume_type_id']) + + def _list_view(self, func, request, volumes): + """Provide a view for a list of volumes.""" + volumes_list = [func(request, volume)['volume'] for volume in volumes] + volumes_links = self._get_collection_links(request, + volumes, + self._collection_name) + volumes_dict = dict(volumes=volumes_list) + + if volumes_links: + volumes_dict['volumes_links'] = volumes_links + + return volumes_dict diff --git a/cinder/api/v2/volumes.py b/cinder/api/v2/volumes.py index f304f143b..90397a15f 100644 --- a/cinder/api/v2/volumes.py +++ b/cinder/api/v2/volumes.py @@ -21,6 +21,7 @@ from xml.dom import minidom from cinder.api import common from cinder.api.openstack import wsgi +from cinder.api.v2.views import volumes as volume_views from cinder.api import xmlutil from cinder import exception from cinder import flags @@ -36,86 +37,6 @@ LOG = logging.getLogger(__name__) FLAGS = flags.FLAGS -def _translate_attachment_detail_view(_context, vol): - """Maps keys for attachment details view.""" - - d = _translate_attachment_summary_view(_context, vol) - - # No additional data / lookups at the moment - - return d - - -def _translate_attachment_summary_view(_context, vol): - """Maps keys for attachment summary view.""" - d = {} - - volume_id = vol['id'] - - # NOTE(justinsb): We use the volume id as the id of the attachment object - d['id'] = volume_id - - d['volume_id'] = volume_id - d['server_id'] = vol['instance_uuid'] - if vol.get('mountpoint'): - d['device'] = vol['mountpoint'] - - return d - - -def _translate_volume_detail_view(context, vol, image_id=None): - """Maps keys for volumes details view.""" - - d = _translate_volume_summary_view(context, vol, image_id) - - # No additional data / lookups at the moment - - return d - - -def _translate_volume_summary_view(context, vol, image_id=None): - """Maps keys for volumes summary view.""" - d = {} - - d['id'] = vol['id'] - d['status'] = vol['status'] - d['size'] = vol['size'] - d['availability_zone'] = vol['availability_zone'] - d['created_at'] = vol['created_at'] - - d['attachments'] = [] - if vol['attach_status'] == 'attached': - attachment = _translate_attachment_detail_view(context, vol) - d['attachments'].append(attachment) - - d['display_name'] = vol['display_name'] - d['display_description'] = vol['display_description'] - - if vol['volume_type_id'] and vol.get('volume_type'): - d['volume_type'] = vol['volume_type']['name'] - else: - # TODO(bcwaldon): remove str cast once we use uuids - d['volume_type'] = str(vol['volume_type_id']) - - d['snapshot_id'] = vol['snapshot_id'] - - if image_id: - d['image_id'] = image_id - - LOG.audit(_("vol=%s"), vol, context=context) - - if vol.get('volume_metadata'): - metadata = vol.get('volume_metadata') - d['metadata'] = dict((item['key'], item['value']) for item in metadata) - # avoid circular ref when vol is a Volume instance - elif vol.get('metadata') and isinstance(vol.get('metadata'), dict): - d['metadata'] = vol['metadata'] - else: - d['metadata'] = {} - - return d - - def make_attachment(elem): elem.set('id') elem.set('server_id') @@ -205,6 +126,8 @@ class CreateDeserializer(CommonDeserializer): class VolumeController(wsgi.Controller): """The Volumes API controller for the OpenStack API.""" + _view_builder_class = volume_views.ViewBuilder + def __init__(self, ext_mgr): self.volume_api = volume.API() self.ext_mgr = ext_mgr @@ -220,7 +143,7 @@ class VolumeController(wsgi.Controller): except exception.NotFound: raise exc.HTTPNotFound() - return {'volume': _translate_volume_detail_view(context, vol)} + return self._view_builder.detail(req, vol) def delete(self, req, id): """Delete a volume.""" @@ -238,15 +161,15 @@ class VolumeController(wsgi.Controller): @wsgi.serializers(xml=VolumesTemplate) def index(self, req): """Returns a summary list of volumes.""" - return self._items(req, entity_maker=_translate_volume_summary_view) + return self._get_volumes(req, is_detail=False) @wsgi.serializers(xml=VolumesTemplate) def detail(self, req): """Returns a detailed list of volumes.""" - return self._items(req, entity_maker=_translate_volume_detail_view) + return self._get_volumes(req, is_detail=True) - def _items(self, req, entity_maker): - """Returns a list of volumes, transformed through entity_maker.""" + def _get_volumes(self, req, is_detail): + """Returns a list of volumes, transformed through view builder.""" search_opts = {} search_opts.update(req.GET) @@ -257,8 +180,11 @@ class VolumeController(wsgi.Controller): volumes = self.volume_api.get_all(context, search_opts=search_opts) limited_list = common.limited(volumes, req) - res = [entity_maker(context, vol) for vol in limited_list] - return {'volumes': res} + if is_detail: + volumes = self._view_builder.detail_list(req, limited_list) + else: + volumes = self._view_builder.summary_list(req, limited_list) + return volumes def _image_uuid_from_href(self, image_href): # If the image href was generated by nova api, strip image_href @@ -333,11 +259,9 @@ class VolumeController(wsgi.Controller): # 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. - retval = _translate_volume_detail_view(context, - dict(new_volume.iteritems()), - image_uuid) + retval = self._view_builder.summary(req, dict(new_volume.iteritems())) - return {'volume': retval} + return retval def _get_volume_search_options(self): """Return volume search options allowed by non-admin.""" @@ -375,7 +299,7 @@ class VolumeController(wsgi.Controller): volume.update(update_dict) - return {'volume': _translate_volume_detail_view(context, volume)} + return self._view_builder.detail(req, volume) def create_resource(ext_mgr): diff --git a/cinder/tests/api/v2/test_volumes.py b/cinder/tests/api/v2/test_volumes.py index b5f52d15e..21dd25ca5 100644 --- a/cinder/tests/api/v2/test_volumes.py +++ b/cinder/tests/api/v2/test_volumes.py @@ -65,6 +65,7 @@ class VolumeApiTest(test.TestCase): stubs.stub_volume_get_all_by_project) self.stubs.Set(volume_api.API, 'get', stubs.stub_volume_get) self.stubs.Set(volume_api.API, 'delete', stubs.stub_volume_delete) + self.maxDiff = None def test_volume_create(self): self.stubs.Set(volume_api.API, "create", stubs.stub_volume_create) @@ -80,24 +81,18 @@ class VolumeApiTest(test.TestCase): res_dict = self.controller.create(req, body) expected = { 'volume': { - 'status': 'fakestatus', - 'display_description': 'Volume Test Desc', - 'availability_zone': 'zone1:host1', 'display_name': 'Volume Test Name', - 'attachments': [ + 'id': '1', + 'links': [ { - 'device': '/', - 'server_id': 'fakeuuid', - 'id': '1', - 'volume_id': '1' + 'href': 'http://localhost/v1/fake/volumes/1', + 'rel': 'self' + }, + { + 'href': 'http://localhost/fake/volumes/1', + 'rel': 'bookmark' } ], - 'volume_type': 'vol_type_name', - 'snapshot_id': None, - 'metadata': {}, - 'id': '1', - 'created_at': datetime.datetime(1, 1, 1, 1, 1, 1), - 'size': 100 } } self.assertEqual(res_dict, expected) @@ -120,8 +115,15 @@ class VolumeApiTest(test.TestCase): body = {"volume": vol} req = fakes.HTTPRequest.blank('/v2/volumes') res_dict = self.controller.create(req, body) - self.assertEquals(res_dict['volume']['volume_type'], - db_vol_type['name']) + volume_id = res_dict['volume']['id'] + self.assertEquals(len(res_dict), 1) + + self.stubs.Set(volume_api.API, 'get_all', + lambda *args, **kwargs: + [stubs.stub_volume(volume_id, + volume_type={'name': vol_type})]) + req = fakes.HTTPRequest.blank('/v2/volumes/detail') + res_dict = self.controller.detail(req) def test_volume_creation_fails_with_bad_size(self): vol = {"size": '', @@ -145,25 +147,19 @@ class VolumeApiTest(test.TestCase): "imageRef": 'c905cedb-7281-47e4-8a62-f26bc5fc4c77'} expected = { 'volume': { - 'status': 'fakestatus', - 'display_description': 'Volume Test Desc', - 'availability_zone': 'nova', 'display_name': 'Volume Test Name', - 'attachments': [ + 'id': '1', + 'links': [ { - 'device': '/', - 'server_id': 'fakeuuid', - 'id': '1', - 'volume_id': '1' + 'href': 'http://localhost/v1/fake/volumes/1', + 'rel': 'self' + }, + { + 'href': 'http://localhost/fake/volumes/1', + 'rel': 'bookmark' } ], - 'volume_type': 'vol_type_name', - 'image_id': 'c905cedb-7281-47e4-8a62-f26bc5fc4c77', - 'snapshot_id': None, - 'metadata': {}, - 'id': '1', - 'created_at': datetime.datetime(1, 1, 1, 1, 1, 1), - 'size': '1'} + } } body = {"volume": vol} req = fakes.HTTPRequest.blank('/v2/volumes') @@ -251,6 +247,16 @@ class VolumeApiTest(test.TestCase): 'id': '1', 'created_at': datetime.datetime(1, 1, 1, 1, 1, 1), 'size': 1, + 'links': [ + { + 'href': 'http://localhost/v1/fake/volumes/1', + 'rel': 'self' + }, + { + 'href': 'http://localhost/fake/volumes/1', + 'rel': 'bookmark' + } + ], } } self.assertEquals(res_dict, expected) @@ -280,6 +286,16 @@ class VolumeApiTest(test.TestCase): 'id': '1', 'created_at': datetime.datetime(1, 1, 1, 1, 1, 1), 'size': 1, + 'links': [ + { + 'href': 'http://localhost/v1/fake/volumes/1', + 'rel': 'self' + }, + { + 'href': 'http://localhost/fake/volumes/1', + 'rel': 'bookmark' + } + ], }} self.assertEquals(res_dict, expected) @@ -310,33 +326,26 @@ class VolumeApiTest(test.TestCase): self.controller.update, req, '1', body) - def test_volume_list(self): + def test_volume_list_summary(self): self.stubs.Set(volume_api.API, 'get_all', stubs.stub_volume_get_all_by_project) - req = fakes.HTTPRequest.blank('/v2/volumes') res_dict = self.controller.index(req) expected = { 'volumes': [ { - 'status': 'fakestatus', - 'display_description': 'displaydesc', - 'availability_zone': 'fakeaz', 'display_name': 'displayname', - 'attachments': [ + 'id': '1', + 'links': [ { - 'device': '/', - 'server_id': 'fakeuuid', - 'id': '1', - 'volume_id': '1' + 'href': 'http://localhost/v1/fake/volumes/1', + 'rel': 'self' + }, + { + 'href': 'http://localhost/fake/volumes/1', + 'rel': 'bookmark' } ], - 'volume_type': 'vol_type_name', - 'snapshot_id': None, - 'metadata': {}, - 'id': '1', - 'created_at': datetime.datetime(1, 1, 1, 1, 1, 1), - 'size': 1 } ] } @@ -346,7 +355,7 @@ class VolumeApiTest(test.TestCase): self.stubs.Set(volume_api.API, 'get_all', stubs.stub_volume_get_all_by_project) req = fakes.HTTPRequest.blank('/v2/volumes/detail') - res_dict = self.controller.index(req) + res_dict = self.controller.detail(req) expected = { 'volumes': [ { @@ -367,7 +376,17 @@ class VolumeApiTest(test.TestCase): 'metadata': {}, 'id': '1', 'created_at': datetime.datetime(1, 1, 1, 1, 1, 1), - 'size': 1 + 'size': 1, + 'links': [ + { + 'href': 'http://localhost/v1/fake/volumes/1', + 'rel': 'self' + }, + { + 'href': 'http://localhost/fake/volumes/1', + 'rel': 'bookmark' + } + ], } ] } @@ -407,31 +426,31 @@ class VolumeApiTest(test.TestCase): self.stubs.Set(db, 'volume_get_all_by_project', stub_volume_get_all_by_project) # no status filter - req = fakes.HTTPRequest.blank('/v2/volumes') - resp = self.controller.index(req) + req = fakes.HTTPRequest.blank('/v2/volumes/details') + resp = self.controller.detail(req) self.assertEqual(len(resp['volumes']), 3) # single match - req = fakes.HTTPRequest.blank('/v2/volumes?status=in-use') - resp = self.controller.index(req) + req = fakes.HTTPRequest.blank('/v2/volumes/details?status=in-use') + resp = self.controller.detail(req) self.assertEqual(len(resp['volumes']), 1) self.assertEqual(resp['volumes'][0]['status'], 'in-use') # multiple match - req = fakes.HTTPRequest.blank('/v2/volumes?status=available') - resp = self.controller.index(req) + req = fakes.HTTPRequest.blank('/v2/volumes/details/?status=available') + resp = self.controller.detail(req) self.assertEqual(len(resp['volumes']), 2) for volume in resp['volumes']: self.assertEqual(volume['status'], 'available') # multiple filters - req = fakes.HTTPRequest.blank('/v2/volumes?status=available&' + req = fakes.HTTPRequest.blank('/v2/volumes/details/?status=available&' 'display_name=vol1') - resp = self.controller.index(req) + resp = self.controller.detail(req) self.assertEqual(len(resp['volumes']), 1) self.assertEqual(resp['volumes'][0]['display_name'], 'vol1') self.assertEqual(resp['volumes'][0]['status'], 'available') # no match - req = fakes.HTTPRequest.blank('/v2/volumes?status=in-use&' + req = fakes.HTTPRequest.blank('/v2/volumes/details?status=in-use&' 'display_name=vol1') - resp = self.controller.index(req) + resp = self.controller.detail(req) self.assertEqual(len(resp['volumes']), 0) def test_volume_show(self): @@ -456,7 +475,17 @@ class VolumeApiTest(test.TestCase): 'metadata': {}, 'id': '1', 'created_at': datetime.datetime(1, 1, 1, 1, 1, 1), - 'size': 1 + 'size': 1, + 'links': [ + { + 'href': 'http://localhost/v1/fake/volumes/1', + 'rel': 'self' + }, + { + 'href': 'http://localhost/fake/volumes/1', + 'rel': 'bookmark' + } + ], } } self.assertEqual(res_dict, expected) @@ -481,9 +510,20 @@ class VolumeApiTest(test.TestCase): 'metadata': {}, 'id': '1', 'created_at': datetime.datetime(1, 1, 1, 1, 1, 1), - 'size': 1 + 'size': 1, + 'links': [ + { + 'href': 'http://localhost/v1/fake/volumes/1', + 'rel': 'self' + }, + { + 'href': 'http://localhost/fake/volumes/1', + 'rel': 'bookmark' + } + ], } } + self.assertEqual(res_dict, expected) def test_volume_show_no_volume(self): -- 2.45.2