Skip to content

Commit 433ceff

Browse files
Zuulopenstack-gerrit
Zuul
authored and
openstack-gerrit
committed
Merge "compute: Improve 'server create --block-device-mapping' option parsing"
2 parents 1f6104c + 074e045 commit 433ceff

2 files changed

Lines changed: 153 additions & 86 deletions

File tree

‎openstackclient/compute/v2/server.py‎

Lines changed: 77 additions & 57 deletions
Original file line numberDiff line numberDiff line change
@@ -678,6 +678,51 @@ def __call__(self, parser, namespace, values, option_string=None):
678678
getattr(namespace, self.dest).append(info)
679679

680680

681+
class BDMLegacyAction(argparse.Action):
682+
683+
def __call__(self, parser, namespace, values, option_string=None):
684+
# Make sure we have an empty dict rather than None
685+
if getattr(namespace, self.dest, None) is None:
686+
setattr(namespace, self.dest, [])
687+
688+
dev_name, sep, dev_map = values.partition('=')
689+
dev_map = dev_map.split(':') if dev_map else dev_map
690+
if not dev_name or not dev_map or len(dev_map) > 4:
691+
msg = _(
692+
"Invalid argument %s; argument must be of form "
693+
"'dev-name=id[:type[:size[:delete-on-terminate]]]'"
694+
)
695+
raise argparse.ArgumentTypeError(msg % values)
696+
697+
mapping = {
698+
'device_name': dev_name,
699+
# store target; this may be a name and will need verification later
700+
'uuid': dev_map[0],
701+
'source_type': 'volume',
702+
'destination_type': 'volume',
703+
}
704+
705+
# decide source and destination type
706+
if len(dev_map) > 1 and dev_map[1]:
707+
if dev_map[1] not in ('volume', 'snapshot', 'image'):
708+
msg = _(
709+
"Invalid argument %s; 'type' must be one of: volume, "
710+
"snapshot, image"
711+
)
712+
raise argparse.ArgumentTypeError(msg % values)
713+
714+
mapping['source_type'] = dev_map[1]
715+
716+
# 3. append size and delete_on_termination, if present
717+
if len(dev_map) > 2 and dev_map[2]:
718+
mapping['volume_size'] = dev_map[2]
719+
720+
if len(dev_map) > 3 and dev_map[3]:
721+
mapping['delete_on_termination'] = dev_map[3]
722+
723+
getattr(namespace, self.dest).append(mapping)
724+
725+
681726
class CreateServer(command.ShowOne):
682727
_description = _("Create a new server")
683728

