Call Glance list with certain image ids

This patch uses glance list filtering, which allow to query images
for the list of required ids. See [1] for details.

Patch rebased to master. Added test for proposed Glance API request

[1] https://developer.openstack.org/api-ref/image/v2/#show-images

Related-Bug: #1508554
Co-Authored-By: Ivan Kolodyazhny <e0ne@e0ne.info>
Co-Authored-By: Vadym Markov <markov.vadim@gmail.com>
Change-Id: Iac485c1b448011fec97ba5bfaa83851914b294d9

openstack_dashboard/api/glance.py

@@ -17,6 +17,7 @@
 #    under the License.
 
 from __future__ import absolute_import
+from __future__ import division
 
 import collections
 import itertools
@@ -56,6 +57,18 @@ try:
 except ImportError:
     pass
 
+# TODO(e0ne): remove this workaround once glanceclient will raise
+# RequestURITooLong exception
+
+# NOTE(e0ne): set MAX_URI_LEN to 8KB like Neutron does
+MAX_URI_LEN = 8192
+
+URI_FILTER_PREFIX = "/v2/images?id=in:"
+# NOTE(e0ne): 36 is a lengght of UUID, we need tp have '+' for sapparator.
+# Decreasing value by 1 to make sure it could be send to a server
+MAX_IMGAGES_PER_REQUEST = \
+    (MAX_URI_LEN - len(URI_FILTER_PREFIX)) // (36 + 1) - 1
+
 
 class Image(base.APIResourceWrapper):
     _attrs = {"architecture", "container_format", "disk_format", "created_at",
@@ -255,6 +268,19 @@ def image_get(request, image_id):
     return Image(image)
 
 
+@profiler.trace
+def image_list_detailed_by_ids(request, ids=None):
+    images = []
+    if not ids:
+        return images
+    for i in range(0, len(ids), MAX_IMGAGES_PER_REQUEST):
+        ids_to_filter = ids[i:i + MAX_IMGAGES_PER_REQUEST]
+        filters = {'id': 'in:' + ','.join(ids_to_filter)}
+        images.extend(image_list_detailed(request, filters=filters)[0])
+
+    return images
+
+
 @profiler.trace
 def image_list_detailed(request, marker=None, sort_dir='desc',
                         sort_key='created_at', filters=None, paginate=False,

openstack_dashboard/dashboards/admin/instances/tests.py

@@ -31,14 +31,16 @@ class InstanceViewTest(test.BaseAdminViewTests):
     @test.create_mocks({
         api.nova: ['flavor_list', 'server_list', 'extension_supported'],
         api.keystone: ['tenant_list'],
-        api.glance: ['image_list_detailed'],
+        api.glance: ['image_list_detailed_by_ids'],
     })
     def test_index(self):
         servers = self.servers.list()
+        # TODO(vmarkov) instances_img_ids should be in test_data
+        instances_img_ids = [instance.image.get('id') for instance in
+                             servers if isinstance(instance.image, dict)]
         self.mock_extension_supported.return_value = True
         self.mock_tenant_list.return_value = [self.tenants.list(), False]
-        self.mock_image_list_detailed.return_value = (self.images.list(),
+        self.mock_image_list_detailed_by_ids.return_value = self.images.list()
-                                                      False, False)
         self.mock_flavor_list.return_value = self.flavors.list()
         self.mock_server_list.return_value = [servers, False]
 
@@ -53,8 +55,8 @@ class InstanceViewTest(test.BaseAdminViewTests):
             mock.call('Shelve', test.IsHttpRequest())] * 4)
         self.assertEqual(12, self.mock_extension_supported.call_count)
         self.mock_tenant_list.assert_called_once_with(test.IsHttpRequest())
-        self.mock_image_list_detailed.assert_called_once_with(
+        self.mock_image_list_detailed_by_ids.assert_called_once_with(
-            test.IsHttpRequest())
+            test.IsHttpRequest(), instances_img_ids)
         self.mock_flavor_list.assert_called_once_with(test.IsHttpRequest())
         search_opts = {'marker': None, 'paginate': True, 'all_tenants': True}
         self.mock_server_list.assert_called_once_with(
@@ -64,11 +66,13 @@ class InstanceViewTest(test.BaseAdminViewTests):
         api.nova: ['flavor_list', 'flavor_get', 'server_list',
                    'extension_supported'],
         api.keystone: ['tenant_list'],
-        api.glance: ['image_list_detailed'],
+        api.glance: ['image_list_detailed_by_ids'],
     })
     def test_index_flavor_list_exception(self):
         servers = self.servers.list()
         flavors = self.flavors.list()
