Switch server create to block_device_mapping_v2

Current compute_client.servers.create() relies on block_device_mapping
arg which is legacy[1]. "block_device_mapping" format require
device_name which is leads to hard-coded hack in --volume key handler
to KVM specific. "block_device_mapping_v2" format is more friendly
to hypervisiors.
Support of block_device_mapping_v2 appear in python-novaclient 2.16.0,
openstackclient require at least 2.29.0

Makes options --volume and --block-device-mapping work simultaneously.
Appends --block-device-mapping data even if --volume used.
After bug 1383338 only --volume was taken when both are used.

[1]http://docs.openstack.org/developer/nova/block_device_mapping.html

NOTE(dtroyer): I moved the new test_boot_from_volume() functional
test to Ie51b1c375c5940856ec76a5770df3c6bd18a3eba to test our
previous behaviour.  The only changes required to support the new
behaviour should be that the empty_volume is now attached in that test.

Change-Id: I7bac3d870dd9ca404093142f8bce22a62e49180d
Closes-Bug: 1647406
Closes-Bug: 1497845

Author: nikitych

openstackclient/compute/v2/server.py

@@ -507,28 +507,40 @@ def take_action(self, parsed_args):
                            "exception": e}
                 )
 
-        block_device_mapping = {}
+        block_device_mapping_v2 = []
         if volume:
-            # When booting from volume, for now assume no other mappings
-            # This device value is likely KVM-specific
-            block_device_mapping = {'vda': volume}
-        else:
-            for dev_map in parsed_args.block_device_mapping:
-                dev_key, dev_vol = dev_map.split('=', 1)
-                block_volume = None
-                if dev_vol:
-                    vol = dev_vol.split(':', 1)[0]
-                    if vol:
-                        vol_id = utils.find_resource(
+            block_device_mapping_v2 = [{'uuid': volume,
+                                        'boot_index': '0',
+                                        '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,
-                            vol,
-                        ).id
-                        block_volume = dev_vol.replace(vol, vol_id)
+                            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:
-                        msg = _("Volume name or ID must be specified if "
-                                "--block-device-mapping is specified")
-                        raise exceptions.CommandError(msg)
-                block_device_mapping.update({dev_key: block_volume})
+                        mapping['source_type'] = 'volume'
+                    mapping['destination_type'] = 'volume'
+                    if len(dev_map) > 2:
+                        mapping['volume_size'] = dev_map[2]
+                    if len(dev_map) > 3:
+                        mapping['delete_on_termination'] = dev_map[3]
+                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)
 
         nics = []
         if parsed_args.nic in ('auto', 'none'):
@@ -598,7 +610,7 @@ def take_action(self, parsed_args):
             userdata=userdata,
             key_name=parsed_args.key_name,
             availability_zone=parsed_args.availability_zone,
-            block_device_mapping=block_device_mapping,
+            block_device_mapping_v2=block_device_mapping_v2,
             nics=nics,
             scheduler_hints=hints,
             config_drive=config_drive)

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

@@ -417,19 +417,23 @@ def test_server_boot_from_volume(self):
 
         # NOTE(dtroyer): Prior to https://review.openstack.org/#/c/407111
         #                --block-device-mapping was ignored if --volume
-        #                present on the command line, so this volume should
-        #                not be attached.
+        #                present on the command line.  Now we should see the
+        #                attachment.
         cmd_output = json.loads(self.openstack(
             'volume show -f json ' +
             empty_volume_name
         ))
         attachments = cmd_output['attachments']
         self.assertEqual(
-            0,
+            1,
             len(attachments),
         )
         self.assertEqual(
-            "available",
+            server['id'],
+            attachments[0]['server_id'],
+        )
+        self.assertEqual(
+            "in-use",
             cmd_output['status'],
         )
 

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

@@ -308,7 +308,7 @@ def test_server_create_minimal(self):
             userdata=None,
             key_name=None,
             availability_zone=None,
-            block_device_mapping={},
+            block_device_mapping_v2=[],
             nics=[],
             scheduler_hints={},
             config_drive=None,
@@ -363,7 +363,7 @@ def test_server_create_with_options(self):
             userdata=None,
             key_name='keyname',
             availability_zone=None,
-            block_device_mapping={},
+            block_device_mapping_v2=[],
             nics=[],
             scheduler_hints={'a': ['b', 'c']},
             config_drive=None,
@@ -443,7 +443,7 @@ def test_server_create_with_network(self):
             userdata=None,
             key_name=None,
             availability_zone=None,
-            block_device_mapping={},
+            block_device_mapping_v2=[],
             nics=[{'net-id': 'net1_uuid',
                    'v4-fixed-ip': '',
                    'v6-fixed-ip': '',
@@ -500,7 +500,7 @@ def test_server_create_with_wait_ok(self, mock_wait_for_status):
             userdata=None,
             key_name=None,
             availability_zone=None,
-            block_device_mapping={},
+            block_device_mapping_v2=[],
             nics=[],
             scheduler_hints={},
             config_drive=None,
@@ -550,7 +550,7 @@ def test_server_create_with_wait_fails(self, mock_wait_for_status):
             userdata=None,
             key_name=None,
             availability_zone=None,
-            block_device_mapping={},
+            block_device_mapping_v2=[],
             nics=[],
             scheduler_hints={},
             config_drive=None,
@@ -605,7 +605,7 @@ def test_server_create_userdata(self, mock_open):
             userdata=mock_file,
             key_name=None,
             availability_zone=None,
-            block_device_mapping={},
+            block_device_mapping_v2=[],
             nics=[],
             scheduler_hints={},
             config_drive=None,
@@ -656,9 +656,14 @@ def test_server_create_with_block_device_mapping(self):
             userdata=None,
             key_name=None,
             availability_zone=None,
-            block_device_mapping={
-                'vda': real_volume_mapping
-            },
+            block_device_mapping_v2=[{
+                'device_name': 'vda',
+                'uuid': real_volume_mapping.split(':', 1)[0],
+                'destination_type': 'volume',
+                'source_type': 'volume',
+                'delete_on_termination': '0',
+                'volume_size': ''
+            }],
             nics=[],
             scheduler_hints={},
             config_drive=None,

releasenotes/notes/bug-1647406-c936581034a1b6e4.yaml

@@ -0,0 +1,16 @@
+---
+fixes:
+  - |
+    Allow ``--block-device-mapping`` option to work correctly with
+    ``--volume`` option in ``server create`` command.
+    After :lpbug:`1383338` ``--block-device-mapping`` was ignored if
+    ``--volume`` was present. Block device mappings are now appended
+    to the mapping created by the ``--volume`` option if it is present.
+    The device name of the boot volume specificed in the ``--volume`` option
+    is no longer assumed to be *'vda'* but now uses the hypervisor's boot
+    index to obtain the device name.  This maintains the status quo for
+    **QEMU/KVM** hypervisors but **XEN**, **parallels** and others 
+    *virt types* that have device naming is different from ``vd*``
+    should now also work correctly.
+    [:lpbug:`1497845`]
+    [:lpbug:`1647406`]