From 885f5912eb531bc5594f98f834f2920a8e103c91 Mon Sep 17 00:00:00 2001
From: Stephen Finucane <stephenfin@redhat.com>
Date: Wed, 15 Nov 2023 11:19:14 +0000
Subject: [PATCH] compute: Address bug in shelve offload logic

We were reusing a variable from a previous loop, which meant this would
never work with multiple servers. Correct the mistake.

Change-Id: I52246e183fb2cf0d855d92058dd305b48783589d
Signed-off-by: Stephen Finucane <stephenfin@redhat.com>
---
 openstackclient/compute/v2/server.py          | 28 +++++++++----------
 .../tests/unit/compute/v2/test_server.py      | 18 +++++++-----
 2 files changed, 25 insertions(+), 21 deletions(-)

diff --git a/openstackclient/compute/v2/server.py b/openstackclient/compute/v2/server.py
index 4854e91f8c..5816f3600a 100644
--- a/openstackclient/compute/v2/server.py
+++ b/openstackclient/compute/v2/server.py
@@ -4416,6 +4416,7 @@ class ShelveServer(command.Command):
                 self.app.stdout.flush()
 
         compute_client = self.app.client_manager.sdk_connection.compute
+        server_ids = []
 
         for server in parsed_args.servers:
             server_obj = compute_client.find_server(
@@ -4425,6 +4426,8 @@ class ShelveServer(command.Command):
             if server_obj.status.lower() in ('shelved', 'shelved_offloaded'):
                 continue
 
+            server_ids.append(server_obj.id)
+
             compute_client.shelve_server(server_obj.id)
 
         # if we don't have to wait, either because it was requested explicitly
@@ -4432,54 +4435,51 @@ class ShelveServer(command.Command):
         if not parsed_args.wait and not parsed_args.offload:
             return
 
-        for server in parsed_args.servers:
+        for server_id in server_ids:
             # We use osc-lib's wait_for_status since that allows for a callback
             # TODO(stephenfin): We should wait for these in parallel using e.g.
             # https://review.opendev.org/c/openstack/osc-lib/+/762503/
             if not utils.wait_for_status(
                 compute_client.get_server,
-                server_obj.id,
+                server_id,
                 success_status=('shelved', 'shelved_offloaded'),
                 callback=_show_progress,
             ):
-                LOG.error(_('Error shelving server: %s'), server_obj.id)
+                LOG.error(_('Error shelving server: %s'), server_id)
                 self.app.stdout.write(
-                    _('Error shelving server: %s\n') % server_obj.id
+                    _('Error shelving server: %s\n') % server_id
                 )
                 raise SystemExit
 
         if not parsed_args.offload:
             return
 
-        for server in parsed_args.servers:
-            server_obj = compute_client.find_server(
-                server,
-                ignore_missing=False,
-            )
+        for server_id in server_ids:
+            server_obj = compute_client.get_server(server_id)
             if server_obj.status.lower() == 'shelved_offloaded':
                 continue
 
-            compute_client.shelve_offload_server(server_obj.id)
+            compute_client.shelve_offload_server(server_id)
 
         if not parsed_args.wait:
             return
 
-        for server in parsed_args.servers:
+        for server_id in server_ids:
             # We use osc-lib's wait_for_status since that allows for a callback
             # TODO(stephenfin): We should wait for these in parallel using e.g.
             # https://review.opendev.org/c/openstack/osc-lib/+/762503/
             if not utils.wait_for_status(
                 compute_client.get_server,
-                server_obj.id,
+                server_id,
                 success_status=('shelved_offloaded',),
                 callback=_show_progress,
             ):
                 LOG.error(
                     _('Error offloading shelved server %s'),
-                    server_obj.id,
+                    server_id,
                 )
                 self.app.stdout.write(
-                    _('Error offloading shelved server: %s\n') % server_obj.id
+                    _('Error offloading shelved server: %s\n') % server_id
                 )
                 raise SystemExit
 
diff --git a/openstackclient/tests/unit/compute/v2/test_server.py b/openstackclient/tests/unit/compute/v2/test_server.py
index 03237e90f5..032560a2d2 100644
--- a/openstackclient/tests/unit/compute/v2/test_server.py
+++ b/openstackclient/tests/unit/compute/v2/test_server.py
@@ -8149,20 +8149,24 @@ class TestServerShelve(TestServer):
         result = self.cmd.take_action(parsed_args)
         self.assertIsNone(result)
 
-        # two calls - one to retrieve the server state before shelving and
-        # another to do this before offloading
-        self.compute_sdk_client.find_server.assert_has_calls(
-            [
-                mock.call(self.server.name, ignore_missing=False),
-                mock.call(self.server.name, ignore_missing=False),
-            ]
+        # one call to retrieve to retrieve the server state before shelving
+        self.compute_sdk_client.find_server.assert_called_once_with(
+            self.server.name,
+            ignore_missing=False,
         )
+        # one call to retrieve the server state before offloading
+        self.compute_sdk_client.get_server.assert_called_once_with(
+            self.server.id
+        )
+        # one call to shelve the server
         self.compute_sdk_client.shelve_server.assert_called_with(
             self.server.id
         )
+        # one call to shelve offload the server
         self.compute_sdk_client.shelve_offload_server.assert_called_once_with(
             self.server.id,
         )
+        # one call to wait for the shelve offload to complete
         mock_wait_for_status.assert_called_once_with(
             self.compute_sdk_client.get_server,
             self.server.id,