Make block-device-mapping more stable and clear

The patch fix the following issues:

1. ValueError is raised if input don't contain "=". Sometimes the whole
"server create" command is very complex, it's difficult to find out root
reason directly.
2. Don't support to add block device from snapshot, like:
--block-device-mapping
vdb=0c8ae9d8-cadc-4a23-8337-4254614d277e:snapshot:1, it's supported by
novaclient, but not in osc.
3. If input "vdb=", not add any mapping information, the server will be
launched successfully, not raise error message to let use add
volume/snapshot id, just ignore "--block-device-mapping" option.
4. The help message of "block-device-mapping" option is so simple, need
to add some details about how to add <type>, <delete_on_terminate>
contains.

Change-Id: Ib7f7a654c3dc2a8272545f168b4c4ced230ce39e
Depends-On: Ib37913891bbf7a31b570404c4668c490d5ac859b
Closes-Bug: #1667266

doc/source/command-objects/server.rst

@@ -193,7 +193,23 @@ Create a new server
 
 .. option:: --block-device-mapping <dev-name=mapping>
 
-    Map block devices; map is <id>:<type>:<size(GB)>:<delete_on_terminate> (optional extension)
+    Create a block device on the server.
+
+    Block device mapping in the format
+
+    <dev-name>=<id>:<type>:<size(GB)>:<delete-on-terminate>
+
+    <dev-name>: block device name, like: vdb, xvdc (required)
+
+    <id>: UUID of the volume or snapshot (required)
+
+    <type>: volume or snapshot; default: volume (optional)
+
+    <size(GB)>: volume size if create from snapshot (optional)
+
+    <delete-on-terminate>: true or false; default: false (optional)
+
+    (optional extension)
 
 .. option:: --nic <net-id=net-uuid,v4-fixed-ip=ip-addr,v6-fixed-ip=ip-addr,port-id=port-uuid,auto,none>
 

openstackclient/compute/v2/server.py

@@ -446,10 +446,22 @@ class CreateServer(command.ShowOne):
         parser.add_argument(
             '--block-device-mapping',
             metavar='<dev-name=mapping>',
-            action='append',
-            default=[],
-            help=_('Map block devices; map is '
-                   '<id>:<type>:<size(GB)>:<delete_on_terminate> '
+            action=parseractions.KeyValueAction,
+            default={},
+            # NOTE(RuiChen): Add '\n' at the end of line to put each item in
+            #                the separated line, avoid the help message looks
+            #                messy, see _SmartHelpFormatter in cliff.
+            help=_('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 '
+                   '(required)\n'
+                   '<id>: UUID of the volume or snapshot (required)\n'
+                   '<type>: volume or snapshot; default: volume (optional)\n'
+                   '<size(GB)>: volume size if create from snapshot '
+                   '(optional)\n'
+                   '<delete-on-terminate>: true or false; default: false '
+                   '(optional)\n'
                    '(optional extension)'),
         )
         parser.add_argument(
@@ -593,33 +605,39 @@ class CreateServer(command.ShowOne):
                                         'source_type': 'volume',
                                         'destination_type': 'volume'
                                         }]
-        for dev_map in parsed_args.block_device_mapping:
-            dev_name, dev_map = dev_map.split('=', 1)
-            if dev_map:
-                dev_map = dev_map.split(':')
-                if len(dev_map) > 0:
-                    mapping = {
-                        'device_name': dev_name,
-                        'uuid': utils.find_resource(
-                            volume_client.volumes,
-                            dev_map[0],
-                        ).id}
-                    # Block device mapping v1 compatibility
-                    if len(dev_map) > 1 and \
-                            dev_map[1] in ('volume', 'snapshot'):
-                        mapping['source_type'] = dev_map[1]
-                    else:
-                        mapping['source_type'] = 'volume'
-                    mapping['destination_type'] = 'volume'
-                    if len(dev_map) > 2 and dev_map[2]:
-                        mapping['volume_size'] = dev_map[2]
-                    if len(dev_map) > 3:
-                        mapping['delete_on_termination'] = dev_map[3]
+        # Handle block device by device name order, like: vdb -> vdc -> vdd
+        for dev_name in sorted(six.iterkeys(parsed_args.block_device_mapping)):
+            dev_map = parsed_args.block_device_mapping[dev_name]
+            dev_map = dev_map.split(':')
+            if dev_map[0]:
+                mapping = {'device_name': dev_name}
+                # 1. decide source and destination type
+                if (len(dev_map) > 1 and
+                        dev_map[1] in ('volume', 'snapshot')):
+                    mapping['source_type'] = dev_map[1]
                 else:
