Skip to content

Commit d3bd014

Browse files
stephenfinstephenfin
committed
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 <sfinucan@redhat.com>
1 parent 7c1d6f7 commit d3bd014

2 files changed

Lines changed: 153 additions & 10 deletions

File tree

‎openstackclient/compute/v2/server.py‎

Lines changed: 70 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -18,8 +18,10 @@
1818
import argparse
1919
import getpass
2020
import io
21+
import json
2122
import logging
2223
import os
24+
import urllib.parse
2325

2426
from cliff import columns as cliff_columns
2527
import iso8601
@@ -681,7 +683,7 @@ def __call__(self, parser, namespace, values, option_string=None):
681683
class BDMLegacyAction(argparse.Action):
682684

683685
def __call__(self, parser, namespace, values, option_string=None):
684-
# Make sure we have an empty dict rather than None
686+
# Make sure we have an empty list rather than None
685687
if getattr(namespace, self.dest, None) is None:
686688
setattr(namespace, self.dest, [])
687689

@@ -723,6 +725,68 @@ def __call__(self, parser, namespace, values, option_string=None):
723725
getattr(namespace, self.dest).append(mapping)
724726

725727

728+
class BDMAction(parseractions.MultiKeyValueAction):
729+
730+
def __init__(self, option_strings, dest, **kwargs):
731+
required_keys = []
732+
optional_keys = [
733+
'uuid', 'source_type', 'destination_type',
734+
'disk_bus', 'device_type', 'device_name', 'volume_size',
735+
'guest_format', 'boot_index', 'delete_on_termination', 'tag',
736+
'volume_type',
737+
]
738+
super().__init__(
739+
option_strings, dest, required_keys=required_keys,
740+
optional_keys=optional_keys, **kwargs,
741+
)
742+
743+
# TODO(stephenfin): Remove once I549d0897ef3704b7f47000f867d6731ad15d3f2b
744+
# or similar lands in a release
745+
def validate_keys(self, keys):
746+
"""Validate the provided keys.
747+
748+
:param keys: A list of keys to validate.
749+
"""
750+
valid_keys = self.required_keys | self.optional_keys
751+
invalid_keys = [k for k in keys if k not in valid_keys]
752+
if invalid_keys:
753+
msg = _(
754+
"Invalid keys %(invalid_keys)s specified.\n"
755+
"Valid keys are: %(valid_keys)s"
756+
)
757+
raise argparse.ArgumentTypeError(msg % {
758+
'invalid_keys': ', '.join(invalid_keys),
759+
'valid_keys': ', '.join(valid_keys),
760+
})
761+
762+
missing_keys = [k for k in self.required_keys if k not in keys]
763+
if missing_keys:
764+
msg = _(
765+
"Missing required keys %(missing_keys)s.\n"
766+
"Required keys are: %(required_keys)s"
767+
)
768+
raise argparse.ArgumentTypeError(msg % {
769+
'missing_keys': ', '.join(missing_keys),
770+
'required_keys': ', '.join(self.required_keys),
771+
})
772+
773+
def __call__(self, parser, namespace, values, option_string=None):
774+
if getattr(namespace, self.dest, None) is None:
775+
setattr(namespace, self.dest, [])
776+
777+
if values.startswith('file://'):
778+
path = urllib.parse.urlparse(values).path
779+
with open(path) as fh:
780+
data = json.load(fh)
781+
782+
# Validate the keys - other validation is left to later
783+
self.validate_keys(list(data))
784+
785+
getattr(namespace, self.dest, []).append(data)
786+
else:
787+
super().__call__(parser, namespace, values, option_string)
788+
789+
726790
class CreateServer(command.ShowOne):
727791
_description = _("Create a new server")
728792

