Fix unspecified bahavior on GET /servers/detail?tenant_id=X as admin

When an admin calls /v2.1/servers/detail?tenant_id=XX, then the
`get_all` method of nova.compute.api.API is called with 2
conflicting search options: {'tenant_id': XX, 'project_id': YY}.
But because, latter on, in that `get_all` method we define a dict
called filter_mapping, on which we iter upon, which value takes
precedence depends on the order in which the dict is iterated upon.

As the order in unpredictable and varies between Py2 and Py3, this
is problematic. Especially, on Python 2 hash randomization is
disabled by default but it's enabled by default on Py3 (unless
the PYTHONHASHSEED env var is set to a fixed value) [0]

The (unreliable) order we iterate on items of that `filter_mapping`
dict is why the Tempest test_list_servers_by_admin_with_specified_tenant
test randomly fails on Py35.

This patch ensures that, if the all_tenants search option is
not set, then the `tenant_id` search option is ignored. Note that
this *is* the current behavior on Py27, as documented in lp:#1185290
and tested by Tempest here [1].

[0] https://docs.python.org/dev/using/cmdline.html#cmdoption-R
[1] fe1a8e289c/tempest/api/compute/admin/test_servers.py (L96)

Change-Id: I1a74ef0f16da14444029c0f184b433df367ffb41
Closes-Bug: #1659811

nova/api/openstack/compute/servers.py

@@ -319,6 +319,11 @@ class ServersController(wsgi.Controller):
                 context.can(server_policies.SERVERS % 'index:get_all_tenants')
             elevated = context.elevated()
         else:
+            # As explained in lp:#1185290, if `all_tenants` is not passed
+            # we must ignore the `tenant_id` search option. As explained
+            # in a above code comment, any change to this behavior would
+            # require a microversion bump.
+            search_opts.pop('tenant_id', None)
             if context.project_id:
                 search_opts['project_id'] = context.project_id
             else:

nova/tests/unit/api/openstack/compute/test_serversV21.py

@@ -779,6 +779,7 @@ class ServersControllerTest(ControllerTest):
     def test_tenant_id_filter_no_admin_context(self):
         def fake_get_all(context, search_opts=None, **kwargs):
             self.assertIsNotNone(search_opts)
+            self.assertNotIn('tenant_id', search_opts)
             self.assertEqual(search_opts['project_id'], 'fake')
             return [fakes.stub_instance_obj(100)]
 
@@ -788,6 +789,21 @@ class ServersControllerTest(ControllerTest):
             servers = self.controller.index(req)['servers']
         self.assertEqual(len(servers), 1)
 
+    def test_tenant_id_filter_admin_context(self):
+        """"Test tenant_id search opt is dropped if all_tenants is not set."""
+        def fake_get_all(context, search_opts=None, **kwargs):
+            self.assertIsNotNone(search_opts)
+            self.assertNotIn('tenant_id', search_opts)
+            self.assertEqual('fake', search_opts['project_id'])
+            return [fakes.stub_instance_obj(100)]
+
+        req = self.req('/fake/servers?tenant_id=newfake',
+                       use_admin_context=True)
+        with mock.patch.object(compute_api.API, 'get_all') as mock_get:
+            mock_get.side_effect = fake_get_all
+            servers = self.controller.index(req)['servers']
+        self.assertEqual(len(servers), 1)
+
     def test_all_tenants_param_normal(self):
         def fake_get_all(context, search_opts=None, **kwargs):
             self.assertNotIn('project_id', search_opts)