-                    msg = _("Volume name or ID must be specified if "
-                            "--block-device-mapping is specified")
-                    raise exceptions.CommandError(msg)
-                block_device_mapping_v2.append(mapping)
+                    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
+                # 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 or snapshot (name or ID) must be specified if "
+                        "--block-device-mapping is specified")
+                raise exceptions.CommandError(msg)
+            block_device_mapping_v2.append(mapping)
 
         nics = []
         auto_or_none = False

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

@@ -388,6 +388,113 @@ class ServerTests(common.ComputeTestCase):
             cmd_output['status'],
         )
 
+    def test_server_boot_with_bdm_snapshot(self):
+        """Test server create from image with bdm snapshot, server delete"""
+        # get volume status wait function
+        volume_wait_for = test_volume.VolumeTests(
+            methodName='wait_for',
+        ).wait_for
+
+        # create source empty volume
+        empty_volume_name = uuid.uuid4().hex
+        cmd_output = json.loads(self.openstack(
+            'volume create -f json ' +
+            '--size 1 ' +
+            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'],
+        )
+        volume_wait_for("volume", empty_volume_name, "available")
+
+        # create snapshot of source empty volume
+        empty_snapshot_name = uuid.uuid4().hex
+        cmd_output = json.loads(self.openstack(
+            'volume snapshot create -f json ' +
+            '--volume ' + empty_volume_name + ' ' +
+            empty_snapshot_name
+        ))
+        self.assertIsNotNone(cmd_output["id"])
+        self.assertEqual(
+            empty_snapshot_name,
+            cmd_output['name'],
+        )
+        volume_wait_for("volume snapshot", empty_snapshot_name, "available")
+
+        # 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 ' +
+            self.network_arg + ' ' +
+            '--wait ' +
+            server_name
+        ))
+        self.assertIsNotNone(server["id"])
+        self.assertEqual(
+            server_name,
+            server['name'],
+        )
+        self.wait_for_status(server_name, 'ACTIVE')
+
+        # 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.assertTrue(volumes_attached.startswith('id='))
+        attached_volume_id = volumes_attached.replace('id=', '')
+
+        # check the volume that attached on server
+        cmd_output = json.loads(self.openstack(
+            'volume show -f json ' +
+            attached_volume_id
+        ))
+        attachments = cmd_output['attachments']
+        self.assertEqual(
+            1,
+            len(attachments),
+        )
+        self.assertEqual(
+            server['id'],
+            attachments[0]['server_id'],
+        )
+        self.assertEqual(
+            "in-use",
+            cmd_output['status'],
+        )
+
+        # delete server, then check the attached volume had been deleted,
+        # <delete-on-terminate>=true
+        self.openstack('server delete --wait ' + server_name)
+        cmd_output = json.loads(self.openstack(
+            'volume list -f json'
+        ))
+        target_volume = [each_volume
+                         for each_volume in cmd_output
+                         if each_volume['ID'] == attached_volume_id]
+        if target_volume:
+            # check the attached volume is 'deleting' status
+            self.assertEqual('deleting', target_volume[0]['Status'])
+        else:
+            # the attached volume had been deleted
+            pass
+
+        # clean up volume snapshot manually, make sure the snapshot and volume
+        # can be deleted sequentially, self.addCleanup so fast, that cause
+        # volume service API 400 error and the volume is left over at the end.
+        self.openstack('volume snapshot delete ' + empty_snapshot_name)
+        volume_wait_for('volume snapshot', empty_snapshot_name, 'disappear')
+
     def test_server_create_with_none_network(self):
         """Test server create with none network option."""
         server_name = uuid.uuid4().hex

openstackclient/tests/functional/volume/v2/test_volume.py

@@ -14,6 +14,8 @@ import json
 import time
 import uuid
 
+from tempest.lib import exceptions
+
 from openstackclient.tests.functional.volume.v2 import common
 
 
@@ -253,7 +255,13 @@ class VolumeTests(common.BaseVolumeTests):
         total_sleep = 0
         opts = self.get_opts(['status'])
         while total_sleep < wait:
-            status = self.openstack(check_type + ' show ' + check_name + opts)
+            try:
+                status = self.openstack(
+                    check_type + ' show ' + check_name + opts
+                )
+            except exceptions.CommandFailed:
+                # Show command raise Exception when object had been deleted
+                status = 'disappear'
             status = status.rstrip()
             print('Checking {} {} Waiting for {} current status: {}'
                   .format(check_type, check_name, desired_status, status))

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

