compute: Add functional tests for --block-device

This mostly reuses the existing tests for '--block-device-mapping',
which can hopefully be removed at some point in the future.

This highlights two issues with the implementation of this option.
Firstly, the 'boot_index' parameter is not required so don't mandate it.
Secondly, and more significantly, we were defaulting the destination
type for the 'image' source type to 'local'. Nova only allows you to
attach a single image to local mapping [1], which means this default
would only make sense if you were expecting users to use the
'--block-device' option exclusively and omit the '--image' option. This
is the *less common* case so this is a bad default. Default instead to a
destination type of 'volume' like everything else, and require users
specifying '--block-device' alone to pass 'destination_type=local'
explicitly.

[1] https://github.com/openstack/nova/blob/c8a6f8d2e/nova/block_device.py#L193-L206

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

Author: stephenfin

openstackclient/compute/v2/server.py

@@ -832,13 +832,12 @@ def get_parser(self, prog_name):
             action=parseractions.MultiKeyValueAction,
             dest='block_devices',
             default=[],
-            required_keys=[
-                'boot_index',
-            ],
+            required_keys=[],
             optional_keys=[
                 'uuid', 'source_type', 'destination_type',
-                'disk_bus', 'device_type', 'device_name', 'guest_format',
-                'volume_size', 'volume_type', 'delete_on_termination', 'tag',
+                'disk_bus', 'device_type', 'device_name', 'volume_size',
+                'guest_format', 'boot_index', 'delete_on_termination', 'tag',
+                'volume_type',
             ],
             help=_(
                 'Create a block device on the server.\n'
@@ -1336,14 +1335,15 @@ def _match_image(image_api, wanted_properties):
             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 'boot_index' in mapping:
+                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')
@@ -1383,9 +1383,9 @@ def _match_image(image_api, wanted_properties):
                     )
                     raise exceptions.CommandError(msg)
             else:
-                if mapping['source_type'] in ('image', 'blank'):
+                if mapping['source_type'] in ('blank',):
                     mapping['destination_type'] = 'local'
-                else:  # volume, snapshot
+                else:  # volume, image, snapshot
                     mapping['destination_type'] = 'volume'
 
             if 'delete_on_termination' in mapping:

openstackclient/tests/functional/compute/v2/test_server.py

@@ -574,7 +574,90 @@ def test_server_boot_from_volume(self):
             cmd_output['status'],
         )
 
-    def test_server_boot_with_bdm_snapshot(self):
+    def _test_server_boot_with_bdm_volume(self, use_legacy):
+        """Test server create from volume, server delete"""
+        # get volume status wait function
+        volume_wait_for = volume_common.BaseVolumeTests.wait_for_status
+
+        # create source empty volume
+        volume_name = uuid.uuid4().hex
+        cmd_output = json.loads(self.openstack(
+            'volume create -f json ' +
+            '--size 1 ' +
+            volume_name
+        ))
+        volume_id = cmd_output["id"]
+        self.assertIsNotNone(volume_id)
+        self.addCleanup(self.openstack, 'volume delete ' + volume_name)
+        self.assertEqual(volume_name, cmd_output['name'])
+        volume_wait_for("volume", volume_name, "available")
+
+        if use_legacy:
+            bdm_arg = f'--block-device-mapping vdb={volume_name}'
+        else:
+            bdm_arg = (
+                f'--block-device '
+                f'device_name=vdb,source_type=volume,boot_index=1,'
+                f'uuid={volume_id}'
+            )
+
+        # create server
+        server_name = uuid.uuid4().hex
+        server = json.loads(self.openstack(
+            'server create -f json ' +
+            '--flavor ' + self.flavor_name + ' ' +
+            '--image ' + self.image_name + ' ' +
+            bdm_arg + ' ' +
+            self.network_arg + ' ' +
+            '--wait ' +
+            server_name
+        ))
+        self.assertIsNotNone(server["id"])
+        self.addCleanup(self.openstack, 'server delete --wait ' + server_name)
+        self.assertEqual(
+            server_name,
+            server['name'],
+        )
+
+        # check server volumes_attached, format is
+        # {"volumes_attached": "id='2518bc76-bf0b-476e-ad6b-571973745bb5'",}
+        cmd_output = json.loads(self.openstack(
+            'server show -f json ' +
+            server_name
+        ))
+        volumes_attached = cmd_output['volumes_attached']
+        self.assertIsNotNone(volumes_attached)
+
+        # check volumes
+        cmd_output = json.loads(self.openstack(
+            'volume show -f json ' +
+            volume_name
+        ))
+        attachments = cmd_output['attachments']
+        self.assertEqual(
+            1,
+            len(attachments),
+        )
+        self.assertEqual(
+            server['id'],
+            attachments[0]['server_id'],
+        )
+        self.assertEqual(
+            "in-use",
+            cmd_output['status'],
+        )
+
+    def test_server_boot_with_bdm_volume(self):
+        """Test server create from image with bdm volume, server delete"""
+        self._test_server_boot_with_bdm_volume(use_legacy=False)
+
+    # TODO(stephenfin): Remove when we drop support for the
+    # '--block-device-mapping' option
+    def test_server_boot_with_bdm_volume_legacy(self):
+        """Test server create from image with bdm volume, server delete"""
+        self._test_server_boot_with_bdm_volume(use_legacy=True)
+
+    def _test_server_boot_with_bdm_snapshot(self, use_legacy):
         """Test server create from image with bdm snapshot, server delete"""
         # get volume status wait function
         volume_wait_for = volume_common.BaseVolumeTests.wait_for_status