+        instances_img_ids = [instance.image.get('id') for instance in
+                             servers if hasattr(instance, 'image')]
         full_flavors = OrderedDict([(f.id, f) for f in flavors])
         self.mock_server_list.return_value = [servers, False]
         self.mock_extension_supported.return_value = True
@@ -79,8 +83,7 @@ class InstanceViewTest(test.BaseAdminViewTests):
             return full_flavors[id]
         self.mock_flavor_get.side_effect = _get_full_flavor
 
-        self.mock_image_list_detailed.return_value = (self.images.list(),
+        self.mock_image_list_detailed_by_ids.return_value = self.images.list()
-                                                      False, False)
 
         res = self.client.get(INDEX_URL)
 
@@ -101,24 +104,25 @@ class InstanceViewTest(test.BaseAdminViewTests):
         self.mock_flavor_get.assert_has_calls(
             [mock.call(test.IsHttpRequest(), s.flavor['id']) for s in servers])
         self.assertEqual(len(servers), self.mock_flavor_get.call_count)
-        self.mock_image_list_detailed.assert_called_once_with(
+        self.mock_image_list_detailed_by_ids.assert_called_once_with(
-            test.IsHttpRequest())
+            test.IsHttpRequest(), instances_img_ids)
 
     @test.create_mocks({
         api.nova: ['flavor_list', 'flavor_get', 'server_list',
                    'extension_supported'],
         api.keystone: ['tenant_list'],
-        api.glance: ['image_list_detailed'],
+        api.glance: ['image_list_detailed_by_ids'],
     })
     def test_index_flavor_get_exception(self):
         servers = self.servers.list()
+        instances_img_ids = [instance.image.get('id') for instance in
+                             servers if hasattr(instance, 'image')]
         # UUIDs generated using indexes are unlikely to match
         # any of existing flavor ids and are guaranteed to be deterministic.
         for i, server in enumerate(servers):
             server.flavor['id'] = str(uuid.UUID(int=i))
 
-        self.mock_image_list_detailed.return_value = \
+        self.mock_image_list_detailed_by_ids.return_value = self.images.list()
-            (self.images.list(), False, False)
         self.mock_flavor_list.return_value = self.flavors.list()
         self.mock_server_list.return_value = [servers, False]
         self.mock_extension_supported.return_value = True
@@ -134,8 +138,8 @@ class InstanceViewTest(test.BaseAdminViewTests):
         self.assertMessageCount(res, error=1)
         self.assertItemsEqual(instances, servers)
 
-        self.mock_image_list_detailed.assert_called_once_with(
+        self.mock_image_list_detailed_by_ids.assert_called_once_with(
-            test.IsHttpRequest())
+            test.IsHttpRequest(), instances_img_ids)
         self.mock_flavor_list.assert_called_once_with(test.IsHttpRequest())
         search_opts = {'marker': None, 'paginate': True, 'all_tenants': True}
         self.mock_server_list.assert_called_once_with(
@@ -153,14 +157,13 @@ class InstanceViewTest(test.BaseAdminViewTests):
     @test.create_mocks({
         api.nova: ['server_list', 'flavor_list'],
         api.keystone: ['tenant_list'],
-        api.glance: ['image_list_detailed'],
+        api.glance: ['image_list_detailed_by_ids'],
     })
     def test_index_server_list_exception(self):
         self.mock_server_list.side_effect = self.exceptions.nova
         self.mock_flavor_list.return_value = self.flavors.list()
         self.mock_tenant_list.return_value = [self.tenants.list(), False]
-        self.mock_image_list_detailed.return_value = (self.images.list(),
+        self.mock_image_list_detailed_by_ids.return_value = self.images.list()
-                                                      False, False)
 
         res = self.client.get(INDEX_URL)
         self.assertTemplateUsed(res, INDEX_TEMPLATE)
@@ -171,8 +174,8 @@ class InstanceViewTest(test.BaseAdminViewTests):
             test.IsHttpRequest(),
             search_opts=search_opts)
         self.mock_tenant_list.assert_called_once_with(test.IsHttpRequest())
