Merge "compute: Add functional tests for --block-device"

3 files changed, 169 insertions, 53 deletions

diff --git a/openstackclient/compute/v2/server.py b/openstackclient/compute/v2/server.py
index 9838ed54..b2ae93c8 100644
--- a/openstackclient/compute/v2/server.py
+++ b/openstackclient/compute/v2/server.py
@@ -832,13 +832,12 @@ class CreateServer(command.ShowOne):
             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 @@ 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 '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 @@ 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:
diff --git a/openstackclient/tests/functional/compute/v2/test_server.py b/openstackclient/tests/functional/compute/v2/test_server.py
index bad3f93d..9cf2fc7f 100644
--- a/openstackclient/tests/functional/compute/v2/test_server.py
+++ b/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,
-                        '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 @@ 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 '
-            'vdb=' + empty_snapshot_name + ':snapshot:1:true ' +
+            bdm_arg + ' ' +
             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 '
-            # 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 @@ 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
diff --git a/openstackclient/tests/unit/compute/v2/test_server.py b/openstackclient/tests/unit/compute/v2/test_server.py
index ced5d458..9666c5fd 100644
--- a/openstackclient/tests/unit/compute/v2/test_server.py
+++ b/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,