From 60a0e379a4fa629b6997699582d1ad8b102600ca Mon Sep 17 00:00:00 2001
From: Johannes Kulik <johannes.kulik@sap.com>
Date: Fri, 22 Sep 2023 16:31:57 +0200
Subject: [PATCH] volume: Support same_host, different_host hint as list

When creating a volume, the scheduler hints can be supplied as strings.
The "same_host" and "different_host" hints can also be supplied as a
list if affinity/anti-affinity to multiple volumes is requested [0]

The previously-used `KeyValueAction` only supplies strings as values - the
last one if multiple --hint contain the same key. An alternative already
used in `CreateServer` would be `KeyValueAppendAction`, but only a subset of
the scheduler hints accept lists, so we cannot use that in general.

Therefore, we create `KeyValueHintAction`. It contains both a
`KeyValueAction` and a `KeyValueAppendAction` object and calls the
appropriate action based on the beginning of the given values as defined
in `APPEND_KEYS`.

[0] https://github.com/sapcc/cinder/blob/d96b705774e8ca85c75d3d0292722da8fe8cb14f/cinder/api/schemas/scheduler_hints.py#L31-L65

Change-Id: Ida7f4662dc9fea24510758541fd4f2622b73bf31
---
 .../tests/unit/volume/v2/test_volume.py       | 68 +++++++++++++++++++
 openstackclient/volume/v2/volume.py           | 31 ++++++++-
 2 files changed, 96 insertions(+), 3 deletions(-)

diff --git a/openstackclient/tests/unit/volume/v2/test_volume.py b/openstackclient/tests/unit/volume/v2/test_volume.py
index 992b99742..ba92f5b33 100644
--- a/openstackclient/tests/unit/volume/v2/test_volume.py
+++ b/openstackclient/tests/unit/volume/v2/test_volume.py
@@ -686,6 +686,74 @@ class TestVolumeCreate(TestVolume):
             verifylist,
         )
 
+    def test_volume_create_hints(self):
+        """--hint needs to behave differently based on the given hint
+
+        different_host and same_host need to append to a list if given multiple
+        times. All other parameter are strings.
+        """
+        arglist = [
+            '--size',
+            str(self.new_volume.size),
+            '--hint',
+            'k=v',
+            '--hint',
+            'k=v2',
+            '--hint',
+            'same_host=v3',
+            '--hint',
+            'same_host=v4',
+            '--hint',
+            'different_host=v5',
+            '--hint',
+            'local_to_instance=v6',
+            '--hint',
+            'different_host=v7',
+            self.new_volume.name,
+        ]
+        verifylist = [
+            ('size', self.new_volume.size),
+            (
+                'hint',
+                {
+                    'k': 'v2',
+                    'same_host': ['v3', 'v4'],
+                    'local_to_instance': 'v6',
+                    'different_host': ['v5', 'v7'],
+                },
+            ),
+            ('name', self.new_volume.name),
+        ]
+        parsed_args = self.check_parser(self.cmd, arglist, verifylist)
+
+        # In base command class ShowOne in cliff, abstract method take_action()
+        # returns a two-part tuple with a tuple of column names and a tuple of
+        # data to be shown.
+        columns, data = self.cmd.take_action(parsed_args)
+
+        self.volumes_mock.create.assert_called_with(
+            size=self.new_volume.size,
+            snapshot_id=None,
+            name=self.new_volume.name,
+            description=None,
+            volume_type=None,
+            availability_zone=None,
+            metadata=None,
+            imageRef=None,
+            source_volid=None,
+            consistencygroup_id=None,
+            scheduler_hints={
+                'k': 'v2',
+                'same_host': ['v3', 'v4'],
+                'local_to_instance': 'v6',
+                'different_host': ['v5', 'v7'],
+            },
+            backup_id=None,
+        )
+
+        self.assertEqual(self.columns, columns)
+        self.assertCountEqual(self.datalist, data)
+
 
 class TestVolumeDelete(TestVolume):
     def setUp(self):
diff --git a/openstackclient/volume/v2/volume.py b/openstackclient/volume/v2/volume.py
index 43a64664b..096305543 100644
--- a/openstackclient/volume/v2/volume.py
+++ b/openstackclient/volume/v2/volume.py
@@ -33,6 +33,29 @@ from openstackclient.identity import common as identity_common
 LOG = logging.getLogger(__name__)
 
 
+class KeyValueHintAction(argparse.Action):
+    """Uses KeyValueAction or KeyValueAppendAction based on the given key"""
+
+    APPEND_KEYS = ('same_host', 'different_host')
+
+    def __init__(self, *args, **kwargs):
+        self._key_value_action = parseractions.KeyValueAction(*args, **kwargs)
+        self._key_value_append_action = parseractions.KeyValueAppendAction(
+            *args, **kwargs
+        )
+        super().__init__(*args, **kwargs)
+
+    def __call__(self, parser, namespace, values, option_string=None):
+        if values.startswith(self.APPEND_KEYS):
+            self._key_value_append_action(
+                parser, namespace, values, option_string=option_string
+            )
+        else:
+            self._key_value_action(
+                parser, namespace, values, option_string=option_string
+            )
+
+
 class AttachmentsColumn(cliff_columns.FormattableColumn):
     """Formattable column for attachments column.
 
@@ -162,10 +185,12 @@ class CreateVolume(command.ShowOne):
         parser.add_argument(
             "--hint",
             metavar="<key=value>",
-            action=parseractions.KeyValueAction,
+            action=KeyValueHintAction,
             help=_(
-                "Arbitrary scheduler hint key-value pairs to help boot "
-                "an instance (repeat option to set multiple hints)"
+                "Arbitrary scheduler hint key-value pairs to help creating "
+                "a volume. Repeat the option to set multiple hints. "
+                "'same_host' and 'different_host' get values appended when "
+                "repeated, all other keys take the last given value"
             ),
         )
         bootable_group = parser.add_mutually_exclusive_group()