-        self.mock_image_list_detailed.assert_called_once_with(
+        self.mock_image_list_detailed_by_ids.assert_called_once_with(
-            test.IsHttpRequest())
+            test.IsHttpRequest(), [])
         self.mock_flavor_list.assert_called_once_with(test.IsHttpRequest())
 
     @test.create_mocks({api.nova: ['server_get', 'flavor_get',
@@ -222,12 +225,14 @@ class InstanceViewTest(test.BaseAdminViewTests):
     @test.create_mocks({
         api.nova: ['flavor_list', 'server_list', 'extension_supported'],
         api.keystone: ['tenant_list'],
-        api.glance: ['image_list_detailed'],
+        api.glance: ['image_list_detailed_by_ids'],
     })
     def test_index_options_before_migrate(self):
+        servers = self.servers.list()
+        instances_img_ids = [instance.image.get('id') for instance in
+                             servers if hasattr(instance, 'image')]
         self.mock_tenant_list.return_value = [self.tenants.list(), False]
-        self.mock_image_list_detailed.return_value = (self.images.list(),
+        self.mock_image_list_detailed_by_ids.return_value = self.images.list()
-                                                      False, False)
         self.mock_flavor_list.return_value = self.flavors.list()
         self.mock_server_list.return_value = [self.servers.list(), False]
         self.mock_extension_supported.return_value = True
@@ -238,8 +243,8 @@ class InstanceViewTest(test.BaseAdminViewTests):
         self.assertNotContains(res, "instances__revert")
 
         self.mock_tenant_list.assert_called_once_with(test.IsHttpRequest())
-        self.mock_image_list_detailed.assert_called_once_with(
+        self.mock_image_list_detailed_by_ids.assert_called_once_with(
-            test.IsHttpRequest())
+            test.IsHttpRequest(), instances_img_ids)
         self.mock_flavor_list.assert_called_once_with(test.IsHttpRequest())
         search_opts = {'marker': None, 'paginate': True, 'all_tenants': True}
         self.mock_server_list.assert_called_once_with(
@@ -253,7 +258,7 @@ class InstanceViewTest(test.BaseAdminViewTests):
     @test.create_mocks({
         api.nova: ['flavor_list', 'server_list', 'extension_supported'],
         api.keystone: ['tenant_list'],
-        api.glance: ['image_list_detailed'],
+        api.glance: ['image_list_detailed_by_ids'],
     })
     def test_index_options_after_migrate(self):
         servers = self.servers.list()
@@ -261,9 +266,10 @@ class InstanceViewTest(test.BaseAdminViewTests):
         server1.status = "VERIFY_RESIZE"
         server2 = servers[2]
         server2.status = "VERIFY_RESIZE"
+        instances_img_ids = [instance.image.get('id') for instance in
+                             servers if hasattr(instance, 'image')]
         self.mock_tenant_list.return_value = [self.tenants.list(), False]
-        self.mock_image_list_detailed.return_value = (self.images.list(),
+        self.mock_image_list_detailed_by_ids.return_value = self.images.list()
-                                                      False, False)
         self.mock_flavor_list.return_value = self.flavors.list()
         self.mock_extension_supported.return_value = True
         self.mock_server_list.return_value = [servers, False]
@@ -274,8 +280,8 @@ class InstanceViewTest(test.BaseAdminViewTests):
         self.assertNotContains(res, "instances__migrate")
 
         self.mock_tenant_list.assert_called_once_with(test.IsHttpRequest())
-        self.mock_image_list_detailed.assert_called_once_with(
+        self.mock_image_list_detailed_by_ids.assert_called_once_with(
-            test.IsHttpRequest())
+            test.IsHttpRequest(), instances_img_ids)
         self.mock_flavor_list.assert_called_once_with(test.IsHttpRequest())
         self.mock_extension_supported.assert_has_calls([
             mock.call('AdminActions', test.IsHttpRequest()),

openstack_dashboard/dashboards/admin/instances/views.py

@@ -90,14 +90,19 @@ class AdminIndexView(tables.DataTableView):
             exceptions.handle(self.request, msg)
             return {}
 
-    def _get_images(self):
+    def _get_images(self, instances=()):
-        # Gather our images to correlate againts IDs
+        # Gather our images to correlate our instances to them
         try:
-            images, __, __ = api.glance.image_list_detailed(self.request)
+            # NOTE(aarefiev): request images, instances was booted from.
-            return dict([(image.id, image) for image in images])
+            img_ids = (instance.image.get('id') for instance in
+                       instances if isinstance(instance.image, dict))
+            real_img_ids = list(filter(None, img_ids))
+            images = api.glance.image_list_detailed_by_ids(
+                self.request, real_img_ids)
+            image_map = dict((image.id, image) for image in images)
+            return image_map
         except Exception:
-            msg = _("Unable to retrieve image list.")
+            exceptions.handle(self.request, ignore=True)
-            exceptions.handle(self.request, msg)
             return {}
 
     def _get_flavors(self):
@@ -123,8 +128,6 @@ class AdminIndexView(tables.DataTableView):
         return instances
 
     def get_data(self):
-        instances = []
-
         marker = self.request.GET.get(
             project_tables.AdminInstancesTable._meta.pagination_param, None)
         default_search_opts = {'marker': marker,
@@ -145,8 +148,11 @@ class AdminIndexView(tables.DataTableView):
 
         self._needs_filter_first = False
 
+        instances = self._get_instances(search_opts)
         results = futurist_utils.call_functions_parallel(
-            self._get_images, self._get_flavors, self._get_tenants)
+                (self._get_images, [tuple(instances)]),
+                self._get_flavors,
+                self._get_tenants)
         image_dict, flavor_dict, tenant_dict = results
 
         non_api_filter_info = (
@@ -158,8 +164,6 @@ class AdminIndexView(tables.DataTableView):
             self._more = False
             return []
 
-        instances = self._get_instances(search_opts)
-
         # Loop through instances to get image, flavor and tenant info.
         for inst in instances:
             if hasattr(inst, 'image') and isinstance(inst.image, dict):

openstack_dashboard/test/unit/api/test_glance.py

@@ -30,6 +30,39 @@ class GlanceApiTests(test.APIMockTestCase):
         super(GlanceApiTests, self).setUp()
         api.glance.VERSIONS.clear_active_cache()
 
+    @override_settings(API_RESULT_PAGE_SIZE=2)
+    @mock.patch.object(api.glance, 'glanceclient')
+    def test_long_url(self, mock_glanceclient):
+        servers = self.servers.list()*100
+        api_images = self.images_api.list()*100
+        instances_img_ids = [instance.image.get('id') for instance in
+                             servers if hasattr(instance, 'image')]
+        expected_images = self.images.list()*100
+        glanceclient = mock_glanceclient.return_value
+        mock_images_list = glanceclient.images.list
+        mock_images_list.return_value = iter(api_images)
+
+        images = api.glance.image_list_detailed_by_ids(self.request,
+                                                       instances_img_ids)
+        self.assertEqual(images, expected_images)
+
+    @override_settings(API_RESULT_PAGE_SIZE=2)
+    @mock.patch.object(api.glance, 'glanceclient')
+    def test_image_list_detailed_by_ids(self, mock_glanceclient):
+        servers = self.servers.list()
+        api_images = self.images_api.list()
+        instances_img_ids = [instance.image.get('id') for instance in
+                             servers if hasattr(instance, 'image')]
+        expected_images = self.images.list()
+
+        glanceclient = mock_glanceclient.return_value
+        mock_images_list = glanceclient.images.list
+        mock_images_list.return_value = iter(api_images)
+
+        images = api.glance.image_list_detailed_by_ids(self.request,
+                                                       instances_img_ids)
+        self.assertEqual(images, expected_images)
+
     @override_settings(API_RESULT_PAGE_SIZE=2)
     @mock.patch.object(api.glance, 'glanceclient')
     def test_image_list_detailed_no_pagination(self, mock_glanceclient):