compute: Add 'server create --block-device' option

One of the last big gaps with novaclient. As noted in the release note,
the current '--block-device-mapping' format is based on the old BDM v1
format, even though it actually results in BDM v2-style requests to the
server. It's time to replace that.

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

2 files changed, 374 insertions, 1 deletions

diff --git a/openstackclient/compute/v2/server.py b/openstackclient/compute/v2/server.py
index 3db36f72..b1a86a23 100644
--- a/openstackclient/compute/v2/server.py
+++ b/openstackclient/compute/v2/server.py
@@ -801,6 +801,7 @@ class CreateServer(command.ShowOne):
                 'options.'
             )
         )
+        # TODO(stephenfin): Remove this in the v7.0
         parser.add_argument(
             '--block-device-mapping',
             metavar='<dev-name=mapping>',
@@ -809,7 +810,7 @@ class CreateServer(command.ShowOne):
             # NOTE(RuiChen): Add '\n' to the end of line to improve formatting;
             # see cliff's _SmartHelpFormatter for more details.
             help=_(
-                'Create a block device on the server.\n'
+                '**Deprecated** Create a block device on the server.\n'
                 'Block device mapping in the format\n'
                 '<dev-name>=<id>:<type>:<size(GB)>:<delete-on-terminate>\n'
                 '<dev-name>: block device name, like: vdb, xvdc '
@@ -822,6 +823,49 @@ class CreateServer(command.ShowOne):
                 '(optional)\n'
                 '<delete-on-terminate>: true or false; default: false '
                 '(optional)\n'
+                'Replaced by --block-device'
+            ),
+        )
+        parser.add_argument(
+            '--block-device',
+            metavar='',
+            action=parseractions.MultiKeyValueAction,
+            dest='block_devices',
+            default=[],
+            required_keys=[
+                'boot_index',
+            ],
+            optional_keys=[
+                'uuid', 'source_type', 'destination_type',
+                'disk_bus', 'device_type', 'device_name', 'guest_format',
+                'volume_size', 'volume_type', 'delete_on_termination', 'tag',
+            ],
+            help=_(
+                'Create a block device on the server.\n'
+                'Block device in the format:\n'
+                'uuid=<uuid>: UUID of the volume, snapshot or ID '
+                '(required if using source image, snapshot or volume),\n'
+                'source_type=<source_type>: source type '
+                '(one of: image, snapshot, volume, blank),\n'
+                'destination_typ=<destination_type>: destination type '
+                '(one of: volume, local) (optional),\n'
+                'disk_bus=<disk_bus>: device bus '
+                '(one of: uml, lxc, virtio, ...) (optional),\n'
+                'device_type=<device_type>: device type '
+                '(one of: disk, cdrom, etc. (optional),\n'
+                'device_name=<device_name>: name of the device (optional),\n'
+                'volume_size=<volume_size>: size of the block device in MiB '
+                '(for swap) or GiB (for everything else) (optional),\n'
+                'guest_format=<guest_format>: format of device (optional),\n'
+                'boot_index=<boot_index>: index of disk used to order boot '
+                'disk '
+                '(required for volume-backed instances),\n'
+                'delete_on_termination=<true|false>: whether to delete the '
+                'volume upon deletion of server (optional),\n'
+                'tag=<tag>: device metadata tag (optional),\n'
+                'volume_type=<volume_type>: type of volume to create (name or '
+                'ID) when source if blank, image or snapshot and dest is '
+                'volume (optional)'
             ),
         )
         parser.add_argument(
@@ -1250,6 +1294,8 @@ class CreateServer(command.ShowOne):
 
         # Handle block device by device name order, like: vdb -> vdc -> vdd
         for mapping in parsed_args.block_device_mapping:
+            # The 'uuid' field isn't necessarily a UUID yet; let's validate it
+            # just in case
             if mapping['source_type'] == 'volume':
                 volume_id = utils.find_resource(
                     volume_client.volumes, mapping['uuid'],
@@ -1279,6 +1325,77 @@ class CreateServer(command.ShowOne):
 
             block_device_mapping_v2.append(mapping)
 
+        for mapping in parsed_args.block_devices:
+            try:
+                mapping['boot_index'] = int(mapping['boot_index'])
+            except ValueError:
+                msg = _(
+                    'The boot_index key of --block-device should be an '
+                    'integer'
+                )
+                raise exceptions.CommandError(msg)
+
+            if 'tag' in mapping and (
+                compute_client.api_version < api_versions.APIVersion('2.42')
+            ):
+                msg = _(
+                    '--os-compute-api-version 2.42 or greater is '
+                    'required to support the tag key of --block-device'
+                )
+                raise exceptions.CommandError(msg)
+
+            if 'volume_type' in mapping and (
+                compute_client.api_version < api_versions.APIVersion('2.67')
+            ):
+                msg = _(
+                    '--os-compute-api-version 2.67 or greater is '
+                    'required to support the volume_type key of --block-device'
+                )
+                raise exceptions.CommandError(msg)
+
+            if 'source_type' in mapping:
+                if mapping['source_type'] not in (
+                    'volume', 'image', 'snapshot', 'blank',
+                ):
+                    msg = _(
+                        'The source_type key of --block-device should be one '
+                        'of: volume, image, snapshot, blank'
+                    )
+                    raise exceptions.CommandError(msg)
+            else:
+                mapping['source_type'] = 'blank'
+
+            if 'destination_type' in mapping:
+                if mapping['destination_type'] not in ('local', 'volume'):
+                    msg = _(
+                        'The destination_type key of --block-device should be '
+                        'one of: local, volume'
+                    )
+                    raise exceptions.CommandError(msg)
+            else:
+                if mapping['source_type'] in ('image', 'blank'):
+                    mapping['destination_type'] = 'local'
+                else:  # volume, snapshot
+                    mapping['destination_type'] = 'volume'
+
+            if 'delete_on_termination' in mapping:
+                try:
+                    value = strutils.bool_from_string(
+                        mapping['delete_on_termination'], strict=True)
+                except ValueError:
+                    msg = _(
+                        'The delete_on_termination key of --block-device '
+                        'should be a boolean-like value'
+                    )
+                    raise exceptions.CommandError(msg)
+
+                mapping['delete_on_termination'] = value
+            else:
+                if mapping['destination_type'] == 'local':
+                    mapping['delete_on_termination'] = True
+
+            block_device_mapping_v2.append(mapping)
+
         nics = parsed_args.nics
 
         if 'auto' in nics or 'none' in nics:
diff --git a/openstackclient/tests/unit/compute/v2/test_server.py b/openstackclient/tests/unit/compute/v2/test_server.py
index ce93f21e..0548924d 100644
--- a/openstackclient/tests/unit/compute/v2/test_server.py
+++ b/openstackclient/tests/unit/compute/v2/test_server.py
@@ -2028,6 +2028,262 @@ class TestServerCreate(TestServer):
         self.assertEqual(self.columns, columns)
         self.assertEqual(self.datalist(), data)
 
+    def test_server_create_with_block_device(self):
+        block_device = f'uuid={self.volume.id},source_type=volume,boot_index=1'
+        arglist = [
+            '--image', 'image1',
+            '--flavor', self.flavor.id,
+            '--block-device', block_device,
+            self.new_server.name,
+        ]
+        verifylist = [
+            ('image', 'image1'),
+            ('flavor', self.flavor.id),
+            ('block_devices', [
+                {
+                    'uuid': self.volume.id,
+                    'source_type': 'volume',
+                    'boot_index': '1',
+                },
+            ]),
+            ('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',
+                'boot_index': 1,
+            }],
+            'nics': [],
+            '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_full(self):
+        self.app.client_manager.compute.api_version = api_versions.APIVersion(
+            '2.67')
+
+        block_device = (
+            f'uuid={self.volume.id},source_type=volume,'
+            f'destination_type=volume,disk_bus=ide,device_type=disk,'
+            f'device_name=sdb,guest_format=ext4,volume_size=64,'
+            f'volume_type=foo,boot_index=1,delete_on_termination=true,'
+            f'tag=foo'
+        )
+
+        arglist = [
+            '--image', 'image1',
+            '--flavor', self.flavor.id,
+            '--block-device', block_device,
+            self.new_server.name,
+        ]
+        verifylist = [
+            ('image', 'image1'),
+            ('flavor', self.flavor.id),
+            ('block_devices', [
+                {
+                    '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',
+                },
+            ]),
+            ('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_no_boot_index(self):
+        block_device = \
+            f'uuid={self.volume.name},source_type=volume'
+        arglist = [
+            '--image', 'image1',
+            '--flavor', self.flavor.id,
+            '--block-device', block_device,
+            self.new_server.name,
+        ]
+        self.assertRaises(
+            argparse.ArgumentTypeError,
+            self.check_parser,
+            self.cmd, arglist, [])
+
+    def test_server_create_with_block_device_invalid_boot_index(self):
+        block_device = \
+            f'uuid={self.volume.name},source_type=volume,boot_index=foo'
+        arglist = [
+            '--image', 'image1',
+            '--flavor', self.flavor.id,
+            '--block-device', block_device,
+            self.new_server.name,
+        ]
+        parsed_args = self.check_parser(self.cmd, arglist, [])
+        ex = self.assertRaises(
+            exceptions.CommandError,
+            self.cmd.take_action, parsed_args)
+        self.assertIn('The boot_index key of --block-device ', str(ex))
+
+    def test_server_create_with_block_device_invalid_source_type(self):
+        block_device = f'uuid={self.volume.name},source_type=foo,boot_index=1'
+        arglist = [
+            '--image', 'image1',
+            '--flavor', self.flavor.id,
+            '--block-device', block_device,
+            self.new_server.name,
+        ]
+        parsed_args = self.check_parser(self.cmd, arglist, [])
+        ex = self.assertRaises(
+            exceptions.CommandError,
+            self.cmd.take_action, parsed_args)
+        self.assertIn('The source_type key of --block-device ', str(ex))
+
+    def test_server_create_with_block_device_invalid_destination_type(self):
+        block_device = \
+            f'uuid={self.volume.name},destination_type=foo,boot_index=1'
+        arglist = [
+            '--image', 'image1',
+            '--flavor', self.flavor.id,
+            '--block-device', block_device,
+            self.new_server.name,
+        ]
+        parsed_args = self.check_parser(self.cmd, arglist, [])
+        ex = self.assertRaises(
+            exceptions.CommandError,
+            self.cmd.take_action, parsed_args)
+        self.assertIn('The destination_type key of --block-device ', str(ex))
+
+    def test_server_create_with_block_device_invalid_shutdown(self):
+        block_device = \
+            f'uuid={self.volume.name},delete_on_termination=foo,boot_index=1'
+        arglist = [
+            '--image', 'image1',
+            '--flavor', self.flavor.id,
+            '--block-device', block_device,
+            self.new_server.name,
+        ]
+        parsed_args = self.check_parser(self.cmd, arglist, [])
+        ex = self.assertRaises(
+            exceptions.CommandError,
+            self.cmd.take_action, parsed_args)
+        self.assertIn(
+            'The delete_on_termination key of --block-device ', str(ex))
+
+    def test_server_create_with_block_device_tag_pre_v242(self):
+        self.app.client_manager.compute.api_version = api_versions.APIVersion(
+            '2.41')
+
+        block_device = \
+            f'uuid={self.volume.name},tag=foo,boot_index=1'
+        arglist = [
+            '--image', 'image1',
+            '--flavor', self.flavor.id,
+            '--block-device', block_device,
+            self.new_server.name,
+        ]
+        parsed_args = self.check_parser(self.cmd, arglist, [])
+        ex = self.assertRaises(
+            exceptions.CommandError,
+            self.cmd.take_action, parsed_args)
+        self.assertIn(
+            '--os-compute-api-version 2.42 or greater is required',
+            str(ex))
+
+    def test_server_create_with_block_device_volume_type_pre_v267(self):
+        self.app.client_manager.compute.api_version = api_versions.APIVersion(
+            '2.66')
+
+        block_device = f'uuid={self.volume.name},volume_type=foo,boot_index=1'
+        arglist = [
+            '--image', 'image1',
+            '--flavor', self.flavor.id,
+            '--block-device', block_device,
+            self.new_server.name,
+        ]
+        parsed_args = self.check_parser(self.cmd, arglist, [])
+        ex = self.assertRaises(
+            exceptions.CommandError,
+            self.cmd.take_action, parsed_args)
+        self.assertIn(
+            '--os-compute-api-version 2.67 or greater is required',
+            str(ex))
+
     def test_server_create_with_block_device_mapping(self):
         arglist = [
             '--image', 'image1',