Add validation of block_device_mapping_v2

Heat assumes image/volume/snapshot in bdm_v2
as bootable sources, if user does not specify
the 'boot_index' for them, heat will give the
value '0' for all of them, but nova does not
allow this.
This patch adds validation to avoid the error.

Closes-Bug: #1658007
Change-Id: I6809c8cb839eba20ffe12f783854fbaa7b9267d6
(cherry picked from commit 8fb7c3b9e43934c601bf78f2f8010aa3d6e8dc9e)

2 files changed, 79 insertions, 79 deletions

diff --git a/heat/engine/resources/openstack/nova/server.py b/heat/engine/resources/openstack/nova/server.py
index 6401834f1..23dfabf02 100644
--- a/heat/engine/resources/openstack/nova/server.py
+++ b/heat/engine/resources/openstack/nova/server.py
@@ -290,7 +290,10 @@ class Server(server_base.BaseServer, sh.SchedulerHintsMixin,
                     ),
                     BLOCK_DEVICE_MAPPING_BOOT_INDEX: properties.Schema(
                         properties.Schema.INTEGER,
-                        _('Integer used for ordering the boot disks.'),
+                        _('Integer used for ordering the boot disks. If '
+                          'it is not specified, value "0" will be set '
+                          'for bootable sources (volume, snapshot, image); '
+                          'value "-1" will be set for non-bootable sources.'),
                     ),
                     BLOCK_DEVICE_MAPPING_VOLUME_SIZE: properties.Schema(
                         properties.Schema.INTEGER,
@@ -1257,11 +1260,16 @@ class Server(server_base.BaseServer, sh.SchedulerHintsMixin,
         # either volume_id or snapshot_id needs to be specified, but not both
         # for block device mapping.
         bdm = self.properties[self.BLOCK_DEVICE_MAPPING] or []
-        bootable_vol = False
+        bdm_v2 = self.properties[self.BLOCK_DEVICE_MAPPING_V2] or []
+        image = self.properties[self.IMAGE]
+        if bdm and bdm_v2:
+            raise exception.ResourcePropertyConflict(
+                self.BLOCK_DEVICE_MAPPING, self.BLOCK_DEVICE_MAPPING_V2)
+        bootable = image is not None
         for mapping in bdm:
             device_name = mapping[self.BLOCK_DEVICE_MAPPING_DEVICE_NAME]
             if device_name == 'vda':
-                bootable_vol = True
+                bootable = True
 
             volume_id = mapping.get(self.BLOCK_DEVICE_MAPPING_VOLUME_ID)
             snapshot_id = mapping.get(self.BLOCK_DEVICE_MAPPING_SNAPSHOT_ID)
@@ -1274,15 +1282,12 @@ class Server(server_base.BaseServer, sh.SchedulerHintsMixin,
                         ' device mapping %s') % device_name
                 raise exception.StackValidationFailed(message=msg)
 
-        bdm_v2 = self.properties[self.BLOCK_DEVICE_MAPPING_V2] or []
-        if bdm and bdm_v2:
-            raise exception.ResourcePropertyConflict(
-                self.BLOCK_DEVICE_MAPPING, self.BLOCK_DEVICE_MAPPING_V2)
-
+        bootable_devs = [image]
         for mapping in bdm_v2:
             volume_id = mapping.get(self.BLOCK_DEVICE_MAPPING_VOLUME_ID)
             snapshot_id = mapping.get(self.BLOCK_DEVICE_MAPPING_SNAPSHOT_ID)
             image_id = mapping.get(self.BLOCK_DEVICE_MAPPING_IMAGE)
+            boot_index = mapping.get(self.BLOCK_DEVICE_MAPPING_BOOT_INDEX)
             swap_size = mapping.get(self.BLOCK_DEVICE_MAPPING_SWAP_SIZE)
             ephemeral = (mapping.get(
                 self.BLOCK_DEVICE_MAPPING_EPHEMERAL_SIZE) or mapping.get(
@@ -1309,9 +1314,21 @@ class Server(server_base.BaseServer, sh.SchedulerHintsMixin,
 
             if any((volume_id is not None, snapshot_id is not None,
                     image_id is not None)):
-                bootable_vol = True
-
-        return bootable_vol
+                # boot_index is not specified, set boot_index=0 when
+                # build_block_device_mapping for volume, snapshot, image
+                if boot_index is None or boot_index == 0:
+                    bootable = True
+                    bootable_devs.append(volume_id)
+                    bootable_devs.append(snapshot_id)
+                    bootable_devs.append(image_id)
+        if not bootable:
+            msg = _('Neither image nor bootable volume is specified for '
+                    'instance %s') % self.name
+            raise exception.StackValidationFailed(message=msg)
+        if bdm_v2 and len(list(
+                dev for dev in bootable_devs if dev is not None)) != 1:
+            msg = _('Multiple bootable sources for instance %s.') % self.name
+            raise exception.StackValidationFailed(message=msg)
 
     def _validate_image_flavor(self, image, flavor):
         try:
@@ -1359,15 +1376,10 @@ class Server(server_base.BaseServer, sh.SchedulerHintsMixin,
                         'with user_data_format of SOFTWARE_CONFIG')
                 raise exception.StackValidationFailed(message=msg)
 
-        bootable_vol = self._validate_block_device_mapping()
+        self._validate_block_device_mapping()
 
         # make sure the image exists if specified.
         image = self.properties[self.IMAGE]
-        if image is None and not bootable_vol:
-            msg = _('Neither image nor bootable volume is specified for'
-                    ' instance %s') % self.name
-            raise exception.StackValidationFailed(message=msg)
-
         flavor = self.properties[self.FLAVOR]
         if image:
             self._validate_image_flavor(image, flavor)
diff --git a/heat/tests/openstack/nova/test_server.py b/heat/tests/openstack/nova/test_server.py
index 005fc48ae..b6c08daee 100644
--- a/heat/tests/openstack/nova/test_server.py
+++ b/heat/tests/openstack/nova/test_server.py
@@ -2870,96 +2870,84 @@ class ServersTest(common.HeatTestCase):
                'block_device_mapping, block_device_mapping_v2.')
         self.assertEqual(msg, six.text_type(exc))
 
-    @mock.patch.object(nova.NovaClientPlugin, '_create')
-    def test_validate_conflict_block_device_mapping_v2_props(self,
-                                                             mock_create):
-        stack_name = 'val_blkdev2'
-        (tmpl, stack) = self._setup_test_stack(stack_name)
-
-        bdm_v2 = [{'volume_id': '1', 'snapshot_id': 2}]
+    def _test_validate_bdm_v2(self, stack_name, bdm_v2, with_image=True,
+                              error_msg=None, raise_exc=None):
+        tmpl, stack = self._setup_test_stack(stack_name)
+        if not with_image:
+            del tmpl['Resources']['WebServer']['Properties']['image']
         wsp = tmpl.t['Resources']['WebServer']['Properties']
         wsp['block_device_mapping_v2'] = bdm_v2
+
         resource_defns = tmpl.resource_definitions(stack)
         server = servers.Server('server_create_image_err',
                                 resource_defns['WebServer'], stack)
+        self.patchobject(nova.NovaClientPlugin, 'get_flavor',
+                         return_value=self.mock_flavor)
+        self.patchobject(glance.GlanceClientPlugin, 'get_image',
+                         return_value=self.mock_image)
         self.stub_VolumeConstraint_validate()
-        self.stub_SnapshotConstraint_validate()
-        self.assertRaises(exception.ResourcePropertyConflict, server.validate)
+        if raise_exc:
+            ex = self.assertRaises(raise_exc, server.validate)
+            self.assertIn(error_msg, six.text_type(ex))
+        else:
+            self.assertIsNone(server.validate())
 
     @mock.patch.object(nova.NovaClientPlugin, '_create')
-    def test_validate_without_bootable_source_in_bdm_v2(self, mock_create):
+    def test_validate_conflict_block_device_mapping_v2_props(self,
+                                                             mock_create):
         stack_name = 'val_blkdev2'
-        (tmpl, stack) = self._setup_test_stack(stack_name)
+        bdm_v2 = [{'volume_id': '1', 'snapshot_id': 2}]
+        error_msg = ('Cannot define the following properties at '
+                     'the same time: volume_id, snapshot_id')
+        self.stub_SnapshotConstraint_validate()
+        self._test_validate_bdm_v2(
+            stack_name, bdm_v2,
+            raise_exc=exception.ResourcePropertyConflict,
+            error_msg=error_msg)
 
+    @mock.patch.object(nova.NovaClientPlugin, '_create')
+    def test_validate_bdm_v2_with_empty_mapping(self, mock_create):
+        stack_name = 'val_blkdev2'
         bdm_v2 = [{}]
-        wsp = tmpl.t['Resources']['WebServer']['Properties']
-        wsp['block_device_mapping_v2'] = bdm_v2
-        resource_defns = tmpl.resource_definitions(stack)
-        server = servers.Server('server_create_image_err',
-                                resource_defns['WebServer'], stack)
-        exc = self.assertRaises(exception.StackValidationFailed,
-                                server.validate)
         msg = ('Either volume_id, snapshot_id, image_id, swap_size, '
                'ephemeral_size or ephemeral_format must be specified.')
-        self.assertEqual(msg, six.text_type(exc))
+        self._test_validate_bdm_v2(stack_name, bdm_v2,
+                                   raise_exc=exception.StackValidationFailed,
+                                   error_msg=msg)
 
     @mock.patch.object(nova.NovaClientPlugin, '_create')
     def test_validate_bdm_v2_properties_success(self, mock_create):
-        stack_name = 'v2_properties'
-        (tmpl, stack) = self._setup_test_stack(stack_name)
-
-        bdm_v2 = [{'volume_id': '1'}]
-        wsp = tmpl.t['Resources']['WebServer']['Properties']
-        wsp['block_device_mapping_v2'] = bdm_v2
-
-        resource_defns = tmpl.resource_definitions(stack)
-        server = servers.Server('server_create_image_err',
-                                resource_defns['WebServer'], stack)
-        self.patchobject(nova.NovaClientPlugin, 'get_flavor',
-                         return_value=self.mock_flavor)
-        self.patchobject(glance.GlanceClientPlugin, 'get_image',
-                         return_value=self.mock_image)
-        self.stub_VolumeConstraint_validate()
-        self.assertIsNone(server.validate())
+        stack_name = 'bdm_v2_success'
+        bdm_v2 = [{'volume_id': '1', 'boot_index': -1}]
+        self._test_validate_bdm_v2(stack_name, bdm_v2)
 
     @mock.patch.object(nova.NovaClientPlugin, '_create')
     def test_validate_bdm_v2_with_unresolved_volume(self, mock_create):
-        stack_name = 'v2_properties'
-        (tmpl, stack) = self._setup_test_stack(stack_name)
-        del tmpl['Resources']['WebServer']['Properties']['image']
-
+        stack_name = 'bdm_v2_with_unresolved_vol'
         # empty string indicates that volume is unresolved
         bdm_v2 = [{'volume_id': ''}]
-        wsp = tmpl.t['Resources']['WebServer']['Properties']
-        wsp['block_device_mapping_v2'] = bdm_v2
+        self._test_validate_bdm_v2(stack_name, bdm_v2, with_image=False)
 
-        resource_defns = tmpl.resource_definitions(stack)
-        server = servers.Server('server_create_image_err',
-                                resource_defns['WebServer'], stack)
-        self.patchobject(nova.NovaClientPlugin, 'get_flavor',
-                         return_value=self.mock_flavor)
-        self.patchobject(glance.GlanceClientPlugin, 'get_image',
-                         return_value=self.mock_image)
-        self.stub_VolumeConstraint_validate()
-        self.assertIsNone(server.validate())
+    @mock.patch.object(nova.NovaClientPlugin, '_create')
+    def test_validate_bdm_v2_multiple_bootable_source(self, mock_create):
+        stack_name = 'v2_multiple_bootable'
+        # with two bootable sources: volume_id and image
+        bdm_v2 = [{'volume_id': '1', 'boot_index': 0}]
+        msg = ('Multiple bootable sources for instance')
+        self._test_validate_bdm_v2(stack_name, bdm_v2,
+                                   raise_exc=exception.StackValidationFailed,
+                                   error_msg=msg)
 
     @mock.patch.object(nova.NovaClientPlugin, '_create')
     def test_validate_bdm_v2_properties_no_bootable_vol(self, mock_create):
-        stack_name = 'v2_properties'
-        (tmpl, stack) = self._setup_test_stack(stack_name)
-
+        stack_name = 'bdm_v2_no_bootable'
         bdm_v2 = [{'swap_size': 10}]
-        wsp = tmpl.t['Resources']['WebServer']['Properties']
-        wsp['block_device_mapping_v2'] = bdm_v2
-        wsp.pop('image')
-        resource_defns = tmpl.resource_definitions(stack)
-        server = servers.Server('server_create_image_err',
-                                resource_defns['WebServer'], stack)
-        exc = self.assertRaises(exception.StackValidationFailed,
-                                server.validate)
         msg = ('Neither image nor bootable volume is specified for instance '
                'server_create_image_err')
-        self.assertEqual(msg, six.text_type(exc))
+        self._test_validate_bdm_v2(stack_name, bdm_v2,
+                                   raise_exc=exception.StackValidationFailed,
+                                   error_msg=msg,
+                                   with_image=False)
 
     def test_validate_metadata_too_many(self):
         stack_name = 'srv_val_metadata'