compute: Improve 'server create --block-device-mapping' option parsing

Once again, custom actions to the rescue.

Change-Id: I6b4f80882dbbeb6a2a7e877f63becae7211b7f9a
Signed-off-by: Stephen Finucane <sfinucan@redhat.com>

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,
+            action=BDMLegacyAction,
-            default={},
+            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):
+        for mapping in parsed_args.block_device_mapping:
-            dev_map = parsed_args.block_device_mapping[dev_name]
+            if mapping['source_type'] == 'volume':
-            dev_map = dev_map.split(':')
+                volume_id = utils.find_resource(
-            if dev_map[0]:
+                    volume_client.volumes, mapping['uuid'],
-                mapping = {'device_name': dev_name}
+                ).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
 
-                # 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)
             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,

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',
+            ('block_device_mapping', [
-             {'vde': self.volume.name + ':volume:3:true'}),
+                {
+                    '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',
+            ('block_device_mapping', [
-             {'vds': self.volume.name + ':snapshot:5:true'}),
+                {
+                    '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',
+            ('block_device_mapping', [
-                                      'vdc': self.volume.name + ':::true'}),
+                {
+                    '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.assertRaises(
-                          self.check_parser,
+            argparse.ArgumentTypeError,
-                          self.cmd, arglist, [])
+            self.check_parser,
-        # 2. block device mapping don't contain device name "=uuid:::true"
+            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.assertRaises(
-                          self.check_parser,
+            argparse.ArgumentTypeError,
-                          self.cmd, arglist, [])
+            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 = [
+        self.assertRaises(
-            ('image', 'image1'),
+            argparse.ArgumentTypeError,
-            ('flavor', self.flavor.id),
+            self.check_parser,
-            ('block_device_mapping', {'vdb': ''}),
+            self.cmd, arglist, [])
-            ('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)
 
     def test_server_create_volume_boot_from_volume_conflict(self):
         # Tests that specifying --volume and --boot-from-volume results in