@@ -12,6 +12,7 @@
 #   License for the specific language governing permissions and limitations
 #   under the License.
 #
+import argparse
 import collections
 import getpass
 import mock
@@ -49,6 +50,10 @@ class TestServer(compute_fakes.TestComputev2):
         self.volumes_mock = self.app.client_manager.volume.volumes
         self.volumes_mock.reset_mock()
 
+        # Get a shortcut to the volume client VolumeManager Mock
+        self.snapshots_mock = self.app.client_manager.volume.volume_snapshots
+        self.snapshots_mock.reset_mock()
+
         # Set object attributes to be tested. Could be overwritten in subclass.
         self.attrs = {}
 
@@ -326,7 +331,9 @@ class TestServerCreate(TestServer):
 
         self.volume = volume_fakes.FakeVolume.create_one_volume()
         self.volumes_mock.get.return_value = self.volume
-        self.block_device_mapping = 'vda=' + self.volume.name + ':::0'
+
+        self.snapshot = volume_fakes.FakeSnapshot.create_one_snapshot()
+        self.snapshots_mock.get.return_value = self.snapshot
 
         # Get the command object to test
         self.cmd = server.CreateServer(self.app, None)
@@ -852,13 +859,13 @@ class TestServerCreate(TestServer):
         arglist = [
             '--image', 'image1',
             '--flavor', self.flavor.id,
-            '--block-device-mapping', self.block_device_mapping,
+            '--block-device-mapping', 'vda=' + self.volume.name + ':::false',
             self.new_server.name,
         ]
         verifylist = [
             ('image', 'image1'),
             ('flavor', self.flavor.id),
-            ('block_device_mapping', [self.block_device_mapping]),
+            ('block_device_mapping', {'vda': self.volume.name + ':::false'}),
             ('config_drive', False),
             ('server_name', self.new_server.name),
         ]
@@ -867,11 +874,6 @@ class TestServerCreate(TestServer):
         # CreateServer.take_action() returns two tuples
         columns, data = self.cmd.take_action(parsed_args)
 
