From 133b194ba079abe84900d09a5c3c74ef9f464bab Mon Sep 17 00:00:00 2001 From: Lucian Petrut Date: Mon, 8 Oct 2018 17:13:55 +0300 Subject: [PATCH] Fix os-simple-tenant-usage result order nova usage-list can return incorrect results, having resources counted twice. This only occurs when using the 2.40 microversion or later. This microversion introduced pagination, which doesn't work properly. Nova API will sort the instances using the tenant id and instance uuid, but 'os-simple-tenant-usage' will not preserve the order when returning the results. For this reason, subsequent API calls made by the client will use the wrong marker (which is supposed to be the last instance id), ending up counting the same instances twice. Change-Id: I6c7a67b23ec49aa207c33c38580acd834bb27e3c Closes-Bug: #1796689 (cherry picked from commit afc3a16ce3364c233e6e1cffc9f38987d1d65318) --- .../v2.40/simple-tenant-usage-get-all.json | 68 +++++++++++++++++++ .../openstack/compute/simple_tenant_usage.py | 3 +- .../api_sample_tests/api_sample_base.py | 4 +- .../simple-tenant-usage-get-all.json.tpl | 68 +++++++++++++++++++ .../test_simple_tenant_usage.py | 28 +++++++- .../tests/functional/api_samples_test_base.py | 41 ++++++++--- .../test_compare_result.py | 2 +- ...ant-usage-pagination-393ed6e7d0e31594.yaml | 6 ++ 8 files changed, 204 insertions(+), 16 deletions(-) create mode 100644 doc/api_samples/os-simple-tenant-usage/v2.40/simple-tenant-usage-get-all.json create mode 100644 nova/tests/functional/api_sample_tests/api_samples/os-simple-tenant-usage/v2.40/simple-tenant-usage-get-all.json.tpl create mode 100644 releasenotes/notes/fix-simple-tenant-usage-pagination-393ed6e7d0e31594.yaml diff --git a/doc/api_samples/os-simple-tenant-usage/v2.40/simple-tenant-usage-get-all.json b/doc/api_samples/os-simple-tenant-usage/v2.40/simple-tenant-usage-get-all.json new file mode 100644 index 000000000000..d6a1be5cc649 --- /dev/null +++ b/doc/api_samples/os-simple-tenant-usage/v2.40/simple-tenant-usage-get-all.json @@ -0,0 +1,68 @@ +{ + "tenant_usages": [ + { + "server_usages": [ + { + "ended_at": null, + "flavor": "m1.tiny", + "hours": 1.0, + "instance_id": "1f1deceb-17b5-4c04-84c7-e0d4499c8f06", + "local_gb": 1, + "memory_mb": 512, + "name": "instance-3", + "started_at": "2018-10-09T11:29:04.166194", + "state": "active", + "tenant_id": "0000000e737461636b20342065000000", + "uptime": 3600, + "vcpus": 1 + } + ], + "start": "2018-10-09T11:29:04.166194", + "stop": "2018-10-09T12:29:04.166194", + "tenant_id": "0000000e737461636b20342065000000", + "total_hours": 1.0, + "total_local_gb_usage": 1.0, + "total_memory_mb_usage": 512.0, + "total_vcpus_usage": 1.0 + }, + { + "server_usages": [ + { + "ended_at": null, + "flavor": "m1.tiny", + "hours": 1.0, + "instance_id": "1f1deceb-17b5-4c04-84c7-e0d4499c8f00", + "local_gb": 1, + "memory_mb": 512, + "name": "instance-1", + "started_at": "2018-10-09T11:29:04.166194", + "state": "active", + "tenant_id": "6f70656e737461636b20342065766572", + "uptime": 3600, + "vcpus": 1 + }, + { + "ended_at": null, + "flavor": "m1.tiny", + "hours": 1.0, + "instance_id": "1f1deceb-17b5-4c04-84c7-e0d4499c8f03", + "local_gb": 1, + "memory_mb": 512, + "name": "instance-2", + "started_at": "2018-10-09T11:29:04.166194", + "state": "active", + "tenant_id": "6f70656e737461636b20342065766572", + "uptime": 3600, + "vcpus": 1 + } + ], + "start": "2018-10-09T11:29:04.166194", + "stop": "2018-10-09T12:29:04.166194", + "tenant_id": "6f70656e737461636b20342065766572", + "total_hours": 2.0, + "total_local_gb_usage": 2.0, + "total_memory_mb_usage": 1024.0, + "total_vcpus_usage": 2.0 + } + ] +} \ No newline at end of file diff --git a/nova/api/openstack/compute/simple_tenant_usage.py b/nova/api/openstack/compute/simple_tenant_usage.py index 7aa4dff1a66c..7bcc08411733 100644 --- a/nova/api/openstack/compute/simple_tenant_usage.py +++ b/nova/api/openstack/compute/simple_tenant_usage.py @@ -13,6 +13,7 @@ # License for the specific language governing permissions and limitations # under the License. +import collections import datetime import iso8601 @@ -142,7 +143,7 @@ class SimpleTenantUsageController(wsgi.Controller): instances = self._get_instances_all_cells(context, period_start, period_stop, tenant_id, limit, marker) - rval = {} + rval = collections.OrderedDict() flavors = {} all_server_usages = [] diff --git a/nova/tests/functional/api_sample_tests/api_sample_base.py b/nova/tests/functional/api_sample_tests/api_sample_base.py index 89d9d806aa66..394c39a8fa7a 100644 --- a/nova/tests/functional/api_sample_tests/api_sample_base.py +++ b/nova/tests/functional/api_sample_tests/api_sample_base.py @@ -62,7 +62,7 @@ class ApiSampleTestBaseV21(testscenarios.WithScenarios, # any additional fixtures needed for this scenario _additional_fixtures = [] sample_dir = None - _project_id = True + _use_project_id = True scenarios = [ # test v2 with the v2.1 compatibility stack @@ -74,7 +74,7 @@ class ApiSampleTestBaseV21(testscenarios.WithScenarios, # test v2.18 code without project id ('v2_1_noproject_id', { 'api_major_version': 'v2.1', - '_project_id': False, + '_use_project_id': False, '_additional_fixtures': [ api_paste_fixture.ApiPasteNoProjectId]}) ] diff --git a/nova/tests/functional/api_sample_tests/api_samples/os-simple-tenant-usage/v2.40/simple-tenant-usage-get-all.json.tpl b/nova/tests/functional/api_sample_tests/api_samples/os-simple-tenant-usage/v2.40/simple-tenant-usage-get-all.json.tpl new file mode 100644 index 000000000000..fde1ac35190b --- /dev/null +++ b/nova/tests/functional/api_sample_tests/api_samples/os-simple-tenant-usage/v2.40/simple-tenant-usage-get-all.json.tpl @@ -0,0 +1,68 @@ +{ + "tenant_usages": [ + { + "server_usages": [ + { + "ended_at": null, + "flavor": "m1.tiny", + "hours": 1.0, + "instance_id": "1f1deceb-17b5-4c04-84c7-e0d4499c8f06", + "local_gb": 1, + "memory_mb": 512, + "name": "instance-3", + "started_at": "%(strtime)s", + "state": "active", + "tenant_id": "0000000e737461636b20342065000000", + "uptime": 3600, + "vcpus": 1 + } + ], + "start": "%(strtime)s", + "stop": "%(strtime)s", + "tenant_id": "0000000e737461636b20342065000000", + "total_hours": 1.0, + "total_local_gb_usage": 1.0, + "total_memory_mb_usage": 512.0, + "total_vcpus_usage": 1.0 + }, + { + "server_usages": [ + { + "ended_at": null, + "flavor": "m1.tiny", + "hours": 1.0, + "instance_id": "1f1deceb-17b5-4c04-84c7-e0d4499c8f00", + "local_gb": 1, + "memory_mb": 512, + "name": "instance-1", + "started_at": "%(strtime)s", + "state": "active", + "tenant_id": "6f70656e737461636b20342065766572", + "uptime": 3600, + "vcpus": 1 + }, + { + "ended_at": null, + "flavor": "m1.tiny", + "hours": 1.0, + "instance_id": "1f1deceb-17b5-4c04-84c7-e0d4499c8f03", + "local_gb": 1, + "memory_mb": 512, + "name": "instance-2", + "started_at": "%(strtime)s", + "state": "active", + "tenant_id": "6f70656e737461636b20342065766572", + "uptime": 3600, + "vcpus": 1 + } + ], + "start": "%(strtime)s", + "stop": "%(strtime)s", + "tenant_id": "6f70656e737461636b20342065766572", + "total_hours": 2.0, + "total_local_gb_usage": 2.0, + "total_memory_mb_usage": 1024.0, + "total_vcpus_usage": 2.0 + } + ] +} diff --git a/nova/tests/functional/api_sample_tests/test_simple_tenant_usage.py b/nova/tests/functional/api_sample_tests/test_simple_tenant_usage.py index 3f71248a4ec4..9f67dbd6d1a9 100644 --- a/nova/tests/functional/api_sample_tests/test_simple_tenant_usage.py +++ b/nova/tests/functional/api_sample_tests/test_simple_tenant_usage.py @@ -79,6 +79,9 @@ class SimpleTenantUsageV240Test(test_servers.ServersSampleBase): super(SimpleTenantUsageV240Test, self).setUp() self.api.microversion = self.microversion + self.project_id_0 = astb.PROJECT_ID + self.project_id_1 = '0000000e737461636b20342065000000' + started = timeutils.utcnow() now = started + datetime.timedelta(hours=1) @@ -87,8 +90,11 @@ class SimpleTenantUsageV240Test(test_servers.ServersSampleBase): # make uuids incrementing, so that sort order is deterministic uuid_format = '1f1deceb-17b5-4c04-84c7-e0d4499c8f%02d' mock_uuids.side_effect = [uuid_format % x for x in range(100)] + self.project_id = self.project_id_0 self.instance1_uuid = self._post_server(name='instance-1') self.instance2_uuid = self._post_server(name='instance-2') + + self.project_id = self.project_id_1 self.instance3_uuid = self._post_server(name='instance-3') timeutils.set_time_override(now) @@ -118,9 +124,27 @@ class SimpleTenantUsageV240Test(test_servers.ServersSampleBase): self._verify_response(template_name, {}, response, 200) def test_get_tenant_usage_details(self): - tenant_id = astb.PROJECT_ID + tenant_id = self.project_id_0 url = 'os-simple-tenant-usage/{tenant}?%s'.format(tenant=tenant_id) response = self._do_get(url % (parse.urlencode(self.query))) template_name = 'simple-tenant-usage-get-specific' - subs = {'tenant_id': self.api.project_id} + subs = {'tenant_id': self.project_id_0} self._verify_response(template_name, subs, response, 200) + + def test_get_tenants_usage_end_marker(self): + # When using the last server retrieved as a marker, + # the subsequent usages list should be empty (bug #1796689). + url = 'os-simple-tenant-usage?%s' + query = dict(detailed=1, + start=self.query['start'], + end=self.query['end']) + + response = self._do_get(url % (parse.urlencode(query))) + template_name = 'simple-tenant-usage-get-all' + self._verify_response(template_name, {}, response, 200) + + last_server = response.json()['tenant_usages'][-1]['server_usages'][-1] + query['marker'] = last_server['instance_id'] + + response = self._do_get(url % (parse.urlencode(query))) + self.assertEqual(0, len(response.json()['tenant_usages'])) diff --git a/nova/tests/functional/api_samples_test_base.py b/nova/tests/functional/api_samples_test_base.py index 130c064a86aa..c1a6ab3798be 100644 --- a/nova/tests/functional/api_samples_test_base.py +++ b/nova/tests/functional/api_samples_test_base.py @@ -303,6 +303,24 @@ class ApiSampleTestBase(integrated_helpers._IntegratedTestBase): return matched_value + @property + def project_id(self): + # We'll allow test cases to override the default project id. This is + # useful when using multiple tenants. + project_id = None + try: + project_id = self.api.project_id + except AttributeError: + pass + + return project_id or PROJECT_ID + + @project_id.setter + def project_id(self, project_id): + self.api.project_id = project_id + # Reset cached credentials + self.api.auth_result = None + def generalize_subs(self, subs, vanilla_regexes): """Give the test a chance to modify subs after the server response was verified, and before the on-disk doc/api_samples file is checked. @@ -317,17 +335,20 @@ class ApiSampleTestBase(integrated_helpers._IntegratedTestBase): def _update_links(self, sample_data): """Process sample data and update version specific links.""" # replace version urls - url_re = self._get_host() + "/v(2|2\.1)/" + PROJECT_ID + + project_id_exp = '(%s|%s)' % (PROJECT_ID, self.project_id) + + url_re = self._get_host() + "/v(2|2\.1)/" + project_id_exp new_url = self._get_host() + "/" + self.api_major_version - if self._project_id: - new_url += "/" + PROJECT_ID + if self._use_project_id: + new_url += "/" + self.project_id updated_data = re.sub(url_re, new_url, sample_data) # replace unversioned urls - url_re = self._get_host() + "/" + PROJECT_ID + url_re = self._get_host() + "/" + project_id_exp new_url = self._get_host() - if self._project_id: - new_url += "/" + PROJECT_ID + if self._use_project_id: + new_url += "/" + self.project_id updated_data = re.sub(url_re, new_url, updated_data) return updated_data @@ -461,17 +482,17 @@ class ApiSampleTestBase(integrated_helpers._IntegratedTestBase): def _get_compute_endpoint(self): # NOTE(sdague): "openstack" is stand in for project_id, it # should be more generic in future. - if self._project_id: - return '%s/%s' % (self._get_host(), PROJECT_ID) + if self._use_project_id: + return '%s/%s' % (self._get_host(), self.project_id) else: return self._get_host() def _get_vers_compute_endpoint(self): # NOTE(sdague): "openstack" is stand in for project_id, it # should be more generic in future. - if self._project_id: + if self._use_project_id: return '%s/%s/%s' % (self._get_host(), self.api_major_version, - PROJECT_ID) + self.project_id) else: return '%s/%s' % (self._get_host(), self.api_major_version) diff --git a/nova/tests/unit/api_samples_test_base/test_compare_result.py b/nova/tests/unit/api_samples_test_base/test_compare_result.py index de7cdce767f2..6066cf186d0e 100644 --- a/nova/tests/unit/api_samples_test_base/test_compare_result.py +++ b/nova/tests/unit/api_samples_test_base/test_compare_result.py @@ -43,7 +43,7 @@ class TestCompareResult(test.NoDBTestCase): # required by ApiSampleTestBase ast_instance.api_major_version = 'v2' - ast_instance._project_id = 'True' + ast_instance._use_project_id = 'True' # automagically create magic methods usually handled by test classes ast_instance.compute = mock.MagicMock() diff --git a/releasenotes/notes/fix-simple-tenant-usage-pagination-393ed6e7d0e31594.yaml b/releasenotes/notes/fix-simple-tenant-usage-pagination-393ed6e7d0e31594.yaml new file mode 100644 index 000000000000..b68db5e457c7 --- /dev/null +++ b/releasenotes/notes/fix-simple-tenant-usage-pagination-393ed6e7d0e31594.yaml @@ -0,0 +1,6 @@ +--- +fixes: + - | + The ``os-simple-tenant-usage`` pagination has been fixed. In some cases, + nova usage-list would have returned incorrect results because of this. + See bug https://launchpad.net/bugs/1796689 for details.