@@ -588,12 +671,8 @@ def test_server_boot_with_bdm_snapshot(self):
             empty_volume_name
         ))
         self.assertIsNotNone(cmd_output["id"])
-        self.addCleanup(self.openstack,
-                        'volume delete ' + empty_volume_name)
-        self.assertEqual(
-            empty_volume_name,
-            cmd_output['name'],
-        )
+        self.addCleanup(self.openstack, 'volume delete ' + empty_volume_name)
+        self.assertEqual(empty_volume_name, cmd_output['name'])
         volume_wait_for("volume", empty_volume_name, "available")
 
         # create snapshot of source empty volume
@@ -603,7 +682,8 @@ def test_server_boot_with_bdm_snapshot(self):
             '--volume ' + empty_volume_name + ' ' +
             empty_snapshot_name
         ))
-        self.assertIsNotNone(cmd_output["id"])
+        empty_snapshot_id = cmd_output["id"]
+        self.assertIsNotNone(empty_snapshot_id)
         # Deleting volume snapshot take time, so we need to wait until the
         # snapshot goes. Entries registered by self.addCleanup will be called
         # in the reverse order, so we need to register wait_for_delete first.
@@ -617,14 +697,26 @@ def test_server_boot_with_bdm_snapshot(self):
         )
         volume_wait_for("volume snapshot", empty_snapshot_name, "available")
 
