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>

openstackclient/compute/v2/server.py

@@ -832,13 +832,12 @@ class CreateServer(command.ShowOne):
             action=parseractions.MultiKeyValueAction,
             dest='block_devices',
             default=[],
-            required_keys=[
+            required_keys=[],
-                'boot_index',
-            ],
             optional_keys=[
                 'uuid', 'source_type', 'destination_type',
-                'disk_bus', 'device_type', 'device_name', 'guest_format',
+                'disk_bus', 'device_type', 'device_name', 'volume_size',
-                'volume_size', 'volume_type', 'delete_on_termination', 'tag',
+                'guest_format', 'boot_index', 'delete_on_termination', 'tag',
+                'volume_type',
             ],
             help=_(
                 'Create a block device on the server.\n'
@@ -1336,14 +1335,15 @@ class CreateServer(command.ShowOne):
             block_device_mapping_v2.append(mapping)
 
         for mapping in parsed_args.block_devices:
-            try:
+            if 'boot_index' in mapping:
-                mapping['boot_index'] = int(mapping['boot_index'])
+                try:
-            except ValueError:
+                    mapping['boot_index'] = int(mapping['boot_index'])
-                msg = _(
+                except ValueError:
-                    'The boot_index key of --block-device should be an '
+                    msg = _(
-                    'integer'
+                        'The boot_index key of --block-device should be an '
-                )
+                        'integer'
-                raise exceptions.CommandError(msg)
+                    )
+                    raise exceptions.CommandError(msg)
 
             if 'tag' in mapping and (
                 compute_client.api_version < api_versions.APIVersion('2.42')
@@ -1383,9 +1383,9 @@ class CreateServer(command.ShowOne):
                     )
                     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 @@ class ServerTests(common.ComputeTestCase):
             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 @@ class ServerTests(common.ComputeTestCase):
             empty_volume_name
         ))
         self.assertIsNotNone(cmd_output["id"])
-        self.addCleanup(self.openstack,
+        self.addCleanup(self.openstack, 'volume delete ' + empty_volume_name)
-                        'volume delete ' + empty_volume_name)
+        self.assertEqual(empty_volume_name, cmd_output['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 @@ class ServerTests(common.ComputeTestCase):
             '--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 @@ class ServerTests(common.ComputeTestCase):
         )
         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 '
+            bdm_arg + ' ' +
-            'vdb=' + empty_snapshot_name + ':snapshot:1:true ' +
             self.network_arg + ' ' +
             '--wait ' +
             server_name
@@ -681,7 +773,17 @@ class ServerTests(common.ComputeTestCase):
             # 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
@@ -689,6 +791,32 @@ class ServerTests(common.ComputeTestCase):
         # 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 @@ class ServerTests(common.ComputeTestCase):
             'server create -f json ' +
             '--flavor ' + self.flavor_name + ' ' +
             '--image ' + self.image_name + ' ' +
-            '--block-device-mapping '
+            bdm_arg + ' ' +
-            # 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 ' +
             self.network_arg + ' ' +
             '--wait ' +
             server_name
@@ -768,6 +892,14 @@ class ServerTests(common.ComputeTestCase):
             # 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 @@ class TestServerCreate(TestServer):
         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 @@ class TestServerCreate(TestServer):
                 {
                     'uuid': self.volume.id,
                     'source_type': 'volume',
-                    'boot_index': '1',
                 },
             ]),
             ('server_name', self.new_server.name),
@@ -2069,7 +2068,6 @@ class TestServerCreate(TestServer):
                 'uuid': self.volume.id,
                 'source_type': 'volume',
                 'destination_type': 'volume',
-                'boot_index': 1,
             }],
             'nics': [],
             'scheduler_hints': {},
@@ -2171,20 +2169,6 @@ class TestServerCreate(TestServer):
         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 @@ class TestServerCreate(TestServer):
         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 @@ class TestServerCreate(TestServer):
 
     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 @@ class TestServerCreate(TestServer):
 
     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 @@ class TestServerCreate(TestServer):
             '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 @@ class TestServerCreate(TestServer):
         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,