Merge "compute: Only retrieve necessary images"

2 files changed, 43 insertions, 12 deletions

diff --git a/openstackclient/compute/v2/server.py b/openstackclient/compute/v2/server.py
index 02fed155..b939a270 100644
--- a/openstackclient/compute/v2/server.py
+++ b/openstackclient/compute/v2/server.py
@@ -2500,28 +2500,45 @@ class ListServer(command.Lister):
         data = compute_client.servers.list(
             search_opts=search_opts,
             marker=marker_id,
-            limit=parsed_args.limit)
+            limit=parsed_args.limit,
+        )
 
         images = {}
         flavors = {}
         if data and not parsed_args.no_name_lookup:
+            # partial responses from down cells will not have an image
+            # attribute so we use getattr
+            image_ids = {
+                s.image['id'] for s in data
+                if getattr(s, 'image', None) and s.image.get('id')
+            }
+
             # create a dict that maps image_id to image object, which is used
             # to display the "Image Name" column. Note that 'image.id' can be
             # empty for BFV instances and 'image' can be missing entirely if
             # there are infra failures
             if parsed_args.name_lookup_one_by_one or image_id:
-                for i_id in set(
-                    s.image['id'] for s in data
-                    if s.image and s.image.get('id')
-                ):
+                for image_id in image_ids:
                     # "Image Name" is not crucial, so we swallow any exceptions
                     try:
-                        images[i_id] = image_client.get_image(i_id)
+                        images[image_id] = image_client.get_image(image_id)
                     except Exception:
                         pass
             else:
                 try:
-                    images_list = image_client.images()
+                    # some deployments can have *loads* of images so we only
+                    # want to list the ones we care about. It would be better
+                    # to only retrun the *fields* we care about (name) but
+                    # glance doesn't support that
+                    # NOTE(stephenfin): This could result in super long URLs
+                    # but it seems unlikely to cause issues. Apache supports
+                    # URL lengths of up to 8190 characters by default, which
+                    # should allow for more than 220 unique image ID (different
+                    # servers are likely use the same image ID) in the filter.
+                    # Who'd need more than that in a single command?
+                    images_list = image_client.images(
+                        id=f"in:{','.join(image_ids)}"
+                    )
                     for i in images_list:
                         images[i.id] = i
                 except Exception:
diff --git a/openstackclient/tests/unit/compute/v2/test_server.py b/openstackclient/tests/unit/compute/v2/test_server.py
index 895d4b54..2e64e071 100644
--- a/openstackclient/tests/unit/compute/v2/test_server.py
+++ b/openstackclient/tests/unit/compute/v2/test_server.py
@@ -4449,8 +4449,8 @@ class TestServerList(_TestServerList):
         columns, data = self.cmd.take_action(parsed_args)
 
         self.servers_mock.list.assert_called_with(**self.kwargs)
-        self.assertEqual(0, self.images_mock.list.call_count)
-        self.assertEqual(0, self.flavors_mock.list.call_count)
+        self.images_mock.assert_not_called()
+        self.flavors_mock.list.assert_not_called()
         self.assertEqual(self.columns, columns)
         self.assertEqual(self.data, tuple(data))
 
@@ -4473,7 +4473,8 @@ class TestServerList(_TestServerList):
                 getattr(s, 'OS-EXT-AZ:availability_zone'),
                 getattr(s, 'OS-EXT-SRV-ATTR:host'),
                 s.Metadata,
-            ) for s in self.servers)
+            ) for s in self.servers
+        )
         arglist = [
             '--long',
         ]
@@ -4485,6 +4486,11 @@ class TestServerList(_TestServerList):
 
         columns, data = self.cmd.take_action(parsed_args)
         self.servers_mock.list.assert_called_with(**self.kwargs)
+        image_ids = {s.image['id'] for s in self.servers if s.image}
+        self.images_mock.assert_called_once_with(
+            id=f'in:{",".join(image_ids)}',
+        )
+        self.flavors_mock.list.assert_called_once_with(is_public=None)
         self.assertEqual(self.columns_long, columns)
         self.assertEqual(self.data, tuple(data))
 
@@ -4548,6 +4554,8 @@ class TestServerList(_TestServerList):
         columns, data = self.cmd.take_action(parsed_args)
 
         self.servers_mock.list.assert_called_with(**self.kwargs)
+        self.images_mock.assert_not_called()
+        self.flavors_mock.list.assert_not_called()
         self.assertEqual(self.columns, columns)
         self.assertEqual(self.data, tuple(data))
 
@@ -4576,6 +4584,8 @@ class TestServerList(_TestServerList):
         columns, data = self.cmd.take_action(parsed_args)
 
         self.servers_mock.list.assert_called_with(**self.kwargs)
+        self.images_mock.assert_not_called()
+        self.flavors_mock.list.assert_not_called()
         self.assertEqual(self.columns, columns)
         self.assertEqual(self.data, tuple(data))
 
@@ -4593,8 +4603,8 @@ class TestServerList(_TestServerList):
         columns, data = self.cmd.take_action(parsed_args)
 
         self.servers_mock.list.assert_called_with(**self.kwargs)
-        self.assertFalse(self.images_mock.list.call_count)
-        self.assertFalse(self.flavors_mock.list.call_count)
+        self.images_mock.assert_not_called()
+        self.flavors_mock.list.assert_not_called()
         self.get_image_mock.assert_called()
         self.flavors_mock.get.assert_called()
 
@@ -4618,6 +4628,8 @@ class TestServerList(_TestServerList):
 
         self.search_opts['image'] = self.image.id
         self.servers_mock.list.assert_called_with(**self.kwargs)
+        self.images_mock.assert_not_called()
+        self.flavors_mock.list.assert_called_once()
 
         self.assertEqual(self.columns, columns)
         self.assertEqual(self.data, tuple(data))
@@ -4638,6 +4650,8 @@ class TestServerList(_TestServerList):
 
         self.search_opts['flavor'] = self.flavor.id
         self.servers_mock.list.assert_called_with(**self.kwargs)
+        self.images_mock.assert_called_once()
+        self.flavors_mock.list.assert_not_called()
 
         self.assertEqual(self.columns, columns)
         self.assertEqual(self.data, tuple(data))