+        if use_legacy:
+            bdm_arg = (
+                f'--block-device-mapping '
+                f'vdb={empty_snapshot_name}:snapshot:1:true'
+            )
+        else:
+            bdm_arg = (
+                f'--block-device '
+                f'device_name=vdb,uuid={empty_snapshot_id},'
+                f'source_type=snapshot,volume_size=1,'
+                f'delete_on_termination=true,boot_index=1'
+            )
+
         # create server with bdm snapshot
         server_name = uuid.uuid4().hex
         server = json.loads(self.openstack(
             'server create -f json ' +
             '--flavor ' + self.flavor_name + ' ' +
             '--image ' + self.image_name + ' ' +
-            '--block-device-mapping '
-            'vdb=' + empty_snapshot_name + ':snapshot:1:true ' +
+            bdm_arg + ' ' +
             self.network_arg + ' ' +
             '--wait ' +
             server_name
@@ -681,14 +773,50 @@ def test_server_boot_with_bdm_snapshot(self):
             # the attached volume had been deleted
             pass
 
-    def test_server_boot_with_bdm_image(self):
+    def test_server_boot_with_bdm_snapshot(self):
+        """Test server create from image with bdm snapshot, server delete"""
+        self._test_server_boot_with_bdm_snapshot(use_legacy=False)
+
+    # TODO(stephenfin): Remove when we drop support for the
+    # '--block-device-mapping' option
+    def test_server_boot_with_bdm_snapshot_legacy(self):
+        """Test server create from image with bdm snapshot, server delete"""
+        self._test_server_boot_with_bdm_snapshot(use_legacy=True)
+
+    def _test_server_boot_with_bdm_image(self, use_legacy):
         # Tests creating a server where the root disk is backed by the given
         # --image but a --block-device-mapping with type=image is provided so
         # that the compute service creates a volume from that image and
         # attaches it as a non-root volume on the server. The block device is
         # marked as delete_on_termination=True so it will be automatically
         # deleted when the server is deleted.
 
+        if use_legacy:
+            # This means create a 1GB volume from the specified image, attach
+            # it to the server at /dev/vdb and delete the volume when the
+            # server is deleted.
+            bdm_arg = (
+                f'--block-device-mapping '
+                f'vdb={self.image_name}:image:1:true '
+            )
+        else:
+            # get image ID
+            cmd_output = json.loads(self.openstack(
+                'image show -f json ' +
+                self.image_name
+            ))
+            image_id = cmd_output['id']
+
+            # This means create a 1GB volume from the specified image, attach
+            # it to the server at /dev/vdb and delete the volume when the
+            # server is deleted.
+            bdm_arg = (
+                f'--block-device '
+                f'device_name=vdb,uuid={image_id},'
+                f'source_type=image,volume_size=1,'
+                f'delete_on_termination=true,boot_index=1'
+            )
+
         # create server with bdm type=image
         # NOTE(mriedem): This test is a bit unrealistic in that specifying the
         # same image in the block device as the --image option does not really
@@ -700,11 +828,7 @@ def test_server_boot_with_bdm_image(self):
             'server create -f json ' +
             '--flavor ' + self.flavor_name + ' ' +
             '--image ' + self.image_name + ' ' +
-            '--block-device-mapping '
-            # This means create a 1GB volume from the specified image, attach
-            # it to the server at /dev/vdb and delete the volume when the
-            # server is deleted.
-            'vdb=' + self.image_name + ':image:1:true ' +
+            bdm_arg + ' ' +
             self.network_arg + ' ' +
             '--wait ' +
             server_name
@@ -768,6 +892,14 @@ def test_server_boot_with_bdm_image(self):
             # the attached volume had been deleted
             pass
 
+    def test_server_boot_with_bdm_image(self):
+        self._test_server_boot_with_bdm_image(use_legacy=False)
+
+    # TODO(stephenfin): Remove when we drop support for the
+    # '--block-device-mapping' option
+    def test_server_boot_with_bdm_image_legacy(self):
+        self._test_server_boot_with_bdm_image(use_legacy=True)
+
     def test_boot_from_volume(self):
         # Tests creating a server using --image and --boot-from-volume where
         # the compute service will create a root volume of the specified size

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

@@ -2029,7 +2029,7 @@ def test_server_create_with_snapshot(self):
         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'
+        block_device = f'uuid={self.volume.id},source_type=volume'
         arglist = [
             '--image', 'image1',
             '--flavor', self.flavor.id,
@@ -2043,7 +2043,6 @@ def test_server_create_with_block_device(self):
                 {
                     'uuid': self.volume.id,
                     'source_type': 'volume',
-                    'boot_index': '1',
                 },
             ]),
             ('server_name', self.new_server.name),
@@ -2069,7 +2068,6 @@ def test_server_create_with_block_device(self):
                 'uuid': self.volume.id,
                 'source_type': 'volume',
                 'destination_type': 'volume',
-                'boot_index': 1,
             }],
             'nics': [],
             'scheduler_hints': {},
@@ -2171,20 +2169,6 @@ def test_server_create_with_block_device_full(self):
         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'
@@ -2201,7 +2185,7 @@ def test_server_create_with_block_device_invalid_boot_index(self):
         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'
+        block_device = f'uuid={self.volume.name},source_type=foo'
         arglist = [
             '--image', 'image1',
             '--flavor', self.flavor.id,
@@ -2216,7 +2200,7 @@ def test_server_create_with_block_device_invalid_source_type(self):
 
     def test_server_create_with_block_device_invalid_destination_type(self):
         block_device = \
-            f'uuid={self.volume.name},destination_type=foo,boot_index=1'
+            f'uuid={self.volume.name},destination_type=foo'
         arglist = [
             '--image', 'image1',
             '--flavor', self.flavor.id,
@@ -2231,7 +2215,7 @@ def test_server_create_with_block_device_invalid_destination_type(self):
 
     def test_server_create_with_block_device_invalid_shutdown(self):
         block_device = \
-            f'uuid={self.volume.name},delete_on_termination=foo,boot_index=1'
+            f'uuid={self.volume.name},delete_on_termination=foo'
         arglist = [
             '--image', 'image1',
             '--flavor', self.flavor.id,
@@ -2250,7 +2234,7 @@ def test_server_create_with_block_device_tag_pre_v242(self):
             '2.41')
 
         block_device = \
-            f'uuid={self.volume.name},tag=foo,boot_index=1'
+            f'uuid={self.volume.name},tag=foo'
         arglist = [
             '--image', 'image1',
             '--flavor', self.flavor.id,
@@ -2269,7 +2253,7 @@ 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'
+        block_device = f'uuid={self.volume.name},volume_type=foo'
         arglist = [
             '--image', 'image1',
             '--flavor', self.flavor.id,