Fix boot volume connectivity type discovery

Per the referenced bug, there was a code path whereby the boot volume
connectivity type could be unset, resulting in an exception.  In fixing
this, I discovered that we were looking for `driver_volume_type` in the
wrong place: it should be in connection_info and we were looking for it
under connection_info['data'].  I preserved the bogus code path in case
it's actually somehow legit, but added a path to the correct location.

We had no direct test for this stuff, so I added that too.

(cherry picked from commit af31c9f977)
Change-Id: I78e55b3d8096d4013afd0abe1e42c32dc9697871
Closes-Bug: #1769648

nova_powervm/tests/virt/powervm/test_driver.py

@@ -915,6 +915,72 @@ class TestPowerVMDriver(test.NoDBTestCase):
         flow.add.assert_has_calls([mock.call('disconn_vol1'),
                                    mock.call('disconn_vol2')])
 
+    @mock.patch('nova_powervm.virt.powervm.driver.PowerVMDriver.'
+                '_is_booted_from_volume', new=mock.Mock(return_value=False))
+    def test_get_boot_connectivity_type_no_bfv(self):
+        # Boot connectivity type defaults to vscsi when not booted from volume.
+        self.assertEqual('vscsi', self.drv._get_boot_connectivity_type(
+            'bdms', 'block_device_info'))
+
+    @mock.patch('nova_powervm.virt.powervm.driver.PowerVMDriver.'
+                '_is_booted_from_volume', new=mock.Mock(return_value=True))
+    def test_get_boot_connectivity_type_no_bdms(self):
+        # Boot connectivity type defaults to vscsi when no BDMs
+        self.assertEqual('vscsi', self.drv._get_boot_connectivity_type(
+            None, 'block_device_info'))
+
+    @mock.patch('nova_powervm.virt.powervm.driver.PowerVMDriver.'
+                '_is_booted_from_volume', new=mock.Mock(return_value=True))
+    def test_get_boot_connectivity_type_no_boot_bdm(self):
+        # Boot connectivity type defaults to vscsi when no BDM has a boot_index
+        # of zero (which should actually never happen IRL).
+        self.assertEqual('vscsi', self.drv._get_boot_connectivity_type(
+            [{'boot_index': 1}], 'block_device_info'))
+
+    @mock.patch('nova_powervm.virt.powervm.driver.PowerVMDriver.'
+                '_is_booted_from_volume', new=mock.Mock(return_value=True))
+    def test_get_boot_connectivity_type_driver_volume_type(self):
+        # Boot connectivity type discovered via BDM driver_volume_type.
+        bdms = self._fake_bdms(set_boot_index=True)['block_device_mapping']
+        self.assertEqual('fibre_channel', self.drv._get_boot_connectivity_type(
+            bdms, 'block_device_info'))
+
+    @mock.patch('nova_powervm.virt.powervm.driver.PowerVMDriver.'
+                '_is_booted_from_volume', new=mock.Mock(return_value=True))
+    def test_get_boot_connectivity_type_data_driver_volume_type(self):
+        # Boot connectivity type discovered via BDM driver_volume_type in
+        # conn_info['data'], which I think is bogus, but preserved for
+        # compatibility.
+        bdms = self._fake_bdms(
+            set_boot_index=True,
+            driver_volume_type_in_data=True)['block_device_mapping']
+        self.assertEqual('fibre_channel', self.drv._get_boot_connectivity_type(
+            bdms, 'block_device_info'))
+
+    @mock.patch('nova_powervm.virt.powervm.driver.PowerVMDriver.'
+                '_is_booted_from_volume', new=mock.Mock(return_value=True))
+    def test_get_boot_connectivity_type_connection_type(self):
+        # Boot connectivity type discovered from BDM's connectivity-type
+        bdms = self._fake_bdms(connection_type='hello',
+                               set_boot_index=True)['block_device_mapping']
+        self.assertEqual('hello', self.drv._get_boot_connectivity_type(
+            bdms, 'block_device_info'))
+        # We convert 'pv_vscsi' to 'vscsi'
+        bdms = self._fake_bdms(connection_type='pv_vscsi',
+                               set_boot_index=True)['block_device_mapping']
+        self.assertEqual('vscsi', self.drv._get_boot_connectivity_type(
+            bdms, 'block_device_info'))
+
+    @mock.patch('nova_powervm.virt.powervm.driver.PowerVMDriver.'
+                '_is_booted_from_volume', new=mock.Mock(return_value=True))
+    def test_get_boot_connectivity_type_driver_volume_type_unset(self):
+        # Boot connectivity type defaults to vscsi when BDM driver_volume_type
+        # is unset.
+        bdms = self._fake_bdms(driver_volume_type=None,
+                               set_boot_index=True)['block_device_mapping']
+        self.assertEqual('vscsi', self.drv._get_boot_connectivity_type(
+            bdms, 'block_device_info'))
+
     @mock.patch('pypowervm.tasks.scsi_mapper.remove_maps')
     @mock.patch('pypowervm.wrappers.entry_wrapper.EntryWrapper.update',
                 new=mock.Mock())
