Stop silently ignoring invalid 'nova boot --hint' options

The '--hint' option for 'nova boot' expects a key-value pair like so:

  nova boot --hint group=245e1dfe-2d0e-4139-80a9-fce124948896 ...

However, the command doesn't complain if this isn't the case, meaning
typos like the below aren't indicated to the user:

  nova boot --hint 245e1dfe-2d0e-4139-80a9-fce124948896

Due to how we'd implemented this here, this ultimately results in us
POSTing the following as part of the body to 'os-servers':

  {
    ...
    "OS-SCH-HNT:scheduler_hints": {
      "245e1dfe-2d0e-4139-80a9-fce124948896": null
    }
    ...
  }

Which is unfortunately allowed and ignored by nova due to the use of
'additionalProperties' in the schema [1]

Do what we do for loads of other options and explicitly fail on invalid
values.

NOTE(stephenfin): This includes the release note first added separately
in change I753e9a0cda1e118578373c519cf2fb2dd605a623.

[1] https://github.com/openstack/nova/blob/19.0.0/nova/api/openstack/compute/schemas/servers.py#L142-L146

Change-Id: I0f9f75cba68e7582d32d4aab2f8f077b4360d386
Signed-off-by: Stephen Finucane <[email protected]>
Closes-Bug: #1845322
(cherry picked from commit 6954aac)
(cherry picked from commit 3362724)

novaclient/tests/unit/v2/test_shell.py

@@ -85,17 +85,27 @@ def setUp(self):
         self.useFixture(fixtures.MonkeyPatch(
             'novaclient.client.Client', fakes.FakeClient))
 
+    # TODO(stephenfin): We should migrate most of the existing assertRaises
+    # calls to simply pass expected_error to this instead so we can easily
+    # capture and compare output
     @mock.patch('sys.stdout', new_callable=six.StringIO)
     @mock.patch('sys.stderr', new_callable=six.StringIO)
-    def run_command(self, cmd, mock_stderr, mock_stdout, api_version=None):
+    def run_command(self, cmd, mock_stderr, mock_stdout, api_version=None,
+                    expected_error=None):
         version_options = []
         if api_version:
             version_options.extend(["--os-compute-api-version", api_version,
                                     "--service-type", "computev21"])
-        if isinstance(cmd, list):
-            self.shell.main(version_options + cmd)
+        if not isinstance(cmd, list):
+            cmd = cmd.split()
+
+        if expected_error:
+            self.assertRaises(expected_error,
+                              self.shell.main,
+                              version_options + cmd)
         else:
-            self.shell.main(version_options + cmd.split())
+            self.shell.main(version_options + cmd)
+
         return mock_stdout.getvalue(), mock_stderr.getvalue()
 
     def assert_called(self, method, url, body=None, **kwargs):
@@ -748,9 +758,12 @@ def test_boot_with_incorrect_metadata(self):
         self.assertEqual(expected, result.args[0])
 
     def test_boot_hints(self):
-        self.run_command('boot --image %s --flavor 1 '
-                         '--hint a=b0=c0 --hint a=b1=c1 some-server ' %
-                         FAKE_UUID_1)
+        cmd = ('boot --image %s --flavor 1 '
+               '--hint same_host=a0cf03a5-d921-4877-bb5c-86d26cf818e1 '
+               '--hint same_host=8c19174f-4220-44f0-824a-cd1eeef10287 '
+               '--hint query=[>=,$free_ram_mb,1024] '
+               'some-server' % FAKE_UUID_1)
+        self.run_command(cmd)
         self.assert_called_anytime(
             'POST', '/servers',
             {
@@ -761,10 +774,25 @@ def test_boot_hints(self):
                     'min_count': 1,
                     'max_count': 1,
                 },
-                'os:scheduler_hints': {'a': ['b0=c0', 'b1=c1']},
+                'os:scheduler_hints': {
+                    'same_host': [
+                        'a0cf03a5-d921-4877-bb5c-86d26cf818e1',
+                        '8c19174f-4220-44f0-824a-cd1eeef10287',
+                    ],
+                    'query': '[>=,$free_ram_mb,1024]',
+                },
             },
         )
 
+    def test_boot_hints_invalid(self):
+        cmd = ('boot --image %s --flavor 1 '
+               '--hint a0cf03a5-d921-4877-bb5c-86d26cf818e1 '
+               'some-server' % FAKE_UUID_1)
+        _, err = self.run_command(cmd, expected_error=SystemExit)
+        self.assertIn("'a0cf03a5-d921-4877-bb5c-86d26cf818e1' is not in "
+                      "the format of 'key=value'",
+                      err)
+
     def test_boot_nic_auto_not_alone_after(self):
         cmd = ('boot --image %s --flavor 1 '
                '--nic auto,tag=foo some-server' %

novaclient/v2/shell.py

@@ -474,8 +474,7 @@ def _boot(cs, args):
 
     hints = {}
     if args.scheduler_hints:
-        for hint in args.scheduler_hints:
-            key, _sep, value = hint.partition('=')
+        for key, value in args.scheduler_hints:
             # NOTE(vish): multiple copies of the same hint will
             #             result in a list of values
             if key in hints:
@@ -789,6 +788,7 @@ def _boot(cs, args):
     '--hint',
     action='append',
     dest='scheduler_hints',
+    type=_key_value_pairing,
     default=[],
     metavar='<key=value>',
     help=_("Send arbitrary key/value pairs to the scheduler for custom "

releasenotes/notes/bug-1845322-463ee407b60131c9.yaml

@@ -0,0 +1,6 @@
+---
+upgrade:
+  - |
+    The ``--hint`` option for the ``boot`` command expects a key-value
+    argument. Previously, if this was not the case, the argument would be
+    silently ignored. It will now raise an error.