From af31c9f97745a8c875cb8829482b7131da080f82 Mon Sep 17 00:00:00 2001 From: Eric Fried Date: Fri, 4 May 2018 17:51:22 -0500 Subject: [PATCH] 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. Change-Id: I78e55b3d8096d4013afd0abe1e42c32dc9697871 Closes-Bug: #1769648 --- .../tests/virt/powervm/test_driver.py | 92 +++++++++++++++++-- nova_powervm/virt/powervm/driver.py | 32 +++---- 2 files changed, 99 insertions(+), 25 deletions(-) diff --git a/nova_powervm/tests/virt/powervm/test_driver.py b/nova_powervm/tests/virt/powervm/test_driver.py index d340a54f..724a0f7a 100644 --- a/nova_powervm/tests/virt/powervm/test_driver.py +++ b/nova_powervm/tests/virt/powervm/test_driver.py @@ -955,6 +955,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()) @@ -1724,18 +1790,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', - 'data': {'volume_id': volume_id, - 'target_lun': target_lun, - 'initiator_target_map': - {'21000024F5': ['50050768']}}} + conninfo = {'data': {'volume_id': volume_id, + 'target_lun': target_lun, + 'initiator_target_map': + {'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': - jsonutils.dumps(connection_info), - } + 'connection_info': jsonutils.dumps(conninfo)} + if set_boot_index: + mapping_dict['boot_index'] = target_lun bdm_dict = nova_block_device.BlockDeviceDict(mapping_dict) bdm_obj = bdmobj.BlockDeviceMapping(**bdm_dict) diff --git a/nova_powervm/virt/powervm/driver.py b/nova_powervm/virt/powervm/driver.py index f0df5b5c..d083660b 100644 --- a/nova_powervm/virt/powervm/driver.py +++ b/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): @@ -1803,36 +1803,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, - If boot volume is a npiv volume, returns 'npiv' - Otherwise, return 'vscsi'. + :return: The boot connectivity type. + If boot volume is an npiv volume, returns 'fibre_channel'. + 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: - return boot_conn_type + # Default connectivity type is 'vscsi' + 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 - connectivity_type) - elif 'driver_volume_type' in conn_info['data']: - boot_conn_type = conn_info['data']['driver_volume_type'] - return boot_conn_type + return ('vscsi' if connectivity_type == 'pv_vscsi' + else connectivity_type) + # Seemingly bogus path (driver_volume_type shouldn't be in 'data'), + # preserved for potential compatibility. + 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?