@@ -1681,18 +1747,26 @@ class TestPowerVMDriver(test.NoDBTestCase):
                           mock.ANY, self.inst)
 
     @staticmethod
-    def _fake_bdms():
+    def _fake_bdms(set_boot_index=False, connection_type=None,
+                   driver_volume_type='fibre_channel',
+                   driver_volume_type_in_data=False):
         def _fake_bdm(volume_id, target_lun):
-            connection_info = {'driver_volume_type': 'fibre_channel',
+            conninfo = {'data': {'volume_id': volume_id,
-                               'data': {'volume_id': volume_id,
+                                 'target_lun': target_lun,
-                                        'target_lun': target_lun,
+                                 'initiator_target_map':
-                                        'initiator_target_map':
+                                     {'21000024F5': ['50050768']}}}
-                                        {'21000024F5': ['50050768']}}}
+            if connection_type is not None:
+                conninfo['data']['connection-type'] = connection_type
+            if driver_volume_type is not None:
+                if driver_volume_type_in_data:
+                    conninfo['data']['driver_volume_type'] = driver_volume_type
+                else:
+                    conninfo['driver_volume_type'] = driver_volume_type
             mapping_dict = {'source_type': 'volume', 'volume_id': volume_id,
                             'destination_type': 'volume',
-                            'connection_info':
+                            'connection_info': jsonutils.dumps(conninfo)}
-                            jsonutils.dumps(connection_info),
+            if set_boot_index:
-                            }
+                mapping_dict['boot_index'] = target_lun
             bdm_dict = nova_block_device.BlockDeviceDict(mapping_dict)
             bdm_obj = bdmobj.BlockDeviceMapping(**bdm_dict)
 

nova_powervm/virt/powervm/driver.py

@@ -454,7 +454,7 @@ class PowerVMDriver(driver.ComputeDriver):
         distro = instance.system_metadata.get('image_os_distro', '')
         if distro.lower() == img.OSDistro.OS400:
             boot_type = self._get_boot_connectivity_type(
-                context, bdms, block_device_info)
+                bdms, block_device_info)
             flow_spawn.add(tf_vm.UpdateIBMiSettings(
                 self.adapter, instance, boot_type))
 
@@ -545,7 +545,7 @@ class PowerVMDriver(driver.ComputeDriver):
 
         root_bdm = block_device.get_root_bdm(
             driver.block_device_info_get_mapping(block_device_info))
-        return (root_bdm is not None)
+        return root_bdm is not None
 
     @property
     def need_legacy_block_device_info(self):
@@ -1801,36 +1801,36 @@ class PowerVMDriver(driver.ComputeDriver):
                   instance=instance)
         return list(xags)
 
-    def _get_boot_connectivity_type(self, context, bdms, block_device_info):
+    def _get_boot_connectivity_type(self, bdms, block_device_info):
         """Get connectivity information for the instance.
 
-        :param context: security context
         :param bdms: The BDMs for the operation. If boot volume of
                      the instance is ssp lu or local disk, the bdms is None.
         :param block_device_info: Instance volume block device info.
-        :return: Returns the boot connectivity type,
+        :return: The boot connectivity type.
-                 If boot volume is a npiv volume, returns 'npiv'
+                 If boot volume is an npiv volume, returns 'fibre_channel'.
-                 Otherwise, return 'vscsi'.
+                 Otherwise, returns 'vscsi'.
         """
-        # Set default boot_conn_type as 'vscsi'
-        boot_conn_type = 'vscsi'
         if self._is_booted_from_volume(block_device_info) and bdms is not None:
             for bdm in bdms:
                 if bdm.get('boot_index') == 0:
                     return self._get_connectivity_type(bdm)
-        else:
+        # Default connectivity type is 'vscsi'
-            return boot_conn_type
+        return 'vscsi'
 
     @staticmethod
     def _get_connectivity_type(bdm):
         conn_info = bdm.get('connection_info')
         if 'connection-type' in conn_info['data']:
             connectivity_type = conn_info['data']['connection-type']
-            boot_conn_type = ('vscsi' if connectivity_type == 'pv_vscsi' else
+            return ('vscsi' if connectivity_type == 'pv_vscsi'
-                              connectivity_type)
+                    else connectivity_type)
-        elif 'driver_volume_type' in conn_info['data']:
+        # Seemingly bogus path (driver_volume_type shouldn't be in 'data'),
-            boot_conn_type = conn_info['data']['driver_volume_type']
+        # preserved for potential compatibility.
-        return boot_conn_type
+        if 'driver_volume_type' in conn_info['data']:
+            return conn_info['data']['driver_volume_type']
+        # Actual location for driver_volume_type.  Default to vscsi.
+        return conn_info.get('driver_volume_type', 'vscsi')
 
     def deallocate_networks_on_reschedule(self, instance):
         """Does the driver want networks deallocated on reschedule?