@@ -745,8 +790,8 @@ def get_parser(self, prog_name):
745790
parser.add_argument(
746791
'--block-device-mapping',
747792
metavar='<dev-name=mapping>',
748-
action=parseractions.KeyValueAction,
749-
default={},
793+
action=BDMLegacyAction,
794+
default=[],
750795
# NOTE(RuiChen): Add '\n' to the end of line to improve formatting;
751796
# see cliff's _SmartHelpFormatter for more details.
752797
help=_(
@@ -1123,62 +1168,35 @@ def _match_image(image_api, wanted_properties):
11231168
# If booting from volume we do not pass an image to compute.
11241169
image = None
11251170

1126-
boot_args = [parsed_args.server_name, image, flavor]
1127-
11281171
# Handle block device by device name order, like: vdb -> vdc -> vdd
1129-
for dev_name in sorted(parsed_args.block_device_mapping):
1130-
dev_map = parsed_args.block_device_mapping[dev_name]
1131-
dev_map = dev_map.split(':')
1132-
if dev_map[0]:
1133-
mapping = {'device_name': dev_name}
1134-
1135-
# 1. decide source and destination type
1136-
if (len(dev_map) > 1 and
1137-
dev_map[1] in ('volume', 'snapshot', 'image')):
1138-
mapping['source_type'] = dev_map[1]
1139-
else:
1140-
mapping['source_type'] = 'volume'
1141-
1142-
mapping['destination_type'] = 'volume'
1143-
1144-
# 2. check target exist, update target uuid according by
1145-
# source type
1146-
if mapping['source_type'] == 'volume':
1147-
volume_id = utils.find_resource(
1148-
volume_client.volumes, dev_map[0]).id
1149-
mapping['uuid'] = volume_id
1150-
elif mapping['source_type'] == 'snapshot':
1151-
snapshot_id = utils.find_resource(
1152-
volume_client.volume_snapshots, dev_map[0]).id
1153-
mapping['uuid'] = snapshot_id
1154-
elif mapping['source_type'] == 'image':
1155-
# NOTE(mriedem): In case --image is specified with the same
1156-
# image, that becomes the root disk for the server. If the
1157-
# block device is specified with a root device name, e.g.
1158-
# vda, then the compute API will likely fail complaining
1159-
# that there is a conflict. So if using the same image ID,
1160-
# which doesn't really make sense but it's allowed, the
1161-
# device name would need to be a non-root device, e.g. vdb.
1162-
# Otherwise if the block device image is different from the
1163-
# one specified by --image, then the compute service will
1164-
# create a volume from the image and attach it to the
1165-
# server as a non-root volume.
1166-
image_id = image_client.find_image(dev_map[0],
1167-
ignore_missing=False).id
1168-
mapping['uuid'] = image_id
1169-
1170-
# 3. append size and delete_on_termination if exist
1171-
if len(dev_map) > 2 and dev_map[2]:
1172-
mapping['volume_size'] = dev_map[2]
1173-
1174-
if len(dev_map) > 3 and dev_map[3]:
1175-
mapping['delete_on_termination'] = dev_map[3]
1176-
else:
1177-
msg = _(
1178-
'Volume, volume snapshot or image (name or ID) must '
1179-
'be specified if --block-device-mapping is specified'
1180-
)
1181-
raise exceptions.CommandError(msg)
1172+
for mapping in parsed_args.block_device_mapping:
1173+
if mapping['source_type'] == 'volume':
1174+
volume_id = utils.find_resource(
1175+
volume_client.volumes, mapping['uuid'],
1176+
).id
1177+
mapping['uuid'] = volume_id
1178+
elif mapping['source_type'] == 'snapshot':
1179+
snapshot_id = utils.find_resource(
1180+
volume_client.volume_snapshots, mapping['uuid'],
1181+
).id
1182+
mapping['uuid'] = snapshot_id
1183+
elif mapping['source_type'] == 'image':
1184+
# NOTE(mriedem): In case --image is specified with the same
1185+
# image, that becomes the root disk for the server. If the
1186+
# block device is specified with a root device name, e.g.
1187+
# vda, then the compute API will likely fail complaining
1188+
# that there is a conflict. So if using the same image ID,
1189+
# which doesn't really make sense but it's allowed, the
1190+
# device name would need to be a non-root device, e.g. vdb.
1191+
# Otherwise if the block device image is different from the
1192+
# one specified by --image, then the compute service will
1193+
# create a volume from the image and attach it to the
1194+
# server as a non-root volume.
1195+
image_id = image_client.find_image(
1196+
mapping['uuid'], ignore_missing=False,
1197+
).id
1198+
mapping['uuid'] = image_id
1199+
11821200
block_device_mapping_v2.append(mapping)
11831201

11841202
nics = parsed_args.nics
@@ -1281,6 +1299,8 @@ def _match_image(image_api, wanted_properties):
12811299
else:
12821300
config_drive = parsed_args.config_drive
12831301

1302+
boot_args = [parsed_args.server_name, image, flavor]
1303+
12841304
boot_kwargs = dict(
12851305
meta=parsed_args.properties,
12861306
files=files,

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

Lines changed: 76 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -1935,7 +1935,15 @@ def test_server_create_with_block_device_mapping(self):
19351935
verifylist = [
19361936
('image', 'image1'),
19371937
('flavor', self.flavor.id),
1938-
('block_device_mapping', {'vda': self.volume.name + ':::false'}),
1938+
('block_device_mapping', [
1939+
{
1940+
'device_name': 'vda',
1941+
'uuid': self.volume.name,
1942+
'source_type': 'volume',
1943+
'destination_type': 'volume',
1944+
'delete_on_termination': 'false',
1945+
}
1946+
]),
19391947
('config_drive', False),
19401948
('server_name', self.new_server.name),
19411949
]
@@ -1988,7 +1996,14 @@ def test_server_create_with_block_device_mapping_min_input(self):
19881996
verifylist = [
19891997
('image', 'image1'),
19901998
('flavor', self.flavor.id),
1991-
('block_device_mapping', {'vdf': self.volume.name}),
1999+
('block_device_mapping', [
2000+
{
2001+
'device_name': 'vdf',
2002+
'uuid': self.volume.name,
2003+
'source_type': 'volume',
2004+
'destination_type': 'volume',
2005+
}
2006+
]),
19922007
('config_drive', False),
19932008
('server_name', self.new_server.name),
19942009
]
@@ -2040,7 +2055,14 @@ def test_server_create_with_block_device_mapping_default_input(self):
20402055
verifylist = [
20412056
('image', 'image1'),
20422057
('flavor', self.flavor.id),
2043-
('block_device_mapping', {'vdf': self.volume.name + ':::'}),
2058+
('block_device_mapping', [
2059+
{
2060+
'device_name': 'vdf',
2061+
'uuid': self.volume.name,
2062+
'source_type': 'volume',
2063+
'destination_type': 'volume',
2064+
}
2065+
]),
20442066
('config_drive', False),
20452067
('server_name', self.new_server.name),
20462068
]
@@ -2093,8 +2115,16 @@ def test_server_create_with_block_device_mapping_full_input(self):
20932115
verifylist = [
20942116
('image', 'image1'),
20952117
('flavor', self.flavor.id),
2096-
('block_device_mapping',
2097-
{'vde': self.volume.name + ':volume:3:true'}),
2118+
('block_device_mapping', [
2119+
{
2120+
'device_name': 'vde',
2121+
'uuid': self.volume.name,
2122+
'source_type': 'volume',
2123+
'destination_type': 'volume',
2124+
'volume_size': '3',
2125+
'delete_on_termination': 'true',
2126+
}
2127+
]),
20982128
('config_drive', False),
20992129
('server_name', self.new_server.name),
21002130
]
@@ -2149,8 +2179,16 @@ def test_server_create_with_block_device_mapping_snapshot(self):
21492179
verifylist = [
21502180
('image', 'image1'),
21512181
('flavor', self.flavor.id),
2152-
('block_device_mapping',
2153-
{'vds': self.volume.name + ':snapshot:5:true'}),
2182+
('block_device_mapping', [
2183+
{
2184+
'device_name': 'vds',
2185+
'uuid': self.volume.name,
2186+
'source_type': 'snapshot',
2187+
'volume_size': '5',
2188+
'destination_type': 'volume',
2189+
'delete_on_termination': 'true',
2190+
}
2191+
]),
21542192
('config_drive', False),
21552193
('server_name', self.new_server.name),
21562194
]
@@ -2205,8 +2243,22 @@ def test_server_create_with_block_device_mapping_multiple(self):
22052243
verifylist = [
22062244
('image', 'image1'),
22072245
('flavor', self.flavor.id),
2208-
('block_device_mapping', {'vdb': self.volume.name + ':::false',
2209-
'vdc': self.volume.name + ':::true'}),
2246+
('block_device_mapping', [
2247+
{
2248+
'device_name': 'vdb',
2249+
'uuid': self.volume.name,
2250+
'source_type': 'volume',
2251+
'destination_type': 'volume',
2252+
'delete_on_termination': 'false',
2253+
},
2254+
{
2255+
'device_name': 'vdc',
2256+
'uuid': self.volume.name,
2257+
'source_type': 'volume',
2258+
'destination_type': 'volume',
2259+
'delete_on_termination': 'true',
2260+
},
2261+
]),
22102262
('config_drive', False),
22112263
('server_name', self.new_server.name),
22122264
]
@@ -2259,26 +2311,29 @@ def test_server_create_with_block_device_mapping_multiple(self):
22592311
self.assertEqual(self.datalist(), data)
22602312

22612313
def test_server_create_with_block_device_mapping_invalid_format(self):
2262-
# 1. block device mapping don't contain equal sign "="
2314+
# block device mapping don't contain equal sign "="
22632315
arglist = [
22642316
'--image', 'image1',
22652317
'--flavor', self.flavor.id,
22662318
'--block-device-mapping', 'not_contain_equal_sign',
22672319
self.new_server.name,
22682320
]
2269-
self.assertRaises(argparse.ArgumentTypeError,
2270-
self.check_parser,
2271-
self.cmd, arglist, [])
2272-
# 2. block device mapping don't contain device name "=uuid:::true"
2321+
self.assertRaises(
2322+
argparse.ArgumentTypeError,
2323+
self.check_parser,
2324+
self.cmd, arglist, [])
2325+
2326+
# block device mapping don't contain device name "=uuid:::true"
22732327
arglist = [
22742328
'--image', 'image1',
22752329
'--flavor', self.flavor.id,
22762330
'--block-device-mapping', '=uuid:::true',
22772331
self.new_server.name,
22782332
]
2279-
self.assertRaises(argparse.ArgumentTypeError,
2280-
self.check_parser,
2281-
self.cmd, arglist, [])
2333+
self.assertRaises(
2334+
argparse.ArgumentTypeError,
2335+
self.check_parser,
2336+
self.cmd, arglist, [])
22822337

22832338
def test_server_create_with_block_device_mapping_no_uuid(self):
22842339
arglist = [
@@ -2287,18 +2342,10 @@ def test_server_create_with_block_device_mapping_no_uuid(self):
22872342
'--block-device-mapping', 'vdb=',
22882343
self.new_server.name,
22892344
]
2290-
verifylist = [
2291-
('image', 'image1'),
2292-
('flavor', self.flavor.id),
2293-
('block_device_mapping', {'vdb': ''}),
2294-
('config_drive', False),
2295-
('server_name', self.new_server.name),
2296-
]
2297-
parsed_args = self.check_parser(self.cmd, arglist, verifylist)
2298-
2299-
self.assertRaises(exceptions.CommandError,
2300-
self.cmd.take_action,
2301-
parsed_args)
2345+
self.assertRaises(
2346+
argparse.ArgumentTypeError,
2347+
self.check_parser,
2348+
self.cmd, arglist, [])
23022349

23032350
def test_server_create_volume_boot_from_volume_conflict(self):
23042351
# Tests that specifying --volume and --boot-from-volume results in

0 commit comments

Comments
 (0)