-        real_volume_mapping = (
-            (self.block_device_mapping.split('=', 1)[1]).replace(
-                self.volume.name,
-                self.volume.id))
-
         # Set expected values
         kwargs = dict(
             meta=None,
@@ -885,10 +887,10 @@ class TestServerCreate(TestServer):
             availability_zone=None,
             block_device_mapping_v2=[{
                 'device_name': 'vda',
-                'uuid': real_volume_mapping.split(':', 1)[0],
+                'uuid': self.volume.id,
                 'destination_type': 'volume',
                 'source_type': 'volume',
-                'delete_on_termination': '0'
+                'delete_on_termination': 'false',
             }],
             nics=[],
             scheduler_hints={},
@@ -905,6 +907,323 @@ class TestServerCreate(TestServer):
         self.assertEqual(self.columns, columns)
         self.assertEqual(self.datalist(), data)
 
+    def test_server_create_with_block_device_mapping_min_input(self):
+        arglist = [
+            '--image', 'image1',
+            '--flavor', self.flavor.id,
+            '--block-device-mapping', 'vdf=' + self.volume.name,
+            self.new_server.name,
+        ]
+        verifylist = [
+            ('image', 'image1'),
+            ('flavor', self.flavor.id),
+            ('block_device_mapping', {'vdf': self.volume.name}),
+            ('config_drive', False),
+            ('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 = dict(
+            meta=None,
+            files={},
+            reservation_id=None,
+            min_count=1,
+            max_count=1,
+            security_groups=[],
+            userdata=None,
+            key_name=None,
+            availability_zone=None,
+            block_device_mapping_v2=[{
+                'device_name': 'vdf',
+                'uuid': self.volume.id,
+                'destination_type': 'volume',
+                'source_type': 'volume',
+            }],
+            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_mapping_default_input(self):
+        arglist = [
+            '--image', 'image1',
+            '--flavor', self.flavor.id,
+            '--block-device-mapping', 'vdf=' + self.volume.name + ':::',
+            self.new_server.name,
+        ]
+        verifylist = [
+            ('image', 'image1'),
+            ('flavor', self.flavor.id),
+            ('block_device_mapping', {'vdf': self.volume.name + ':::'}),
+            ('config_drive', False),
+            ('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 = dict(
+            meta=None,
+            files={},
+            reservation_id=None,
+            min_count=1,
+            max_count=1,
+            security_groups=[],
+            userdata=None,
+            key_name=None,
+            availability_zone=None,
+            block_device_mapping_v2=[{
+                'device_name': 'vdf',
+                'uuid': self.volume.id,
+                'destination_type': 'volume',
+                'source_type': 'volume',
+            }],
+            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_mapping_full_input(self):
+        arglist = [
+            '--image', 'image1',
+            '--flavor', self.flavor.id,
+            '--block-device-mapping',
+            'vde=' + self.volume.name + ':volume:3:true',
+            self.new_server.name,
+        ]
+        verifylist = [
+            ('image', 'image1'),
+            ('flavor', self.flavor.id),
+            ('block_device_mapping',
+             {'vde': self.volume.name + ':volume:3:true'}),
+            ('config_drive', False),
+            ('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 = dict(
+            meta=None,
+            files={},
+            reservation_id=None,
+            min_count=1,
+            max_count=1,
+            security_groups=[],
+            userdata=None,
+            key_name=None,
+            availability_zone=None,
+            block_device_mapping_v2=[{
+                'device_name': 'vde',
+                'uuid': self.volume.id,
+                'destination_type': 'volume',
+                'source_type': 'volume',
+                'delete_on_termination': 'true',
+                'volume_size': '3'
+            }],
+            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_mapping_snapshot(self):
+        arglist = [
+            '--image', 'image1',
+            '--flavor', self.flavor.id,
+            '--block-device-mapping',
+            'vds=' + self.volume.name + ':snapshot:5:true',
+            self.new_server.name,
+        ]
+        verifylist = [
+            ('image', 'image1'),
+            ('flavor', self.flavor.id),
+            ('block_device_mapping',
+             {'vds': self.volume.name + ':snapshot:5:true'}),
+            ('config_drive', False),
+            ('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 = dict(
+            meta=None,
+            files={},
+            reservation_id=None,
+            min_count=1,
+            max_count=1,
+            security_groups=[],
+            userdata=None,
+            key_name=None,
+            availability_zone=None,
+            block_device_mapping_v2=[{
+                'device_name': 'vds',
+                'uuid': self.snapshot.id,
+                'destination_type': 'volume',
+                'source_type': 'snapshot',
+                'delete_on_termination': 'true',
+                'volume_size': '5'
+            }],
+            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_mapping_multiple(self):
+        arglist = [
+            '--image', 'image1',
+            '--flavor', self.flavor.id,
+            '--block-device-mapping', 'vdb=' + self.volume.name + ':::false',
+            '--block-device-mapping', 'vdc=' + self.volume.name + ':::true',
+            self.new_server.name,
+        ]
+        verifylist = [
+            ('image', 'image1'),
+            ('flavor', self.flavor.id),
+            ('block_device_mapping', {'vdb': self.volume.name + ':::false',
+                                      'vdc': self.volume.name + ':::true'}),
+            ('config_drive', False),
+            ('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 = dict(
+            meta=None,
+            files={},
+            reservation_id=None,
+            min_count=1,
+            max_count=1,
+            security_groups=[],
+            userdata=None,
+            key_name=None,
+            availability_zone=None,
+            block_device_mapping_v2=[
+                {
+                    'device_name': 'vdb',
+                    'uuid': self.volume.id,
+                    'destination_type': 'volume',
+                    'source_type': 'volume',
+                    'delete_on_termination': 'false',
+                },
+                {
+                    'device_name': 'vdc',
+                    'uuid': self.volume.id,
+                    'destination_type': 'volume',
+                    'source_type': 'volume',
+                    'delete_on_termination': 'true',
+                }
+            ],
+            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_mapping_invalid_format(self):
+        # 1. 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.check_parser,
+                          self.cmd, arglist, [])
+        # 2. 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.check_parser,
+                          self.cmd, arglist, [])
+
+    def test_server_create_with_block_device_mapping_no_uuid(self):
+        arglist = [
+            '--image', 'image1',
+            '--flavor', self.flavor.id,
+            '--block-device-mapping', 'vdb=',
+            self.new_server.name,
+        ]
+        verifylist = [
+            ('image', 'image1'),
+            ('flavor', self.flavor.id),
+            ('block_device_mapping', {'vdb': ''}),
+            ('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)
+
 
 class TestServerDelete(TestServer):
 

releasenotes/notes/bug-1667266-6497727abc2af9a5.yaml

@@ -0,0 +1,9 @@
+---
+fixes:
+  - |
+    Make ``block-device-mapping`` option of ``server create`` command more
+    stable and clear. Fix ValueError when input block device mapping option in
+    wrong format. Support to create block device from snapshot. Add details in
+    help message about block-device-mapping option format and regular value of
+    each item.
+    [Bug `1667266 <https://bugs.launchpad.net/python-openstackclient/+bug/1667266>`_]