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

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
+        }
+    ]
+}

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 = []
 

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]})
     ]

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
+        }
+    ]
+}

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']))

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:
+        if self._use_project_id:
-            new_url += "/" + 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:
+        if self._use_project_id:
-            new_url += "/" + 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:
+        if self._use_project_id:
-            return '%s/%s' % (self._get_host(), 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)
 

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()

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.