From d3bd0146ae64ebc3a7c5d7a3a2fc90efe4071495 Mon Sep 17 00:00:00 2001 From: Stephen Finucane Date: Fri, 5 Mar 2021 12:39:37 +0000 Subject: [PATCH] compute: Add support for loading BDMs from files The syntax of the '--block-device' parameter is complex and easily screwed up. Allow users to load a block device config from a file. For example: $ openstack server create ... --block-device file:///tmp/bdm.json ... This should alleviate the pain that is BDMv2 somewhat. No functional tests are provided since we already have tests for the CSV style of passing parameters and the unit tests show that the net result is the same. Change-Id: I3e3299bbdbbb343863b4c14fb4d9196ff3e1698d Signed-off-by: Stephen Finucane --- openstackclient/compute/v2/server.py | 80 +++++++++++++++--- .../tests/unit/compute/v2/test_server.py | 83 +++++++++++++++++++ 2 files changed, 153 insertions(+), 10 deletions(-) diff --git a/openstackclient/compute/v2/server.py b/openstackclient/compute/v2/server.py index b2ae93c807..cd6e2c9165 100644 --- a/openstackclient/compute/v2/server.py +++ b/openstackclient/compute/v2/server.py @@ -18,8 +18,10 @@ import argparse import getpass import io +import json import logging import os +import urllib.parse from cliff import columns as cliff_columns import iso8601 @@ -681,7 +683,7 @@ class NICAction(argparse.Action): class BDMLegacyAction(argparse.Action): def __call__(self, parser, namespace, values, option_string=None): - # Make sure we have an empty dict rather than None + # Make sure we have an empty list rather than None if getattr(namespace, self.dest, None) is None: setattr(namespace, self.dest, []) @@ -723,6 +725,68 @@ class BDMLegacyAction(argparse.Action): getattr(namespace, self.dest).append(mapping) +class BDMAction(parseractions.MultiKeyValueAction): + + def __init__(self, option_strings, dest, **kwargs): + required_keys = [] + optional_keys = [ + 'uuid', 'source_type', 'destination_type', + 'disk_bus', 'device_type', 'device_name', 'volume_size', + 'guest_format', 'boot_index', 'delete_on_termination', 'tag', + 'volume_type', + ] + super().__init__( + option_strings, dest, required_keys=required_keys, + optional_keys=optional_keys, **kwargs, + ) + + # TODO(stephenfin): Remove once I549d0897ef3704b7f47000f867d6731ad15d3f2b + # or similar lands in a release + def validate_keys(self, keys): + """Validate the provided keys. + + :param keys: A list of keys to validate. + """ + valid_keys = self.required_keys | self.optional_keys + invalid_keys = [k for k in keys if k not in valid_keys] + if invalid_keys: + msg = _( + "Invalid keys %(invalid_keys)s specified.\n" + "Valid keys are: %(valid_keys)s" + ) + raise argparse.ArgumentTypeError(msg % { + 'invalid_keys': ', '.join(invalid_keys), + 'valid_keys': ', '.join(valid_keys), + }) + + missing_keys = [k for k in self.required_keys if k not in keys] + if missing_keys: + msg = _( + "Missing required keys %(missing_keys)s.\n" + "Required keys are: %(required_keys)s" + ) + raise argparse.ArgumentTypeError(msg % { + 'missing_keys': ', '.join(missing_keys), + 'required_keys': ', '.join(self.required_keys), + }) + + def __call__(self, parser, namespace, values, option_string=None): + if getattr(namespace, self.dest, None) is None: + setattr(namespace, self.dest, []) + + if values.startswith('file://'): + path = urllib.parse.urlparse(values).path + with open(path) as fh: + data = json.load(fh) + + # Validate the keys - other validation is left to later + self.validate_keys(list(data)) + + getattr(namespace, self.dest, []).append(data) + else: + super().__call__(parser, namespace, values, option_string) + + class CreateServer(command.ShowOne): _description = _("Create a new server") @@ -829,19 +893,15 @@ class CreateServer(command.ShowOne): parser.add_argument( '--block-device', metavar='', - action=parseractions.MultiKeyValueAction, + action=BDMAction, dest='block_devices', default=[], - required_keys=[], - optional_keys=[ - 'uuid', 'source_type', 'destination_type', - 'disk_bus', 'device_type', 'device_name', 'volume_size', - 'guest_format', 'boot_index', 'delete_on_termination', 'tag', - 'volume_type', - ], help=_( 'Create a block device on the server.\n' - 'Block device in the format:\n' + 'Either a URI-style path (\'file:\\\\{path}\') to a JSON file ' + 'or a CSV-serialized string describing the block device ' + 'mapping.\n' + 'The following keys are accepted:\n' 'uuid=: UUID of the volume, snapshot or ID ' '(required if using source image, snapshot or volume),\n' 'source_type=: source type ' diff --git a/openstackclient/tests/unit/compute/v2/test_server.py b/openstackclient/tests/unit/compute/v2/test_server.py index 9666c5fdba..775ad0d911 100644 --- a/openstackclient/tests/unit/compute/v2/test_server.py +++ b/openstackclient/tests/unit/compute/v2/test_server.py @@ -16,6 +16,8 @@ import argparse import collections import copy import getpass +import json +import tempfile from unittest import mock from unittest.mock import call @@ -2169,6 +2171,87 @@ class TestServerCreate(TestServer): self.assertEqual(self.columns, columns) self.assertEqual(self.datalist(), data) + def test_server_create_with_block_device_from_file(self): + self.app.client_manager.compute.api_version = api_versions.APIVersion( + '2.67') + + block_device = { + 'uuid': self.volume.id, + 'source_type': 'volume', + 'destination_type': 'volume', + 'disk_bus': 'ide', + 'device_type': 'disk', + 'device_name': 'sdb', + 'guest_format': 'ext4', + 'volume_size': 64, + 'volume_type': 'foo', + 'boot_index': 1, + 'delete_on_termination': True, + 'tag': 'foo', + } + + with tempfile.NamedTemporaryFile(mode='w+') as fp: + json.dump(block_device, fp=fp) + fp.flush() + + arglist = [ + '--image', 'image1', + '--flavor', self.flavor.id, + '--block-device', f'file://{fp.name}', + self.new_server.name, + ] + verifylist = [ + ('image', 'image1'), + ('flavor', self.flavor.id), + ('block_devices', [block_device]), + ('server_name', self.new_server.name), + ] + parsed_args = self.check_parser(self.cmd, arglist, verifylist) + + # CreateServer.take_action() returns two tuples + columns, data = self.cmd.take_action(parsed_args) + + # Set expected values + kwargs = { + 'meta': None, + 'files': {}, + 'reservation_id': None, + 'min_count': 1, + 'max_count': 1, + 'security_groups': [], + 'userdata': None, + 'key_name': None, + 'availability_zone': None, + 'admin_pass': None, + 'block_device_mapping_v2': [{ + 'uuid': self.volume.id, + 'source_type': 'volume', + 'destination_type': 'volume', + 'disk_bus': 'ide', + 'device_name': 'sdb', + 'volume_size': 64, + 'guest_format': 'ext4', + 'boot_index': 1, + 'device_type': 'disk', + 'delete_on_termination': True, + 'tag': 'foo', + 'volume_type': 'foo', + }], + 'nics': 'auto', + 'scheduler_hints': {}, + 'config_drive': None, + } + # ServerManager.create(name, image, flavor, **kwargs) + self.servers_mock.create.assert_called_with( + self.new_server.name, + self.image, + self.flavor, + **kwargs + ) + + self.assertEqual(self.columns, columns) + self.assertEqual(self.datalist(), data) + def test_server_create_with_block_device_invalid_boot_index(self): block_device = \ f'uuid={self.volume.name},source_type=volume,boot_index=foo'