@@ -829,19 +893,15 @@ def get_parser(self, prog_name):
829893
parser.add_argument(
830894
'--block-device',
831895
metavar='',
832-
action=parseractions.MultiKeyValueAction,
896+
action=BDMAction,
833897
dest='block_devices',
834898
default=[],
835-
required_keys=[],
836-
optional_keys=[
837-
'uuid', 'source_type', 'destination_type',
838-
'disk_bus', 'device_type', 'device_name', 'volume_size',
839-
'guest_format', 'boot_index', 'delete_on_termination', 'tag',
840-
'volume_type',
841-
],
842899
help=_(
843900
'Create a block device on the server.\n'
844-
'Block device in the format:\n'
901+
'Either a URI-style path (\'file:\\\\{path}\') to a JSON file '
902+
'or a CSV-serialized string describing the block device '
903+
'mapping.\n'
904+
'The following keys are accepted:\n'
845905
'uuid=<uuid>: UUID of the volume, snapshot or ID '
846906
'(required if using source image, snapshot or volume),\n'
847907
'source_type=<source_type>: source type '

‎openstackclient/tests/unit/compute/v2/test_server.py‎

Lines changed: 83 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,8 @@
1616
import collections
1717
import copy
1818
import getpass
19+
import json
20+
import tempfile
1921
from unittest import mock
2022
from unittest.mock import call
2123

@@ -2169,6 +2171,87 @@ def test_server_create_with_block_device_full(self):
21692171
self.assertEqual(self.columns, columns)
21702172
self.assertEqual(self.datalist(), data)
21712173

2174+
def test_server_create_with_block_device_from_file(self):
2175+
self.app.client_manager.compute.api_version = api_versions.APIVersion(
2176+
'2.67')
2177+
2178+
block_device = {
2179+
'uuid': self.volume.id,
2180+
'source_type': 'volume',
2181+
'destination_type': 'volume',
2182+
'disk_bus': 'ide',
2183+
'device_type': 'disk',
2184+
'device_name': 'sdb',
2185+
'guest_format': 'ext4',
2186+
'volume_size': 64,
2187+
'volume_type': 'foo',
2188+
'boot_index': 1,
2189+
'delete_on_termination': True,
2190+
'tag': 'foo',
2191+
}
2192+
2193+
with tempfile.NamedTemporaryFile(mode='w+') as fp:
2194+
json.dump(block_device, fp=fp)
2195+
fp.flush()
2196+
2197+
arglist = [
2198+
'--image', 'image1',
2199+
'--flavor', self.flavor.id,
2200+
'--block-device', f'file://{fp.name}',
2201+
self.new_server.name,
2202+
]
2203+
verifylist = [
2204+
('image', 'image1'),
2205+
('flavor', self.flavor.id),
2206+
('block_devices', [block_device]),
2207+
('server_name', self.new_server.name),
2208+
]
2209+
parsed_args = self.check_parser(self.cmd, arglist, verifylist)
2210+
2211+
# CreateServer.take_action() returns two tuples
2212+
columns, data = self.cmd.take_action(parsed_args)
2213+
2214+
# Set expected values
2215+
kwargs = {
2216+
'meta': None,
2217+
'files': {},
2218+
'reservation_id': None,
2219+
'min_count': 1,
2220+
'max_count': 1,
2221+
'security_groups': [],
2222+
'userdata': None,
2223+
'key_name': None,
2224+
'availability_zone': None,
2225+
'admin_pass': None,
2226+
'block_device_mapping_v2': [{
2227+
'uuid': self.volume.id,
2228+
'source_type': 'volume',
2229+
'destination_type': 'volume',
2230+
'disk_bus': 'ide',
2231+
'device_name': 'sdb',
2232+
'volume_size': 64,
2233+
'guest_format': 'ext4',
2234+
'boot_index': 1,
2235+
'device_type': 'disk',
2236+
'delete_on_termination': True,
2237+
'tag': 'foo',
2238+
'volume_type': 'foo',
2239+
}],
2240+
'nics': 'auto',
2241+
'scheduler_hints': {},
2242+
'config_drive': None,
2243+
}
2244+
# ServerManager.create(name, image, flavor, **kwargs)
2245+
self.servers_mock.create.assert_called_with(
2246+
self.new_server.name,
2247+
self.image,
2248+
self.flavor,
2249+
**kwargs
2250+
)
2251+
2252+
self.assertEqual(self.columns, columns)
2253+
self.assertEqual(self.datalist(), data)
2254+
21722255
def test_server_create_with_block_device_invalid_boot_index(self):
21732256
block_device = \
21742257
f'uuid={self.volume.name},source_type=volume,boot_index=foo'

0 commit comments

Comments
 (0)