From 33627242e8f845934bcc5affb616108a79d28cbe Mon Sep 17 00:00:00 2001 From: Stephen Finucane Date: Wed, 25 Sep 2019 12:10:21 +0100 Subject: [PATCH] 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 Closes-Bug: #1845322 (cherry picked from commit 6954aacd54e85859fecde22ac04db1ce7601dd35) --- novaclient/tests/unit/v2/test_shell.py | 44 +++++++++++++++---- novaclient/v2/shell.py | 4 +- .../notes/bug-1845322-463ee407b60131c9.yaml | 6 +++ 3 files changed, 44 insertions(+), 10 deletions(-) create mode 100644 releasenotes/notes/bug-1845322-463ee407b60131c9.yaml diff --git a/novaclient/tests/unit/v2/test_shell.py b/novaclient/tests/unit/v2/test_shell.py index a57c37948..9f4d084db 100644 --- a/novaclient/tests/unit/v2/test_shell.py +++ b/novaclient/tests/unit/v2/test_shell.py @@ -85,17 +85,27 @@ class ShellTest(utils.TestCase): 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 @@ class ShellTest(utils.TestCase): 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 @@ class ShellTest(utils.TestCase): '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' % diff --git a/novaclient/v2/shell.py b/novaclient/v2/shell.py index 2dfd5c92d..b95c7d6d3 100644 --- a/novaclient/v2/shell.py +++ b/novaclient/v2/shell.py @@ -489,8 +489,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: @@ -810,6 +809,7 @@ def _boot(cs, args): '--hint', action='append', dest='scheduler_hints', + type=_key_value_pairing, default=[], metavar='', help=_("Send arbitrary key/value pairs to the scheduler for custom " diff --git a/releasenotes/notes/bug-1845322-463ee407b60131c9.yaml b/releasenotes/notes/bug-1845322-463ee407b60131c9.yaml new file mode 100644 index 000000000..9c60e72fb --- /dev/null +++ b/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.