diff options
| -rw-r--r-- | nova/compute/manager.py | 37 | ||||
| -rw-r--r-- | nova/exception.py | 4 | ||||
| -rw-r--r-- | nova/network/neutronv2/api.py | 47 | ||||
| -rw-r--r-- | nova/objects/migration_context.py | 32 | ||||
| -rw-r--r-- | nova/tests/unit/compute/test_compute.py | 14 | ||||
| -rw-r--r-- | nova/tests/unit/compute/test_compute_mgr.py | 85 | ||||
| -rw-r--r-- | nova/tests/unit/network/test_neutronv2.py | 55 | ||||
| -rw-r--r-- | nova/tests/unit/objects/test_migration_context.py | 32 | ||||
| -rw-r--r-- | nova/tests/unit/virt/ironic/test_driver.py | 27 | ||||
| -rw-r--r-- | nova/tests/unit/virt/libvirt/volume/test_quobyte.py | 34 | ||||
| -rw-r--r-- | nova/tests/unit/virt/xenapi/test_agent.py | 14 | ||||
| -rw-r--r-- | nova/virt/ironic/driver.py | 13 | ||||
| -rw-r--r-- | nova/virt/libvirt/volume/quobyte.py | 54 | ||||
| -rw-r--r-- | nova/virt/xenapi/agent.py | 17 | ||||
| -rw-r--r-- | releasenotes/notes/qb-bug-1730933-6695470ebaee0fbd.yaml | 5 |
15 files changed, 332 insertions, 138 deletions
diff --git a/nova/compute/manager.py b/nova/compute/manager.py index c9c99b8348..c60a16d938 100644 --- a/nova/compute/manager.py +++ b/nova/compute/manager.py @@ -28,6 +28,7 @@ terminating it. import base64 import binascii import contextlib +import copy import functools import inspect import sys @@ -4025,6 +4026,33 @@ class ComputeManager(manager.Manager): do_confirm_resize(context, instance, migration.id) + def _get_updated_nw_info_with_pci_mapping(self, nw_info, pci_mapping): + # NOTE(adrianc): This method returns a copy of nw_info if modifications + # are made else it returns the original nw_info. + updated_nw_info = nw_info + if nw_info and pci_mapping: + updated_nw_info = copy.deepcopy(nw_info) + for vif in updated_nw_info: + if vif['vnic_type'] in network_model.VNIC_TYPES_SRIOV: + try: + vif_pci_addr = vif['profile']['pci_slot'] + new_addr = pci_mapping[vif_pci_addr].address + vif['profile']['pci_slot'] = new_addr + LOG.debug("Updating VIF's PCI address for VIF %(id)s. " + "Original value %(orig_val)s, " + "new value %(new_val)s", + {'id': vif['id'], + 'orig_val': vif_pci_addr, + 'new_val': new_addr}) + except (KeyError, AttributeError): + with excutils.save_and_reraise_exception(): + # NOTE(adrianc): This should never happen. If we + # get here it means there is some inconsistency + # with either 'nw_info' or 'pci_mapping'. + LOG.error("Unexpected error when updating network " + "information with PCI mapping.") + return updated_nw_info + def _confirm_resize(self, context, instance, migration=None): """Destroys the source instance.""" self._notify_about_instance_usage(context, instance, @@ -4043,9 +4071,16 @@ class ComputeManager(manager.Manager): # NOTE(tr3buchet): tear down networks on source host self.network_api.setup_networks_on_host(context, instance, migration.source_compute, teardown=True) - network_info = self.network_api.get_instance_nw_info(context, instance) + + # NOTE(adrianc): Populate old PCI device in VIF profile + # to allow virt driver to properly unplug it from Hypervisor. + pci_mapping = (instance.migration_context. + get_pci_mapping_for_migration(True)) + network_info = self._get_updated_nw_info_with_pci_mapping( + network_info, pci_mapping) + # TODO(mriedem): Get BDMs here and pass them to the driver. self.driver.confirm_migration(context, migration, instance, network_info) diff --git a/nova/exception.py b/nova/exception.py index 64dc557d25..7658f08ea9 100644 --- a/nova/exception.py +++ b/nova/exception.py @@ -359,6 +359,10 @@ class InvalidVolumeAccessMode(Invalid): msg_fmt = _("Invalid volume access mode: %(access_mode)s") +class StaleVolumeMount(InvalidVolume): + msg_fmt = _("The volume mount at %(mount_path)s is unusable.") + + class InvalidMetadata(Invalid): msg_fmt = _("Invalid metadata: %(reason)s") diff --git a/nova/network/neutronv2/api.py b/nova/network/neutronv2/api.py index 8128ebf7a2..63c5ce0597 100644 --- a/nova/network/neutronv2/api.py +++ b/nova/network/neutronv2/api.py @@ -3253,45 +3253,14 @@ class API(base_api.NetworkAPI): # device_id field on the port which is not what we'd want for shelve. pass - def _get_pci_devices_from_migration_context(self, migration_context, - migration): - if migration and migration.get('status') == 'reverted': - # In case of revert, swap old and new devices to - # update the ports back to the original devices. - return (migration_context.new_pci_devices, - migration_context.old_pci_devices) - return (migration_context.old_pci_devices, - migration_context.new_pci_devices) - - def _get_pci_mapping_for_migration(self, context, instance, migration): - """Get the mapping between the old PCI devices and the new PCI - devices that have been allocated during this migration. The - correlation is based on PCI request ID which is unique per PCI - devices for SR-IOV ports. - - :param context: The request context. - :param instance: Get PCI mapping for this instance. - :param migration: The migration for this instance. - :Returns: dictionary of mapping {'<old pci address>': <New PciDevice>} - """ - migration_context = instance.migration_context - if not migration_context: + def _get_pci_mapping_for_migration(self, instance, migration): + if not instance.migration_context: return {} - - old_pci_devices, new_pci_devices = \ - self._get_pci_devices_from_migration_context(migration_context, - migration) - if old_pci_devices and new_pci_devices: - LOG.debug("Determining PCI devices mapping using migration " - "context: old_pci_devices: %(old)s, " - "new_pci_devices: %(new)s", - {'old': [dev for dev in old_pci_devices], - 'new': [dev for dev in new_pci_devices]}) - return {old.address: new - for old in old_pci_devices - for new in new_pci_devices - if old.request_id == new.request_id} - return {} + # In case of revert, swap old and new devices to + # update the ports back to the original devices. + revert = (migration and + migration.get('status') == 'reverted') + return instance.migration_context.get_pci_mapping_for_migration(revert) def _update_port_binding_for_instance(self, context, instance, host, migration=None): @@ -3337,7 +3306,7 @@ class API(base_api.NetworkAPI): if (vnic_type in network_model.VNIC_TYPES_SRIOV and migration is not None): if not pci_mapping: - pci_mapping = self._get_pci_mapping_for_migration(context, + pci_mapping = self._get_pci_mapping_for_migration( instance, migration) pci_slot = binding_profile.get('pci_slot') diff --git a/nova/objects/migration_context.py b/nova/objects/migration_context.py index cbc3adc1aa..3adf23f0f7 100644 --- a/nova/objects/migration_context.py +++ b/nova/objects/migration_context.py @@ -12,6 +12,7 @@ # License for the specific language governing permissions and limitations # under the License. +from oslo_log import log as logging from oslo_serialization import jsonutils from oslo_utils import versionutils @@ -20,6 +21,8 @@ from nova import exception from nova.objects import base from nova.objects import fields +LOG = logging.getLogger(__name__) + @base.NovaObjectRegistry.register class MigrationContext(base.NovaPersistentObject, base.NovaObject): @@ -80,3 +83,32 @@ class MigrationContext(base.NovaPersistentObject, base.NovaObject): return None return cls.obj_from_db_obj(db_extra['migration_context']) + + def get_pci_mapping_for_migration(self, revert): + """Get the mapping between the old PCI devices and the new PCI + devices that have been allocated during this migration. The + correlation is based on PCI request ID which is unique per PCI + devices for SR-IOV ports. + + :param revert: If True, return a reverse mapping i.e + mapping between new PCI devices and old PCI devices. + :returns: dictionary of PCI mapping. + if revert==False: + {'<old pci address>': <New PciDevice>} + if revert==True: + {'<new pci address>': <Old PciDevice>} + """ + step = -1 if revert else 1 + current_pci_devs, updated_pci_devs = (self.old_pci_devices, + self.new_pci_devices)[::step] + if current_pci_devs and updated_pci_devs: + LOG.debug("Determining PCI devices mapping using migration " + "context: current_pci_devs: %(cur)s, " + "updated_pci_devs: %(upd)s", + {'cur': [dev for dev in current_pci_devs], + 'upd': [dev for dev in updated_pci_devs]}) + return {curr_dev.address: upd_dev + for curr_dev in current_pci_devs + for upd_dev in updated_pci_devs + if curr_dev.request_id == upd_dev.request_id} + return {} diff --git a/nova/tests/unit/compute/test_compute.py b/nova/tests/unit/compute/test_compute.py index 94dcca95f9..000206e236 100644 --- a/nova/tests/unit/compute/test_compute.py +++ b/nova/tests/unit/compute/test_compute.py @@ -5814,6 +5814,8 @@ class ComputeTestCase(BaseTestCase, migration_context.migration_id = migration.id migration_context.old_numa_topology = old_inst_topology migration_context.new_numa_topology = new_inst_topology + migration_context.old_pci_devices = None + migration_context.new_pci_devices = None instance.migration_context = migration_context instance.vm_state = vm_states.RESIZED @@ -5850,12 +5852,14 @@ class ComputeTestCase(BaseTestCase, old_pci_devices = objects.PciDeviceList( objects=[objects.PciDevice(vendor_id='1377', product_id='0047', - address='0000:0a:00.1')]) + address='0000:0a:00.1', + request_id=uuids.req1)]) new_pci_devices = objects.PciDeviceList( objects=[objects.PciDevice(vendor_id='1377', product_id='0047', - address='0000:0b:00.1')]) + address='0000:0b:00.1', + request_id=uuids.req2)]) if expected_pci_addr == old_pci_devices[0].address: expected_pci_device = old_pci_devices[0] @@ -8270,7 +8274,10 @@ class ComputeTestCase(BaseTestCase, self.compute.confirm_resize(self.context, instance=instance, migration=migration) - def test_allow_confirm_resize_on_instance_in_deleting_task_state(self): + @mock.patch.object(objects.MigrationContext, + 'get_pci_mapping_for_migration') + def test_allow_confirm_resize_on_instance_in_deleting_task_state( + self, mock_pci_mapping): instance = self._create_fake_instance_obj() old_type = instance.flavor new_type = flavors.get_flavor_by_flavor_id('4') @@ -8278,6 +8285,7 @@ class ComputeTestCase(BaseTestCase, instance.flavor = new_type instance.old_flavor = old_type instance.new_flavor = new_type + instance.migration_context = objects.MigrationContext() def fake_drop_move_claim(*args, **kwargs): pass diff --git a/nova/tests/unit/compute/test_compute_mgr.py b/nova/tests/unit/compute/test_compute_mgr.py index b181bcbd34..3e6216bb95 100644 --- a/nova/tests/unit/compute/test_compute_mgr.py +++ b/nova/tests/unit/compute/test_compute_mgr.py @@ -7294,7 +7294,9 @@ class ComputeManagerMigrationTestCase(test.NoDBTestCase, mock_confirm, mock_nwapi, mock_notify, mock_mig_save, mock_mig_get, mock_inst_get): self._mock_rt() - self.instance.migration_context = objects.MigrationContext() + self.instance.migration_context = objects.MigrationContext( + new_pci_devices=None, + old_pci_devices=None) self.migration.source_compute = self.instance['host'] self.migration.source_node = self.instance['node'] self.migration.status = 'confirming' @@ -7310,6 +7312,7 @@ class ComputeManagerMigrationTestCase(test.NoDBTestCase, do_confirm_resize() + @mock.patch('nova.objects.MigrationContext.get_pci_mapping_for_migration') @mock.patch('nova.compute.utils.add_instance_fault_from_exc') @mock.patch('nova.objects.Migration.get_by_id') @mock.patch('nova.objects.Instance.get_by_uuid') @@ -7318,7 +7321,7 @@ class ComputeManagerMigrationTestCase(test.NoDBTestCase, @mock.patch('nova.objects.Instance.save') def test_confirm_resize_driver_confirm_migration_fails( self, instance_save, notify_action, notify_usage, - instance_get_by_uuid, migration_get_by_id, add_fault): + instance_get_by_uuid, migration_get_by_id, add_fault, get_mapping): """Tests the scenario that driver.confirm_migration raises some error to make sure the error is properly handled, like the instance and migration status is set to 'error'. @@ -7326,6 +7329,7 @@ class ComputeManagerMigrationTestCase(test.NoDBTestCase, self.migration.status = 'confirming' migration_get_by_id.return_value = self.migration instance_get_by_uuid.return_value = self.instance + self.instance.migration_context = objects.MigrationContext() error = exception.HypervisorUnavailable( host=self.migration.source_compute) @@ -7333,9 +7337,11 @@ class ComputeManagerMigrationTestCase(test.NoDBTestCase, mock.patch.object(self.compute, 'network_api'), mock.patch.object(self.compute.driver, 'confirm_migration', side_effect=error), - mock.patch.object(self.compute, '_delete_allocation_after_move') + mock.patch.object(self.compute, '_delete_allocation_after_move'), + mock.patch.object(self.compute, + '_get_updated_nw_info_with_pci_mapping') ) as ( - network_api, confirm_migration, delete_allocation + network_api, confirm_migration, delete_allocation, pci_mapping ): self.assertRaises(exception.HypervisorUnavailable, self.compute.confirm_resize, @@ -7362,6 +7368,59 @@ class ComputeManagerMigrationTestCase(test.NoDBTestCase, instance_get_by_uuid.assert_called_once() migration_get_by_id.assert_called_once() + def test_confirm_resize_calls_virt_driver_with_old_pci(self): + @mock.patch.object(self.migration, 'save') + @mock.patch.object(self.compute, '_notify_about_instance_usage') + @mock.patch.object(self.compute, 'network_api') + @mock.patch.object(self.compute.driver, 'confirm_migration') + @mock.patch.object(self.compute, '_delete_allocation_after_move') + @mock.patch.object(self.instance, 'drop_migration_context') + @mock.patch.object(self.instance, 'save') + def do_confirm_resize(mock_save, mock_drop, mock_delete, + mock_confirm, mock_nwapi, mock_notify, + mock_mig_save): + # Mock virt driver confirm_resize() to save the provided + # network_info, we will check it later. + updated_nw_info = [] + + def driver_confirm_resize(*args, **kwargs): + if 'network_info' in kwargs: + nw_info = kwargs['network_info'] + else: + nw_info = args[3] + updated_nw_info.extend(nw_info) + + mock_confirm.side_effect = driver_confirm_resize + self._mock_rt() + old_devs = objects.PciDeviceList( + objects=[objects.PciDevice( + address='0000:04:00.2', + request_id=uuids.pcidev1)]) + new_devs = objects.PciDeviceList( + objects=[objects.PciDevice( + address='0000:05:00.3', + request_id=uuids.pcidev1)]) + self.instance.migration_context = objects.MigrationContext( + new_pci_devices=new_devs, + old_pci_devices=old_devs) + # Create VIF with new_devs[0] PCI address. + nw_info = network_model.NetworkInfo([ + network_model.VIF( + id=uuids.port1, + vnic_type=network_model.VNIC_TYPE_DIRECT, + profile={'pci_slot': new_devs[0].address})]) + mock_nwapi.get_instance_nw_info.return_value = nw_info + self.migration.source_compute = self.instance['host'] + self.migration.source_node = self.instance['node'] + self.compute._confirm_resize(self.context, self.instance, + self.migration) + # Assert virt driver confirm_migration() was called + # with the updated nw_info object. + self.assertEqual(old_devs[0].address, + updated_nw_info[0]['profile']['pci_slot']) + + do_confirm_resize() + def test_delete_allocation_after_move_confirm_by_migration(self): with mock.patch.object(self.compute, 'reportclient') as mock_report: mock_report.delete_allocation_for_instance.return_value = True @@ -8757,6 +8816,24 @@ class ComputeManagerMigrationTestCase(test.NoDBTestCase, doit() + def test_get_updated_nw_info_with_pci_mapping(self): + old_dev = objects.PciDevice(address='0000:04:00.2') + new_dev = objects.PciDevice(address='0000:05:00.3') + pci_mapping = {old_dev.address: new_dev} + nw_info = network_model.NetworkInfo([ + network_model.VIF( + id=uuids.port1, + vnic_type=network_model.VNIC_TYPE_NORMAL), + network_model.VIF( + id=uuids.port2, + vnic_type=network_model.VNIC_TYPE_DIRECT, + profile={'pci_slot': old_dev.address})]) + updated_nw_info = self.compute._get_updated_nw_info_with_pci_mapping( + nw_info, pci_mapping) + self.assertDictEqual(nw_info[0], updated_nw_info[0]) + self.assertEqual(new_dev.address, + updated_nw_info[1]['profile']['pci_slot']) + class ComputeManagerInstanceUsageAuditTestCase(test.TestCase): def setUp(self): diff --git a/nova/tests/unit/network/test_neutronv2.py b/nova/tests/unit/network/test_neutronv2.py index aaf80ba0a0..6ccf8deac4 100644 --- a/nova/tests/unit/network/test_neutronv2.py +++ b/nova/tests/unit/network/test_neutronv2.py @@ -4681,56 +4681,29 @@ class TestNeutronv2WithMock(_TestNeutronv2Common): def test_get_pci_mapping_for_migration(self): instance = fake_instance.fake_instance_obj(self.context) instance.migration_context = objects.MigrationContext() - old_pci_devices = objects.PciDeviceList( - objects=[objects.PciDevice(vendor_id='1377', - product_id='0047', - address='0000:0a:00.1', - compute_node_id=1, - request_id='1234567890')]) - - new_pci_devices = objects.PciDeviceList( - objects=[objects.PciDevice(vendor_id='1377', - product_id='0047', - address='0000:0b:00.1', - compute_node_id=2, - request_id='1234567890')]) - - instance.migration_context.old_pci_devices = old_pci_devices - instance.migration_context.new_pci_devices = new_pci_devices - instance.pci_devices = instance.migration_context.old_pci_devices migration = {'status': 'confirmed'} - pci_mapping = self.api._get_pci_mapping_for_migration( - self.context, instance, migration) - self.assertEqual( - {old_pci_devices[0].address: new_pci_devices[0]}, pci_mapping) + with mock.patch.object(instance.migration_context, + 'get_pci_mapping_for_migration') as map_func: + self.api._get_pci_mapping_for_migration(instance, migration) + map_func.assert_called_with(False) def test_get_pci_mapping_for_migration_reverted(self): instance = fake_instance.fake_instance_obj(self.context) instance.migration_context = objects.MigrationContext() - old_pci_devices = objects.PciDeviceList( - objects=[objects.PciDevice(vendor_id='1377', - product_id='0047', - address='0000:0a:00.1', - compute_node_id=1, - request_id='1234567890')]) - - new_pci_devices = objects.PciDeviceList( - objects=[objects.PciDevice(vendor_id='1377', - product_id='0047', - address='0000:0b:00.1', - compute_node_id=2, - request_id='1234567890')]) - - instance.migration_context.old_pci_devices = old_pci_devices - instance.migration_context.new_pci_devices = new_pci_devices - instance.pci_devices = instance.migration_context.old_pci_devices migration = {'status': 'reverted'} + with mock.patch.object(instance.migration_context, + 'get_pci_mapping_for_migration') as map_func: + self.api._get_pci_mapping_for_migration(instance, migration) + map_func.assert_called_with(True) + + def test_get_pci_mapping_for_migration_no_migration_context(self): + instance = fake_instance.fake_instance_obj(self.context) + instance.migration_context = None pci_mapping = self.api._get_pci_mapping_for_migration( - self.context, instance, migration) - self.assertEqual( - {new_pci_devices[0].address: old_pci_devices[0]}, pci_mapping) + instance, None) + self.assertDictEqual({}, pci_mapping) @mock.patch.object(neutronapi, 'get_client', return_value=mock.Mock()) def test_update_port_profile_for_migration_teardown_false( diff --git a/nova/tests/unit/objects/test_migration_context.py b/nova/tests/unit/objects/test_migration_context.py index 03200bc83f..42962bf354 100644 --- a/nova/tests/unit/objects/test_migration_context.py +++ b/nova/tests/unit/objects/test_migration_context.py @@ -52,6 +52,23 @@ def get_fake_migration_context_obj(ctxt): return obj +def get_fake_migration_context_with_pci_devs(ctxt=None): + obj = get_fake_migration_context_obj(ctxt) + obj.old_pci_devices = objects.PciDeviceList( + objects=[objects.PciDevice(vendor_id='1377', + product_id='0047', + address='0000:0a:00.1', + compute_node_id=1, + request_id=uuids.pcidev)]) + obj.new_pci_devices = objects.PciDeviceList( + objects=[objects.PciDevice(vendor_id='1377', + product_id='0047', + address='0000:0b:00.1', + compute_node_id=2, + request_id=uuids.pcidev)]) + return obj + + class _TestMigrationContext(object): def _test_get_by_instance_uuid(self, db_data): @@ -104,7 +121,20 @@ class _TestMigrationContext(object): class TestMigrationContext(test_objects._LocalTest, _TestMigrationContext): - pass + + def test_pci_mapping_for_migration(self): + mig_ctx = get_fake_migration_context_with_pci_devs() + pci_mapping = mig_ctx.get_pci_mapping_for_migration(False) + self.assertDictEqual( + {mig_ctx.old_pci_devices[0].address: mig_ctx.new_pci_devices[0]}, + pci_mapping) + + def test_pci_mapping_for_migration_revert(self): + mig_ctx = get_fake_migration_context_with_pci_devs() + pci_mapping = mig_ctx.get_pci_mapping_for_migration(True) + self.assertDictEqual( + {mig_ctx.new_pci_devices[0].address: mig_ctx.old_pci_devices[0]}, + pci_mapping) class TestMigrationContextRemote(test_objects._RemoteTest, diff --git a/nova/tests/unit/virt/ironic/test_driver.py b/nova/tests/unit/virt/ironic/test_driver.py index 6a422a71c0..c3f4692c72 100644 --- a/nova/tests/unit/virt/ironic/test_driver.py +++ b/nova/tests/unit/virt/ironic/test_driver.py @@ -1666,7 +1666,8 @@ class IronicDriverTestCase(test.NoDBTestCase): mock_node.get_by_instance_uuid.assert_called_with( instance.uuid, fields=ironic_driver._NODE_FIELDS) - mock_cleanup_deploy.assert_called_with(node, instance, network_info) + mock_cleanup_deploy.assert_called_with(node, instance, network_info, + remove_instance_info=False) # For states that makes sense check if set_provision_state has # been called @@ -1699,7 +1700,8 @@ class IronicDriverTestCase(test.NoDBTestCase): mock_sps.side_effect = exception.NovaException() self.assertRaises(exception.NovaException, self.driver.destroy, self.ctx, instance, None, None) - mock_clean.assert_called_once_with(node, instance, None) + mock_clean.assert_called_once_with(node, instance, None, + remove_instance_info=False) @mock.patch.object(FAKE_CLIENT.node, 'update') @mock.patch.object(ironic_driver.IronicDriver, @@ -1718,7 +1720,8 @@ class IronicDriverTestCase(test.NoDBTestCase): mock_update.side_effect = SystemError('unexpected error') self.assertRaises(SystemError, self.driver.destroy, self.ctx, instance, None, None) - mock_clean.assert_called_once_with(node, instance, None) + mock_clean.assert_called_once_with(node, instance, None, + remove_instance_info=False) @mock.patch.object(FAKE_CLIENT.node, 'set_provision_state') @mock.patch.object(ironic_driver.IronicDriver, @@ -2549,6 +2552,24 @@ class IronicDriverTestCase(test.NoDBTestCase): mock_call.has_calls( [mock.call('node.update', node.uuid, expected_patch)]) + @mock.patch.object(ironic_driver.IronicDriver, '_stop_firewall') + @mock.patch.object(ironic_driver.IronicDriver, '_unplug_vifs') + @mock.patch.object(ironic_driver.IronicDriver, + '_cleanup_volume_target_info') + @mock.patch.object(cw.IronicClientWrapper, 'call') + def test__cleanup_deploy_no_remove_ii(self, mock_call, mock_vol, + mock_unvif, mock_stop_fw): + # TODO(TheJulia): This REALLY should be updated to cover all of the + # calls that take place. + node = ironic_utils.get_test_node(driver='fake') + instance = fake_instance.fake_instance_obj(self.ctx, + node=node.uuid) + self.driver._cleanup_deploy(node, instance, remove_instance_info=False) + mock_vol.assert_called_once_with(instance) + mock_unvif.assert_called_once_with(node, instance, None) + mock_stop_fw.assert_called_once_with(instance, None) + self.assertFalse(mock_call.called) + class IronicDriverSyncTestCase(IronicDriverTestCase): diff --git a/nova/tests/unit/virt/libvirt/volume/test_quobyte.py b/nova/tests/unit/virt/libvirt/volume/test_quobyte.py index e84b9472c3..61b5dad86b 100644 --- a/nova/tests/unit/virt/libvirt/volume/test_quobyte.py +++ b/nova/tests/unit/virt/libvirt/volume/test_quobyte.py @@ -293,13 +293,15 @@ class LibvirtQuobyteVolumeDriverTestCase( test_volume.LibvirtVolumeBaseTestCase): """Tests the LibvirtQuobyteVolumeDriver class.""" + @mock.patch.object(quobyte, 'umount_volume') @mock.patch.object(quobyte, 'validate_volume') @mock.patch.object(quobyte, 'mount_volume') @mock.patch.object(libvirt_utils, 'is_mounted', return_value=False) def test_libvirt_quobyte_driver_mount(self, mock_is_mounted, mock_mount_volume, - mock_validate_volume + mock_validate_volume, + mock_umount_volume ): mnt_base = '/mnt' self.flags(quobyte_mount_point_base=mnt_base, group='libvirt') @@ -313,6 +315,9 @@ class LibvirtQuobyteVolumeDriverTestCase( connection_info = {'data': {'export': export_string, 'name': self.name}} + mock_validate_volume.side_effect = [nova_exception.StaleVolumeMount( + "This shall fail."), True, True] + libvirt_driver.connect_volume(connection_info, mock.sentinel.instance) conf = libvirt_driver.get_config(connection_info, self.disk_info) @@ -324,12 +329,12 @@ class LibvirtQuobyteVolumeDriverTestCase( export_mnt_base, mock.ANY) mock_validate_volume.assert_called_with(export_mnt_base) + mock_umount_volume.assert_called_once_with( + libvirt_driver._get_mount_path(connection_info)) - @mock.patch.object(quobyte, 'validate_volume') + @mock.patch.object(quobyte, 'validate_volume', return_value=True) @mock.patch.object(quobyte, 'umount_volume') - @mock.patch.object(libvirt_utils, 'is_mounted', return_value=True) - def test_libvirt_quobyte_driver_umount(self, mock_is_mounted, - mock_umount_volume, + def test_libvirt_quobyte_driver_umount(self, mock_umount_volume, mock_validate_volume): mnt_base = '/mnt' self.flags(quobyte_mount_point_base=mnt_base, group='libvirt') @@ -352,7 +357,9 @@ class LibvirtQuobyteVolumeDriverTestCase( libvirt_driver.disconnect_volume(connection_info, mock.sentinel.instance) - mock_validate_volume.assert_called_once_with(export_mnt_base) + mock_validate_volume.assert_has_calls([mock.call(export_mnt_base), + mock.call(export_mnt_base), + mock.call(export_mnt_base)]) mock_umount_volume.assert_called_once_with(export_mnt_base) @mock.patch.object(quobyte, 'validate_volume') @@ -385,15 +392,14 @@ class LibvirtQuobyteVolumeDriverTestCase( mock.sentinel.instance) mock_umount_volume.assert_called_once_with(export_mnt_base) - mock_validate_volume.assert_called_once_with(export_mnt_base) + mock_validate_volume.assert_has_calls([mock.call(export_mnt_base), + mock.call(export_mnt_base)]) + @mock.patch.object(quobyte, 'umount_volume') @mock.patch.object(quobyte, 'validate_volume') @mock.patch.object(quobyte, 'mount_volume') - @mock.patch.object(libvirt_utils, 'is_mounted', return_value=False) - def test_libvirt_quobyte_driver_qcow2(self, mock_is_mounted, - mock_mount_volume, - mock_validate_volume - ): + def test_libvirt_quobyte_driver_qcow2(self, mock_mount_volume, + mock_validate_volume, mock_umount): mnt_base = '/mnt' self.flags(quobyte_mount_point_base=mnt_base, group='libvirt') libvirt_driver = quobyte.LibvirtQuobyteVolumeDriver(self.fake_host) @@ -408,6 +414,8 @@ class LibvirtQuobyteVolumeDriverTestCase( export_mnt_base = os.path.join(mnt_base, utils.get_hash_str(quobyte_volume)) + mock_validate_volume.side_effect = [nova_exception.StaleVolumeMount( + "This shall fail."), True, True] libvirt_driver.connect_volume(connection_info, mock.sentinel.instance) conf = libvirt_driver.get_config(connection_info, self.disk_info) @@ -423,6 +431,8 @@ class LibvirtQuobyteVolumeDriverTestCase( libvirt_driver.disconnect_volume(connection_info, mock.sentinel.instance) + mock_umount.assert_has_calls([mock.call(export_mnt_base), + mock.call(export_mnt_base)]) @mock.patch.object(libvirt_utils, 'is_mounted', return_value=True) def test_libvirt_quobyte_driver_mount_non_quobyte_volume(self, diff --git a/nova/tests/unit/virt/xenapi/test_agent.py b/nova/tests/unit/virt/xenapi/test_agent.py index 8c77445c22..2848fc2882 100644 --- a/nova/tests/unit/virt/xenapi/test_agent.py +++ b/nova/tests/unit/virt/xenapi/test_agent.py @@ -19,6 +19,7 @@ import time import mock from os_xenapi.client import host_agent from os_xenapi.client import XenAPI +from oslo_concurrency import processutils from oslo_utils import uuidutils from nova import exception @@ -311,6 +312,19 @@ class SetAdminPasswordTestCase(AgentTestCaseBase): mock_add_fault.assert_called_once_with(error, mock.ANY) + @mock.patch('oslo_concurrency.processutils.execute') + def test_run_ssl_successful(self, mock_execute): + mock_execute.return_value = ('0', + '*** WARNING : deprecated key derivation used.' + 'Using -iter or -pbkdf2 would be better.') + agent.SimpleDH()._run_ssl('foo') + + @mock.patch('oslo_concurrency.processutils.execute', + side_effect=processutils.ProcessExecutionError( + exit_code=1, stderr=('ERROR: Something bad happened'))) + def test_run_ssl_failure(self, mock_execute): + self.assertRaises(RuntimeError, agent.SimpleDH()._run_ssl, 'foo') + class UpgradeRequiredTestCase(test.NoDBTestCase): def test_less_than(self): diff --git a/nova/virt/ironic/driver.py b/nova/virt/ironic/driver.py index e3899093c5..12c08cce03 100644 --- a/nova/virt/ironic/driver.py +++ b/nova/virt/ironic/driver.py @@ -474,11 +474,13 @@ class IronicDriver(virt_driver.ComputeDriver): 'reason': e}, instance=instance) - def _cleanup_deploy(self, node, instance, network_info=None): + def _cleanup_deploy(self, node, instance, network_info=None, + remove_instance_info=True): self._cleanup_volume_target_info(instance) self._unplug_vifs(node, instance, network_info) self._stop_firewall(instance, network_info) - self._remove_instance_info_from_node(node, instance) + if remove_instance_info: + self._remove_instance_info_from_node(node, instance) def _wait_for_active(self, instance): """Wait for the node to be marked as ACTIVE in Ironic.""" @@ -1324,7 +1326,12 @@ class IronicDriver(virt_driver.ComputeDriver): # removed from ironic node. self._remove_instance_info_from_node(node, instance) finally: - self._cleanup_deploy(node, instance, network_info) + # NOTE(mgoddard): We don't need to remove instance info at this + # point since we will have already done it. The destroy will only + # succeed if this method returns without error, so we will end up + # removing the instance info eventually. + self._cleanup_deploy(node, instance, network_info, + remove_instance_info=False) LOG.info('Successfully unprovisioned Ironic node %s', node.uuid, instance=instance) diff --git a/nova/virt/libvirt/volume/quobyte.py b/nova/virt/libvirt/volume/quobyte.py index e0885931cd..94ea0c8010 100644 --- a/nova/virt/libvirt/volume/quobyte.py +++ b/nova/virt/libvirt/volume/quobyte.py @@ -13,7 +13,6 @@ # License for the specific language governing permissions and limitations # under the License. -import errno import os from oslo_concurrency import processutils @@ -26,7 +25,6 @@ import nova.conf from nova import exception as nova_exception from nova.i18n import _ from nova import utils -from nova.virt.libvirt import utils as libvirt_utils from nova.virt.libvirt.volume import fs LOG = logging.getLogger(__name__) @@ -75,7 +73,10 @@ def umount_volume(mnt_base): def validate_volume(mount_path): - """Runs a number of tests to be sure this is a (working) Quobyte mount""" + """Determine if the volume is a valid Quobyte mount. + + Runs a number of tests to be sure this is a (working) Quobyte mount + """ partitions = psutil.disk_partitions(all=True) for p in partitions: if mount_path != p.mountpoint: @@ -90,10 +91,10 @@ def validate_volume(mount_path): msg = (_("The mount %(mount_path)s is not a " "valid Quobyte volume. Stale mount?") % {'mount_path': mount_path}) - raise nova_exception.InvalidVolume(msg) + raise nova_exception.StaleVolumeMount(msg, mount_path=mount_path) else: - msg = (_("The mount %(mount_path)s is not a valid" - " Quobyte volume according to partition list.") + msg = (_("The mount %(mount_path)s is not a valid " + "Quobyte volume according to partition list.") % {'mount_path': mount_path}) raise nova_exception.InvalidVolume(msg) msg = (_("No matching Quobyte mount entry for %(mount_path)s" @@ -128,39 +129,40 @@ class LibvirtQuobyteVolumeDriver(fs.LibvirtBaseFileSystemVolumeDriver): data = connection_info['data'] quobyte_volume = self._normalize_export(data['export']) mount_path = self._get_mount_path(connection_info) - mounted = libvirt_utils.is_mounted(mount_path, - SOURCE_PROTOCOL - + '@' + quobyte_volume) - if mounted: - try: - os.stat(mount_path) - except OSError as exc: - if exc.errno == errno.ENOTCONN: - mounted = False - LOG.info('Fixing previous mount %s which was not' - ' unmounted correctly.', mount_path) - umount_volume(mount_path) + try: + validate_volume(mount_path) + mounted = True + except nova_exception.StaleVolumeMount: + mounted = False + LOG.info('Fixing previous mount %s which was not ' + 'unmounted correctly.', mount_path) + umount_volume(mount_path) + except nova_exception.InvalidVolume: + mounted = False if not mounted: mount_volume(quobyte_volume, mount_path, CONF.libvirt.quobyte_client_cfg) - validate_volume(mount_path) + try: + validate_volume(mount_path) + except (nova_exception.InvalidVolume, + nova_exception.StaleVolumeMount) as nex: + LOG.error("Could not mount Quobyte volume: %s", nex) @utils.synchronized('connect_qb_volume') def disconnect_volume(self, connection_info, instance): """Disconnect the volume.""" - quobyte_volume = self._normalize_export( - connection_info['data']['export']) mount_path = self._get_mount_path(connection_info) - - if libvirt_utils.is_mounted(mount_path, 'quobyte@' + quobyte_volume): - umount_volume(mount_path) + try: + validate_volume(mount_path) + except (nova_exception.InvalidVolume, + nova_exception.StaleVolumeMount) as exc: + LOG.warning("Could not disconnect Quobyte volume mount: %s", exc) else: - LOG.info("Trying to disconnected unmounted volume at %s", - mount_path) + umount_volume(mount_path) def _normalize_export(self, export): protocol = SOURCE_PROTOCOL + "://" diff --git a/nova/virt/xenapi/agent.py b/nova/virt/xenapi/agent.py index d5f060af5d..e763cc2cfd 100644 --- a/nova/virt/xenapi/agent.py +++ b/nova/virt/xenapi/agent.py @@ -422,11 +422,18 @@ class SimpleDH(object): 'pass:%s' % self._shared, '-nosalt'] if decrypt: cmd.append('-d') - out, err = processutils.execute( - *cmd, process_input=encodeutils.safe_encode(text)) - if err: - raise RuntimeError(_('OpenSSL error: %s') % err) - return out + try: + out, err = processutils.execute( + *cmd, + process_input=encodeutils.safe_encode(text), + check_exit_code=True) + if err: + LOG.warning("OpenSSL stderr: %s", err) + return out + except processutils.ProcessExecutionError as e: + raise RuntimeError( + _('OpenSSL errored with exit code %(exit_code)d: %(stderr)s') % + {'exit_code': e.exit_code, 'stderr': e.stderr}) def encrypt(self, text): return self._run_ssl(text).strip('\n') diff --git a/releasenotes/notes/qb-bug-1730933-6695470ebaee0fbd.yaml b/releasenotes/notes/qb-bug-1730933-6695470ebaee0fbd.yaml new file mode 100644 index 0000000000..f1e98d7a04 --- /dev/null +++ b/releasenotes/notes/qb-bug-1730933-6695470ebaee0fbd.yaml @@ -0,0 +1,5 @@ +--- +fixes: + - | + Fixes a bug that caused Nova to fail on mounting Quobyte volumes + whose volume URL contained multiple registries. |