From 5d8cf41bc2122d271af4ba7988fc3fd39f02e987 Mon Sep 17 00:00:00 2001 From: Vincent Hou Date: Wed, 29 Oct 2014 01:40:31 -0700 Subject: [PATCH] Remove the useless next link for volumes, transfers and backups Two cases this patch will resolve: 1) Currently if the number of items equals the osapi_max_limit or the last page of items equals the osapi_max_limit without the parameter limit set in the user request, a next link is generated in the response, though this next link will return empty volume list. In fact it is unnecessary to generate the next link in this case. 2) If the number of items equals the osapi_max_limit and limit is greater than osapi_max_limit, a next link is generated. Actually, the next link does not need to be generated, because it is certain that there is no more volumes left in the database. The method _get_collection_links has been called in volumes, volume_transfers and backups. The patch can only affect the next link generation for three of them. However, other lists like consistency groups, qos specs, cgsnapshots have not implemented the generation for the next link. Potentially this can be a wishlist item for them. Change-Id: I0f1f449c73d51675281497a095d869c1e72c889f closes-bug: #1350558 --- cinder/api/common.py | 65 ++++++--- cinder/api/contrib/backups.py | 7 +- cinder/api/contrib/volume_transfer.py | 7 +- cinder/api/v2/views/volumes.py | 17 ++- cinder/api/v2/volumes.py | 7 +- cinder/api/views/backups.py | 15 +- cinder/api/views/transfers.py | 15 +- cinder/tests/api/test_common.py | 200 ++++++++++++++++++++++++++ cinder/tests/api/v2/test_volumes.py | 7 +- 9 files changed, 292 insertions(+), 48 deletions(-) diff --git a/cinder/api/common.py b/cinder/api/common.py index 41a006ee6..81a0f2a9c 100644 --- a/cinder/api/common.py +++ b/cinder/api/common.py @@ -287,39 +287,64 @@ class ViewBuilder(object): str(identifier)) def _get_collection_links(self, request, items, collection_name, - id_key="uuid"): + item_count, id_key="uuid"): """Retrieve 'next' link, if applicable. The next link is included if: - 1) 'limit' param is specified and equals the number of volumes. - 2) 'limit' param is specified but it exceeds CONF.osapi_max_limit, - in this case the number of volumes is CONF.osapi_max_limit. - 3) 'limit' param is NOT specified but the number of volumes is - CONF.osapi_max_limit. + 1) 'limit' param is specified and equal to or less than the + number of items. + 2) 'limit' param is specified and both the limit and the number + of items are greater than CONF.osapi_max_limit, even if limit + is greater than the number of items. + 3) 'limit' param is NOT specified and the number of items is + greater than CONF.osapi_max_limit. + Notes: The case limit equals to 0 or CONF.osapi_max_limit equals to 0 + is not included in the above conditions. :param request: API request :param items: List of collection items :param collection_name: Name of collection, used to generate the next link for a pagination query + :param item_count: Length of the list of the original collection + items :param id_key: Attribute key used to retrieve the unique ID, used to generate the next link marker for a pagination query :returns links """ + limit = request.params.get("limit", None) + if limit is None or int(limit) > CONF.osapi_max_limit: + # If limit is not set in the request or greater than + # osapi_max_limit, len(items) < item_count means the items + # are limited by osapi_max_limit and we need to generate the + # next link. Otherwise, all the items will be returned in + # the response, no next link is generated. + if len(items) < item_count: + return self._generate_next_link(items, id_key, request, + collection_name) + else: + # If limit is set in the request and not more than + # osapi_max_limit, int(limit) == len(items) means it is possible + # that the DB still have more items left. In this case, + # we generate the next link. + limit = int(limit) + if limit and limit == len(items): + return self._generate_next_link(items, id_key, request, + collection_name) + return [] + + def _generate_next_link(self, items, id_key, request, + collection_name): links = [] - max_items = min( - int(request.params.get("limit", CONF.osapi_max_limit)), - CONF.osapi_max_limit) - if max_items and max_items == len(items): - last_item = items[-1] - if id_key in last_item: - last_item_id = last_item[id_key] - else: - last_item_id = last_item["id"] - links.append({ - "rel": "next", - "href": self._get_next_link(request, last_item_id, - collection_name), - }) + last_item = items[-1] + if id_key in last_item: + last_item_id = last_item[id_key] + else: + last_item_id = last_item["id"] + links.append({ + "rel": "next", + "href": self._get_next_link(request, last_item_id, + collection_name), + }) return links def _update_link_prefix(self, orig_url, prefix): diff --git a/cinder/api/contrib/backups.py b/cinder/api/contrib/backups.py index f53569c2f..e29e06fac 100644 --- a/cinder/api/contrib/backups.py +++ b/cinder/api/contrib/backups.py @@ -219,13 +219,16 @@ class BackupsController(wsgi.Controller): del filters['name'] backups = self.backup_api.get_all(context, search_opts=filters) + backup_count = len(backups) limited_list = common.limited(backups, req) req.cache_db_backups(limited_list) if is_detail: - backups = self._view_builder.detail_list(req, limited_list) + backups = self._view_builder.detail_list(req, limited_list, + backup_count) else: - backups = self._view_builder.summary_list(req, limited_list) + backups = self._view_builder.summary_list(req, limited_list, + backup_count) return backups # TODO(frankm): Add some checks here including diff --git a/cinder/api/contrib/volume_transfer.py b/cinder/api/contrib/volume_transfer.py index 833f30d87..5f60df40d 100644 --- a/cinder/api/contrib/volume_transfer.py +++ b/cinder/api/contrib/volume_transfer.py @@ -131,12 +131,15 @@ class VolumeTransferController(wsgi.Controller): filters = req.params.copy() LOG.debug('Listing volume transfers') transfers = self.transfer_api.get_all(context, filters=filters) + transfer_count = len(transfers) limited_list = common.limited(transfers, req) if is_detail: - transfers = self._view_builder.detail_list(req, limited_list) + transfers = self._view_builder.detail_list(req, limited_list, + transfer_count) else: - transfers = self._view_builder.summary_list(req, limited_list) + transfers = self._view_builder.summary_list(req, limited_list, + transfer_count) return transfers diff --git a/cinder/api/v2/views/volumes.py b/cinder/api/v2/views/volumes.py index 77303b298..5d904772f 100644 --- a/cinder/api/v2/views/volumes.py +++ b/cinder/api/v2/views/volumes.py @@ -29,13 +29,15 @@ class ViewBuilder(common.ViewBuilder): """Initialize view builder.""" super(ViewBuilder, self).__init__() - def summary_list(self, request, volumes): + def summary_list(self, request, volumes, volume_count): """Show a list of volumes without many details.""" - return self._list_view(self.summary, request, volumes) + return self._list_view(self.summary, request, volumes, + volume_count) - def detail_list(self, request, volumes): + def detail_list(self, request, volumes, volume_count): """Detailed view of a list of volumes.""" return self._list_view(self.detail, request, volumes, + volume_count, coll_name=self._collection_name + '/detail') def summary(self, request, volume): @@ -116,12 +118,14 @@ class ViewBuilder(common.ViewBuilder): else: return volume['volume_type_id'] - def _list_view(self, func, request, volumes, coll_name=_collection_name): + def _list_view(self, func, request, volumes, volume_count, + coll_name=_collection_name): """Provide a view for a list of volumes. :param func: Function used to format the volume data :param request: API request - :param servers: List of volumes in dictionary format + :param volumes: List of volumes in dictionary format + :param volume_count: Length of the original list of volumes :param coll_name: Name of collection, used to generate the next link for a pagination query :returns: Volume data in dictionary format @@ -129,7 +133,8 @@ class ViewBuilder(common.ViewBuilder): volumes_list = [func(request, volume)['volume'] for volume in volumes] volumes_links = self._get_collection_links(request, volumes, - coll_name) + coll_name, + volume_count) volumes_dict = dict(volumes=volumes_list) if volumes_links: diff --git a/cinder/api/v2/volumes.py b/cinder/api/v2/volumes.py index 6a2e34707..fa549d03c 100644 --- a/cinder/api/v2/volumes.py +++ b/cinder/api/v2/volumes.py @@ -249,12 +249,15 @@ class VolumeController(wsgi.Controller): utils.add_visible_admin_metadata(volume) limited_list = common.limited(volumes, req) + volume_count = len(volumes) req.cache_db_volumes(limited_list) if is_detail: - volumes = self._view_builder.detail_list(req, limited_list) + volumes = self._view_builder.detail_list(req, limited_list, + volume_count) else: - volumes = self._view_builder.summary_list(req, limited_list) + volumes = self._view_builder.summary_list(req, limited_list, + volume_count) return volumes def _image_uuid_from_ref(self, image_ref, context): diff --git a/cinder/api/views/backups.py b/cinder/api/views/backups.py index aafd11ef9..578b86b95 100644 --- a/cinder/api/views/backups.py +++ b/cinder/api/views/backups.py @@ -29,13 +29,15 @@ class ViewBuilder(common.ViewBuilder): """Initialize view builder.""" super(ViewBuilder, self).__init__() - def summary_list(self, request, backups): + def summary_list(self, request, backups, origin_backup_count): """Show a list of backups without many details.""" - return self._list_view(self.summary, request, backups) + return self._list_view(self.summary, request, backups, + origin_backup_count) - def detail_list(self, request, backups): + def detail_list(self, request, backups, origin_backup_count): """Detailed view of a list of backups .""" - return self._list_view(self.detail, request, backups) + return self._list_view(self.detail, request, backups, + origin_backup_count) def summary(self, request, backup): """Generic, non-detailed view of a backup.""" @@ -76,12 +78,13 @@ class ViewBuilder(common.ViewBuilder): } } - def _list_view(self, func, request, backups): + def _list_view(self, func, request, backups, origin_backup_count): """Provide a view for a list of backups.""" backups_list = [func(request, backup)['backup'] for backup in backups] backups_links = self._get_collection_links(request, backups, - self._collection_name) + self._collection_name, + origin_backup_count) backups_dict = dict(backups=backups_list) if backups_links: diff --git a/cinder/api/views/transfers.py b/cinder/api/views/transfers.py index 3cc757f86..6fdd249a0 100644 --- a/cinder/api/views/transfers.py +++ b/cinder/api/views/transfers.py @@ -29,13 +29,15 @@ class ViewBuilder(common.ViewBuilder): """Initialize view builder.""" super(ViewBuilder, self).__init__() - def summary_list(self, request, transfers): + def summary_list(self, request, transfers, origin_transfer_count): """Show a list of transfers without many details.""" - return self._list_view(self.summary, request, transfers) + return self._list_view(self.summary, request, transfers, + origin_transfer_count) - def detail_list(self, request, transfers): + def detail_list(self, request, transfers, origin_transfer_count): """Detailed view of a list of transfers .""" - return self._list_view(self.detail, request, transfers) + return self._list_view(self.detail, request, transfers, + origin_transfer_count) def summary(self, request, transfer): """Generic, non-detailed view of a transfer.""" @@ -74,13 +76,14 @@ class ViewBuilder(common.ViewBuilder): } } - def _list_view(self, func, request, transfers): + def _list_view(self, func, request, transfers, origin_transfer_count): """Provide a view for a list of transfers.""" transfers_list = [func(request, transfer)['transfer'] for transfer in transfers] transfers_links = self._get_collection_links(request, transfers, - self._collection_name) + self._collection_name, + origin_transfer_count) transfers_dict = dict(transfers=transfers_list) if transfers_links: diff --git a/cinder/tests/api/test_common.py b/cinder/tests/api/test_common.py index c5be2bdbc..75ef32894 100644 --- a/cinder/tests/api/test_common.py +++ b/cinder/tests/api/test_common.py @@ -17,6 +17,8 @@ Test suites for 'common' code used throughout the OpenStack HTTP API. """ +import mock +from testtools import matchers import webob import webob.exc @@ -347,3 +349,201 @@ class MiscFunctionsTest(test.TestCase): self.assertRaises(ValueError, common.remove_version_from_href, fixture) + + +class TestCollectionLinks(test.TestCase): + """Tests the _get_collection_links method.""" + + def _validate_next_link(self, href_link_mock, item_count, + osapi_max_limit, limit, should_link_exist): + req = mock.MagicMock() + href_link_mock.return_value = [{"rel": "next", + "href": "fake_link"}] + self.flags(osapi_max_limit=osapi_max_limit) + if limit is None: + params = mock.PropertyMock(return_value=dict()) + limited_list_size = min(item_count, osapi_max_limit) + else: + params = mock.PropertyMock(return_value=dict(limit=limit)) + limited_list_size = min(item_count, osapi_max_limit, + limit) + limited_list = [{"uuid": str(i)} for i in range(limited_list_size)] + type(req).params = params + builder = common.ViewBuilder() + results = builder._get_collection_links(req, limited_list, + mock.sentinel.coll_key, + item_count, "uuid") + if should_link_exist: + href_link_mock.assert_called_once_with(limited_list, "uuid", + req, + mock.sentinel.coll_key) + self.assertThat(results, matchers.HasLength(1)) + else: + self.assertFalse(href_link_mock.called) + self.assertThat(results, matchers.HasLength(0)) + + @mock.patch('cinder.api.common.ViewBuilder._generate_next_link') + def test_items_equals_osapi_max_no_limit(self, href_link_mock): + item_count = 5 + osapi_max_limit = 5 + limit = None + should_link_exist = False + self._validate_next_link(href_link_mock, item_count, + osapi_max_limit, + limit, should_link_exist) + + @mock.patch('cinder.api.common.ViewBuilder._generate_next_link') + def test_items_equals_osapi_max_greater_than_limit(self, + href_link_mock): + item_count = 5 + osapi_max_limit = 5 + limit = 4 + should_link_exist = True + self._validate_next_link(href_link_mock, item_count, + osapi_max_limit, + limit, should_link_exist) + + @mock.patch('cinder.api.common.ViewBuilder._generate_next_link') + def test_items_equals_osapi_max_equals_limit(self, href_link_mock): + item_count = 5 + osapi_max_limit = 5 + limit = 5 + should_link_exist = True + self._validate_next_link(href_link_mock, item_count, + osapi_max_limit, + limit, should_link_exist) + + @mock.patch('cinder.api.common.ViewBuilder._generate_next_link') + def test_items_equals_osapi_max_less_than_limit(self, href_link_mock): + item_count = 5 + osapi_max_limit = 5 + limit = 6 + should_link_exist = False + self._validate_next_link(href_link_mock, item_count, + osapi_max_limit, + limit, should_link_exist) + + @mock.patch('cinder.api.common.ViewBuilder._generate_next_link') + def test_items_less_than_osapi_max_no_limit(self, href_link_mock): + item_count = 5 + osapi_max_limit = 7 + limit = None + should_link_exist = False + self._validate_next_link(href_link_mock, item_count, + osapi_max_limit, + limit, should_link_exist) + + @mock.patch('cinder.api.common.ViewBuilder._generate_next_link') + def test_limit_less_than_items_less_than_osapi_max(self, href_link_mock): + item_count = 5 + osapi_max_limit = 7 + limit = 4 + should_link_exist = True + self._validate_next_link(href_link_mock, item_count, + osapi_max_limit, + limit, should_link_exist) + + @mock.patch('cinder.api.common.ViewBuilder._generate_next_link') + def test_limit_equals_items_less_than_osapi_max(self, href_link_mock): + item_count = 5 + osapi_max_limit = 7 + limit = 5 + should_link_exist = True + self._validate_next_link(href_link_mock, item_count, + osapi_max_limit, + limit, should_link_exist) + + @mock.patch('cinder.api.common.ViewBuilder._generate_next_link') + def test_items_less_than_limit_less_than_osapi_max(self, href_link_mock): + item_count = 5 + osapi_max_limit = 7 + limit = 6 + should_link_exist = False + self._validate_next_link(href_link_mock, item_count, + osapi_max_limit, + limit, should_link_exist) + + @mock.patch('cinder.api.common.ViewBuilder._generate_next_link') + def test_items_less_than_osapi_max_equals_limit(self, href_link_mock): + item_count = 5 + osapi_max_limit = 7 + limit = 7 + should_link_exist = False + self._validate_next_link(href_link_mock, item_count, + osapi_max_limit, + limit, should_link_exist) + + @mock.patch('cinder.api.common.ViewBuilder._generate_next_link') + def test_items_less_than_osapi_max_less_than_limit(self, href_link_mock): + item_count = 5 + osapi_max_limit = 7 + limit = 8 + should_link_exist = False + self._validate_next_link(href_link_mock, item_count, + osapi_max_limit, + limit, should_link_exist) + + @mock.patch('cinder.api.common.ViewBuilder._generate_next_link') + def test_items_greater_than_osapi_max_no_limit(self, href_link_mock): + item_count = 5 + osapi_max_limit = 3 + limit = None + should_link_exist = True + self._validate_next_link(href_link_mock, item_count, + osapi_max_limit, + limit, should_link_exist) + + @mock.patch('cinder.api.common.ViewBuilder._generate_next_link') + def test_limit_less_than_items_greater_than_osapi_max(self, + href_link_mock): + item_count = 5 + osapi_max_limit = 3 + limit = 2 + should_link_exist = True + self._validate_next_link(href_link_mock, item_count, + osapi_max_limit, + limit, should_link_exist) + + @mock.patch('cinder.api.common.ViewBuilder._generate_next_link') + def test_items_greater_than_osapi_max_equals_limit(self, + href_link_mock): + item_count = 5 + osapi_max_limit = 3 + limit = 3 + should_link_exist = True + self._validate_next_link(href_link_mock, item_count, + osapi_max_limit, + limit, should_link_exist) + + @mock.patch('cinder.api.common.ViewBuilder._generate_next_link') + def test_items_greater_than_limit_greater_than_osapi_max(self, + href_link_mock): + item_count = 5 + osapi_max_limit = 3 + limit = 4 + should_link_exist = True + self._validate_next_link(href_link_mock, item_count, + osapi_max_limit, + limit, should_link_exist) + + @mock.patch('cinder.api.common.ViewBuilder._generate_next_link') + def test_items_equals_limit_greater_than_osapi_max(self, + href_link_mock): + item_count = 5 + osapi_max_limit = 3 + limit = 5 + should_link_exist = True + self._validate_next_link(href_link_mock, item_count, + osapi_max_limit, + limit, should_link_exist) + + @mock.patch('cinder.api.common.ViewBuilder._generate_next_link') + def test_limit_greater_than_items_greater_than_osapi_max(self, + href_link_mock): + item_count = 5 + osapi_max_limit = 3 + limit = 6 + should_link_exist = True + self._validate_next_link(href_link_mock, item_count, + osapi_max_limit, + limit, should_link_exist) diff --git a/cinder/tests/api/v2/test_volumes.py b/cinder/tests/api/v2/test_volumes.py index a6bb9ef41..921639890 100644 --- a/cinder/tests/api/v2/test_volumes.py +++ b/cinder/tests/api/v2/test_volumes.py @@ -1111,7 +1111,7 @@ class VolumeApiTest(test.TestCase): api_keys = ['volumes', 'volumes/detail'] fns = [self.controller.index, self.controller.detail] - # Number of volumes equals the max, include next link + # Number of volumes equals the max, next link not included def stub_volume_get_all(context, marker, limit, sort_keys=None, sort_dirs=None, filters=None, @@ -1127,8 +1127,7 @@ class VolumeApiTest(test.TestCase): use_admin_context=True) res_dict = fn(req) self.assertEqual(len(res_dict['volumes']), CONF.osapi_max_limit) - volumes_links = res_dict['volumes_links'] - _verify_links(volumes_links, key) + self.assertFalse('volumes_links' in res_dict) # Number of volumes less than max, do not include def stub_volume_get_all2(context, marker, limit, @@ -1168,7 +1167,7 @@ class VolumeApiTest(test.TestCase): _verify_links(volumes_links, key) # Pass a limit that is greater than the max and the total number of # volumes, ensure only the maximum is returned and that the next - # link is present + # link is present. for key, fn in zip(api_keys, fns): req = fakes.HTTPRequest.blank('/v2/%s?all_tenants=1&limit=%d' % (key, CONF.osapi_max_limit * 2), -- 2.45.2