diff options
author | Zuul <zuul@review.opendev.org> | 2021-03-04 21:27:44 +0000 |
---|---|---|
committer | Gerrit Code Review <review@openstack.org> | 2021-03-04 21:27:44 +0000 |
commit | 433ceff0518294484b5666b3d57fd4986746d000 (patch) | |
tree | 75e84d1446ba6ff78f3149d5f8521795591d7032 | |
parent | 1f6104c7607bf2f8b1bf6ea6ea9d23d5f794a0ff (diff) | |
parent | 074e045c6938189c77fd13ce4dd9fccf69d3a12a (diff) | |
download | python-openstackclient-433ceff0518294484b5666b3d57fd4986746d000.tar.gz |
Merge "compute: Improve 'server create --block-device-mapping' option parsing"
-rw-r--r-- | openstackclient/compute/v2/server.py | 134 | ||||
-rw-r--r-- | openstackclient/tests/unit/compute/v2/test_server.py | 105 |
2 files changed, 153 insertions, 86 deletions
diff --git a/openstackclient/compute/v2/server.py b/openstackclient/compute/v2/server.py index 419ae4a5..7edbb18e 100644 --- a/openstackclient/compute/v2/server.py +++ b/openstackclient/compute/v2/server.py @@ -678,6 +678,51 @@ class NICAction(argparse.Action): getattr(namespace, self.dest).append(info) +class BDMLegacyAction(argparse.Action): + + def __call__(self, parser, namespace, values, option_string=None): + # Make sure we have an empty dict rather than None + if getattr(namespace, self.dest, None) is None: + setattr(namespace, self.dest, []) + + dev_name, sep, dev_map = values.partition('=') + dev_map = dev_map.split(':') if dev_map else dev_map + if not dev_name or not dev_map or len(dev_map) > 4: + msg = _( + "Invalid argument %s; argument must be of form " + "'dev-name=id[:type[:size[:delete-on-terminate]]]'" + ) + raise argparse.ArgumentTypeError(msg % values) + + mapping = { + 'device_name': dev_name, + # store target; this may be a name and will need verification later + 'uuid': dev_map[0], + 'source_type': 'volume', + 'destination_type': 'volume', + } + + # decide source and destination type + if len(dev_map) > 1 and dev_map[1]: + if dev_map[1] not in ('volume', 'snapshot', 'image'): + msg = _( + "Invalid argument %s; 'type' must be one of: volume, " + "snapshot, image" + ) + raise argparse.ArgumentTypeError(msg % values) + + mapping['source_type'] = dev_map[1] + + # 3. append size and delete_on_termination, if present + if len(dev_map) > 2 and dev_map[2]: + mapping['volume_size'] = dev_map[2] + + if len(dev_map) > 3 and dev_map[3]: + mapping['delete_on_termination'] = dev_map[3] + + getattr(namespace, self.dest).append(mapping) + + class CreateServer(command.ShowOne): _description = _("Create a new server") @@ -745,8 +790,8 @@ class CreateServer(command.ShowOne): parser.add_argument( '--block-device-mapping', metavar='<dev-name=mapping>', - action=parseractions.KeyValueAction, - default={}, + action=BDMLegacyAction, + default=[], # NOTE(RuiChen): Add '\n' to the end of line to improve formatting; # see cliff's _SmartHelpFormatter for more details. help=_( @@ -1123,62 +1168,35 @@ class CreateServer(command.ShowOne): # If booting from volume we do not pass an image to compute. image = None - boot_args = [parsed_args.server_name, image, flavor] - # Handle block device by device name order, like: vdb -> vdc -> vdd - for dev_name in sorted(parsed_args.block_device_mapping): - dev_map = parsed_args.block_device_mapping[dev_name] - dev_map = dev_map.split(':') - if dev_map[0]: - mapping = {'device_name': dev_name} - - # 1. decide source and destination type - if (len(dev_map) > 1 and - dev_map[1] in ('volume', 'snapshot', 'image')): - mapping['source_type'] = dev_map[1] - else: - mapping['source_type'] = 'volume' - - mapping['destination_type'] = 'volume' - - # 2. check target exist, update target uuid according by - # source type - if mapping['source_type'] == 'volume': - volume_id = utils.find_resource( - volume_client.volumes, dev_map[0]).id - mapping['uuid'] = volume_id - elif mapping['source_type'] == 'snapshot': - snapshot_id = utils.find_resource( - volume_client.volume_snapshots, dev_map[0]).id - mapping['uuid'] = snapshot_id - elif mapping['source_type'] == 'image': - # NOTE(mriedem): In case --image is specified with the same - # image, that becomes the root disk for the server. If the - # block device is specified with a root device name, e.g. - # vda, then the compute API will likely fail complaining - # that there is a conflict. So if using the same image ID, - # which doesn't really make sense but it's allowed, the - # device name would need to be a non-root device, e.g. vdb. - # Otherwise if the block device image is different from the - # one specified by --image, then the compute service will - # create a volume from the image and attach it to the - # server as a non-root volume. - image_id = image_client.find_image(dev_map[0], - ignore_missing=False).id - mapping['uuid'] = image_id - - # 3. append size and delete_on_termination if exist - if len(dev_map) > 2 and dev_map[2]: - mapping['volume_size'] = dev_map[2] - - if len(dev_map) > 3 and dev_map[3]: - mapping['delete_on_termination'] = dev_map[3] - else: - msg = _( - 'Volume, volume snapshot or image (name or ID) must ' - 'be specified if --block-device-mapping is specified' - ) - raise exceptions.CommandError(msg) + for mapping in parsed_args.block_device_mapping: + if mapping['source_type'] == 'volume': + volume_id = utils.find_resource( + volume_client.volumes, mapping['uuid'], + ).id + mapping['uuid'] = volume_id + elif mapping['source_type'] == 'snapshot': + snapshot_id = utils.find_resource( + volume_client.volume_snapshots, mapping['uuid'], + ).id + mapping['uuid'] = snapshot_id + elif mapping['source_type'] == 'image': + # NOTE(mriedem): In case --image is specified with the same + # image, that becomes the root disk for the server. If the + # block device is specified with a root device name, e.g. + # vda, then the compute API will likely fail complaining + # that there is a conflict. So if using the same image ID, + # which doesn't really make sense but it's allowed, the + # device name would need to be a non-root device, e.g. vdb. + # Otherwise if the block device image is different from the + # one specified by --image, then the compute service will + # create a volume from the image and attach it to the + # server as a non-root volume. + image_id = image_client.find_image( + mapping['uuid'], ignore_missing=False, + ).id + mapping['uuid'] = image_id + block_device_mapping_v2.append(mapping) nics = parsed_args.nics @@ -1281,6 +1299,8 @@ class CreateServer(command.ShowOne): else: config_drive = parsed_args.config_drive + boot_args = [parsed_args.server_name, image, flavor] + boot_kwargs = dict( meta=parsed_args.properties, files=files, diff --git a/openstackclient/tests/unit/compute/v2/test_server.py b/openstackclient/tests/unit/compute/v2/test_server.py index ce04ea4c..58daf531 100644 --- a/openstackclient/tests/unit/compute/v2/test_server.py +++ b/openstackclient/tests/unit/compute/v2/test_server.py @@ -1935,7 +1935,15 @@ class TestServerCreate(TestServer): verifylist = [ ('image', 'image1'), ('flavor', self.flavor.id), - ('block_device_mapping', {'vda': self.volume.name + ':::false'}), + ('block_device_mapping', [ + { + 'device_name': 'vda', + 'uuid': self.volume.name, + 'source_type': 'volume', + 'destination_type': 'volume', + 'delete_on_termination': 'false', + } + ]), ('config_drive', False), ('server_name', self.new_server.name), ] @@ -1988,7 +1996,14 @@ class TestServerCreate(TestServer): verifylist = [ ('image', 'image1'), ('flavor', self.flavor.id), - ('block_device_mapping', {'vdf': self.volume.name}), + ('block_device_mapping', [ + { + 'device_name': 'vdf', + 'uuid': self.volume.name, + 'source_type': 'volume', + 'destination_type': 'volume', + } + ]), ('config_drive', False), ('server_name', self.new_server.name), ] @@ -2040,7 +2055,14 @@ class TestServerCreate(TestServer): verifylist = [ ('image', 'image1'), ('flavor', self.flavor.id), - ('block_device_mapping', {'vdf': self.volume.name + ':::'}), + ('block_device_mapping', [ + { + 'device_name': 'vdf', + 'uuid': self.volume.name, + 'source_type': 'volume', + 'destination_type': 'volume', + } + ]), ('config_drive', False), ('server_name', self.new_server.name), ] @@ -2093,8 +2115,16 @@ class TestServerCreate(TestServer): verifylist = [ ('image', 'image1'), ('flavor', self.flavor.id), - ('block_device_mapping', - {'vde': self.volume.name + ':volume:3:true'}), + ('block_device_mapping', [ + { + 'device_name': 'vde', + 'uuid': self.volume.name, + 'source_type': 'volume', + 'destination_type': 'volume', + 'volume_size': '3', + 'delete_on_termination': 'true', + } + ]), ('config_drive', False), ('server_name', self.new_server.name), ] @@ -2149,8 +2179,16 @@ class TestServerCreate(TestServer): verifylist = [ ('image', 'image1'), ('flavor', self.flavor.id), - ('block_device_mapping', - {'vds': self.volume.name + ':snapshot:5:true'}), + ('block_device_mapping', [ + { + 'device_name': 'vds', + 'uuid': self.volume.name, + 'source_type': 'snapshot', + 'volume_size': '5', + 'destination_type': 'volume', + 'delete_on_termination': 'true', + } + ]), ('config_drive', False), ('server_name', self.new_server.name), ] @@ -2205,8 +2243,22 @@ class TestServerCreate(TestServer): verifylist = [ ('image', 'image1'), ('flavor', self.flavor.id), - ('block_device_mapping', {'vdb': self.volume.name + ':::false', - 'vdc': self.volume.name + ':::true'}), + ('block_device_mapping', [ + { + 'device_name': 'vdb', + 'uuid': self.volume.name, + 'source_type': 'volume', + 'destination_type': 'volume', + 'delete_on_termination': 'false', + }, + { + 'device_name': 'vdc', + 'uuid': self.volume.name, + 'source_type': 'volume', + 'destination_type': 'volume', + 'delete_on_termination': 'true', + }, + ]), ('config_drive', False), ('server_name', self.new_server.name), ] @@ -2259,26 +2311,29 @@ class TestServerCreate(TestServer): self.assertEqual(self.datalist(), data) def test_server_create_with_block_device_mapping_invalid_format(self): - # 1. block device mapping don't contain equal sign "=" + # block device mapping don't contain equal sign "=" arglist = [ '--image', 'image1', '--flavor', self.flavor.id, '--block-device-mapping', 'not_contain_equal_sign', self.new_server.name, ] - self.assertRaises(argparse.ArgumentTypeError, - self.check_parser, - self.cmd, arglist, []) - # 2. block device mapping don't contain device name "=uuid:::true" + self.assertRaises( + argparse.ArgumentTypeError, + self.check_parser, + self.cmd, arglist, []) + + # block device mapping don't contain device name "=uuid:::true" arglist = [ '--image', 'image1', '--flavor', self.flavor.id, '--block-device-mapping', '=uuid:::true', self.new_server.name, ] - self.assertRaises(argparse.ArgumentTypeError, - self.check_parser, - self.cmd, arglist, []) + self.assertRaises( + argparse.ArgumentTypeError, + self.check_parser, + self.cmd, arglist, []) def test_server_create_with_block_device_mapping_no_uuid(self): arglist = [ @@ -2287,18 +2342,10 @@ class TestServerCreate(TestServer): '--block-device-mapping', 'vdb=', self.new_server.name, ] - verifylist = [ - ('image', 'image1'), - ('flavor', self.flavor.id), - ('block_device_mapping', {'vdb': ''}), - ('config_drive', False), - ('server_name', self.new_server.name), - ] - parsed_args = self.check_parser(self.cmd, arglist, verifylist) - - self.assertRaises(exceptions.CommandError, - self.cmd.take_action, - parsed_args) + self.assertRaises( + argparse.ArgumentTypeError, + self.check_parser, + self.cmd, arglist, []) def test_server_create_volume_boot_from_volume_conflict(self): # Tests that specifying --volume and --boot-from-volume results in |