diff --git a/nova/compute/manager.py b/nova/compute/manager.py index 27e76963ef7..a51558cea84 100644 --- a/nova/compute/manager.py +++ b/nova/compute/manager.py @@ -9896,6 +9896,8 @@ def update_available_resource(self, context, startup=False): use_slave=True, startup=startup) + self.rt.clean_compute_node_cache(compute_nodes_in_db) + # Delete orphan compute node not reported by driver but still in db for cn in compute_nodes_in_db: if cn.hypervisor_hostname not in nodenames: @@ -9904,17 +9906,31 @@ def update_available_resource(self, context, startup=False): "nodes are %(nodes)s", {'id': cn.id, 'hh': cn.hypervisor_hostname, 'nodes': nodenames}) - cn.destroy() - self.rt.remove_node(cn.hypervisor_hostname) - # Delete the corresponding resource provider in placement, - # along with any associated allocations. try: - self.reportclient.delete_resource_provider(context, cn, - cascade=True) - except keystone_exception.ClientException as e: - LOG.error( - "Failed to delete compute node resource provider " - "for compute node %s: %s", cn.uuid, six.text_type(e)) + cn.destroy() + except exception.ObjectActionError: + # NOTE(mgoddard): it's possible that another compute + # service took ownership of this compute node since we + # queried it due to a rebalance, and this will cause the + # deletion to fail. Ignore the error in that case. + LOG.info("Ignoring failure to delete orphan compute node " + "%(id)s on hypervisor host %(hh)s due to " + "possible node rebalance", + {'id': cn.id, 'hh': cn.hypervisor_hostname}) + self.rt.remove_node(cn.hypervisor_hostname) + self.reportclient.invalidate_resource_provider(cn.uuid) + else: + self.rt.remove_node(cn.hypervisor_hostname) + # Delete the corresponding resource provider in placement, + # along with any associated allocations. + try: + self.reportclient.delete_resource_provider( + context, cn, cascade=True) + except keystone_exception.ClientException as e: + LOG.error( + "Failed to delete compute node resource provider " + "for compute node %s: %s", cn.uuid, + six.text_type(e)) for nodename in nodenames: self._update_available_resource_for_node(context, nodename, diff --git a/nova/compute/resource_tracker.py b/nova/compute/resource_tracker.py index c12ada21e7a..a351a21afa6 100644 --- a/nova/compute/resource_tracker.py +++ b/nova/compute/resource_tracker.py @@ -1831,3 +1831,21 @@ def finish_evacuation(self, instance, node, migration): if migration: migration.status = 'done' migration.save() + + @utils.synchronized(COMPUTE_RESOURCE_SEMAPHORE, fair=True) + def clean_compute_node_cache(self, compute_nodes_in_db): + """Clean the compute node cache of any nodes that no longer exist. + + :param compute_nodes_in_db: list of ComputeNode objects from the DB. + """ + compute_nodes_in_db_nodenames = {cn.hypervisor_hostname + for cn in compute_nodes_in_db} + stale_cns = set(self.compute_nodes) - compute_nodes_in_db_nodenames + + for stale_cn in stale_cns: + # NOTE(mgoddard): we have found a node in the cache that has no + # compute node in the DB. This could be due to a node rebalance + # where another compute service took ownership of the node. Clean + # up the cache. + self.remove_node(stale_cn) + self.reportclient.invalidate_resource_provider(stale_cn) diff --git a/nova/conf/compute.py b/nova/conf/compute.py index 8faddc721c8..c012e2d0815 100644 --- a/nova/conf/compute.py +++ b/nova/conf/compute.py @@ -959,6 +959,15 @@ * Any integer >= 1 represents the maximum allowed. A value of 0 will cause the ``nova-compute`` service to fail to start, as 0 disk devices is an invalid configuration that would prevent instances from being able to boot. +"""), + cfg.ListOpt('vmdk_allowed_types', + default=['streamOptimized', 'monolithicSparse'], + help=""" +A list of strings describing allowed VMDK "create-type" subformats +that will be allowed. This is recommended to only include +single-file-with-sparse-header variants to avoid potential host file +exposure due to processing named extents. If this list is empty, then no +form of VMDK image will be allowed. """), ] diff --git a/nova/conf/workarounds.py b/nova/conf/workarounds.py index 8eadc0b6ec3..ee2abad4d09 100644 --- a/nova/conf/workarounds.py +++ b/nova/conf/workarounds.py @@ -345,6 +345,14 @@ * :oslo.config:option:`DEFAULT.instances_path` * :oslo.config:option:`image_cache.subdirectory_name` * :oslo.config:option:`update_resources_interval` +"""), + cfg.BoolOpt('skip_cpu_compare_on_dest', + default=False, + help=""" +With the libvirt driver, during live migration, skip comparing guest CPU +with the destination host. When using QEMU >= 2.9 and libvirt >= +4.4.0, libvirt will do the correct thing with respect to checking CPU +compatibility on the destination host during live migration. """), ] diff --git a/nova/db/sqlalchemy/api.py b/nova/db/sqlalchemy/api.py index 1fa7f5a322b..81b9b92a186 100644 --- a/nova/db/sqlalchemy/api.py +++ b/nova/db/sqlalchemy/api.py @@ -690,7 +690,7 @@ def compute_node_search_by_hypervisor(context, hypervisor_match): @pick_context_manager_writer -def compute_node_create(context, values): +def _compute_node_create(context, values): """Creates a new ComputeNode and populates the capacity fields with the most recent data. """ @@ -698,8 +698,21 @@ def compute_node_create(context, values): compute_node_ref = models.ComputeNode() compute_node_ref.update(values) + compute_node_ref.save(context.session) + return compute_node_ref + + +# NOTE(mgoddard): We avoid decorating this with @pick_context_manager_writer, +# so that we get a separate transaction in the exception handler. This avoids +# an error message about inactive DB sessions during a transaction rollback. +# See https://bugs.launchpad.net/nova/+bug/1853159. +def compute_node_create(context, values): + """Creates a new ComputeNode and populates the capacity fields + with the most recent data. Will restore a soft deleted compute node if a + UUID has been explicitly requested. + """ try: - compute_node_ref.save(context.session) + compute_node_ref = _compute_node_create(context, values) except db_exc.DBDuplicateEntry: with excutils.save_and_reraise_exception(logger=LOG) as err_ctx: # Check to see if we have a (soft) deleted ComputeNode with the @@ -763,14 +776,24 @@ def compute_node_update(context, compute_id, values): @pick_context_manager_writer -def compute_node_delete(context, compute_id): +def compute_node_delete(context, compute_id, constraint=None): """Delete a ComputeNode record.""" - result = model_query(context, models.ComputeNode).\ - filter_by(id=compute_id).\ - soft_delete(synchronize_session=False) + query = model_query(context, models.ComputeNode).filter_by(id=compute_id) + + if constraint is not None: + query = constraint.apply(models.ComputeNode, query) + + result = query.soft_delete(synchronize_session=False) if not result: - raise exception.ComputeHostNotFound(host=compute_id) + # The soft_delete could fail for one of two reasons: + # 1) The compute node no longer exists + # 2) The constraint, if specified, was not met + # Try to read the compute node and let it raise ComputeHostNotFound if + # 1) happened. + compute_node_get(context, compute_id) + # Else, raise ConstraintNotMet if 2) happened. + raise exception.ConstraintNotMet() @pick_context_manager_reader diff --git a/nova/objects/compute_node.py b/nova/objects/compute_node.py index b7f7f966b9d..33782bc4419 100644 --- a/nova/objects/compute_node.py +++ b/nova/objects/compute_node.py @@ -354,7 +354,20 @@ def save(self, prune_stats=False): @base.remotable def destroy(self): - db.compute_node_delete(self._context, self.id) + if self.obj_attr_is_set('host') and self.host: + # NOTE(melwitt): If our host is set, avoid a race between + # nova-computes during ironic driver node rebalances which can + # change node ownership. + constraint = db.constraint(host=db.equal_any(self.host)) + else: + constraint = None + + try: + sa_api.compute_node_delete( + self._context, self.id, constraint=constraint) + except exception.ConstraintNotMet: + raise exception.ObjectActionError(action='destroy', + reason='host changed') def update_from_virt_driver(self, resources): # NOTE(pmurray): the virt driver provides a dict of values that diff --git a/nova/scheduler/client/report.py b/nova/scheduler/client/report.py index 350825af146..8faae5b65ab 100644 --- a/nova/scheduler/client/report.py +++ b/nova/scheduler/client/report.py @@ -676,11 +676,7 @@ def _delete_provider(self, rp_uuid, global_request_id=None): if resp: LOG.info("Deleted resource provider %s", rp_uuid) # clean the caches - try: - self._provider_tree.remove(rp_uuid) - except ValueError: - pass - self._association_refresh_time.pop(rp_uuid, None) + self.invalidate_resource_provider(rp_uuid) return msg = ("[%(placement_req_id)s] Failed to delete resource provider " @@ -2161,6 +2157,17 @@ def delete_resource_provider(self, context, compute_node, cascade=False): # for backward compatibility. pass + def invalidate_resource_provider(self, name_or_uuid): + """Invalidate the cache for a resource provider. + + :param name_or_uuid: Name or UUID of the resource provider to look up. + """ + try: + self._provider_tree.remove(name_or_uuid) + except ValueError: + pass + self._association_refresh_time.pop(name_or_uuid, None) + def get_provider_by_name(self, context, name): """Queries the placement API for resource provider information matching a supplied name. diff --git a/nova/tests/functional/regressions/test_bug_1853009.py b/nova/tests/functional/regressions/test_bug_1853009.py new file mode 100644 index 00000000000..2ec69482a25 --- /dev/null +++ b/nova/tests/functional/regressions/test_bug_1853009.py @@ -0,0 +1,171 @@ +# Licensed under the Apache License, Version 2.0 (the "License"); you may +# not use this file except in compliance with the License. You may obtain +# a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT +# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the +# License for the specific language governing permissions and limitations +# under the License. + +import mock + +from nova import context +from nova import objects +from nova.tests.functional import integrated_helpers + + +class NodeRebalanceDeletedComputeNodeRaceTestCase( + integrated_helpers.ProviderUsageBaseTestCase, +): + """Regression test for bug 1853009 observed in Rocky & later. + + When an ironic node re-balances from one host to another, there can be a + race where the old host deletes the orphan compute node after the new host + has taken ownership of it which results in the new host failing to create + the compute node and resource provider because the ResourceTracker does not + detect a change. + """ + # Make sure we're using the fake driver that has predictable uuids + # for each node. + compute_driver = 'fake.PredictableNodeUUIDDriver' + + def _assert_hypervisor_api(self, nodename, expected_host): + # We should have one compute node shown by the API. + hypervisors = self.api.api_get( + '/os-hypervisors/detail' + ).body['hypervisors'] + self.assertEqual(1, len(hypervisors), hypervisors) + hypervisor = hypervisors[0] + self.assertEqual(nodename, hypervisor['hypervisor_hostname']) + self.assertEqual(expected_host, hypervisor['service']['host']) + + def _start_compute(self, host): + host = self.start_service('compute', host) + # Ironic compute driver has rebalances_nodes = True. + host.manager.driver.rebalances_nodes = True + return host + + def setUp(self): + super(NodeRebalanceDeletedComputeNodeRaceTestCase, self).setUp() + + self.nodename = 'fake-node' + self.ctxt = context.get_admin_context() + + def test_node_rebalance_deleted_compute_node_race(self): + # Simulate a service running and then stopping. host_a runs, creates + # fake-node, then is stopped. The fake-node compute node is destroyed. + # This leaves a soft-deleted node in the DB. + host_a = self._start_compute('host_a') + host_a.manager.driver._set_nodes([self.nodename]) + host_a.manager.update_available_resource(self.ctxt) + host_a.stop() + cn = objects.ComputeNode.get_by_host_and_nodename( + self.ctxt, 'host_a', self.nodename, + ) + cn.destroy() + + self.assertEqual(0, len(objects.ComputeNodeList.get_all(self.ctxt))) + + # Now we create a new compute service to manage our node. + host_b = self._start_compute('host_b') + host_b.manager.driver._set_nodes([self.nodename]) + + # When start_service runs, it will create a host_b ComputeNode. We want + # to delete that and inject our fake node into the driver which will + # be re-balanced to another host later. First assert this actually + # exists. + self._assert_hypervisor_api('host_b', expected_host='host_b') + + # Now run the update_available_resource periodic to register fake-node + # and have it managed by host_b. This will also detect the "host_b" + # node as orphaned and delete it along with its resource provider. + + # host_b[1]: Finds no compute record in RT. Tries to create one + # (_init_compute_node). + host_b.manager.update_available_resource(self.ctxt) + self._assert_hypervisor_api(self.nodename, expected_host='host_b') + # There should only be one resource provider (fake-node). + original_rps = self._get_all_providers() + self.assertEqual(1, len(original_rps), original_rps) + self.assertEqual(self.nodename, original_rps[0]['name']) + + # Simulate a re-balance by restarting host_a and make it manage + # fake-node. At this point both host_b and host_a think they own + # fake-node. + host_a = self._start_compute('host_a') + host_a.manager.driver._set_nodes([self.nodename]) + + # host_a[1]: Finds no compute record in RT, 'moves' existing node from + # host_b + host_a.manager.update_available_resource(self.ctxt) + # Assert that fake-node was re-balanced from host_b to host_a. + self.assertIn( + 'ComputeNode fake-node moving from host_b to host_a', + self.stdlog.logger.output) + self._assert_hypervisor_api(self.nodename, expected_host='host_a') + + # host_a[2]: Begins periodic update, queries compute nodes for this + # host, finds the fake-node. + cn = objects.ComputeNode.get_by_host_and_nodename( + self.ctxt, 'host_a', self.nodename, + ) + + # host_b[2]: Finds no compute record in RT, 'moves' existing node from + # host_a + host_b.manager.update_available_resource(self.ctxt) + # Assert that fake-node was re-balanced from host_a to host_b. + self.assertIn( + 'ComputeNode fake-node moving from host_a to host_b', + self.stdlog.logger.output) + self._assert_hypervisor_api(self.nodename, expected_host='host_b') + + # Complete rebalance, as host_a realises it does not own fake-node. + host_a.manager.driver._set_nodes([]) + + # host_a[2]: Deletes orphan compute node. + # Mock out the compute node query to simulate a race condition where + # the list includes an orphan compute node that is newly owned by + # host_b by the time host_a attempts to delete it. + with mock.patch( + 'nova.compute.manager.ComputeManager._get_compute_nodes_in_db' + ) as mock_get: + mock_get.return_value = [cn] + host_a.manager.update_available_resource(self.ctxt) + + # Verify that the node was almost deleted, but was saved by the host + # check + self.assertIn( + 'Deleting orphan compute node %s hypervisor host ' + 'is fake-node, nodes are' % cn.id, + self.stdlog.logger.output) + self.assertIn( + 'Ignoring failure to delete orphan compute node %s on ' + 'hypervisor host fake-node' % cn.id, + self.stdlog.logger.output) + self._assert_hypervisor_api(self.nodename, 'host_b') + rps = self._get_all_providers() + self.assertEqual(1, len(rps), rps) + self.assertEqual(self.nodename, rps[0]['name']) + + # Simulate deletion of an orphan by host_a. It shouldn't happen + # anymore, but let's assume it already did. + cn = objects.ComputeNode.get_by_host_and_nodename( + self.ctxt, 'host_b', self.nodename) + cn.destroy() + host_a.manager.rt.remove_node(cn.hypervisor_hostname) + host_a.manager.reportclient.delete_resource_provider( + self.ctxt, cn, cascade=True) + + # host_b[3]: Should recreate compute node and resource provider. + host_b.manager.update_available_resource(self.ctxt) + + # Verify that the node was recreated. + self._assert_hypervisor_api(self.nodename, 'host_b') + + # The resource provider has now been created. + rps = self._get_all_providers() + self.assertEqual(1, len(rps), rps) + self.assertEqual(self.nodename, rps[0]['name']) diff --git a/nova/tests/unit/compute/test_compute.py b/nova/tests/unit/compute/test_compute.py index c12d83ca550..57101d6d184 100644 --- a/nova/tests/unit/compute/test_compute.py +++ b/nova/tests/unit/compute/test_compute.py @@ -202,13 +202,14 @@ def fake_get_compute_nodes_in_db(self, context, *args, **kwargs): context, objects.ComputeNode(), cn) for cn in fake_compute_nodes] - def fake_compute_node_delete(context, compute_node_id): + def fake_compute_node_delete(context, compute_node_id, + constraint=None): self.assertEqual(2, compute_node_id) self.stub_out( 'nova.compute.manager.ComputeManager._get_compute_nodes_in_db', fake_get_compute_nodes_in_db) - self.stub_out('nova.db.api.compute_node_delete', + self.stub_out('nova.db.sqlalchemy.api.compute_node_delete', fake_compute_node_delete) self.compute.update_available_resource( diff --git a/nova/tests/unit/compute/test_compute_mgr.py b/nova/tests/unit/compute/test_compute_mgr.py index 728f433084c..cd530cc79d5 100644 --- a/nova/tests/unit/compute/test_compute_mgr.py +++ b/nova/tests/unit/compute/test_compute_mgr.py @@ -376,18 +376,20 @@ def test_update_available_resource(self, get_db_nodes, get_avail_nodes, ) # First node in set should have been removed from DB + # Last node in set should have been added to DB. for db_node in db_nodes: if db_node.hypervisor_hostname == 'node1': db_node.destroy.assert_called_once_with() rc_mock.delete_resource_provider.assert_called_once_with( self.context, db_node, cascade=True) - mock_rt.remove_node.assert_called_once_with( - 'node1') + mock_rt.remove_node.assert_called_once_with('node1') mock_log.error.assert_called_once_with( "Failed to delete compute node resource provider for " "compute node %s: %s", db_node.uuid, mock.ANY) else: self.assertFalse(db_node.destroy.called) + self.assertEqual(1, mock_rt.remove_node.call_count) + mock_rt.clean_compute_node_cache.assert_called_once_with(db_nodes) @mock.patch('nova.scheduler.client.report.SchedulerReportClient.' 'delete_resource_provider') @@ -409,6 +411,33 @@ def test_update_available_resource_not_ready(self, get_db_nodes, update_mock.assert_not_called() del_rp_mock.assert_not_called() + @mock.patch.object(manager.ComputeManager, + '_update_available_resource_for_node') + @mock.patch.object(fake_driver.FakeDriver, 'get_available_nodes') + @mock.patch.object(manager.ComputeManager, '_get_compute_nodes_in_db') + def test_update_available_resource_destroy_rebalance( + self, get_db_nodes, get_avail_nodes, update_mock): + mock_rt = self._mock_rt() + rc_mock = self.useFixture(fixtures.fixtures.MockPatchObject( + self.compute, 'reportclient')).mock + db_nodes = [self._make_compute_node('node1', 1)] + get_db_nodes.return_value = db_nodes + # Destroy can fail if nodes were rebalanced between getting the node + # list and calling destroy. + db_nodes[0].destroy.side_effect = exception.ObjectActionError( + action='destroy', reason='host changed') + get_avail_nodes.return_value = set() + self.compute.update_available_resource(self.context) + get_db_nodes.assert_called_once_with(self.context, set(), + use_slave=True, startup=False) + self.assertEqual(0, update_mock.call_count) + + db_nodes[0].destroy.assert_called_once_with() + self.assertEqual(0, rc_mock.delete_resource_provider.call_count) + mock_rt.remove_node.assert_called_once_with('node1') + rc_mock.invalidate_resource_provider.assert_called_once_with( + db_nodes[0].uuid) + @mock.patch('nova.context.get_admin_context') def test_pre_start_hook(self, get_admin_context): """Very simple test just to make sure update_available_resource is diff --git a/nova/tests/unit/compute/test_resource_tracker.py b/nova/tests/unit/compute/test_resource_tracker.py index 9ded03602e7..cd77f70e4e2 100644 --- a/nova/tests/unit/compute/test_resource_tracker.py +++ b/nova/tests/unit/compute/test_resource_tracker.py @@ -3927,3 +3927,24 @@ def foo(self): self.assertRaises(AssertionError, _test_explict_unfair) self.assertRaises(AssertionError, _test_implicit_unfair) + + +class TestCleanComputeNodeCache(BaseTestCase): + + def setUp(self): + super(TestCleanComputeNodeCache, self).setUp() + self._setup_rt() + self.context = context.RequestContext( + mock.sentinel.user_id, mock.sentinel.project_id) + + @mock.patch.object(resource_tracker.ResourceTracker, "remove_node") + def test_clean_compute_node_cache(self, mock_remove): + invalid_nodename = "invalid-node" + self.rt.compute_nodes[_NODENAME] = self.compute + self.rt.compute_nodes[invalid_nodename] = mock.sentinel.compute + with mock.patch.object( + self.rt.reportclient, "invalidate_resource_provider", + ) as mock_invalidate: + self.rt.clean_compute_node_cache([self.compute]) + mock_remove.assert_called_once_with(invalid_nodename) + mock_invalidate.assert_called_once_with(invalid_nodename) diff --git a/nova/tests/unit/db/test_db_api.py b/nova/tests/unit/db/test_db_api.py index 6b80f216612..3b26735e381 100644 --- a/nova/tests/unit/db/test_db_api.py +++ b/nova/tests/unit/db/test_db_api.py @@ -5283,6 +5283,22 @@ def test_compute_node_create_duplicate_host_hypervisor_hostname(self): self.assertRaises(db_exc.DBDuplicateEntry, db.compute_node_create, self.ctxt, other_node) + def test_compute_node_create_duplicate_uuid(self): + """Tests to make sure that no exception is raised when trying to create + a compute node with the same host, hypervisor_hostname and uuid values + as another compute node that was previously soft-deleted. + """ + # Prior to fixing https://bugs.launchpad.net/nova/+bug/1853159, this + # raised the following error: + # sqlalchemy.exc.InvalidRequestError: This session is in 'inactive' + # state, due to the SQL transaction being rolled back; no further SQL + # can be emitted within this transaction. + constraint = db.constraint(host=db.equal_any(self.item['host'])) + sqlalchemy_api.compute_node_delete( + self.ctxt, self.item['id'], constraint=constraint) + new_node = db.compute_node_create(self.ctxt, self.compute_node_dict) + self.assertEqual(self.item['uuid'], new_node['uuid']) + def test_compute_node_get_all(self): nodes = db.compute_node_get_all(self.ctxt) self.assertEqual(1, len(nodes)) @@ -5571,6 +5587,13 @@ def test_compute_node_delete(self): nodes = db.compute_node_get_all(self.ctxt) self.assertEqual(len(nodes), 0) + def test_compute_node_delete_different_host(self): + compute_node_id = self.item['id'] + constraint = db.constraint(host=db.equal_any('different-host')) + self.assertRaises(exception.ConstraintNotMet, + sqlalchemy_api.compute_node_delete, + self.ctxt, compute_node_id, constraint=constraint) + def test_compute_node_search_by_hypervisor(self): nodes_created = [] new_service = copy.copy(self.service_dict) diff --git a/nova/tests/unit/objects/test_compute_node.py b/nova/tests/unit/objects/test_compute_node.py index aeaf4b957fe..747cc1a36eb 100644 --- a/nova/tests/unit/objects/test_compute_node.py +++ b/nova/tests/unit/objects/test_compute_node.py @@ -24,6 +24,7 @@ from nova import conf from nova.db import api as db +from nova.db.sqlalchemy import api as sa_api from nova import exception from nova import objects from nova.objects import base @@ -410,12 +411,22 @@ def test_set_id_failure(self, mock_get, db_mock): self.assertRaises(ovo_exc.ReadOnlyFieldError, setattr, compute, 'id', 124) - @mock.patch.object(db, 'compute_node_delete') + @mock.patch.object(sa_api, 'compute_node_delete') def test_destroy(self, mock_delete): compute = compute_node.ComputeNode(context=self.context) compute.id = 123 compute.destroy() - mock_delete.assert_called_once_with(self.context, 123) + mock_delete.assert_called_once_with(self.context, 123, constraint=None) + + def test_destroy_host_constraint(self): + # Create compute node with host='fake' + compute = fake_compute_with_resources.obj_clone() + compute._context = self.context + compute.host = 'fake' + compute.create() + # Simulate a compute node ownership change due to a node rebalance + compute.host = 'different' + self.assertRaises(exception.ObjectActionError, compute.destroy) @mock.patch.object(db, 'compute_node_get_all') def test_get_all(self, mock_get_all): diff --git a/nova/tests/unit/virt/libvirt/test_driver.py b/nova/tests/unit/virt/libvirt/test_driver.py index cea25218936..f5b82065869 100644 --- a/nova/tests/unit/virt/libvirt/test_driver.py +++ b/nova/tests/unit/virt/libvirt/test_driver.py @@ -10514,6 +10514,25 @@ def test_check_can_live_migrate_guest_cpu_none_model( '_create_shared_storage_test_file', return_value='fake') @mock.patch.object(libvirt_driver.LibvirtDriver, '_compare_cpu') + def test_check_can_live_migrate_guest_cpu_none_model_skip_compare( + self, mock_cpu, mock_test_file): + self.flags(group='workarounds', skip_cpu_compare_on_dest=True) + instance_ref = objects.Instance(**self.test_instance) + instance_ref.vcpu_model = test_vcpu_model.fake_vcpumodel + instance_ref.vcpu_model.model = None + drvr = libvirt_driver.LibvirtDriver(fake.FakeVirtAPI(), False) + compute_info = {'cpu_info': 'asdf', 'disk_available_least': 1} + drvr.check_can_live_migrate_destination( + self.context, instance_ref, compute_info, compute_info) + mock_cpu.assert_not_called() + + @mock.patch( + 'nova.network.neutron.API.has_port_binding_extension', + new=mock.Mock(return_value=False)) + @mock.patch.object(libvirt_driver.LibvirtDriver, + '_create_shared_storage_test_file', + return_value='fake') + @mock.patch.object(libvirt_driver.LibvirtDriver, '_compare_cpu') def test_check_can_live_migrate_dest_numa_lm( self, mock_cpu, mock_test_file): instance_ref = objects.Instance(**self.test_instance) diff --git a/nova/tests/unit/virt/test_images.py b/nova/tests/unit/virt/test_images.py index 199d4cf8e1c..e3279be820b 100644 --- a/nova/tests/unit/virt/test_images.py +++ b/nova/tests/unit/virt/test_images.py @@ -16,6 +16,8 @@ import mock from oslo_concurrency import processutils +from oslo_serialization import jsonutils +from oslo_utils import imageutils import six from nova.compute import utils as compute_utils @@ -136,3 +138,47 @@ def test_convert_image_without_direct_io_support(self, mock_execute, '-O', 'out_format', '-f', 'in_format', 'source', 'dest') mock_disk_op_sema.__enter__.assert_called_once() self.assertTupleEqual(expected, mock_execute.call_args[0]) + + def test_convert_image_vmdk_allowed_list_checking(self): + info = {'format': 'vmdk', + 'format-specific': { + 'type': 'vmdk', + 'data': { + 'create-type': 'monolithicFlat', + }}} + + # If the format is not in the allowed list, we should get an error + self.assertRaises(exception.ImageUnacceptable, + images.check_vmdk_image, 'foo', + imageutils.QemuImgInfo(jsonutils.dumps(info), + format='json')) + + # With the format in the allowed list, no error + self.flags(vmdk_allowed_types=['streamOptimized', 'monolithicFlat', + 'monolithicSparse'], + group='compute') + images.check_vmdk_image('foo', + imageutils.QemuImgInfo(jsonutils.dumps(info), + format='json')) + + # With an empty list, allow nothing + self.flags(vmdk_allowed_types=[], group='compute') + self.assertRaises(exception.ImageUnacceptable, + images.check_vmdk_image, 'foo', + imageutils.QemuImgInfo(jsonutils.dumps(info), + format='json')) + + @mock.patch.object(images, 'fetch') + @mock.patch('nova.privsep.qemu.unprivileged_qemu_img_info') + def test_fetch_checks_vmdk_rules(self, mock_info, mock_fetch): + info = {'format': 'vmdk', + 'format-specific': { + 'type': 'vmdk', + 'data': { + 'create-type': 'monolithicFlat', + }}} + mock_info.return_value = jsonutils.dumps(info) + with mock.patch('os.path.exists', return_value=True): + e = self.assertRaises(exception.ImageUnacceptable, + images.fetch_to_raw, None, 'foo', 'anypath') + self.assertIn('Invalid VMDK create-type specified', str(e)) diff --git a/nova/virt/images.py b/nova/virt/images.py index 5358f3766ac..f13c8722909 100644 --- a/nova/virt/images.py +++ b/nova/virt/images.py @@ -110,6 +110,34 @@ def get_info(context, image_href): return IMAGE_API.get(context, image_href) +def check_vmdk_image(image_id, data): + # Check some rules about VMDK files. Specifically we want to make + # sure that the "create-type" of the image is one that we allow. + # Some types of VMDK files can reference files outside the disk + # image and we do not want to allow those for obvious reasons. + + types = CONF.compute.vmdk_allowed_types + + if not len(types): + LOG.warning('Refusing to allow VMDK image as vmdk_allowed_' + 'types is empty') + msg = _('Invalid VMDK create-type specified') + raise exception.ImageUnacceptable(image_id=image_id, reason=msg) + + try: + create_type = data.format_specific['data']['create-type'] + except KeyError: + msg = _('Unable to determine VMDK create-type') + raise exception.ImageUnacceptable(image_id=image_id, reason=msg) + + if create_type not in CONF.compute.vmdk_allowed_types: + LOG.warning('Refusing to process VMDK file with create-type of %r ' + 'which is not in allowed set of: %s', create_type, + ','.join(CONF.compute.vmdk_allowed_types)) + msg = _('Invalid VMDK create-type specified') + raise exception.ImageUnacceptable(image_id=image_id, reason=msg) + + def fetch_to_raw(context, image_href, path, trusted_certs=None): path_tmp = "%s.part" % path fetch(context, image_href, path_tmp, trusted_certs) @@ -129,6 +157,9 @@ def fetch_to_raw(context, image_href, path, trusted_certs=None): reason=(_("fmt=%(fmt)s backed by: %(backing_file)s") % {'fmt': fmt, 'backing_file': backing_file})) + if fmt == 'vmdk': + check_vmdk_image(image_href, data) + if fmt != "raw" and CONF.force_raw_images: staged = "%s.converted" % path LOG.debug("%s was %s, converting to raw", image_href, fmt) diff --git a/nova/virt/libvirt/driver.py b/nova/virt/libvirt/driver.py index 08251516e30..0a505f3bcc9 100644 --- a/nova/virt/libvirt/driver.py +++ b/nova/virt/libvirt/driver.py @@ -8445,15 +8445,16 @@ def check_can_live_migrate_destination(self, context, instance, disk_available_mb = ( (disk_available_gb * units.Ki) - CONF.reserved_host_disk_mb) - # Compare CPU - try: - if not instance.vcpu_model or not instance.vcpu_model.model: - source_cpu_info = src_compute_info['cpu_info'] - self._compare_cpu(None, source_cpu_info, instance) - else: - self._compare_cpu(instance.vcpu_model, None, instance) - except exception.InvalidCPUInfo as e: - raise exception.MigrationPreCheckError(reason=e) + if not CONF.workarounds.skip_cpu_compare_on_dest: + # Compare CPU + try: + if not instance.vcpu_model or not instance.vcpu_model.model: + source_cpu_info = src_compute_info['cpu_info'] + self._compare_cpu(None, source_cpu_info, instance) + else: + self._compare_cpu(instance.vcpu_model, None, instance) + except exception.InvalidCPUInfo as e: + raise exception.MigrationPreCheckError(reason=e) # Create file on storage, to be checked on source host filename = self._create_shared_storage_test_file(instance) diff --git a/releasenotes/notes/bug-1853009-99414e14d1491b5f.yaml b/releasenotes/notes/bug-1853009-99414e14d1491b5f.yaml new file mode 100644 index 00000000000..d6e71c42224 --- /dev/null +++ b/releasenotes/notes/bug-1853009-99414e14d1491b5f.yaml @@ -0,0 +1,7 @@ +--- +fixes: + - | + Fixes an issue with multiple ``nova-compute`` services used with Ironic, + where a rebalance operation could result in a compute node being deleted + from the database and not recreated. See `bug 1853009 + `__ for details. diff --git a/releasenotes/notes/skip-compare-cpu-on-dest-6ae419ddd61fd0f8.yaml b/releasenotes/notes/skip-compare-cpu-on-dest-6ae419ddd61fd0f8.yaml new file mode 100644 index 00000000000..e7cd4041b16 --- /dev/null +++ b/releasenotes/notes/skip-compare-cpu-on-dest-6ae419ddd61fd0f8.yaml @@ -0,0 +1,24 @@ +--- +issues: + - | + Nova's use of libvirt's compareCPU() API served its purpose over the + years, but its design limitations break live migration in subtle + ways. For example, the compareCPU() API compares against the host + physical CPUID. Some of the features from this CPUID aren not + exposed by KVM, and then there are some features that KVM emulates + that are not in the host CPUID. The latter can cause bogus live + migration failures. + + With QEMU >=2.9 and libvirt >= 4.4.0, libvirt will do the right + thing in terms of CPU compatibility checks on the destination host + during live migration. Nova satisfies these minimum version + requirements by a good margin. So, this workaround provides a way to + skip the CPU comparison check on the destination host before + migrating a guest, and let libvirt handle it correctly. + + This workaround will be deprecated and removed once Nova replaces + the older libvirt APIs with their newer counterparts. The work is + being tracked via this `blueprint + cpu-selection-with-hypervisor-consideration`_. + + .. _blueprint cpu-selection-with-hypervisor-consideration: https://blueprints.launchpad.net/nova/+spec/cpu-selection-with-hypervisor-consideration