From b87ced4a8d7179005e1db3d6b3e02a297285c734 Mon Sep 17 00:00:00 2001 From: Sylvain Bauza Date: Wed, 23 Sep 2020 12:52:52 +0200 Subject: [PATCH 01/40] libvirt: make mdev types name attribute be optional Some GPU drivers like i915 don't provide a name attribute for mdev types. As we don't use this attribute yet, let's just make sure we support the fact it's optional. Change-Id: Ia745ed7095c74e2bfba38379e623a3f81e7799eb Closes-Bug: #1896741 (cherry picked from commit 416cd1ab18180fc09b915f4517aca03651f01eea) --- nova/tests/unit/virt/libvirt/test_driver.py | 45 +++++++++++++++++++++ nova/virt/libvirt/driver.py | 3 +- 2 files changed, 47 insertions(+), 1 deletion(-) diff --git a/nova/tests/unit/virt/libvirt/test_driver.py b/nova/tests/unit/virt/libvirt/test_driver.py index 1a7e7b7d59d..f1cd319b773 100644 --- a/nova/tests/unit/virt/libvirt/test_driver.py +++ b/nova/tests/unit/virt/libvirt/test_driver.py @@ -347,6 +347,29 @@ + """, + "pci_0000_06_00_1": """ + + pci_0000_06_00_1 + /sys/devices/pci0000:00/0000:00:06.1 + + + i915 + + + 0 + 6 + 0 + 1 + HD Graphics P630 + Intel Corporation + + + vfio-pci + 2 + + + """, "mdev_4b20d080_1b54_4048_85b3_a6a62d165c01": """ @@ -24999,6 +25022,28 @@ def fake_nodeDeviceLookupByName(name): self.assertEqual([], drvr._get_mdev_capable_devices(types=['nvidia-12'])) + @mock.patch.object(host.Host, 'device_lookup_by_name') + def test_get_mdev_capabilities_for_dev_name_optional( + self, device_lookup_by_name): + # We use another PCI device that doesn't provide a name attribute for + # each mdev type. + def fake_nodeDeviceLookupByName(name): + return FakeNodeDevice(_fake_NodeDevXml[name]) + device_lookup_by_name.side_effect = fake_nodeDeviceLookupByName + + drvr = libvirt_driver.LibvirtDriver(fake.FakeVirtAPI(), False) + + expected = {"dev_id": "pci_0000_06_00_1", + "vendor_id": 0x8086, + "types": {'i915-GVTg_V5_8': {'availableInstances': 2, + 'name': None, + 'deviceAPI': 'vfio-pci'}, + } + } + self.assertEqual( + expected, + drvr._get_mdev_capabilities_for_dev("pci_0000_06_00_1")) + @mock.patch.object(host.Host, 'device_lookup_by_name') @mock.patch.object(host.Host, 'list_mediated_devices') def test_get_mediated_devices(self, list_mediated_devices, diff --git a/nova/virt/libvirt/driver.py b/nova/virt/libvirt/driver.py index 4da0b5bff4b..7860e431106 100644 --- a/nova/virt/libvirt/driver.py +++ b/nova/virt/libvirt/driver.py @@ -7246,7 +7246,8 @@ def _get_mdev_capabilities_for_dev(self, devname, types=None): if not types or cap['type'] in types: device["types"].update({cap['type']: { 'availableInstances': cap['availableInstances'], - 'name': cap['name'], + # This attribute is optional + 'name': cap.get('name'), 'deviceAPI': cap['deviceAPI']}}) return device From cbf44b7d137f3e70baf0d19b08e7a5bf6544a32a Mon Sep 17 00:00:00 2001 From: Mark Goddard Date: Tue, 19 Nov 2019 14:45:02 +0000 Subject: [PATCH 02/40] Add functional regression test for bug 1853009 Bug 1853009 describes a race condition involving multiple nova-compute services with ironic. As the compute services start up, the hash ring rebalances, and the compute services have an inconsistent view of which is responsible for a compute node. The sequence of actions here is adapted from a real world log [1], where multiple nova-compute services were started simultaneously. In some cases mocks are used to simulate race conditions. There are three main issues with the behaviour: * host2 deletes the orphan node compute node after host1 has taken ownership of it. * host1 assumes that another compute service will not delete its nodes. Once a node is in rt.compute_nodes, it is not removed again unless the node is orphaned. This prevents host1 from recreating the compute node. * host1 assumes that another compute service will not delete its resource providers. Once an RP is in the provider tree, it is not removed. This functional test documents the current behaviour, with the idea that it can be updated as this behaviour is fixed. [1] http://paste.openstack.org/show/786272/ Co-Authored-By: Matt Riedemann Change-Id: Ice4071722de54e8d20bb8c3795be22f1995940cd Related-Bug: #1853009 Related-Bug: #1853159 (cherry picked from commit 4a894fefc4f99db81d967cae0c23454c5a62b8ba) --- .../regressions/test_bug_1853009.py | 187 ++++++++++++++++++ 1 file changed, 187 insertions(+) create mode 100644 nova/tests/functional/regressions/test_bug_1853009.py 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..6f7e13cbbbf --- /dev/null +++ b/nova/tests/functional/regressions/test_bug_1853009.py @@ -0,0 +1,187 @@ +# 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). + # FIXME(mgoddard): This shows a traceback with SQL rollback due to + # soft-deleted node. The create seems to succeed but breaks the RT + # update for this node. See + # https://bugs.launchpad.net/nova/+bug/1853159. + host_b.manager.update_available_resource(self.ctxt) + self.assertIn( + 'Deleting orphan compute node %s hypervisor host ' + 'is host_b, nodes are' % cn.id, + self.stdlog.logger.output) + 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. + # FIXME(mgoddard): Ideally host_a would not delete a node that does not + # belong to it. See https://bugs.launchpad.net/nova/+bug/1853009. + 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 deleted. + self.assertIn( + 'Deleting orphan compute node %s hypervisor host ' + 'is fake-node, nodes are' % cn.id, + self.stdlog.logger.output) + hypervisors = self.api.api_get( + '/os-hypervisors/detail').body['hypervisors'] + self.assertEqual(0, len(hypervisors), hypervisors) + rps = self._get_all_providers() + self.assertEqual(0, len(rps), rps) + + # host_b[3]: Should recreate compute node and resource provider. + # FIXME(mgoddard): Compute node not recreated here, because it is + # already in RT.compute_nodes. See + # https://bugs.launchpad.net/nova/+bug/1853009. + # FIXME(mgoddard): Resource provider not recreated here, because it + # exists in the provider tree. See + # https://bugs.launchpad.net/nova/+bug/1841481. + host_b.manager.update_available_resource(self.ctxt) + + # Verify that the node was not recreated. + hypervisors = self.api.api_get( + '/os-hypervisors/detail').body['hypervisors'] + self.assertEqual(0, len(hypervisors), hypervisors) + + # But the compute node exists in the RT. + self.assertIn(self.nodename, host_b.manager.rt.compute_nodes) + + # There is no RP. + rps = self._get_all_providers() + self.assertEqual(0, len(rps), rps) + + # But the RP exists in the provider tree. + self.assertTrue(host_b.manager.rt.reportclient._provider_tree.exists( + self.nodename)) + + # This fails due to the lack of a resource provider. + self.assertIn( + 'Skipping removal of allocations for deleted instances', + self.stdlog.logger.output) From 730071f6bfafbddf07419aa65cdf1bb150185cfa Mon Sep 17 00:00:00 2001 From: Stephen Finucane Date: Wed, 28 Apr 2021 13:53:39 +0100 Subject: [PATCH 03/40] Clear rebalanced compute nodes from resource tracker There is a race condition in nova-compute with the ironic virt driver as nodes get rebalanced. It can lead to compute nodes being removed in the DB and not repopulated. Ultimately this prevents these nodes from being scheduled to. The issue being addressed here is that if a compute node is deleted by a host which thinks it is an orphan, then the compute host that actually owns the node might not recreate it if the node is already in its resource tracker cache. This change fixes the issue by clearing nodes from the resource tracker cache for which a compute node entry does not exist. Then, when the available resource for the node is updated, the compute node object is not found in the cache and gets recreated. Change-Id: I39241223b447fcc671161c370dbf16e1773b684a Partial-Bug: #1853009 (cherry picked from commit 8f5a078dd7bbe5b6b38cf8e04d916281dc418409) --- nova/compute/manager.py | 2 + nova/compute/resource_tracker.py | 17 ++++++++ .../regressions/test_bug_1853009.py | 39 ++++++++++++------- nova/tests/unit/compute/test_compute_mgr.py | 6 ++- .../unit/compute/test_resource_tracker.py | 17 ++++++++ 5 files changed, 66 insertions(+), 15 deletions(-) diff --git a/nova/compute/manager.py b/nova/compute/manager.py index 65245af1c34..2acc257c582 100644 --- a/nova/compute/manager.py +++ b/nova/compute/manager.py @@ -9898,6 +9898,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: diff --git a/nova/compute/resource_tracker.py b/nova/compute/resource_tracker.py index 5b9e64e2190..58fe54b9222 100644 --- a/nova/compute/resource_tracker.py +++ b/nova/compute/resource_tracker.py @@ -1988,3 +1988,20 @@ 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) diff --git a/nova/tests/functional/regressions/test_bug_1853009.py b/nova/tests/functional/regressions/test_bug_1853009.py index 6f7e13cbbbf..16b8209a852 100644 --- a/nova/tests/functional/regressions/test_bug_1853009.py +++ b/nova/tests/functional/regressions/test_bug_1853009.py @@ -90,10 +90,6 @@ def test_node_rebalance_deleted_compute_node_race(self): # update for this node. See # https://bugs.launchpad.net/nova/+bug/1853159. host_b.manager.update_available_resource(self.ctxt) - self.assertIn( - 'Deleting orphan compute node %s hypervisor host ' - 'is host_b, nodes are' % cn.id, - self.stdlog.logger.output) 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() @@ -157,21 +153,17 @@ def test_node_rebalance_deleted_compute_node_race(self): self.assertEqual(0, len(rps), rps) # host_b[3]: Should recreate compute node and resource provider. - # FIXME(mgoddard): Compute node not recreated here, because it is - # already in RT.compute_nodes. See - # https://bugs.launchpad.net/nova/+bug/1853009. # FIXME(mgoddard): Resource provider not recreated here, because it # exists in the provider tree. See # https://bugs.launchpad.net/nova/+bug/1841481. host_b.manager.update_available_resource(self.ctxt) - # Verify that the node was not recreated. - hypervisors = self.api.api_get( - '/os-hypervisors/detail').body['hypervisors'] - self.assertEqual(0, len(hypervisors), hypervisors) + # Verify that the node was recreated. + self._assert_hypervisor_api(self.nodename, 'host_b') - # But the compute node exists in the RT. - self.assertIn(self.nodename, host_b.manager.rt.compute_nodes) + # But due to https://bugs.launchpad.net/nova/+bug/1853159 the compute + # node is not cached in the RT. + self.assertNotIn(self.nodename, host_b.manager.rt.compute_nodes) # There is no RP. rps = self._get_all_providers() @@ -181,6 +173,27 @@ def test_node_rebalance_deleted_compute_node_race(self): self.assertTrue(host_b.manager.rt.reportclient._provider_tree.exists( self.nodename)) + # host_b[1]: Should add compute node to RT cache and recreate resource + # provider. + # FIXME(mgoddard): Resource provider not recreated here, because it + # exists in the provider tree. See + # https://bugs.launchpad.net/nova/+bug/1841481. + host_b.manager.update_available_resource(self.ctxt) + + # Verify that the node still exists. + self._assert_hypervisor_api(self.nodename, 'host_b') + + # And it is now in the RT cache. + self.assertIn(self.nodename, host_b.manager.rt.compute_nodes) + + # There is still no RP. + rps = self._get_all_providers() + self.assertEqual(0, len(rps), rps) + + # But the RP it exists in the provider tree. + self.assertTrue(host_b.manager.rt.reportclient._provider_tree.exists( + self.nodename)) + # This fails due to the lack of a resource provider. self.assertIn( 'Skipping removal of allocations for deleted instances', diff --git a/nova/tests/unit/compute/test_compute_mgr.py b/nova/tests/unit/compute/test_compute_mgr.py index 93c895e8734..6dbc31c8813 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') diff --git a/nova/tests/unit/compute/test_resource_tracker.py b/nova/tests/unit/compute/test_resource_tracker.py index ecac5ffb179..c988a0c2a66 100644 --- a/nova/tests/unit/compute/test_resource_tracker.py +++ b/nova/tests/unit/compute/test_resource_tracker.py @@ -4227,3 +4227,20 @@ def test__get_providers_to_update_not_in_tree(self, mock_log): mock_log.warning.assert_called_once_with(*expected_log_call) self.assertIn(uuids.unknown, self.rt.absent_providers) self.assertEqual(result, []) + + +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 + self.rt.clean_compute_node_cache([self.compute]) + mock_remove.assert_called_once_with(invalid_nodename) From 4f1e4b3393d23b3146a1b8b9e4ed81a46a0ec23c Mon Sep 17 00:00:00 2001 From: Mark Goddard Date: Tue, 19 Nov 2019 16:51:01 +0000 Subject: [PATCH 04/40] Invalidate provider tree when compute node disappears There is a race condition in nova-compute with the ironic virt driver as nodes get rebalanced. It can lead to compute nodes being removed in the DB and not repopulated. Ultimately this prevents these nodes from being scheduled to. The issue being addressed here is that if a compute node is deleted by a host which thinks it is an orphan, then the resource provider for that node might also be deleted. The compute host that owns the node might not recreate the resource provider if it exists in the provider tree cache. This change fixes the issue by clearing resource providers from the provider tree cache for which a compute node entry does not exist. Then, when the available resource for the node is updated, the resource providers are not found in the cache and get recreated in placement. Change-Id: Ia53ff43e6964963cdf295604ba0fb7171389606e Related-Bug: #1853009 Related-Bug: #1841481 (cherry picked from commit 7dcc6bfa63c1ab06d86b07bcdd05838a8ad35dec) --- nova/compute/resource_tracker.py | 1 + nova/scheduler/client/report.py | 17 ++++++++++++----- .../regressions/test_bug_1853009.py | 19 ++++++------------- .../unit/compute/test_resource_tracker.py | 8 ++++++-- 4 files changed, 25 insertions(+), 20 deletions(-) diff --git a/nova/compute/resource_tracker.py b/nova/compute/resource_tracker.py index 58fe54b9222..cbd92b2f9aa 100644 --- a/nova/compute/resource_tracker.py +++ b/nova/compute/resource_tracker.py @@ -2005,3 +2005,4 @@ def clean_compute_node_cache(self, compute_nodes_in_db): # 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/scheduler/client/report.py b/nova/scheduler/client/report.py index d09f1334877..f1f60fd67bc 100644 --- a/nova/scheduler/client/report.py +++ b/nova/scheduler/client/report.py @@ -678,11 +678,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 " @@ -2181,6 +2177,17 @@ def delete_resource_provider(self, context, compute_node, cascade=False): # left a no-op 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 index 16b8209a852..dddc79606f0 100644 --- a/nova/tests/functional/regressions/test_bug_1853009.py +++ b/nova/tests/functional/regressions/test_bug_1853009.py @@ -153,9 +153,8 @@ def test_node_rebalance_deleted_compute_node_race(self): self.assertEqual(0, len(rps), rps) # host_b[3]: Should recreate compute node and resource provider. - # FIXME(mgoddard): Resource provider not recreated here, because it - # exists in the provider tree. See - # https://bugs.launchpad.net/nova/+bug/1841481. + # FIXME(mgoddard): Resource provider not recreated here, due to + # https://bugs.launchpad.net/nova/+bug/1853159. host_b.manager.update_available_resource(self.ctxt) # Verify that the node was recreated. @@ -170,14 +169,11 @@ def test_node_rebalance_deleted_compute_node_race(self): self.assertEqual(0, len(rps), rps) # But the RP exists in the provider tree. - self.assertTrue(host_b.manager.rt.reportclient._provider_tree.exists( + self.assertFalse(host_b.manager.rt.reportclient._provider_tree.exists( self.nodename)) # host_b[1]: Should add compute node to RT cache and recreate resource # provider. - # FIXME(mgoddard): Resource provider not recreated here, because it - # exists in the provider tree. See - # https://bugs.launchpad.net/nova/+bug/1841481. host_b.manager.update_available_resource(self.ctxt) # Verify that the node still exists. @@ -186,13 +182,10 @@ def test_node_rebalance_deleted_compute_node_race(self): # And it is now in the RT cache. self.assertIn(self.nodename, host_b.manager.rt.compute_nodes) - # There is still no RP. + # The resource provider has now been created. rps = self._get_all_providers() - self.assertEqual(0, len(rps), rps) - - # But the RP it exists in the provider tree. - self.assertTrue(host_b.manager.rt.reportclient._provider_tree.exists( - self.nodename)) + self.assertEqual(1, len(rps), rps) + self.assertEqual(self.nodename, rps[0]['name']) # This fails due to the lack of a resource provider. self.assertIn( diff --git a/nova/tests/unit/compute/test_resource_tracker.py b/nova/tests/unit/compute/test_resource_tracker.py index c988a0c2a66..bb846dba3dd 100644 --- a/nova/tests/unit/compute/test_resource_tracker.py +++ b/nova/tests/unit/compute/test_resource_tracker.py @@ -4242,5 +4242,9 @@ 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 - self.rt.clean_compute_node_cache([self.compute]) - mock_remove.assert_called_once_with(invalid_nodename) + 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) From 054b08e071e255dc0a7690a93ce26585a6d7eaa7 Mon Sep 17 00:00:00 2001 From: Mark Goddard Date: Mon, 18 Nov 2019 12:06:47 +0000 Subject: [PATCH 05/40] Prevent deletion of a compute node belonging to another host There is a race condition in nova-compute with the ironic virt driver as nodes get rebalanced. It can lead to compute nodes being removed in the DB and not repopulated. Ultimately this prevents these nodes from being scheduled to. The main race condition involved is in update_available_resources in the compute manager. When the list of compute nodes is queried, there is a compute node belonging to the host that it does not expect to be managing, i.e. it is an orphan. Between that time and deleting the orphan, the real owner of the compute node takes ownership of it ( in the resource tracker). However, the node is still deleted as the first host is unaware of the ownership change. This change prevents this from occurring by filtering on the host when deleting a compute node. If another compute host has taken ownership of a node, it will have updated the host field and this will prevent deletion from occurring. The first host sees this has happened via the ComputeHostNotFound exception, and avoids deleting its resource provider. Closes-Bug: #1853009 Related-Bug: #1841481 Change-Id: I260c1fded79a85d4899e94df4d9036a1ee437f02 (cherry picked from commit 65a6756fa17ebfd644c76953c943ba8a4ad3787d) --- nova/compute/manager.py | 33 +++++++++++++------ nova/db/api.py | 5 +-- nova/db/sqlalchemy/api.py | 4 +-- nova/objects/compute_node.py | 2 +- .../regressions/test_bug_1853009.py | 25 ++++++++++---- nova/tests/unit/compute/test_compute.py | 4 ++- nova/tests/unit/compute/test_compute_mgr.py | 27 +++++++++++++++ nova/tests/unit/db/test_db_api.py | 12 +++++-- nova/tests/unit/objects/test_compute_node.py | 3 +- .../notes/bug-1853009-99414e14d1491b5f.yaml | 7 ++++ 10 files changed, 96 insertions(+), 26 deletions(-) create mode 100644 releasenotes/notes/bug-1853009-99414e14d1491b5f.yaml diff --git a/nova/compute/manager.py b/nova/compute/manager.py index 2acc257c582..cf93e5e04c6 100644 --- a/nova/compute/manager.py +++ b/nova/compute/manager.py @@ -9908,17 +9908,30 @@ 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.ComputeHostNotFound: + # 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, str(e)) for nodename in nodenames: self._update_available_resource_for_node(context, nodename, diff --git a/nova/db/api.py b/nova/db/api.py index 14578c1efdb..22d263f2a59 100644 --- a/nova/db/api.py +++ b/nova/db/api.py @@ -344,15 +344,16 @@ def compute_node_update(context, compute_id, values): return IMPL.compute_node_update(context, compute_id, values) -def compute_node_delete(context, compute_id): +def compute_node_delete(context, compute_id, compute_host): """Delete a compute node from the database. :param context: The security context :param compute_id: ID of the compute node + :param compute_host: Hostname of the compute service Raises ComputeHostNotFound if compute node with the given ID doesn't exist. """ - return IMPL.compute_node_delete(context, compute_id) + return IMPL.compute_node_delete(context, compute_id, compute_host) def compute_node_statistics(context): diff --git a/nova/db/sqlalchemy/api.py b/nova/db/sqlalchemy/api.py index 9bc673faa82..382211128e3 100644 --- a/nova/db/sqlalchemy/api.py +++ b/nova/db/sqlalchemy/api.py @@ -763,10 +763,10 @@ 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, compute_host): """Delete a ComputeNode record.""" result = model_query(context, models.ComputeNode).\ - filter_by(id=compute_id).\ + filter_by(id=compute_id, host=compute_host).\ soft_delete(synchronize_session=False) if not result: diff --git a/nova/objects/compute_node.py b/nova/objects/compute_node.py index cf5d3ca8d88..fa327fbefdb 100644 --- a/nova/objects/compute_node.py +++ b/nova/objects/compute_node.py @@ -360,7 +360,7 @@ def save(self, prune_stats=False): @base.remotable def destroy(self): - db.compute_node_delete(self._context, self.id) + db.compute_node_delete(self._context, self.id, self.host) def update_from_virt_driver(self, resources): # NOTE(pmurray): the virt driver provides a dict of values that diff --git a/nova/tests/functional/regressions/test_bug_1853009.py b/nova/tests/functional/regressions/test_bug_1853009.py index dddc79606f0..a3ccd69b1b6 100644 --- a/nova/tests/functional/regressions/test_bug_1853009.py +++ b/nova/tests/functional/regressions/test_bug_1853009.py @@ -133,24 +133,35 @@ def test_node_rebalance_deleted_compute_node_race(self): # 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. - # FIXME(mgoddard): Ideally host_a would not delete a node that does not - # belong to it. See https://bugs.launchpad.net/nova/+bug/1853009. 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 deleted. + # 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) - hypervisors = self.api.api_get( - '/os-hypervisors/detail').body['hypervisors'] - self.assertEqual(0, len(hypervisors), hypervisors) + 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(0, len(rps), rps) + 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. # FIXME(mgoddard): Resource provider not recreated here, due to diff --git a/nova/tests/unit/compute/test_compute.py b/nova/tests/unit/compute/test_compute.py index fadf14f7fee..50a06fa9eae 100644 --- a/nova/tests/unit/compute/test_compute.py +++ b/nova/tests/unit/compute/test_compute.py @@ -202,8 +202,10 @@ 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, + compute_node_host): self.assertEqual(2, compute_node_id) + self.assertEqual('fake_phyp1', compute_node_host) self.stub_out( 'nova.compute.manager.ComputeManager._get_compute_nodes_in_db', diff --git a/nova/tests/unit/compute/test_compute_mgr.py b/nova/tests/unit/compute/test_compute_mgr.py index 6dbc31c8813..dffb34bac6b 100644 --- a/nova/tests/unit/compute/test_compute_mgr.py +++ b/nova/tests/unit/compute/test_compute_mgr.py @@ -411,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.ComputeHostNotFound( + 'node1') + 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/db/test_db_api.py b/nova/tests/unit/db/test_db_api.py index 6b80f216612..b59ca474ad8 100644 --- a/nova/tests/unit/db/test_db_api.py +++ b/nova/tests/unit/db/test_db_api.py @@ -5393,7 +5393,7 @@ def test_compute_node_get_all_deleted_compute_node(self): # Now delete the newly-created compute node to ensure the related # compute node stats are wiped in a cascaded fashion - db.compute_node_delete(self.ctxt, node['id']) + db.compute_node_delete(self.ctxt, node['id'], node['host']) # Clean up the service db.service_destroy(self.ctxt, service['id']) @@ -5567,10 +5567,18 @@ def test_compute_node_update(self): def test_compute_node_delete(self): compute_node_id = self.item['id'] - db.compute_node_delete(self.ctxt, compute_node_id) + compute_node_host = self.item['host'] + db.compute_node_delete(self.ctxt, compute_node_id, compute_node_host) 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'] + compute_node_host = 'invalid-host' + self.assertRaises(exception.ComputeHostNotFound, + db.compute_node_delete, + self.ctxt, compute_node_id, compute_node_host) + 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..237c0c7cb27 100644 --- a/nova/tests/unit/objects/test_compute_node.py +++ b/nova/tests/unit/objects/test_compute_node.py @@ -414,8 +414,9 @@ def test_set_id_failure(self, mock_get, db_mock): def test_destroy(self, mock_delete): compute = compute_node.ComputeNode(context=self.context) compute.id = 123 + compute.host = 'fake' compute.destroy() - mock_delete.assert_called_once_with(self.context, 123) + mock_delete.assert_called_once_with(self.context, 123, 'fake') @mock.patch.object(db, 'compute_node_get_all') def test_get_all(self, mock_get_all): 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. From 03fe127f8ab3d11e44e1bd60db741f50324909d3 Mon Sep 17 00:00:00 2001 From: Mark Goddard Date: Wed, 20 Nov 2019 12:01:33 +0000 Subject: [PATCH 06/40] Fix inactive session error in compute node creation In the fix for bug 1839560 [1][2], soft-deleted compute nodes may be restored, to ensure we can reuse ironic node UUIDs as compute node UUIDs. While this seems to largely work, it results in some nasty errors being generated [3]: InvalidRequestError This session is in 'inactive' state, due to the SQL transaction being rolled back; no further SQL can be emitted within this transaction. This happens because compute_node_create is decorated with pick_context_manager_writer, which begins a transaction. While _compute_node_get_and_update_deleted claims that calling a second pick_context_manager_writer decorated function will begin a new subtransaction, this does not appear to be the case. This change removes pick_context_manager_writer from the compute_node_create function, and adds a new _compute_node_create function which ensures the transaction is finished if _compute_node_get_and_update_deleted is called. The new unit test added here fails without this change. This change marks the removal of the final FIXME from the functional test added in [4]. [1] https://bugs.launchpad.net/nova/+bug/1839560 [2] https://git.openstack.org/cgit/openstack/nova/commit/?id=89dd74ac7f1028daadf86cb18948e27fe9d1d411 [3] http://paste.openstack.org/show/786350/ [4] https://review.opendev.org/#/c/695012/ Change-Id: Iae119ea8776bc7f2e5dbe2e502a743217beded73 Closes-Bug: #1853159 Related-Bug: #1853009 (cherry picked from commit 54038b7f914d624a6684b5c0f168bdf84872a60c) --- nova/db/sqlalchemy/api.py | 17 ++++++++-- .../regressions/test_bug_1853009.py | 33 ------------------- nova/tests/unit/db/test_db_api.py | 14 ++++++++ 3 files changed, 29 insertions(+), 35 deletions(-) diff --git a/nova/db/sqlalchemy/api.py b/nova/db/sqlalchemy/api.py index 382211128e3..03240bd2abe 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 diff --git a/nova/tests/functional/regressions/test_bug_1853009.py b/nova/tests/functional/regressions/test_bug_1853009.py index a3ccd69b1b6..2ec69482a25 100644 --- a/nova/tests/functional/regressions/test_bug_1853009.py +++ b/nova/tests/functional/regressions/test_bug_1853009.py @@ -85,10 +85,6 @@ def test_node_rebalance_deleted_compute_node_race(self): # host_b[1]: Finds no compute record in RT. Tries to create one # (_init_compute_node). - # FIXME(mgoddard): This shows a traceback with SQL rollback due to - # soft-deleted node. The create seems to succeed but breaks the RT - # update for this node. See - # https://bugs.launchpad.net/nova/+bug/1853159. 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). @@ -164,41 +160,12 @@ def test_node_rebalance_deleted_compute_node_race(self): self.ctxt, cn, cascade=True) # host_b[3]: Should recreate compute node and resource provider. - # FIXME(mgoddard): Resource provider not recreated here, due to - # https://bugs.launchpad.net/nova/+bug/1853159. host_b.manager.update_available_resource(self.ctxt) # Verify that the node was recreated. self._assert_hypervisor_api(self.nodename, 'host_b') - # But due to https://bugs.launchpad.net/nova/+bug/1853159 the compute - # node is not cached in the RT. - self.assertNotIn(self.nodename, host_b.manager.rt.compute_nodes) - - # There is no RP. - rps = self._get_all_providers() - self.assertEqual(0, len(rps), rps) - - # But the RP exists in the provider tree. - self.assertFalse(host_b.manager.rt.reportclient._provider_tree.exists( - self.nodename)) - - # host_b[1]: Should add compute node to RT cache and recreate resource - # provider. - host_b.manager.update_available_resource(self.ctxt) - - # Verify that the node still exists. - self._assert_hypervisor_api(self.nodename, 'host_b') - - # And it is now in the RT cache. - self.assertIn(self.nodename, host_b.manager.rt.compute_nodes) - # 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']) - - # This fails due to the lack of a resource provider. - self.assertIn( - 'Skipping removal of allocations for deleted instances', - self.stdlog.logger.output) diff --git a/nova/tests/unit/db/test_db_api.py b/nova/tests/unit/db/test_db_api.py index b59ca474ad8..47b7e87c61c 100644 --- a/nova/tests/unit/db/test_db_api.py +++ b/nova/tests/unit/db/test_db_api.py @@ -5283,6 +5283,20 @@ 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. + db.compute_node_delete(self.ctxt, self.item['id'], self.item['host']) + 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)) From 0354d4d9f47354e2b4fc0b2343c27e734fe2e494 Mon Sep 17 00:00:00 2001 From: Balazs Gibizer Date: Thu, 8 Oct 2020 14:13:38 +0200 Subject: [PATCH 07/40] Reproduce bug 1897528 The nova-compute fails to start if the hypervisor has PCI addresses 32bit domain. Change-Id: I48dcb7faa17fe9f8346445a1746cff5845baf358 Related-Bug: #1897528 (cherry picked from commit 976ac722d36439d16ea4ec1bf5037c482c89ef55) --- nova/tests/unit/pci/test_manager.py | 37 +++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/nova/tests/unit/pci/test_manager.py b/nova/tests/unit/pci/test_manager.py index fe7d918e27b..683f3079299 100644 --- a/nova/tests/unit/pci/test_manager.py +++ b/nova/tests/unit/pci/test_manager.py @@ -22,6 +22,7 @@ import nova from nova.compute import vm_states from nova import context +from nova import exception from nova import objects from nova.objects import fields from nova.pci import manager @@ -236,6 +237,42 @@ def test_update_devices_from_hypervisor_resources(self, _mock_dev_assign): tracker.update_devices_from_hypervisor_resources(fake_pci_devs_json) self.assertEqual(2, len(tracker.pci_devs)) + def test_update_devices_from_hypervisor_resources_32bit_domain(self): + self.flags( + group='pci', + passthrough_whitelist=[ + '{"product_id":"2032", "vendor_id":"8086"}']) + # There are systems where 32 bit PCI domain is used. See bug 1897528 + # for example. While nova (and qemu) does not support assigning such + # devices but the existence of such device in the system should not + # lead to an error. + fake_pci = { + 'compute_node_id': 1, + 'address': '10000:00:02.0', + 'product_id': '2032', + 'vendor_id': '8086', + 'request_id': None, + 'status': fields.PciDeviceStatus.AVAILABLE, + 'dev_type': fields.PciDeviceType.STANDARD, + 'parent_addr': None, + 'numa_node': 0} + + fake_pci_devs = [fake_pci] + fake_pci_devs_json = jsonutils.dumps(fake_pci_devs) + tracker = manager.PciDevTracker(self.fake_context) + # We expect that the device with 32bit PCI domain is ignored + # tracker.update_devices_from_hypervisor_resources(fake_pci_devs_json) + # self.assertEqual(0, len(tracker.pci_devs)) + # + # This is the bug 1897528 + ex = self.assertRaises( + exception.PciConfigInvalidWhitelist, + tracker.update_devices_from_hypervisor_resources, + fake_pci_devs_json) + self.assertEqual( + 'Invalid PCI devices Whitelist config: property domain (10000) is ' + 'greater than the maximum allowable value (FFFF).', str(ex)) + def test_set_hvdev_new_dev(self): fake_pci_3 = dict(fake_pci, address='0000:00:00.4', vendor_id='v2') fake_pci_devs = [copy.deepcopy(fake_pci), copy.deepcopy(fake_pci_1), From 90ffc553d7f4152a6a4a8708787150d3c3c40b03 Mon Sep 17 00:00:00 2001 From: Balazs Gibizer Date: Thu, 8 Oct 2020 14:27:44 +0200 Subject: [PATCH 08/40] Ignore PCI devices with 32bit domain Nova and QEMU[1] supports PCI devices with a PCI address that has 16 bit domain. However there are hypervisors that reports PCI addresses with 32 bit domain. While today we cannot assign these to guests this should not prevent the nova-compute service to start. This patch changes the PCI manager to ignore such PCI devices. Please note that this patch does not change fact that nova does not allow specifying PCI addresses with 32bit domain in the [pci]/passthrough_whitelist configuration. Such configuration is still rejected at nova-compute service startup. Closes-Bug: #1897528 [1] https://github.com/qemu/qemu/blob/f2a1cf9180f63e88bb38ff21c169da97c3f2bad5/hw/core/qdev-properties.c#L993 Change-Id: I59a0746b864610b6a314078cf5661d3d2b84b1d4 (cherry picked from commit 8c9d6fc8f073cde78b79ae259c9915216f5d59b0) --- doc/source/admin/networking.rst | 12 +++++++++ doc/source/admin/pci-passthrough.rst | 12 +++++++++ nova/conf/pci.py | 8 +++++- nova/pci/manager.py | 38 ++++++++++++++++++++++++++-- nova/tests/unit/pci/test_manager.py | 23 ++++++++--------- 5 files changed, 77 insertions(+), 16 deletions(-) diff --git a/doc/source/admin/networking.rst b/doc/source/admin/networking.rst index 407a43aafea..626bb895922 100644 --- a/doc/source/admin/networking.rst +++ b/doc/source/admin/networking.rst @@ -24,6 +24,18 @@ A full guide on configuring and using SR-IOV is provided in the :neutron-doc:`OpenStack Networking service documentation ` +.. note:: + + Nova only supports PCI addresses where the fields are restricted to the + following maximum value: + + * domain - 0xFFFF + * bus - 0xFF + * slot - 0x1F + * function - 0x7 + + Nova will ignore PCI devices reported by the hypervisor if the address is + outside of these ranges. NUMA Affinity ------------- diff --git a/doc/source/admin/pci-passthrough.rst b/doc/source/admin/pci-passthrough.rst index 227538edb0c..5a4c001e6d6 100644 --- a/doc/source/admin/pci-passthrough.rst +++ b/doc/source/admin/pci-passthrough.rst @@ -37,6 +37,18 @@ devices with potentially different capabilities. supported until the 14.0.0 Newton release, see `bug 1512800 `_ for details. +.. note:: + + Nova only supports PCI addresses where the fields are restricted to the + following maximum value: + + * domain - 0xFFFF + * bus - 0xFF + * slot - 0x1F + * function - 0x7 + + Nova will ignore PCI devices reported by the hypervisor if the address is + outside of these ranges. Configure host (Compute) ------------------------ diff --git a/nova/conf/pci.py b/nova/conf/pci.py index b812b39676a..b383d0a69fc 100644 --- a/nova/conf/pci.py +++ b/nova/conf/pci.py @@ -116,7 +116,13 @@ ``address`` PCI address of the device. Both traditional glob style and regular - expression syntax is supported. + expression syntax is supported. Please note that the address fields are + restricted to the following maximum values: + + * domain - 0xFFFF + * bus - 0xFF + * slot - 0x1F + * function - 0x7 ``devname`` Device name of the device (for e.g. interface name). Not all PCI devices diff --git a/nova/pci/manager.py b/nova/pci/manager.py index 05930b0bebb..c55e732c01f 100644 --- a/nova/pci/manager.py +++ b/nova/pci/manager.py @@ -117,8 +117,42 @@ def update_devices_from_hypervisor_resources(self, devices_json): devices = [] for dev in jsonutils.loads(devices_json): - if self.dev_filter.device_assignable(dev): - devices.append(dev) + try: + if self.dev_filter.device_assignable(dev): + devices.append(dev) + except exception.PciConfigInvalidWhitelist as e: + # The raised exception is misleading as the problem is not with + # the whitelist config but with the host PCI device reported by + # libvirt. The code that matches the host PCI device to the + # withelist spec reuses the WhitelistPciAddress object to parse + # the host PCI device address. That parsing can fail if the + # PCI address has a 32 bit domain. But this should not prevent + # processing the rest of the devices. So we simply skip this + # device and continue. + # Please note that this except block does not ignore the + # invalid whitelist configuration. The whitelist config has + # already been parsed or rejected in case it was invalid. At + # this point the self.dev_filter representes the parsed and + # validated whitelist config. + LOG.debug( + 'Skipping PCI device %s reported by the hypervisor: %s', + {k: v for k, v in dev.items() + if k in ['address', 'parent_addr']}, + # NOTE(gibi): this is ugly but the device_assignable() call + # uses the PhysicalPciAddress class to parse the PCI + # addresses and that class reuses the code from + # PciAddressSpec that was originally designed to parse + # whitelist spec. Hence the raised exception talks about + # whitelist config. This is misleading as in our case the + # PCI address that we failed to parse came from the + # hypervisor. + # TODO(gibi): refactor the false abstraction to make the + # code reuse clean from the false assumption that we only + # parse whitelist config with + # devspec.PciAddressSpec._set_pci_dev_info() + str(e).replace( + 'Invalid PCI devices Whitelist config:', 'The')) + self._set_hvdevs(devices) @staticmethod diff --git a/nova/tests/unit/pci/test_manager.py b/nova/tests/unit/pci/test_manager.py index 683f3079299..c1b26d9726c 100644 --- a/nova/tests/unit/pci/test_manager.py +++ b/nova/tests/unit/pci/test_manager.py @@ -22,7 +22,6 @@ import nova from nova.compute import vm_states from nova import context -from nova import exception from nova import objects from nova.objects import fields from nova.pci import manager @@ -237,7 +236,9 @@ def test_update_devices_from_hypervisor_resources(self, _mock_dev_assign): tracker.update_devices_from_hypervisor_resources(fake_pci_devs_json) self.assertEqual(2, len(tracker.pci_devs)) - def test_update_devices_from_hypervisor_resources_32bit_domain(self): + @mock.patch("nova.pci.manager.LOG.debug") + def test_update_devices_from_hypervisor_resources_32bit_domain( + self, mock_debug): self.flags( group='pci', passthrough_whitelist=[ @@ -261,17 +262,13 @@ def test_update_devices_from_hypervisor_resources_32bit_domain(self): fake_pci_devs_json = jsonutils.dumps(fake_pci_devs) tracker = manager.PciDevTracker(self.fake_context) # We expect that the device with 32bit PCI domain is ignored - # tracker.update_devices_from_hypervisor_resources(fake_pci_devs_json) - # self.assertEqual(0, len(tracker.pci_devs)) - # - # This is the bug 1897528 - ex = self.assertRaises( - exception.PciConfigInvalidWhitelist, - tracker.update_devices_from_hypervisor_resources, - fake_pci_devs_json) - self.assertEqual( - 'Invalid PCI devices Whitelist config: property domain (10000) is ' - 'greater than the maximum allowable value (FFFF).', str(ex)) + tracker.update_devices_from_hypervisor_resources(fake_pci_devs_json) + self.assertEqual(0, len(tracker.pci_devs)) + mock_debug.assert_called_once_with( + 'Skipping PCI device %s reported by the hypervisor: %s', + {'address': '10000:00:02.0', 'parent_addr': None}, + 'The property domain (10000) is greater than the maximum ' + 'allowable value (FFFF).') def test_set_hvdev_new_dev(self): fake_pci_3 = dict(fake_pci, address='0000:00:00.4', vendor_id='v2') From 6b70350bdcf59a9712f88b6435ba2c6500133e5b Mon Sep 17 00:00:00 2001 From: melanie witt Date: Thu, 13 May 2021 05:43:42 +0000 Subject: [PATCH 09/40] Reject open redirection in the console proxy Our console proxies (novnc, serial, spice) run in a websockify server whose request handler inherits from the python standard SimpleHTTPRequestHandler. There is a known issue [1] in the SimpleHTTPRequestHandler which allows open redirects by way of URLs in the following format: http://vncproxy.my.domain.com//example.com/%2F.. which if visited, will redirect a user to example.com. We can intercept a request and reject requests that pass a redirection URL beginning with "//" by implementing the SimpleHTTPRequestHandler.send_head() method containing the vulnerability to reject such requests with a 400 Bad Request. This code is copied from a patch suggested in one of the issue comments [2]. Closes-Bug: #1927677 [1] https://bugs.python.org/issue32084 [2] https://bugs.python.org/issue32084#msg306545 Conflicts: nova/console/websocketproxy.py nova/tests/unit/console/test_websocketproxy.py NOTE(melwitt): The conflicts are because the following changes are not in Victoria: Ib2c406327fef2fb4868d8050fc476a7d17706e23 (Remove six.moves) I58b0382c86d4ef798572edb63d311e0e3e6937bb (Refactor and rename test_tcp_rst_no_compute_rpcapi) Change-Id: Ie36401c782f023d1d5f2623732619105dc2cfa24 (cherry picked from commit 781612b33282ed298f742c85dab58a075c8b793e) (cherry picked from commit 470925614223c8dd9b1233f54f5a96c02b2d4f70) --- nova/console/websocketproxy.py | 23 +++++++++++++ .../tests/unit/console/test_websocketproxy.py | 34 +++++++++++++++++++ ...reject-open-redirect-4ac0a7895acca7eb.yaml | 19 +++++++++++ 3 files changed, 76 insertions(+) create mode 100644 releasenotes/notes/console-proxy-reject-open-redirect-4ac0a7895acca7eb.yaml diff --git a/nova/console/websocketproxy.py b/nova/console/websocketproxy.py index 5f2985dfbbc..1410d642608 100644 --- a/nova/console/websocketproxy.py +++ b/nova/console/websocketproxy.py @@ -19,6 +19,8 @@ ''' import copy +from http import HTTPStatus +import os import socket import sys @@ -281,6 +283,27 @@ def new_websocket_client(self): def socket(self, *args, **kwargs): return websockifyserver.WebSockifyServer.socket(*args, **kwargs) + def send_head(self): + # This code is copied from this example patch: + # https://bugs.python.org/issue32084#msg306545 + path = self.translate_path(self.path) + if os.path.isdir(path): + parts = urlparse.urlsplit(self.path) + if not parts.path.endswith('/'): + # redirect browser - doing basically what apache does + new_parts = (parts[0], parts[1], parts[2] + '/', + parts[3], parts[4]) + new_url = urlparse.urlunsplit(new_parts) + + # Browsers interpret "Location: //uri" as an absolute URI + # like "http://URI" + if new_url.startswith('//'): + self.send_error(HTTPStatus.BAD_REQUEST, + "URI must not start with //") + return None + + return super(NovaProxyRequestHandler, self).send_head() + class NovaWebSocketProxy(websockify.WebSocketProxy): def __init__(self, *args, **kwargs): diff --git a/nova/tests/unit/console/test_websocketproxy.py b/nova/tests/unit/console/test_websocketproxy.py index 3c234df891c..4ed2d2d4dc0 100644 --- a/nova/tests/unit/console/test_websocketproxy.py +++ b/nova/tests/unit/console/test_websocketproxy.py @@ -626,6 +626,40 @@ def test_tcp_rst_no_compute_rpcapi(self): self.wh.server.top_new_client(conn, address) self.assertIsNone(self.wh._compute_rpcapi) + def test_reject_open_redirect(self): + # This will test the behavior when an attempt is made to cause an open + # redirect. It should be rejected. + mock_req = mock.MagicMock() + mock_req.makefile().readline.side_effect = [ + b'GET //example.com/%2F.. HTTP/1.1\r\n', + b'' + ] + + # Collect the response data to verify at the end. The + # SimpleHTTPRequestHandler writes the response data by calling the + # request socket sendall() method. + self.data = b'' + + def fake_sendall(data): + self.data += data + + mock_req.sendall.side_effect = fake_sendall + + client_addr = ('8.8.8.8', 54321) + mock_server = mock.MagicMock() + # This specifies that the server will be able to handle requests other + # than only websockets. + mock_server.only_upgrade = False + + # Constructing a handler will process the mock_req request passed in. + websocketproxy.NovaProxyRequestHandler( + mock_req, client_addr, mock_server) + + # Verify no redirect happens and instead a 400 Bad Request is returned. + self.data = self.data.decode() + self.assertIn('Error code: 400', self.data) + self.assertIn('Message: URI must not start with //', self.data) + @mock.patch('websockify.websocketproxy.select_ssl_version') def test_ssl_min_version_is_not_set(self, mock_select_ssl): websocketproxy.NovaWebSocketProxy() diff --git a/releasenotes/notes/console-proxy-reject-open-redirect-4ac0a7895acca7eb.yaml b/releasenotes/notes/console-proxy-reject-open-redirect-4ac0a7895acca7eb.yaml new file mode 100644 index 00000000000..ce05b9a8670 --- /dev/null +++ b/releasenotes/notes/console-proxy-reject-open-redirect-4ac0a7895acca7eb.yaml @@ -0,0 +1,19 @@ +--- +security: + - | + A vulnerability in the console proxies (novnc, serial, spice) that allowed + open redirection has been `patched`_. The novnc, serial, and spice console + proxies are implemented as websockify servers and the request handler + inherits from the python standard SimpleHTTPRequestHandler. There is a + `known issue`_ in the SimpleHTTPRequestHandler which allows open redirects + by way of URLs in the following format:: + + http://vncproxy.my.domain.com//example.com/%2F.. + + which if visited, will redirect a user to example.com. + + The novnc, serial, and spice console proxies will now reject requests that + pass a redirection URL beginning with "//" with a 400 Bad Request. + + .. _patched: https://bugs.launchpad.net/nova/+bug/1927677 + .. _known issue: https://bugs.python.org/issue32084 From 04057099991579bccfb2accb6b0952ba589fe092 Mon Sep 17 00:00:00 2001 From: melanie witt Date: Mon, 10 May 2021 17:31:25 +0000 Subject: [PATCH 10/40] rbd: Get rbd_utils unit tests running again Awhile back, change I25baf5edd25d9e551686b7ed317a63fd778be533 moved rbd_utils out from the libvirt driver and into a central location under nova/storage. This move missed adding a __init__.py file under nova/tests/unit/storage, so unit test discovery wasn't picking up the rbd_utils tests and couldn't run them. This adds a __init__.py file under nova/tests/unit/storage to get the tests running again. This also fixes a small bug introduced by change I3032bbe6bd2d6acc9ba0f0cac4d00ed4b4464ceb in RbdTestCase.setUp() that passed nonexistent self.images_rbd_pool to self.flags. It should be self.rbd_pool. Closes-Bug: #1928007 Change-Id: Ic03a5336abdced883f62f395690c0feac12075c8 (cherry picked from commit 8b647f1b3f56879be221b3925570790a1e0e77f8) (cherry picked from commit 8f018d754d5c55e432cd51df99278382b527283e) --- nova/tests/unit/storage/__init__.py | 0 nova/tests/unit/storage/test_rbd.py | 2 +- 2 files changed, 1 insertion(+), 1 deletion(-) create mode 100644 nova/tests/unit/storage/__init__.py diff --git a/nova/tests/unit/storage/__init__.py b/nova/tests/unit/storage/__init__.py new file mode 100644 index 00000000000..e69de29bb2d diff --git a/nova/tests/unit/storage/test_rbd.py b/nova/tests/unit/storage/test_rbd.py index 5a39bdbd5a4..f0b3f705320 100644 --- a/nova/tests/unit/storage/test_rbd.py +++ b/nova/tests/unit/storage/test_rbd.py @@ -110,7 +110,7 @@ def setUp(self): self.rbd_pool = 'rbd' self.rbd_connect_timeout = 5 self.flags( - images_rbd_pool=self.images_rbd_pool, + images_rbd_pool=self.rbd_pool, images_rbd_ceph_conf='/foo/bar.conf', rbd_connect_timeout=self.rbd_connect_timeout, rbd_user='foo', group='libvirt') From 2af08fb5ead8ca1fa4d6b8ea00f3c5c3d26e562c Mon Sep 17 00:00:00 2001 From: Lee Yarwood Date: Fri, 5 Mar 2021 11:33:29 +0000 Subject: [PATCH 11/40] zuul: Replace grenade and nova-grenade-multinode with grenade-multinode If2608406776e0d5a06b726e65b55881e70562d18 dropped the single node grenade job from the integrated-gate-compute template as it duplicates the existing grenade-multinode job. However it doesn't remove the remianing single node grenade job still present in the Nova project. This change replaces the dsvm based nova-grenade-multinode job with the zuulv3 native grenade-multinode based job. Various legacy playbooks and hook scripts are also removed as they are no longer used. Note that this does result in a loss of coverage for ceph that should be replaced as soon as a zuulv3 native ceph based multinode job is available. Change-Id: I02b2b851a74f24816d2f782a66d94de81ee527b0 (cherry picked from commit 91e53e4c2b90ea57aeac4ec522dd7c8c54961d09) (cherry picked from commit c45bedd98d50af865d727b7456c974c8e27bff8b) --- .zuul.yaml | 32 +-- gate/live_migration/hooks/ceph.sh | 208 ------------------ gate/live_migration/hooks/nfs.sh | 50 ----- gate/live_migration/hooks/run_tests.sh | 72 ------ gate/live_migration/hooks/utils.sh | 11 - .../legacy/nova-grenade-multinode/post.yaml | 15 -- .../legacy/nova-grenade-multinode/run.yaml | 65 ------ .../legacy/nova-live-migration/post.yaml | 15 -- playbooks/legacy/nova-live-migration/run.yaml | 60 ----- 9 files changed, 16 insertions(+), 512 deletions(-) delete mode 100755 gate/live_migration/hooks/ceph.sh delete mode 100755 gate/live_migration/hooks/nfs.sh delete mode 100755 gate/live_migration/hooks/run_tests.sh delete mode 100755 gate/live_migration/hooks/utils.sh delete mode 100644 playbooks/legacy/nova-grenade-multinode/post.yaml delete mode 100644 playbooks/legacy/nova-grenade-multinode/run.yaml delete mode 100644 playbooks/legacy/nova-live-migration/post.yaml delete mode 100644 playbooks/legacy/nova-live-migration/run.yaml diff --git a/.zuul.yaml b/.zuul.yaml index 785522e964e..2c887b4d841 100644 --- a/.zuul.yaml +++ b/.zuul.yaml @@ -253,22 +253,24 @@ - job: name: nova-grenade-multinode - parent: nova-dsvm-multinode-base + parent: grenade-multinode description: | - Multi-node grenade job which runs gate/live_migration/hooks tests under - python 3. - In other words, this tests live and cold migration and resize with - mixed-version compute services which is important for things like - rolling upgrade support. + Run a multinode grenade job and run the smoke, cold and live migration + tests with the controller upgraded and the compute on the older release. The former names for this job were "nova-grenade-live-migration" and "legacy-grenade-dsvm-neutron-multinode-live-migration". - run: playbooks/legacy/nova-grenade-multinode/run.yaml - post-run: playbooks/legacy/nova-grenade-multinode/post.yaml - required-projects: - - openstack/grenade - - openstack/devstack-gate - - openstack/nova irrelevant-files: *dsvm-irrelevant-files + vars: + devstack_local_conf: + test-config: + $TEMPEST_CONFIG: + compute-feature-enabled: + live_migration: true + volume_backed_live_migration: true + block_migration_for_live_migration: true + block_migrate_cinder_iscsi: true + tox_envlist: all + tempest_test_regex: ((tempest\.(api\.compute|scenario)\..*smoke.*)|(^tempest\.api\.compute\.admin\.(test_live_migration|test_migration))) - job: name: nova-multi-cell @@ -418,7 +420,6 @@ # so that we only run it on changes to networking and libvirt/vif # code; we don't need to run this on all changes. - ^(?!nova/network/.*)(?!nova/virt/libvirt/vif.py).*$ - - nova-grenade-multinode - nova-live-migration - nova-lvm - nova-multi-cell @@ -443,7 +444,7 @@ - ^setup.cfg$ - ^tools/.*$ - ^tox.ini$ - - grenade: + - nova-grenade-multinode: irrelevant-files: *policies-irrelevant-files - tempest-ipv6-only: irrelevant-files: *dsvm-irrelevant-files @@ -457,7 +458,6 @@ voting: false gate: jobs: - - nova-grenade-multinode - nova-live-migration - nova-tox-functional-py38 - nova-multi-cell @@ -472,7 +472,7 @@ - ^(?!nova/network/.*)(?!nova/virt/libvirt/vif.py).*$ - tempest-integrated-compute: irrelevant-files: *policies-irrelevant-files - - grenade: + - nova-grenade-multinode: irrelevant-files: *policies-irrelevant-files - tempest-ipv6-only: irrelevant-files: *dsvm-irrelevant-files diff --git a/gate/live_migration/hooks/ceph.sh b/gate/live_migration/hooks/ceph.sh deleted file mode 100755 index 3d596ff0b31..00000000000 --- a/gate/live_migration/hooks/ceph.sh +++ /dev/null @@ -1,208 +0,0 @@ -#!/bin/bash - -function prepare_ceph { - git clone https://opendev.org/openstack/devstack-plugin-ceph /tmp/devstack-plugin-ceph - source /tmp/devstack-plugin-ceph/devstack/settings - source /tmp/devstack-plugin-ceph/devstack/lib/ceph - install_ceph - configure_ceph - #install ceph-common package and additional python3 ceph libraries on compute nodes - $ANSIBLE subnodes --become -f 5 -i "$WORKSPACE/inventory" -m raw -a "executable=/bin/bash - USE_PYTHON3=${USE_PYTHON3:-True} - source $BASE/new/devstack/functions - source $BASE/new/devstack/functions-common - git clone https://opendev.org/openstack/devstack-plugin-ceph /tmp/devstack-plugin-ceph - source /tmp/devstack-plugin-ceph/devstack/lib/ceph - install_ceph_remote - " - - #copy ceph admin keyring to compute nodes - sudo cp /etc/ceph/ceph.client.admin.keyring /tmp/ceph.client.admin.keyring - sudo chown ${STACK_USER}:${STACK_USER} /tmp/ceph.client.admin.keyring - sudo chmod 644 /tmp/ceph.client.admin.keyring - $ANSIBLE subnodes --become -f 5 -i "$WORKSPACE/inventory" -m copy -a "src=/tmp/ceph.client.admin.keyring dest=/etc/ceph/ceph.client.admin.keyring owner=ceph group=ceph" - sudo rm -f /tmp/ceph.client.admin.keyring - #copy ceph.conf to compute nodes - $ANSIBLE subnodes --become -f 5 -i "$WORKSPACE/inventory" -m copy -a "src=/etc/ceph/ceph.conf dest=/etc/ceph/ceph.conf owner=root group=root" - - start_ceph -} - -function _ceph_configure_glance { - GLANCE_API_CONF=${GLANCE_API_CONF:-/etc/glance/glance-api.conf} - sudo ceph -c ${CEPH_CONF_FILE} osd pool create ${GLANCE_CEPH_POOL} ${GLANCE_CEPH_POOL_PG} ${GLANCE_CEPH_POOL_PGP} - sudo ceph -c ${CEPH_CONF_FILE} auth get-or-create client.${GLANCE_CEPH_USER} \ - mon "allow r" \ - osd "allow class-read object_prefix rbd_children, allow rwx pool=${GLANCE_CEPH_POOL}" | \ - sudo tee ${CEPH_CONF_DIR}/ceph.client.${GLANCE_CEPH_USER}.keyring - sudo chown ${STACK_USER}:$(id -g -n $whoami) ${CEPH_CONF_DIR}/ceph.client.${GLANCE_CEPH_USER}.keyring - - $ANSIBLE primary --become -f 5 -i "$WORKSPACE/inventory" -m ini_file -a "dest=${GLANCE_API_CONF} section=DEFAULT option=show_image_direct_url value=True" - $ANSIBLE primary --become -f 5 -i "$WORKSPACE/inventory" -m ini_file -a "dest=${GLANCE_API_CONF} section=glance_store option=default_store value=rbd" - $ANSIBLE primary --become -f 5 -i "$WORKSPACE/inventory" -m ini_file -a "dest=${GLANCE_API_CONF} section=glance_store option=stores value='file, http, rbd'" - $ANSIBLE primary --become -f 5 -i "$WORKSPACE/inventory" -m ini_file -a "dest=${GLANCE_API_CONF} section=glance_store option=rbd_store_ceph_conf value=$CEPH_CONF_FILE" - $ANSIBLE primary --become -f 5 -i "$WORKSPACE/inventory" -m ini_file -a "dest=${GLANCE_API_CONF} section=glance_store option=rbd_store_user value=$GLANCE_CEPH_USER" - $ANSIBLE primary --become -f 5 -i "$WORKSPACE/inventory" -m ini_file -a "dest=${GLANCE_API_CONF} section=glance_store option=rbd_store_pool value=$GLANCE_CEPH_POOL" - - sudo ceph -c ${CEPH_CONF_FILE} osd pool set ${GLANCE_CEPH_POOL} size ${CEPH_REPLICAS} - if [[ $CEPH_REPLICAS -ne 1 ]]; then - sudo ceph -c ${CEPH_CONF_FILE} osd pool set ${GLANCE_CEPH_POOL} crush_ruleset ${RULE_ID} - fi - - #copy glance keyring to compute only node - sudo cp /etc/ceph/ceph.client.glance.keyring /tmp/ceph.client.glance.keyring - sudo chown $STACK_USER:$STACK_USER /tmp/ceph.client.glance.keyring - $ANSIBLE subnodes --become -f 5 -i "$WORKSPACE/inventory" -m copy -a "src=/tmp/ceph.client.glance.keyring dest=/etc/ceph/ceph.client.glance.keyring" - sudo rm -f /tmp/ceph.client.glance.keyring -} - -function configure_and_start_glance { - _ceph_configure_glance - echo 'check processes before glance-api stop' - $ANSIBLE primary --become -f 5 -i "$WORKSPACE/inventory" -m shell -a "ps aux | grep glance-api" - - # restart glance - $ANSIBLE primary --become -f 5 -i "$WORKSPACE/inventory" -m shell -a "systemctl restart devstack@g-api" - - echo 'check processes after glance-api stop' - $ANSIBLE primary --become -f 5 -i "$WORKSPACE/inventory" -m shell -a "ps aux | grep glance-api" -} - -function _ceph_configure_nova { - #setup ceph for nova, we don't reuse configure_ceph_nova - as we need to emulate case where cinder is not configured for ceph - sudo ceph -c ${CEPH_CONF_FILE} osd pool create ${NOVA_CEPH_POOL} ${NOVA_CEPH_POOL_PG} ${NOVA_CEPH_POOL_PGP} - NOVA_CONF=${NOVA_CPU_CONF:-/etc/nova/nova.conf} - $ANSIBLE all --become -f 5 -i "$WORKSPACE/inventory" -m ini_file -a "dest=${NOVA_CONF} section=libvirt option=rbd_user value=${CINDER_CEPH_USER}" - $ANSIBLE all --become -f 5 -i "$WORKSPACE/inventory" -m ini_file -a "dest=${NOVA_CONF} section=libvirt option=rbd_secret_uuid value=${CINDER_CEPH_UUID}" - $ANSIBLE all --become -f 5 -i "$WORKSPACE/inventory" -m ini_file -a "dest=${NOVA_CONF} section=libvirt option=inject_key value=false" - $ANSIBLE all --become -f 5 -i "$WORKSPACE/inventory" -m ini_file -a "dest=${NOVA_CONF} section=libvirt option=inject_partition value=-2" - $ANSIBLE all --become -f 5 -i "$WORKSPACE/inventory" -m ini_file -a "dest=${NOVA_CONF} section=libvirt option=disk_cachemodes value='network=writeback'" - $ANSIBLE all --become -f 5 -i "$WORKSPACE/inventory" -m ini_file -a "dest=${NOVA_CONF} section=libvirt option=images_type value=rbd" - $ANSIBLE all --become -f 5 -i "$WORKSPACE/inventory" -m ini_file -a "dest=${NOVA_CONF} section=libvirt option=images_rbd_pool value=${NOVA_CEPH_POOL}" - $ANSIBLE all --become -f 5 -i "$WORKSPACE/inventory" -m ini_file -a "dest=${NOVA_CONF} section=libvirt option=images_rbd_ceph_conf value=${CEPH_CONF_FILE}" - - sudo ceph -c ${CEPH_CONF_FILE} auth get-or-create client.${CINDER_CEPH_USER} \ - mon "allow r" \ - osd "allow class-read object_prefix rbd_children, allow rwx pool=${CINDER_CEPH_POOL}, allow rwx pool=${NOVA_CEPH_POOL},allow rwx pool=${GLANCE_CEPH_POOL}" | \ - sudo tee ${CEPH_CONF_DIR}/ceph.client.${CINDER_CEPH_USER}.keyring > /dev/null - sudo chown ${STACK_USER}:$(id -g -n $whoami) ${CEPH_CONF_DIR}/ceph.client.${CINDER_CEPH_USER}.keyring - - #copy cinder keyring to compute only node - sudo cp /etc/ceph/ceph.client.cinder.keyring /tmp/ceph.client.cinder.keyring - sudo chown stack:stack /tmp/ceph.client.cinder.keyring - $ANSIBLE subnodes --become -f 5 -i "$WORKSPACE/inventory" -m copy -a "src=/tmp/ceph.client.cinder.keyring dest=/etc/ceph/ceph.client.cinder.keyring" - sudo rm -f /tmp/ceph.client.cinder.keyring - - sudo ceph -c ${CEPH_CONF_FILE} osd pool set ${NOVA_CEPH_POOL} size ${CEPH_REPLICAS} - if [[ $CEPH_REPLICAS -ne 1 ]]; then - sudo ceph -c ${CEPH_CONF_FILE} osd pool set ${NOVA_CEPH_POOL} crush_ruleset ${RULE_ID} - fi -} - -function _wait_for_nova_compute_service_state { - source $BASE/new/devstack/openrc admin admin - local status=$1 - local attempt=1 - local max_attempts=24 - local attempt_sleep=5 - local computes_count=$(openstack compute service list | grep -c nova-compute) - local computes_ready=$(openstack compute service list | grep nova-compute | grep $status | wc -l) - - echo "Waiting for $computes_count computes to report as $status" - while [ "$computes_ready" -ne "$computes_count" ]; do - if [ "$attempt" -eq "$max_attempts" ]; then - echo "Failed waiting for computes to report as ${status}, ${computes_ready}/${computes_count} ${status} after ${max_attempts} attempts" - exit 4 - fi - echo "Waiting ${attempt_sleep} seconds for ${computes_count} computes to report as ${status}, ${computes_ready}/${computes_count} ${status} after ${attempt}/${max_attempts} attempts" - sleep $attempt_sleep - attempt=$((attempt+1)) - computes_ready=$(openstack compute service list | grep nova-compute | grep $status | wc -l) - done - echo "All computes are now reporting as ${status} after ${attempt} attempts" -} - -function configure_and_start_nova { - - echo "Checking all n-cpu services" - $ANSIBLE all --become -f 5 -i "$WORKSPACE/inventory" -m shell -a "pgrep -u stack -a nova-compute" - - # stop nova-compute - echo "Stopping all n-cpu services" - $ANSIBLE all --become -f 5 -i "$WORKSPACE/inventory" -m shell -a "systemctl stop devstack@n-cpu" - - # Wait for the service to be marked as down - _wait_for_nova_compute_service_state "down" - - _ceph_configure_nova - - #import secret to libvirt - _populate_libvirt_secret - - # start nova-compute - echo "Starting all n-cpu services" - $ANSIBLE all --become -f 5 -i "$WORKSPACE/inventory" -m shell -a "systemctl start devstack@n-cpu" - - echo "Checking all n-cpu services" - # test that they are all running again - $ANSIBLE all --become -f 5 -i "$WORKSPACE/inventory" -m shell -a "pgrep -u stack -a nova-compute" - - # Wait for the service to be marked as up - _wait_for_nova_compute_service_state "up" -} - -function _ceph_configure_cinder { - sudo ceph -c ${CEPH_CONF_FILE} osd pool create ${CINDER_CEPH_POOL} ${CINDER_CEPH_POOL_PG} ${CINDER_CEPH_POOL_PGP} - sudo ceph -c ${CEPH_CONF_FILE} osd pool set ${CINDER_CEPH_POOL} size ${CEPH_REPLICAS} - if [[ $CEPH_REPLICAS -ne 1 ]]; then - sudo ceph -c ${CEPH_CONF_FILE} osd pool set ${CINDER_CEPH_POOL} crush_ruleset ${RULE_ID} - fi - - CINDER_CONF=${CINDER_CONF:-/etc/cinder/cinder.conf} - $ANSIBLE primary --become -f 5 -i "$WORKSPACE/inventory" -m ini_file -a "dest=$CINDER_CONF section=ceph option=volume_backend_name value=ceph" - $ANSIBLE primary --become -f 5 -i "$WORKSPACE/inventory" -m ini_file -a "dest=$CINDER_CONF section=ceph option=volume_driver value=cinder.volume.drivers.rbd.RBDDriver" - $ANSIBLE primary --become -f 5 -i "$WORKSPACE/inventory" -m ini_file -a "dest=$CINDER_CONF section=ceph option=rbd_ceph_conf value=$CEPH_CONF_FILE" - $ANSIBLE primary --become -f 5 -i "$WORKSPACE/inventory" -m ini_file -a "dest=$CINDER_CONF section=ceph option=rbd_pool value=$CINDER_CEPH_POOL" - $ANSIBLE primary --become -f 5 -i "$WORKSPACE/inventory" -m ini_file -a "dest=$CINDER_CONF section=ceph option=rbd_user value=$CINDER_CEPH_USER" - $ANSIBLE primary --become -f 5 -i "$WORKSPACE/inventory" -m ini_file -a "dest=$CINDER_CONF section=ceph option=rbd_uuid value=$CINDER_CEPH_UUID" - $ANSIBLE primary --become -f 5 -i "$WORKSPACE/inventory" -m ini_file -a "dest=$CINDER_CONF section=ceph option=rbd_flatten_volume_from_snapshot value=False" - $ANSIBLE primary --become -f 5 -i "$WORKSPACE/inventory" -m ini_file -a "dest=$CINDER_CONF section=ceph option=rbd_max_clone_depth value=5" - $ANSIBLE primary --become -f 5 -i "$WORKSPACE/inventory" -m ini_file -a "dest=$CINDER_CONF section=DEFAULT option=default_volume_type value=ceph" - $ANSIBLE primary --become -f 5 -i "$WORKSPACE/inventory" -m ini_file -a "dest=$CINDER_CONF section=DEFAULT option=enabled_backends value=ceph" - -} - -function configure_and_start_cinder { - _ceph_configure_cinder - - # restart cinder - $ANSIBLE primary --become -f 5 -i "$WORKSPACE/inventory" -m shell -a "systemctl restart devstack@c-vol" - - source $BASE/new/devstack/openrc - - export OS_USERNAME=admin - export OS_PROJECT_NAME=admin - lvm_type=$(cinder type-list | awk -F "|" 'NR==4{ print $2}') - cinder type-delete $lvm_type - openstack volume type create --os-volume-api-version 1 --property volume_backend_name="ceph" ceph -} - -function _populate_libvirt_secret { - cat > /tmp/secret.xml < - ${CINDER_CEPH_UUID} - - client.${CINDER_CEPH_USER} secret - - -EOF - - $ANSIBLE subnodes --become -f 5 -i "$WORKSPACE/inventory" -m copy -a "src=/tmp/secret.xml dest=/tmp/secret.xml" - $ANSIBLE all --become -f 5 -i "$WORKSPACE/inventory" -m shell -a "virsh secret-define --file /tmp/secret.xml" - local secret=$(sudo ceph -c ${CEPH_CONF_FILE} auth get-key client.${CINDER_CEPH_USER}) - # TODO(tdurakov): remove this escaping as https://github.com/ansible/ansible/issues/13862 fixed - secret=${secret//=/'\='} - $ANSIBLE all --become -f 5 -i "$WORKSPACE/inventory" -m shell -a "virsh secret-set-value --secret ${CINDER_CEPH_UUID} --base64 $secret" - $ANSIBLE all --become -f 5 -i "$WORKSPACE/inventory" -m file -a "path=/tmp/secret.xml state=absent" - -} diff --git a/gate/live_migration/hooks/nfs.sh b/gate/live_migration/hooks/nfs.sh deleted file mode 100755 index acadb36d6ca..00000000000 --- a/gate/live_migration/hooks/nfs.sh +++ /dev/null @@ -1,50 +0,0 @@ -#!/bin/bash - -function nfs_setup { - if uses_debs; then - module=apt - elif is_fedora; then - module=yum - fi - $ANSIBLE all --become -f 5 -i "$WORKSPACE/inventory" -m $module \ - -a "name=nfs-common state=present" - $ANSIBLE primary --become -f 5 -i "$WORKSPACE/inventory" -m $module \ - -a "name=nfs-kernel-server state=present" - - $ANSIBLE primary --become -f 5 -i "$WORKSPACE/inventory" -m ini_file -a "dest=/etc/idmapd.conf section=Mapping option=Nobody-User value=nova" - - $ANSIBLE primary --become -f 5 -i "$WORKSPACE/inventory" -m ini_file -a "dest=/etc/idmapd.conf section=Mapping option=Nobody-Group value=nova" - - for SUBNODE in $SUBNODES ; do - $ANSIBLE primary --become -f 5 -i "$WORKSPACE/inventory" -m lineinfile -a "dest=/etc/exports line='/opt/stack/data/nova/instances $SUBNODE(rw,fsid=0,insecure,no_subtree_check,async,no_root_squash)'" - done - - $ANSIBLE primary --become -f 5 -i "$WORKSPACE/inventory" -m shell -a "exportfs -a" - $ANSIBLE primary --become -f 5 -i "$WORKSPACE/inventory" -m service -a "name=nfs-kernel-server state=restarted" - GetDistro - if [[ ! ${DISTRO} =~ (xenial) ]]; then - $ANSIBLE primary --become -f 5 -i "$WORKSPACE/inventory" -m service -a "name=idmapd state=restarted" - fi - $ANSIBLE primary --become -f 5 -i "$WORKSPACE/inventory" -m shell -a "iptables -A INPUT -p tcp --dport 111 -j ACCEPT" - $ANSIBLE primary --become -f 5 -i "$WORKSPACE/inventory" -m shell -a "iptables -A INPUT -p udp --dport 111 -j ACCEPT" - $ANSIBLE primary --become -f 5 -i "$WORKSPACE/inventory" -m shell -a "iptables -A INPUT -p tcp --dport 2049 -j ACCEPT" - $ANSIBLE primary --become -f 5 -i "$WORKSPACE/inventory" -m shell -a "iptables -A INPUT -p udp --dport 2049 -j ACCEPT" - $ANSIBLE subnodes --become -f 5 -i "$WORKSPACE/inventory" -m shell -a "mount -t nfs4 -o proto\=tcp,port\=2049 $primary_node:/ /opt/stack/data/nova/instances/" -} - -function nfs_configure_tempest { - $ANSIBLE primary --become -f 5 -i "$WORKSPACE/inventory" -m ini_file -a "dest=$BASE/new/tempest/etc/tempest.conf section=compute-feature-enabled option=block_migration_for_live_migration value=False" -} - -function nfs_verify_setup { - $ANSIBLE subnodes --become -f 5 -i "$WORKSPACE/inventory" -m file -a "path=/opt/stack/data/nova/instances/test_file state=touch" - if [ ! -e '/opt/stack/data/nova/instances/test_file' ]; then - die $LINENO "NFS configuration failure" - fi -} - -function nfs_teardown { - #teardown nfs shared storage - $ANSIBLE subnodes --become -f 5 -i "$WORKSPACE/inventory" -m shell -a "umount -t nfs4 /opt/stack/data/nova/instances/" - $ANSIBLE primary --become -f 5 -i "$WORKSPACE/inventory" -m service -a "name=nfs-kernel-server state=stopped" -} \ No newline at end of file diff --git a/gate/live_migration/hooks/run_tests.sh b/gate/live_migration/hooks/run_tests.sh deleted file mode 100755 index 8334df633dd..00000000000 --- a/gate/live_migration/hooks/run_tests.sh +++ /dev/null @@ -1,72 +0,0 @@ -#!/bin/bash -# Live migration dedicated ci job will be responsible for testing different -# environments based on underlying storage, used for ephemerals. -# This hook allows to inject logic of environment reconfiguration in ci job. -# Base scenario for this would be: -# -# 1. test with all local storage (use default for volumes) -# 2. test with NFS for root + ephemeral disks -# 3. test with Ceph for root + ephemeral disks -# 4. test with Ceph for volumes and root + ephemeral disk - -set -xe -cd $BASE/new/tempest - -source $BASE/new/devstack/functions -source $BASE/new/devstack/functions-common -source $BASE/new/devstack/lib/nova -source $WORKSPACE/devstack-gate/functions.sh -source $BASE/new/nova/gate/live_migration/hooks/utils.sh -source $BASE/new/nova/gate/live_migration/hooks/nfs.sh -source $BASE/new/nova/gate/live_migration/hooks/ceph.sh -primary_node=$(cat /etc/nodepool/primary_node_private) -SUBNODES=$(cat /etc/nodepool/sub_nodes_private) -SERVICE_HOST=$primary_node -STACK_USER=${STACK_USER:-stack} - -echo '1. test with all local storage (use default for volumes)' -echo 'NOTE: test_volume_backed_live_migration is skipped due to https://bugs.launchpad.net/nova/+bug/1524898' -echo 'NOTE: test_live_block_migration_paused is skipped due to https://bugs.launchpad.net/nova/+bug/1901739' -run_tempest "block migration test" "^.*test_live_migration(?!.*(test_volume_backed_live_migration|test_live_block_migration_paused))" - -# TODO(mriedem): Run $BASE/new/nova/gate/test_evacuate.sh for local storage - -#all tests bellow this line use shared storage, need to update tempest.conf -echo 'disabling block_migration in tempest' -$ANSIBLE primary --become -f 5 -i "$WORKSPACE/inventory" -m ini_file -a "dest=$BASE/new/tempest/etc/tempest.conf section=compute-feature-enabled option=block_migration_for_live_migration value=False" - -echo '2. NFS testing is skipped due to setup failures with Ubuntu 16.04' -#echo '2. test with NFS for root + ephemeral disks' - -#nfs_setup -#nfs_configure_tempest -#nfs_verify_setup -#run_tempest "NFS shared storage test" "live_migration" -#nfs_teardown - -# The nova-grenade-multinode job also runs resize and cold migration tests -# so we check for a grenade-only variable. -if [[ -n "$GRENADE_NEW_BRANCH" ]]; then - echo '3. test cold migration and resize' - run_tempest "cold migration and resize test" "test_resize_server|test_cold_migration|test_revert_cold_migration" -else - echo '3. cold migration and resize is skipped for non-grenade jobs' -fi - -echo '4. test with Ceph for root + ephemeral disks' -# Discover and set variables for the OS version so the devstack-plugin-ceph -# scripts can find the correct repository to install the ceph packages. -GetOSVersion -USE_PYTHON3=${USE_PYTHON3:-True} -prepare_ceph -GLANCE_API_CONF=${GLANCE_API_CONF:-/etc/glance/glance-api.conf} -configure_and_start_glance - -configure_and_start_nova -run_tempest "Ceph nova&glance test" "^.*test_live_migration(?!.*(test_volume_backed_live_migration))" - -set +e -#echo '5. test with Ceph for volumes and root + ephemeral disk' - -#configure_and_start_cinder -#run_tempest "Ceph nova&glance&cinder test" "live_migration" diff --git a/gate/live_migration/hooks/utils.sh b/gate/live_migration/hooks/utils.sh deleted file mode 100755 index 9f98ca2e254..00000000000 --- a/gate/live_migration/hooks/utils.sh +++ /dev/null @@ -1,11 +0,0 @@ -#!/bin/bash - -function run_tempest { - local message=$1 - local tempest_regex=$2 - sudo -H -u tempest tox -eall -- $tempest_regex --concurrency=$TEMPEST_CONCURRENCY - exitcode=$? - if [[ $exitcode -ne 0 ]]; then - die $LINENO "$message failure" - fi -} diff --git a/playbooks/legacy/nova-grenade-multinode/post.yaml b/playbooks/legacy/nova-grenade-multinode/post.yaml deleted file mode 100644 index e07f5510ae7..00000000000 --- a/playbooks/legacy/nova-grenade-multinode/post.yaml +++ /dev/null @@ -1,15 +0,0 @@ -- hosts: primary - tasks: - - - name: Copy files from {{ ansible_user_dir }}/workspace/ on node - synchronize: - src: '{{ ansible_user_dir }}/workspace/' - dest: '{{ zuul.executor.log_root }}' - mode: pull - copy_links: true - verify_host: true - rsync_opts: - - --include=/logs/** - - --include=*/ - - --exclude=* - - --prune-empty-dirs diff --git a/playbooks/legacy/nova-grenade-multinode/run.yaml b/playbooks/legacy/nova-grenade-multinode/run.yaml deleted file mode 100644 index 18f7c753ebc..00000000000 --- a/playbooks/legacy/nova-grenade-multinode/run.yaml +++ /dev/null @@ -1,65 +0,0 @@ -- hosts: primary - name: nova-grenade-multinode - tasks: - - - name: Ensure legacy workspace directory - file: - path: '{{ ansible_user_dir }}/workspace' - state: directory - - - shell: - cmd: | - set -e - set -x - cat > clonemap.yaml << EOF - clonemap: - - name: openstack/devstack-gate - dest: devstack-gate - EOF - /usr/zuul-env/bin/zuul-cloner -m clonemap.yaml --cache-dir /opt/git \ - https://opendev.org \ - openstack/devstack-gate - executable: /bin/bash - chdir: '{{ ansible_user_dir }}/workspace' - environment: '{{ zuul | zuul_legacy_vars }}' - - - shell: - cmd: | - set -e - set -x - export PROJECTS="openstack/grenade $PROJECTS" - export PYTHONUNBUFFERED=true - export DEVSTACK_GATE_CONFIGDRIVE=0 - export DEVSTACK_GATE_NEUTRON=1 - # NOTE(mriedem): Run tempest smoke tests specific to compute on the - # new side of the grenade environment. The post-test hook script will - # run non-smoke migration tests in a local/block and shared/ceph - # setup. Note that grenade hard-codes "tox -esmoke" for tempest on - # the old side so the regex is not appied there. - export DEVSTACK_GATE_TEMPEST=1 - export DEVSTACK_GATE_TEMPEST_REGEX="tempest\.(api\.compute|scenario)\..*smoke.*" - export DEVSTACK_GATE_GRENADE=pullup - export DEVSTACK_GATE_USE_PYTHON3=True - # By default grenade runs only smoke tests so we need to set - # RUN_SMOKE to False in order to run live migration tests using - # grenade - export DEVSTACK_LOCAL_CONFIG="RUN_SMOKE=False" - # LIVE_MIGRATE_BACK_AND_FORTH will tell Tempest to run a live - # migration of the same instance to one compute node and then back - # to the other, which is mostly only interesting for grenade since - # we have mixed level computes. - export DEVSTACK_LOCAL_CONFIG+=$'\n'"LIVE_MIGRATE_BACK_AND_FORTH=True" - export BRANCH_OVERRIDE=default - export DEVSTACK_GATE_TOPOLOGY="multinode" - if [ "$BRANCH_OVERRIDE" != "default" ] ; then - export OVERRIDE_ZUUL_BRANCH=$BRANCH_OVERRIDE - fi - function post_test_hook { - /opt/stack/new/nova/gate/live_migration/hooks/run_tests.sh - } - export -f post_test_hook - cp devstack-gate/devstack-vm-gate-wrap.sh ./safe-devstack-vm-gate-wrap.sh - ./safe-devstack-vm-gate-wrap.sh - executable: /bin/bash - chdir: '{{ ansible_user_dir }}/workspace' - environment: '{{ zuul | zuul_legacy_vars }}' diff --git a/playbooks/legacy/nova-live-migration/post.yaml b/playbooks/legacy/nova-live-migration/post.yaml deleted file mode 100644 index e07f5510ae7..00000000000 --- a/playbooks/legacy/nova-live-migration/post.yaml +++ /dev/null @@ -1,15 +0,0 @@ -- hosts: primary - tasks: - - - name: Copy files from {{ ansible_user_dir }}/workspace/ on node - synchronize: - src: '{{ ansible_user_dir }}/workspace/' - dest: '{{ zuul.executor.log_root }}' - mode: pull - copy_links: true - verify_host: true - rsync_opts: - - --include=/logs/** - - --include=*/ - - --exclude=* - - --prune-empty-dirs diff --git a/playbooks/legacy/nova-live-migration/run.yaml b/playbooks/legacy/nova-live-migration/run.yaml deleted file mode 100644 index ef8853135bb..00000000000 --- a/playbooks/legacy/nova-live-migration/run.yaml +++ /dev/null @@ -1,60 +0,0 @@ -- hosts: primary - name: nova-live-migration - tasks: - - - name: Ensure legacy workspace directory - file: - path: '{{ ansible_user_dir }}/workspace' - state: directory - - - shell: - cmd: | - set -e - set -x - cat > clonemap.yaml << EOF - clonemap: - - name: openstack/devstack-gate - dest: devstack-gate - EOF - /usr/zuul-env/bin/zuul-cloner -m clonemap.yaml --cache-dir /opt/git \ - https://opendev.org \ - openstack/devstack-gate - executable: /bin/bash - chdir: '{{ ansible_user_dir }}/workspace' - environment: '{{ zuul | zuul_legacy_vars }}' - - - name: Configure devstack - shell: - # Force config drive. - cmd: | - set -e - set -x - cat << 'EOF' >>"/tmp/dg-local.conf" - [[local|localrc]] - FORCE_CONFIG_DRIVE=True - - EOF - executable: /bin/bash - chdir: '{{ ansible_user_dir }}/workspace' - environment: '{{ zuul | zuul_legacy_vars }}' - - - shell: - cmd: | - set -e - set -x - export PYTHONUNBUFFERED=true - export DEVSTACK_GATE_CONFIGDRIVE=0 - export DEVSTACK_GATE_TEMPEST=1 - export DEVSTACK_GATE_TEMPEST_NOTESTS=1 - export DEVSTACK_GATE_TOPOLOGY="multinode" - export DEVSTACK_GATE_USE_PYTHON3=True - function post_test_hook { - /opt/stack/new/nova/gate/live_migration/hooks/run_tests.sh - $BASE/new/nova/gate/test_evacuate.sh - } - export -f post_test_hook - cp devstack-gate/devstack-vm-gate-wrap.sh ./safe-devstack-vm-gate-wrap.sh - ./safe-devstack-vm-gate-wrap.sh - executable: /bin/bash - chdir: '{{ ansible_user_dir }}/workspace' - environment: '{{ zuul | zuul_legacy_vars }}' From f20346bc00a30c914cbefb48009db776f8e00b09 Mon Sep 17 00:00:00 2001 From: melanie witt Date: Fri, 28 May 2021 00:26:04 +0000 Subject: [PATCH 12/40] Honor [neutron]http_retries in the manual client Change Ifb3afb13aff7e103c2e80ade817d0e63b624604a added a nova side config option for specifying neutron client retries that maps to the ksa connect_retries config option to provide parity with the cinder and glance clients that have nova side config options. That change missed passing CONF.neutron.http_retries to the manual client used for calling the port binding API. This sets the connect_retries attribute on the manual ksa client so http_retries will be honored. Closes-Bug: #1929886 Related-Bug: #1866937 Change-Id: I8296e4be9f0706fab043451b856efadbb7bd41f6 (cherry picked from commit 56eb253e9febccf721df6bca4eb851ad26cb70a6) (cherry picked from commit 46aa3f4ec769e948d9eb73604bf9b66f4b0230b0) --- nova/network/neutron.py | 1 + nova/tests/unit/network/test_neutron.py | 2 ++ 2 files changed, 3 insertions(+) diff --git a/nova/network/neutron.py b/nova/network/neutron.py index db4cb44d473..59212eef124 100644 --- a/nova/network/neutron.py +++ b/nova/network/neutron.py @@ -271,6 +271,7 @@ def _get_ksa_client(context, admin=False): client = utils.get_ksa_adapter( 'network', ksa_auth=auth_plugin, ksa_session=session) client.additional_headers = {'accept': 'application/json'} + client.connect_retries = CONF.neutron.http_retries return client diff --git a/nova/tests/unit/network/test_neutron.py b/nova/tests/unit/network/test_neutron.py index e23203c04aa..fb7fa8713ca 100644 --- a/nova/tests/unit/network/test_neutron.py +++ b/nova/tests/unit/network/test_neutron.py @@ -253,6 +253,8 @@ def test_neutron_http_retries(self): auth_token='token') cl = neutronapi.get_client(my_context) self.assertEqual(retries, cl.httpclient.connect_retries) + kcl = neutronapi._get_ksa_client(my_context) + self.assertEqual(retries, kcl.connect_retries) class TestAPIBase(test.TestCase): From e3085fa6310ddeaafa493c3f718aab0ce64f0994 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Herv=C3=A9=20Beraud?= Date: Thu, 4 Jun 2020 09:49:59 +0200 Subject: [PATCH 13/40] Initialize global data separately and run_once in WSGI app init NOTE(melwitt): This is a combination of two changes to avoid intermittent test failure that was introduced by the original bug fix, and was fixed by change I2bd360dcc6501feea7baf02d4510b282205fc061. We have discovered that if an exception is raised at any point during the running of the init_application WSGI script in an apache/mod_wsgi Daemon Mode environment, it will prompt apache/mod_wsgi to re-run the script without starting a fresh python process. Because we initialize global data structures during app init, subsequent runs of the script blow up as some global data do *not* support re-initialization. It is anyway not safe to assume that init of global data is safe to run multiple times. This mod_wsgi behavior appears to be a special situation that does not behave the same as a normal reload in Daemon Mode as the script file is being reloaded upon failure instead of the daemon process being shutdown and restarted as described in the documentation [1]. In order to handle this situation, we can move the initialization of global data structures to a helper method that is decorated to run only once per python interpreter instance. This way, we will not attempt to re-initialize global data that are not safe to init more than once. Co-Authored-By: Michele Baldessari Co-Authored-By: melanie witt Conflicts: nova/api/openstack/wsgi_app.py NOTE(melwitt): The conflict is because change If4783adda92da33d512d7c2834f0bb2e2a9b9654 (Support sys.argv in wsgi app) is not in Victoria. Closes-Bug: #1882094 [1] https://modwsgi.readthedocs.io/en/develop/user-guides/reloading-source-code.html#reloading-in-daemon-mode Reset global wsgi app state in unit test Since I2bd360dcc6501feea7baf02d4510b282205fc061 there is a global state set during the wsgi_app init making our unit test cases non-deterministic based on the order of them. This patch makes sure that the global state is reset for each test case. Closes-Bug: #1921098 (cherry picked from commit bc2c19bb2db901af0c48d34fb15a335f4e343361) Change-Id: I2bd360dcc6501feea7baf02d4510b282205fc061 (cherry picked from commit 7c9edc02eda45aafbbb539b759e6b92f7aeb5ea8) --- nova/api/openstack/wsgi_app.py | 24 ++++- nova/test.py | 5 + .../tests/unit/api/openstack/test_wsgi_app.py | 85 ++++++++++++++++ nova/tests/unit/test_utils.py | 98 +++++++++++++++++++ nova/utils.py | 46 +++++++++ 5 files changed, 256 insertions(+), 2 deletions(-) create mode 100644 nova/tests/unit/api/openstack/test_wsgi_app.py diff --git a/nova/api/openstack/wsgi_app.py b/nova/api/openstack/wsgi_app.py index 8e8fbd6eacf..9039b888cb0 100644 --- a/nova/api/openstack/wsgi_app.py +++ b/nova/api/openstack/wsgi_app.py @@ -29,6 +29,8 @@ CONFIG_FILES = ['api-paste.ini', 'nova.conf'] +LOG = logging.getLogger(__name__) + objects.register_all() @@ -77,8 +79,12 @@ def application(environ, start_response): return application -def init_application(name): - conf_files = _get_config_files() +@utils.run_once('Global data already initialized, not re-initializing.', + LOG.info) +def init_global_data(conf_files): + # NOTE(melwitt): parse_args initializes logging and calls global rpc.init() + # and db_api.configure(). The db_api.configure() call does not initiate any + # connection to the database. config.parse_args([], default_config_files=conf_files) logging.setup(CONF, "nova") @@ -93,11 +99,25 @@ def init_application(name): logging.getLogger(__name__), logging.DEBUG) + +def init_application(name): + conf_files = _get_config_files() + + # NOTE(melwitt): The init_application method can be called multiple times + # within a single python interpreter instance if any exception is raised + # during it (example: DBConnectionError while setting up the service) and + # apache/mod_wsgi reloads the init_application script. So, we initialize + # global data separately and decorate the method to run only once in a + # python interpreter instance. + init_global_data(conf_files) + try: _setup_service(CONF.host, name) except exception.ServiceTooOld as exc: return error_application(exc, name) + # This global init is safe because if we got here, we already successfully + # set up the service and setting up the profile cannot fail. service.setup_profiler(name, CONF.host) conf = conf_files[0] diff --git a/nova/test.py b/nova/test.py index 89e4bed0842..9bb6e5e032a 100644 --- a/nova/test.py +++ b/nova/test.py @@ -54,6 +54,7 @@ from sqlalchemy.dialects import sqlite import testtools +from nova.api.openstack import wsgi_app from nova.compute import rpcapi as compute_rpcapi from nova import context from nova.db.sqlalchemy import api as sqlalchemy_api @@ -285,6 +286,10 @@ def setUp(self): self.useFixture(nova_fixtures.GenericPoisonFixture()) + # make sure that the wsgi app is fully initialized for all testcase + # instead of only once initialized for test worker + wsgi_app.init_global_data.reset() + def _setup_cells(self): """Setup a normal cellsv2 environment. diff --git a/nova/tests/unit/api/openstack/test_wsgi_app.py b/nova/tests/unit/api/openstack/test_wsgi_app.py new file mode 100644 index 00000000000..4cb7459c982 --- /dev/null +++ b/nova/tests/unit/api/openstack/test_wsgi_app.py @@ -0,0 +1,85 @@ +# 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 tempfile + +import fixtures +import mock +from oslo_config import fixture as config_fixture +from oslotest import base + +from nova.api.openstack import wsgi_app +from nova import test +from nova.tests import fixtures as nova_fixtures + + +class WSGIAppTest(base.BaseTestCase): + + _paste_config = """ +[app:nova-api] +use = egg:Paste#static +document_root = /tmp + """ + + def setUp(self): + # Ensure BaseTestCase's ConfigureLogging fixture is disabled since + # we're using our own (StandardLogging). + with fixtures.EnvironmentVariable('OS_LOG_CAPTURE', '0'): + super(WSGIAppTest, self).setUp() + self.stdlog = self.useFixture(nova_fixtures.StandardLogging()) + self.conf = tempfile.NamedTemporaryFile(mode='w+t') + self.conf.write(self._paste_config.lstrip()) + self.conf.seek(0) + self.conf.flush() + self.addCleanup(self.conf.close) + # Use of this fixture takes care of cleaning up config settings for + # subsequent tests. + self.useFixture(config_fixture.Config()) + + @mock.patch('sys.argv', return_value=mock.sentinel.argv) + @mock.patch('nova.db.sqlalchemy.api.configure') + @mock.patch('nova.api.openstack.wsgi_app._setup_service') + @mock.patch('nova.api.openstack.wsgi_app._get_config_files') + def test_init_application_called_twice(self, mock_get_files, mock_setup, + mock_db_configure, mock_argv): + """Test that init_application can tolerate being called twice in a + single python interpreter instance. + + When nova-api is run via mod_wsgi, if any exception is raised during + init_application, mod_wsgi will re-run the WSGI script without + restarting the daemon process even when configured for Daemon Mode. + + We access the database as part of init_application, so if nova-api + starts up before the database is up, we'll get, for example, a + DBConnectionError raised during init_application and our WSGI script + will get reloaded/re-run by mod_wsgi. + """ + mock_get_files.return_value = [self.conf.name] + mock_setup.side_effect = [test.TestingException, None] + # We need to mock the global database configure() method, else we will + # be affected by global database state altered by other tests that ran + # before this test, causing this test to fail with + # oslo_db.sqlalchemy.enginefacade.AlreadyStartedError. We can instead + # mock the method to raise an exception if it's called a second time in + # this test to simulate the fact that the database does not tolerate + # re-init [after a database query has been made]. + mock_db_configure.side_effect = [None, test.TestingException] + # Run init_application the first time, simulating an exception being + # raised during it. + self.assertRaises(test.TestingException, wsgi_app.init_application, + 'nova-api') + # Now run init_application a second time, it should succeed since no + # exception is being raised (the init of global data should not be + # re-attempted). + wsgi_app.init_application('nova-api') + self.assertIn('Global data already initialized, not re-initializing.', + self.stdlog.logger.output) diff --git a/nova/tests/unit/test_utils.py b/nova/tests/unit/test_utils.py index b64a753aeb2..b7516ef7e82 100644 --- a/nova/tests/unit/test_utils.py +++ b/nova/tests/unit/test_utils.py @@ -1294,3 +1294,101 @@ def test_api_db_is_configured_but_the_service_cannot_access_db( 'not allowed to directly access the database. You should run this ' 'service without the [api_database]/connection config option. The ' 'service version check will only query the local cell.') + + +class RunOnceTests(test.NoDBTestCase): + + fake_logger = mock.MagicMock() + + @utils.run_once("already ran once", fake_logger) + def dummy_test_func(self, fail=False): + if fail: + raise ValueError() + return True + + def setUp(self): + super(RunOnceTests, self).setUp() + self.dummy_test_func.reset() + RunOnceTests.fake_logger.reset_mock() + + def test_wrapped_funtions_called_once(self): + self.assertFalse(self.dummy_test_func.called) + result = self.dummy_test_func() + self.assertTrue(result) + self.assertTrue(self.dummy_test_func.called) + + # assert that on second invocation no result + # is returned and that the logger is invoked. + result = self.dummy_test_func() + RunOnceTests.fake_logger.assert_called_once() + self.assertIsNone(result) + + def test_wrapped_funtions_called_once_raises(self): + self.assertFalse(self.dummy_test_func.called) + self.assertRaises(ValueError, self.dummy_test_func, fail=True) + self.assertTrue(self.dummy_test_func.called) + + # assert that on second invocation no result + # is returned and that the logger is invoked. + result = self.dummy_test_func() + RunOnceTests.fake_logger.assert_called_once() + self.assertIsNone(result) + + def test_wrapped_funtions_can_be_reset(self): + # assert we start with a clean state + self.assertFalse(self.dummy_test_func.called) + result = self.dummy_test_func() + self.assertTrue(result) + + self.dummy_test_func.reset() + # assert we restored a clean state + self.assertFalse(self.dummy_test_func.called) + result = self.dummy_test_func() + self.assertTrue(result) + + # assert that we never called the logger + RunOnceTests.fake_logger.assert_not_called() + + def test_reset_calls_cleanup(self): + mock_clean = mock.Mock() + + @utils.run_once("already ran once", self.fake_logger, + cleanup=mock_clean) + def f(): + pass + + f() + self.assertTrue(f.called) + + f.reset() + self.assertFalse(f.called) + mock_clean.assert_called_once_with() + + def test_clean_is_not_called_at_reset_if_wrapped_not_called(self): + mock_clean = mock.Mock() + + @utils.run_once("already ran once", self.fake_logger, + cleanup=mock_clean) + def f(): + pass + + self.assertFalse(f.called) + + f.reset() + self.assertFalse(f.called) + self.assertFalse(mock_clean.called) + + def test_reset_works_even_if_cleanup_raises(self): + mock_clean = mock.Mock(side_effect=ValueError()) + + @utils.run_once("already ran once", self.fake_logger, + cleanup=mock_clean) + def f(): + pass + + f() + self.assertTrue(f.called) + + self.assertRaises(ValueError, f.reset) + self.assertFalse(f.called) + mock_clean.assert_called_once_with() diff --git a/nova/utils.py b/nova/utils.py index 181d1f1affa..ec5e3e86dc9 100644 --- a/nova/utils.py +++ b/nova/utils.py @@ -1103,3 +1103,49 @@ def raise_if_old_compute(): scope=scope, min_service_level=current_service_version, oldest_supported_service=oldest_supported_service_level) + + +def run_once(message, logger, cleanup=None): + """This is a utility function decorator to ensure a function + is run once and only once in an interpreter instance. + + Note: this is copied from the placement repo (placement/util.py) + + The decorated function object can be reset by calling its + reset function. All exceptions raised by the wrapped function, + logger and cleanup function will be propagated to the caller. + """ + def outer_wrapper(func): + @functools.wraps(func) + def wrapper(*args, **kwargs): + if not wrapper.called: + # Note(sean-k-mooney): the called state is always + # updated even if the wrapped function completes + # by raising an exception. If the caller catches + # the exception it is their responsibility to call + # reset if they want to re-execute the wrapped function. + try: + return func(*args, **kwargs) + finally: + wrapper.called = True + else: + logger(message) + + wrapper.called = False + + def reset(wrapper, *args, **kwargs): + # Note(sean-k-mooney): we conditionally call the + # cleanup function if one is provided only when the + # wrapped function has been called previously. We catch + # and reraise any exception that may be raised and update + # the called state in a finally block to ensure its + # always updated if reset is called. + try: + if cleanup and wrapper.called: + return cleanup(*args, **kwargs) + finally: + wrapper.called = False + + wrapper.reset = functools.partial(reset, wrapper) + return wrapper + return outer_wrapper From 6ede6df7f41db809de19e124d3d4994180598f19 Mon Sep 17 00:00:00 2001 From: Rodrigo Barbieri Date: Wed, 31 Mar 2021 11:06:49 -0300 Subject: [PATCH 14/40] Error anti-affinity violation on migrations Error-out the migrations (cold and live) whenever the anti-affinity policy is violated. This addresses violations when multiple concurrent migrations are requested. Added detection on: - prep_resize - check_can_live_migration_destination - pre_live_migration The improved method of detection now locks based on group_id and considers other migrations in-progress as well. Closes-bug: #1821755 Change-Id: I32e6214568bb57f7613ddeba2c2c46da0320fabc (cherry picked from commit 33c8af1f8c46c9c37fcc28fb3409fbd3a78ae39f) (cherry picked from commit 8b62a4ec9bf617dfb2da046c25a9f76b33516508) --- nova/compute/manager.py | 110 ++++++++++++++--- nova/tests/unit/compute/test_compute_mgr.py | 114 ++++++++++++++++-- .../notes/bug-1821755-7bd03319e34b6b10.yaml | 11 ++ 3 files changed, 211 insertions(+), 24 deletions(-) create mode 100644 releasenotes/notes/bug-1821755-7bd03319e34b6b10.yaml diff --git a/nova/compute/manager.py b/nova/compute/manager.py index 65245af1c34..72e88ce6e6e 100644 --- a/nova/compute/manager.py +++ b/nova/compute/manager.py @@ -1646,7 +1646,11 @@ def _decode(f): return [_decode(f) for f in injected_files] def _validate_instance_group_policy(self, context, instance, - scheduler_hints): + scheduler_hints=None): + + if CONF.workarounds.disable_group_policy_check_upcall: + return + # NOTE(russellb) Instance group policy is enforced by the scheduler. # However, there is a race condition with the enforcement of # the policy. Since more than one instance may be scheduled at the @@ -1655,29 +1659,63 @@ def _validate_instance_group_policy(self, context, instance, # multiple instances with an affinity policy could end up on different # hosts. This is a validation step to make sure that starting the # instance here doesn't violate the policy. - group_hint = scheduler_hints.get('group') - if not group_hint: - return - - # The RequestSpec stores scheduler_hints as key=list pairs so we need - # to check the type on the value and pull the single entry out. The - # API request schema validates that the 'group' hint is a single value. - if isinstance(group_hint, list): - group_hint = group_hint[0] + if scheduler_hints is not None: + # only go through here if scheduler_hints is provided, even if it + # is empty. + group_hint = scheduler_hints.get('group') + if not group_hint: + return + else: + # The RequestSpec stores scheduler_hints as key=list pairs so + # we need to check the type on the value and pull the single + # entry out. The API request schema validates that + # the 'group' hint is a single value. + if isinstance(group_hint, list): + group_hint = group_hint[0] + + group = objects.InstanceGroup.get_by_hint(context, group_hint) + else: + # TODO(ganso): a call to DB can be saved by adding request_spec + # to rpcapi payload of live_migration, pre_live_migration and + # check_can_live_migrate_destination + try: + group = objects.InstanceGroup.get_by_instance_uuid( + context, instance.uuid) + except exception.InstanceGroupNotFound: + return - @utils.synchronized(group_hint) - def _do_validation(context, instance, group_hint): - group = objects.InstanceGroup.get_by_hint(context, group_hint) + @utils.synchronized(group['uuid']) + def _do_validation(context, instance, group): if group.policy and 'anti-affinity' == group.policy: + + # instances on host instances_uuids = objects.InstanceList.get_uuids_by_host( context, self.host) ins_on_host = set(instances_uuids) + + # instance param is just for logging, the nodename obtained is + # not actually related to the instance at all + nodename = self._get_nodename(instance) + + # instances being migrated to host + migrations = ( + objects.MigrationList.get_in_progress_by_host_and_node( + context, self.host, nodename)) + migration_vm_uuids = set([mig['instance_uuid'] + for mig in migrations]) + + total_instances = migration_vm_uuids | ins_on_host + + # refresh group to get updated members within locked block + group = objects.InstanceGroup.get_by_uuid(context, + group['uuid']) members = set(group.members) # Determine the set of instance group members on this host # which are not the instance in question. This is used to # determine how many other members from the same anti-affinity # group can be on this host. - members_on_host = ins_on_host & members - set([instance.uuid]) + members_on_host = (total_instances & members - + set([instance.uuid])) rules = group.rules if rules and 'max_server_per_host' in rules: max_server = rules['max_server_per_host'] @@ -1689,6 +1727,12 @@ def _do_validation(context, instance, group_hint): raise exception.RescheduledException( instance_uuid=instance.uuid, reason=msg) + + # NOTE(ganso): The check for affinity below does not work and it + # can easily be violated because the lock happens in different + # compute hosts. + # The only fix seems to be a DB lock to perform the check whenever + # setting the host field to an instance. elif group.policy and 'affinity' == group.policy: group_hosts = group.get_hosts(exclude=[instance.uuid]) if group_hosts and self.host not in group_hosts: @@ -1697,8 +1741,7 @@ def _do_validation(context, instance, group_hint): instance_uuid=instance.uuid, reason=msg) - if not CONF.workarounds.disable_group_policy_check_upcall: - _do_validation(context, instance, group_hint) + _do_validation(context, instance, group) def _log_original_error(self, exc_info, instance_uuid): LOG.error('Error: %s', exc_info[1], instance_uuid=instance_uuid, @@ -5217,10 +5260,24 @@ def prep_resize(self, context, image, instance, instance_type, with self._error_out_instance_on_exception( context, instance, instance_state=instance_state),\ errors_out_migration_ctxt(migration): + self._send_prep_resize_notifications( context, instance, fields.NotificationPhase.START, instance_type) try: + scheduler_hints = self._get_scheduler_hints(filter_properties, + request_spec) + # Error out if this host cannot accept the new instance due + # to anti-affinity. At this point the migration is already + # in-progress, so this is the definitive moment to abort due to + # the policy violation. Also, exploding here is covered by the + # cleanup methods in except block. + try: + self._validate_instance_group_policy(context, instance, + scheduler_hints) + except exception.RescheduledException as e: + raise exception.InstanceFaultRollback(inner_exception=e) + self._prep_resize(context, image, instance, instance_type, filter_properties, node, migration, request_spec, @@ -7823,6 +7880,20 @@ def check_can_live_migrate_destination(self, ctxt, instance, :param limits: objects.SchedulerLimits object for this live migration. :returns: a LiveMigrateData object (hypervisor-dependent) """ + + # Error out if this host cannot accept the new instance due + # to anti-affinity. This check at this moment is not very accurate, as + # multiple requests may be happening concurrently and miss the lock, + # but when it works it provides a better user experience by failing + # earlier. Also, it should be safe to explode here, error becomes + # NoValidHost and instance status remains ACTIVE. + try: + self._validate_instance_group_policy(ctxt, instance) + except exception.RescheduledException as e: + msg = ("Failed to validate instance group policy " + "due to: {}".format(e)) + raise exception.MigrationPreCheckError(reason=msg) + src_compute_info = obj_base.obj_to_primitive( self._get_compute_info(ctxt, instance.host)) dst_compute_info = obj_base.obj_to_primitive( @@ -7965,6 +8036,13 @@ def pre_live_migration(self, context, instance, block_migration, disk, """ LOG.debug('pre_live_migration data is %s', migrate_data) + # Error out if this host cannot accept the new instance due + # to anti-affinity. At this point the migration is already in-progress, + # so this is the definitive moment to abort due to the policy + # violation. Also, it should be safe to explode here. The instance + # status remains ACTIVE, migration status failed. + self._validate_instance_group_policy(context, instance) + migrate_data.old_vol_attachment_ids = {} bdms = objects.BlockDeviceMappingList.get_by_instance_uuid( context, instance.uuid) diff --git a/nova/tests/unit/compute/test_compute_mgr.py b/nova/tests/unit/compute/test_compute_mgr.py index 93c895e8734..bb6629908ac 100644 --- a/nova/tests/unit/compute/test_compute_mgr.py +++ b/nova/tests/unit/compute/test_compute_mgr.py @@ -3387,12 +3387,16 @@ def _test_check_can_live_migrate_destination(self, do_raise=False, CONF.host, instance.uuid, graceful_exit=False) return result + @mock.patch('nova.objects.InstanceGroup.get_by_instance_uuid', mock.Mock( + side_effect=exception.InstanceGroupNotFound(group_uuid=''))) def test_check_can_live_migrate_destination_success(self): self.useFixture(std_fixtures.MonkeyPatch( 'nova.network.neutron.API.supports_port_binding_extension', lambda *args: True)) self._test_check_can_live_migrate_destination() + @mock.patch('nova.objects.InstanceGroup.get_by_instance_uuid', mock.Mock( + side_effect=exception.InstanceGroupNotFound(group_uuid=''))) def test_check_can_live_migrate_destination_fail(self): self.useFixture(std_fixtures.MonkeyPatch( 'nova.network.neutron.API.supports_port_binding_extension', @@ -3402,7 +3406,9 @@ def test_check_can_live_migrate_destination_fail(self): self._test_check_can_live_migrate_destination, do_raise=True) - def test_check_can_live_migrate_destination_contins_vifs(self): + @mock.patch('nova.objects.InstanceGroup.get_by_instance_uuid', mock.Mock( + side_effect=exception.InstanceGroupNotFound(group_uuid=''))) + def test_check_can_live_migrate_destination_contains_vifs(self): self.useFixture(std_fixtures.MonkeyPatch( 'nova.network.neutron.API.supports_port_binding_extension', lambda *args: True)) @@ -3410,6 +3416,8 @@ def test_check_can_live_migrate_destination_contins_vifs(self): self.assertIn('vifs', migrate_data) self.assertIsNotNone(migrate_data.vifs) + @mock.patch('nova.objects.InstanceGroup.get_by_instance_uuid', mock.Mock( + side_effect=exception.InstanceGroupNotFound(group_uuid=''))) def test_check_can_live_migrate_destination_no_binding_extended(self): self.useFixture(std_fixtures.MonkeyPatch( 'nova.network.neutron.API.supports_port_binding_extension', @@ -3417,18 +3425,40 @@ def test_check_can_live_migrate_destination_no_binding_extended(self): migrate_data = self._test_check_can_live_migrate_destination() self.assertNotIn('vifs', migrate_data) + @mock.patch('nova.objects.InstanceGroup.get_by_instance_uuid', mock.Mock( + side_effect=exception.InstanceGroupNotFound(group_uuid=''))) def test_check_can_live_migrate_destination_src_numa_lm_false(self): self.useFixture(std_fixtures.MonkeyPatch( 'nova.network.neutron.API.supports_port_binding_extension', lambda *args: True)) self._test_check_can_live_migrate_destination(src_numa_lm=False) + @mock.patch('nova.objects.InstanceGroup.get_by_instance_uuid', mock.Mock( + side_effect=exception.InstanceGroupNotFound(group_uuid=''))) def test_check_can_live_migrate_destination_src_numa_lm_true(self): self.useFixture(std_fixtures.MonkeyPatch( 'nova.network.neutron.API.supports_port_binding_extension', lambda *args: True)) self._test_check_can_live_migrate_destination(src_numa_lm=True) + @mock.patch.object(compute_utils, 'add_instance_fault_from_exc') + def test_check_can_live_migrate_destination_fail_group_policy( + self, mock_fail_db): + + instance = fake_instance.fake_instance_obj( + self.context, host=self.compute.host, vm_state=vm_states.ACTIVE, + node='fake-node') + + ex = exception.RescheduledException( + instance_uuid=instance.uuid, reason="policy violated") + + with mock.patch.object(self.compute, '_validate_instance_group_policy', + side_effect=ex): + self.assertRaises( + exception.MigrationPreCheckError, + self.compute.check_can_live_migrate_destination, + self.context, instance, None, None, None, None) + def test_dest_can_numa_live_migrate(self): positive_dest_check_data = objects.LibvirtLiveMigrateData( dst_supports_numa_live_migration=True) @@ -7532,7 +7562,8 @@ def test_prep_block_device_maintain_original_error_message(self, def test_validate_policy_honors_workaround_disabled(self, mock_get): instance = objects.Instance(uuid=uuids.instance) hints = {'group': 'foo'} - mock_get.return_value = objects.InstanceGroup(policy=None) + mock_get.return_value = objects.InstanceGroup(policy=None, + uuid=uuids.group) self.compute._validate_instance_group_policy(self.context, instance, hints) mock_get.assert_called_once_with(self.context, 'foo') @@ -7558,10 +7589,14 @@ def test_validate_instance_group_policy_handles_hint_list(self, mock_get): instance, hints) mock_get.assert_called_once_with(self.context, uuids.group_hint) + @mock.patch('nova.objects.InstanceGroup.get_by_uuid') @mock.patch('nova.objects.InstanceList.get_uuids_by_host') @mock.patch('nova.objects.InstanceGroup.get_by_hint') - def test_validate_instance_group_policy_with_rules(self, mock_get_by_hint, - mock_get_by_host): + @mock.patch.object(fake_driver.FakeDriver, 'get_available_nodes') + @mock.patch('nova.objects.MigrationList.get_in_progress_by_host_and_node') + def test_validate_instance_group_policy_with_rules( + self, migration_list, nodes, mock_get_by_hint, mock_get_by_host, + mock_get_by_uuid): # Create 2 instance in same host, inst2 created before inst1 instance = objects.Instance(uuid=uuids.inst1) hints = {'group': [uuids.group_hint]} @@ -7570,17 +7605,26 @@ def test_validate_instance_group_policy_with_rules(self, mock_get_by_hint, mock_get_by_host.return_value = existing_insts # if group policy rules limit to 1, raise RescheduledException - mock_get_by_hint.return_value = objects.InstanceGroup( + group = objects.InstanceGroup( policy='anti-affinity', rules={'max_server_per_host': '1'}, - hosts=['host1'], members=members_uuids) + hosts=['host1'], members=members_uuids, + uuid=uuids.group) + mock_get_by_hint.return_value = group + mock_get_by_uuid.return_value = group + nodes.return_value = ['nodename'] + migration_list.return_value = [objects.Migration( + uuid=uuids.migration, instance_uuid=uuids.instance)] self.assertRaises(exception.RescheduledException, self.compute._validate_instance_group_policy, self.context, instance, hints) # if group policy rules limit change to 2, validate OK - mock_get_by_hint.return_value = objects.InstanceGroup( + group2 = objects.InstanceGroup( policy='anti-affinity', rules={'max_server_per_host': 2}, - hosts=['host1'], members=members_uuids) + hosts=['host1'], members=members_uuids, + uuid=uuids.group) + mock_get_by_hint.return_value = group2 + mock_get_by_uuid.return_value = group2 self.compute._validate_instance_group_policy(self.context, instance, hints) @@ -9107,6 +9151,8 @@ def test_max_concurrent_live_semaphore_unlimited(self, mock_executor): manager.ComputeManager() mock_executor.assert_called_once_with() + @mock.patch('nova.objects.InstanceGroup.get_by_instance_uuid', mock.Mock( + side_effect=exception.InstanceGroupNotFound(group_uuid=''))) def test_pre_live_migration_cinder_v3_api(self): # This tests that pre_live_migration with a bdm with an # attachment_id, will create a new attachment and update @@ -9184,6 +9230,8 @@ def _test(mock_attach, mock_get_bdms, mock_ivbi, _test() + @mock.patch('nova.objects.InstanceGroup.get_by_instance_uuid', mock.Mock( + side_effect=exception.InstanceGroupNotFound(group_uuid=''))) def test_pre_live_migration_exception_cinder_v3_api(self): # The instance in this test has 2 attachments. The second attach_create # will throw an exception. This will test that the first attachment @@ -9253,6 +9301,8 @@ def _test(mock_attach_create, mock_attach_delete, mock_get_bdms, self.assertGreater(len(m.mock_calls), 0) _test() + @mock.patch('nova.objects.InstanceGroup.get_by_instance_uuid', mock.Mock( + side_effect=exception.InstanceGroupNotFound(group_uuid=''))) def test_pre_live_migration_exceptions_delete_attachments(self): # The instance in this test has 2 attachments. The call to # driver.pre_live_migration will raise an exception. This will test @@ -10631,6 +10681,54 @@ def fake_reschedule_resize_or_reraise(*args, **kwargs): # (_error_out_instance_on_exception will set to ACTIVE by default). self.assertEqual(vm_states.STOPPED, instance.vm_state) + @mock.patch('nova.compute.utils.notify_usage_exists') + @mock.patch('nova.compute.manager.ComputeManager.' + '_notify_about_instance_usage') + @mock.patch('nova.compute.utils.notify_about_resize_prep_instance') + @mock.patch('nova.objects.Instance.save') + @mock.patch('nova.compute.manager.ComputeManager._revert_allocation') + @mock.patch('nova.compute.manager.ComputeManager.' + '_reschedule_resize_or_reraise') + @mock.patch('nova.compute.utils.add_instance_fault_from_exc') + # this is almost copy-paste from test_prep_resize_fails_rollback + def test_prep_resize_fails_group_validation( + self, add_instance_fault_from_exc, _reschedule_resize_or_reraise, + _revert_allocation, mock_instance_save, + notify_about_resize_prep_instance, _notify_about_instance_usage, + notify_usage_exists): + """Tests that if _validate_instance_group_policy raises + InstanceFaultRollback, the instance.vm_state is reset properly in + _error_out_instance_on_exception + """ + instance = fake_instance.fake_instance_obj( + self.context, host=self.compute.host, vm_state=vm_states.STOPPED, + node='fake-node', expected_attrs=['system_metadata', 'flavor']) + migration = mock.MagicMock(spec='nova.objects.Migration') + request_spec = mock.MagicMock(spec='nova.objects.RequestSpec') + ex = exception.RescheduledException( + instance_uuid=instance.uuid, reason="policy violated") + ex2 = exception.InstanceFaultRollback( + inner_exception=ex) + + def fake_reschedule_resize_or_reraise(*args, **kwargs): + raise ex2 + + _reschedule_resize_or_reraise.side_effect = ( + fake_reschedule_resize_or_reraise) + + with mock.patch.object(self.compute, '_validate_instance_group_policy', + side_effect=ex): + self.assertRaises( + # _error_out_instance_on_exception should reraise the + # RescheduledException inside InstanceFaultRollback. + exception.RescheduledException, self.compute.prep_resize, + self.context, instance.image_meta, instance, instance.flavor, + request_spec, filter_properties={}, node=instance.node, + clean_shutdown=True, migration=migration, host_list=[]) + # The instance.vm_state should remain unchanged + # (_error_out_instance_on_exception will set to ACTIVE by default). + self.assertEqual(vm_states.STOPPED, instance.vm_state) + @mock.patch('nova.compute.rpcapi.ComputeAPI.resize_instance') @mock.patch('nova.compute.resource_tracker.ResourceTracker.resize_claim') @mock.patch('nova.objects.Instance.save') diff --git a/releasenotes/notes/bug-1821755-7bd03319e34b6b10.yaml b/releasenotes/notes/bug-1821755-7bd03319e34b6b10.yaml new file mode 100644 index 00000000000..4c6135311bb --- /dev/null +++ b/releasenotes/notes/bug-1821755-7bd03319e34b6b10.yaml @@ -0,0 +1,11 @@ +--- +fixes: + - | + Improved detection of anti-affinity policy violation when performing live + and cold migrations. Most of the violations caused by race conditions due + to performing concurrent live or cold migrations should now be addressed + by extra checks in the compute service. Upon detection, cold migration + operations are automatically rescheduled, while live migrations have two + checks and will be rescheduled if detected by the first one, otherwise the + live migration will fail cleanly and revert the instance state back to its + previous value. From 6305ae491e86ee1a408213da9d89d5cdecbaa869 Mon Sep 17 00:00:00 2001 From: Mohammed Naser Date: Mon, 14 Jun 2021 16:53:02 -0400 Subject: [PATCH 15/40] Allow X-OpenStack-Nova-API-Version header in CORS By default, we don't currently allow the Nova microversion header for CORS requests. It should be something that is included out of the box because it's part of the core API. Deployers can workaround this by overriding allow_headers, but we should provide a better experience out of the box. Closes-Bug: #1931908 Change-Id: Idf4650f36952331f02d7512580c21451f3ee3b63 (cherry picked from commit b02a95a18b5da37db6d4f30a5dea07e2a4187245) --- nova/middleware.py | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/nova/middleware.py b/nova/middleware.py index 717fecd4efe..8b0fc595615 100644 --- a/nova/middleware.py +++ b/nova/middleware.py @@ -24,11 +24,15 @@ def set_defaults(): 'X-Roles', 'X-Service-Catalog', 'X-User-Id', - 'X-Tenant-Id'], + 'X-Tenant-Id', + 'X-OpenStack-Nova-API-Version', + 'OpenStack-API-Version'], expose_headers=['X-Auth-Token', 'X-Openstack-Request-Id', 'X-Subject-Token', - 'X-Service-Token'], + 'X-Service-Token', + 'X-OpenStack-Nova-API-Version', + 'OpenStack-API-Version'], allow_methods=['GET', 'PUT', 'POST', From b7677ae08ae151858ecb0e67039e54bb3df89700 Mon Sep 17 00:00:00 2001 From: Stephen Finucane Date: Wed, 16 Jun 2021 12:00:19 +0100 Subject: [PATCH 16/40] Move 'check-cherry-picks' test to gate, n-v check This currently runs in the 'check' pipeline, as part of the pep8 job, which causes otherwise perfectly valid backports to report as failing CI. There's no reason a stable core shouldn't be encouraged to review these patches: we simply want to prevent them *merging* before their parent(s). Resolve this conflict by moving the check to separate voting job in the 'gate' pipeline as well as a non-voting job in the 'check' pipeline to catch more obvious issues. NOTE(lyarwood): Conflict as I672904e9bfb45a66a82331063c7d49c4bc0439df isn't present on stable/victoria. Conflicts: .zuul.yaml Change-Id: Id3e4452883f6a3cf44ff58b39ded82e882e28c23 Signed-off-by: Stephen Finucane (cherry picked from commit 98b01c9a59df4912f5a162c2c52d1f00c84d24c2) (cherry picked from commit fef0305abefbf165fecb883f03bce97f525a790a) --- .zuul.yaml | 14 ++++++++++++++ tools/check-cherry-picks.sh | 5 ----- tox.ini | 12 +++++++++--- 3 files changed, 23 insertions(+), 8 deletions(-) diff --git a/.zuul.yaml b/.zuul.yaml index 2c887b4d841..c00865e5044 100644 --- a/.zuul.yaml +++ b/.zuul.yaml @@ -56,6 +56,17 @@ bindep_profile: test py38 timeout: 3600 +- job: + name: nova-tox-validate-backport + parent: openstack-tox + description: | + Determine whether a backport is ready to be merged by checking whether it + has already been merged to master or more recent stable branches. + + Uses tox with the ``validate-backport`` environment. + vars: + tox_envlist: validate-backport + - job: name: nova-live-migration parent: tempest-multinode-full-py3 @@ -424,6 +435,8 @@ - nova-lvm - nova-multi-cell - nova-next + - nova-tox-validate-backport: + voting: false - nova-tox-functional-py38 - tempest-integrated-compute: # NOTE(gmann): Policies changes do not need to run all the @@ -462,6 +475,7 @@ - nova-tox-functional-py38 - nova-multi-cell - nova-next + - nova-tox-validate-backport - nova-ceph-multistore: irrelevant-files: *dsvm-irrelevant-files - neutron-tempest-linuxbridge: diff --git a/tools/check-cherry-picks.sh b/tools/check-cherry-picks.sh index 5ca6ded2033..5a449c520b7 100755 --- a/tools/check-cherry-picks.sh +++ b/tools/check-cherry-picks.sh @@ -4,11 +4,6 @@ # to verify that they're all on either master or stable/ branches # -# Allow this script to be disabled by a simple env var -if [ ${DISABLE_CHERRY_PICK_CHECK:-0} -eq 1 ]; then - exit 0 -fi - commit_hash="" # Check if the patch is a merge patch by counting the number of parents. diff --git a/tox.ini b/tox.ini index 61969cf5687..be5f18d0130 100644 --- a/tox.ini +++ b/tox.ini @@ -49,8 +49,6 @@ commands = description = Run style checks. envdir = {toxworkdir}/shared -passenv = - DISABLE_CHERRY_PICK_CHECK commands = {[testenv:mypy]commands} bash tools/flake8wrap.sh {posargs} @@ -58,7 +56,6 @@ commands = bash -c "! find doc/ -type f -name *.json | xargs grep -U -n $'\r'" # Check that all included JSON files are valid JSON bash -c '! find doc/ -type f -name *.json | xargs -t -n1 python -m json.tool 2>&1 > /dev/null | grep -B1 -v ^python' - bash tools/check-cherry-picks.sh [testenv:fast8] description = @@ -67,6 +64,15 @@ envdir = {toxworkdir}/shared commands = bash tools/flake8wrap.sh -HEAD +[testenv:validate-backport] +description = + Determine whether a backport is ready to be merged by checking whether it has + already been merged to master or more recent stable branches. +deps = +skipsdist = true +commands = + bash tools/check-cherry-picks.sh + [testenv:functional] description = Run functional tests using python3. From f7d84db5876b30d6849877799c08ebc65ac077ca Mon Sep 17 00:00:00 2001 From: Slawek Kaplonski Date: Fri, 26 Mar 2021 12:18:37 +0100 Subject: [PATCH 17/40] [neutron] Get only ID and name of the SGs from Neutron During the VM booting process Nova asks Neutron for the security groups of the project. If there are no any fields specified, Neutron will prepare list of security groups with all fields, including rules. In case if project got many SGs, it may take long time as rules needs to be loaded separately for each SG on Neutron's side. During booting of the VM, Nova really needs only "id" and "name" of the security groups so this patch limits request to only those 2 fields. This lazy loading of the SG rules was introduced in Neutron in [1] and [2]. [1] https://review.opendev.org/#/c/630401/ [2] https://review.opendev.org/#/c/637407/ Related-Bug: #1865223 Change-Id: I15c3119857970c38541f4de270bd561166334548 (cherry picked from commit 388498ac5fa15ed8deef06ec061ea47e4a1b7377) (cherry picked from commit 4f49545afaf3cd453796d48ba96b9a82d11c01bf) --- nova/network/neutron.py | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/nova/network/neutron.py b/nova/network/neutron.py index db4cb44d473..098f9b3e894 100644 --- a/nova/network/neutron.py +++ b/nova/network/neutron.py @@ -802,9 +802,15 @@ def _process_security_groups(self, instance, neutron, security_groups): # TODO(arosen) Should optimize more to do direct query for security # group if len(security_groups) == 1 if len(security_groups): + # NOTE(slaweq): fields other than name and id aren't really needed + # so asking only about those fields will allow Neutron to not + # prepare list of rules for each found security group. That may + # speed processing of this request a lot in case when tenant has + # got many security groups + sg_fields = ['id', 'name'] search_opts = {'tenant_id': instance.project_id} user_security_groups = neutron.list_security_groups( - **search_opts).get('security_groups') + fields=sg_fields, **search_opts).get('security_groups') for security_group in security_groups: name_match = None From bec6dd475243b027ce5ca487e1a9bffdb866d25f Mon Sep 17 00:00:00 2001 From: Tobias Urdin Date: Mon, 3 May 2021 17:25:43 +0200 Subject: [PATCH 18/40] Stop leaking ceph df cmd in RBD utils If the ceph df command fails in the get_pool_info method of RBD utils the actual command executed if seen by the users in the fault error message. This hides the command behind a StorageError exception and logs the exception instead of leaking it to the users. Change-Id: I6e3a73f2e04d1a7636daf96d5af73c9cf2fbe220 Closes-Bug: 1926978 (cherry picked from commit 86af7feed06f08ddb3ef65122089216708d53a06) (cherry picked from commit 5ede75c65edbcb27557831ae6f5c3a9f81f23a0e) --- nova/storage/rbd_utils.py | 9 ++++++++- nova/tests/unit/storage/test_rbd.py | 6 ++++++ 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/nova/storage/rbd_utils.py b/nova/storage/rbd_utils.py index 22bafe50536..431dfc9aec0 100644 --- a/nova/storage/rbd_utils.py +++ b/nova/storage/rbd_utils.py @@ -405,7 +405,14 @@ def get_pool_info(self): # MAX_AVAIL stat will divide by the replication size when doing the # calculation. args = ['ceph', 'df', '--format=json'] + self.ceph_args() - out, _ = processutils.execute(*args) + + try: + out, _ = processutils.execute(*args) + except processutils.ProcessExecutionError: + LOG.exception('Could not determine disk usage') + raise exception.StorageError( + reason='Could not determine disk usage') + stats = jsonutils.loads(out) # Find the pool for which we are configured. diff --git a/nova/tests/unit/storage/test_rbd.py b/nova/tests/unit/storage/test_rbd.py index f0b3f705320..65796ebc1fb 100644 --- a/nova/tests/unit/storage/test_rbd.py +++ b/nova/tests/unit/storage/test_rbd.py @@ -13,6 +13,7 @@ from eventlet import tpool import mock +from oslo_concurrency import processutils from oslo_serialization import jsonutils from oslo_utils.fixture import uuidsentinel as uuids @@ -653,6 +654,11 @@ def test_get_pool_info(self, mock_execute): 'used': ceph_df_json['pools'][1]['stats']['bytes_used']} self.assertDictEqual(expected, self.driver.get_pool_info()) + @mock.patch('oslo_concurrency.processutils.execute', autospec=True, + side_effect=processutils.ProcessExecutionError("failed")) + def test_get_pool_info_execute_failed(self, mock_execute): + self.assertRaises(exception.StorageError, self.driver.get_pool_info) + @mock.patch('oslo_concurrency.processutils.execute') def test_get_pool_info_not_found(self, mock_execute): # Make the pool something other than self.rbd_pool so it won't be found From 9efdd0b085733a0a4c4192aab8fc870c8aadf316 Mon Sep 17 00:00:00 2001 From: Artom Lifshitz Date: Wed, 28 Jul 2021 12:48:50 +0200 Subject: [PATCH 19/40] Reproducer unit test for bug 1860312 Consider the following situation: - Using the Ironic virt driver - Replacing (so removing and re-adding) all baremetal nodes associated with a single nova-compute service The update resources periodic will have destroyed the compute node records because they're no longer being reported by the virt driver. If we then attempt to manually delete the compute service record, the datbase layer will raise an exception, as there are no longer any compute node records for the host. This exception gets bubbled up as an error 500 in the API. This patch adds a unit test to demonstrate this. Related bug: 1860312 Change-Id: I03eec634b25582ec9643cacf3e5868c101176983 (cherry picked from commit 32257a2a6d159406577c885a9d7e366cbf0c72b9) (cherry picked from commit e6cd23c3b4928b421b8c706f9cc218020779e367) --- .../api/openstack/compute/test_services.py | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/nova/tests/unit/api/openstack/compute/test_services.py b/nova/tests/unit/api/openstack/compute/test_services.py index 170b39f2084..07acbaab16c 100644 --- a/nova/tests/unit/api/openstack/compute/test_services.py +++ b/nova/tests/unit/api/openstack/compute/test_services.py @@ -701,6 +701,25 @@ def test_services_delete(self, mock_get_compute_nodes): mock_get_compute_nodes.assert_called_once_with( self.req.environ['nova.context'], compute.host) + @mock.patch( + 'nova.objects.ComputeNodeList.get_all_by_host', + side_effect=exception.ComputeHostNotFound(host='fake-compute-host')) + def test_services_delete_compute_host_not_found( + self, mock_get_all_by_host): + compute = objects.Service(self.ctxt, + **{'host': 'fake-compute-host', + 'binary': 'nova-compute', + 'topic': 'compute', + 'report_count': 0}) + compute.create() + # FIXME(artom) Until bug 1860312 is fixed, the ComputeHostNotFound + # error will get bubbled up to the API as an error 500. + self.assertRaises( + webob.exc.HTTPInternalServerError, + self.controller.delete, self.req, compute.id) + mock_get_all_by_host.assert_called_with( + self.req.environ['nova.context'], 'fake-compute-host') + def test_services_delete_not_found(self): self.assertRaises(webob.exc.HTTPNotFound, From e238cc9cd6ee7ba9d556c0c29f6f69dc3e3a7af9 Mon Sep 17 00:00:00 2001 From: Artom Lifshitz Date: Mon, 19 Jul 2021 12:58:29 +0200 Subject: [PATCH 20/40] Allow deletion of compute service with no compute nodes Consider the following situation: - Using the Ironic virt driver - Replacing (so removing and re-adding) all baremetal nodes associated with a single nova-compute service The update resources periodic will have destroyed the compute node records because they're no longer being reported by the virt driver. If we then attempt to manually delete the compute service record, the datbase layer will raise an exception, as there are no longer any compute node records for the host. Previously, this exception would get bubbled up as an error 500 in the API. This patch catches it and allows service deletion to complete succefully. Closes bug: 1860312 Change-Id: I2f9ad3df25306e070c8c3538bfed1212d6d8682f (cherry picked from commit 880611df0b6b967adabd3f08886e385d0a100c5c) (cherry picked from commit df5158bf3f80fd4362725dc280de67b88ece9952) --- nova/api/openstack/compute/services.py | 23 +++++++++++++++---- .../api/openstack/compute/test_services.py | 6 +---- 2 files changed, 20 insertions(+), 9 deletions(-) diff --git a/nova/api/openstack/compute/services.py b/nova/api/openstack/compute/services.py index b462b1a9d94..be35f62ee3f 100644 --- a/nova/api/openstack/compute/services.py +++ b/nova/api/openstack/compute/services.py @@ -267,10 +267,25 @@ def delete(self, req, id): # service delete since we could orphan resource providers and # break the ability to do things like confirm/revert instances # in VERIFY_RESIZE status. - compute_nodes = objects.ComputeNodeList.get_all_by_host( - context, service.host) - self._assert_no_in_progress_migrations( - context, id, compute_nodes) + compute_nodes = [] + try: + compute_nodes = objects.ComputeNodeList.get_all_by_host( + context, service.host) + self._assert_no_in_progress_migrations( + context, id, compute_nodes) + except exception.ComputeHostNotFound: + # NOTE(artom) Consider the following situation: + # - Using the Ironic virt driver + # - Replacing (so removing and re-adding) all baremetal + # nodes associated with a single nova-compute service + # The update resources periodic will have destroyed the + # compute node records because they're no longer being + # reported by the virt driver. If we then attempt to + # manually delete the compute service record, + # get_all_host() above will raise, as there are no longer + # any compute node records for the host. Catch it here and + # continue to allow compute service deletion. + pass aggrs = self.aggregate_api.get_aggregates_by_host(context, service.host) diff --git a/nova/tests/unit/api/openstack/compute/test_services.py b/nova/tests/unit/api/openstack/compute/test_services.py index 07acbaab16c..94263833c72 100644 --- a/nova/tests/unit/api/openstack/compute/test_services.py +++ b/nova/tests/unit/api/openstack/compute/test_services.py @@ -712,11 +712,7 @@ def test_services_delete_compute_host_not_found( 'topic': 'compute', 'report_count': 0}) compute.create() - # FIXME(artom) Until bug 1860312 is fixed, the ComputeHostNotFound - # error will get bubbled up to the API as an error 500. - self.assertRaises( - webob.exc.HTTPInternalServerError, - self.controller.delete, self.req, compute.id) + self.controller.delete(self.req, compute.id) mock_get_all_by_host.assert_called_with( self.req.environ['nova.context'], 'fake-compute-host') From 94e265f3ca615aa18de0081a76975019997b8709 Mon Sep 17 00:00:00 2001 From: melanie witt Date: Sat, 31 Jul 2021 00:30:16 +0000 Subject: [PATCH 21/40] Reduce mocking in test_reject_open_redirect for compat This is a followup for change Ie36401c782f023d1d5f2623732619105dc2cfa24 to reduce mocking in the unit test coverage for it. While backporting the bug fix, it was found to be incompatible with earlier versions of Python < 3.6 due to a difference in internal implementation [1]. This reduces the mocking in the unit test to be more agnostic to the internals of the StreamRequestHandler (ancestor of SimpleHTTPRequestHandler) and work across Python versions >= 2.7. Related-Bug: #1927677 [1] https://github.com/python/cpython/commit/34eeed42901666fce099947f93dfdfc05411f286 Change-Id: I546d376869a992601b443fb95acf1034da2a8f36 (cherry picked from commit 214cabe6848a1fdb4f5941d994c6cc11107fc4af) (cherry picked from commit 9c2f29783734cb5f9cb05a08d328c10e1d16c4f1) --- .../tests/unit/console/test_websocketproxy.py | 27 +++++++++---------- 1 file changed, 13 insertions(+), 14 deletions(-) diff --git a/nova/tests/unit/console/test_websocketproxy.py b/nova/tests/unit/console/test_websocketproxy.py index 4ed2d2d4dc0..11561f4c533 100644 --- a/nova/tests/unit/console/test_websocketproxy.py +++ b/nova/tests/unit/console/test_websocketproxy.py @@ -15,6 +15,7 @@ """Tests for nova websocketproxy.""" import copy +import io import socket import mock @@ -635,16 +636,6 @@ def test_reject_open_redirect(self): b'' ] - # Collect the response data to verify at the end. The - # SimpleHTTPRequestHandler writes the response data by calling the - # request socket sendall() method. - self.data = b'' - - def fake_sendall(data): - self.data += data - - mock_req.sendall.side_effect = fake_sendall - client_addr = ('8.8.8.8', 54321) mock_server = mock.MagicMock() # This specifies that the server will be able to handle requests other @@ -652,13 +643,21 @@ def fake_sendall(data): mock_server.only_upgrade = False # Constructing a handler will process the mock_req request passed in. - websocketproxy.NovaProxyRequestHandler( + handler = websocketproxy.NovaProxyRequestHandler( mock_req, client_addr, mock_server) + # Collect the response data to verify at the end. The + # SimpleHTTPRequestHandler writes the response data to a 'wfile' + # attribute. + output = io.BytesIO() + handler.wfile = output + # Process the mock_req again to do the capture. + handler.do_GET() + output.seek(0) + result = output.readlines() + # Verify no redirect happens and instead a 400 Bad Request is returned. - self.data = self.data.decode() - self.assertIn('Error code: 400', self.data) - self.assertIn('Message: URI must not start with //', self.data) + self.assertIn('400 URI must not start with //', result[0].decode()) @mock.patch('websockify.websocketproxy.select_ssl_version') def test_ssl_min_version_is_not_set(self, mock_select_ssl): From 7dbceeceef0ca3657c72341375afc639be0b5c02 Mon Sep 17 00:00:00 2001 From: Takashi Kajinami Date: Thu, 15 Jul 2021 11:25:15 +0900 Subject: [PATCH 22/40] Fix request path to query a resource provider by uuid To query a resource provider by uuid, request path should look like /resource_providers?uuid= istead of /resource_providers&uuid= This change fixes the wrong path so that "nova-manage placement audit" command can look up the target resource provider properly. Closes-Bug: #1936278 Change-Id: I2ae7e9782316e3662e4e51e3f5ba0ef597bf281b (cherry picked from commit 1d3373dcf0a05d4a2c5b51fc1b74d41ec1bb1175) (cherry picked from commit 62a3fa4fff70a1d03998626406a71b7dc09da733) --- nova/cmd/manage.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/nova/cmd/manage.py b/nova/cmd/manage.py index 92c48c71e68..e704336d51b 100644 --- a/nova/cmd/manage.py +++ b/nova/cmd/manage.py @@ -2668,7 +2668,7 @@ def _get_resource_providers(self, context, placement, **kwargs): """ url = '/resource_providers' if 'uuid' in kwargs: - url += '&uuid=%s' % kwargs['uuid'] + url += '?uuid=%s' % kwargs['uuid'] resp = placement.get(url, global_request_id=context.global_id, version='1.14') From 1eceeebfb25ccdacdb8c74c5171f0fa9e591ce82 Mon Sep 17 00:00:00 2001 From: Balazs Gibizer Date: Mon, 23 Aug 2021 17:13:14 +0200 Subject: [PATCH 23/40] Avoid modifying the Mock class in test The rdb unit test defines a shutdown field on the Mock class unintentionally. This causes that a later test in the same executor expecting that mock.Mock().shutdown is a newly auto generated mock.Mock() but it founds that is an already used (called) object. This causes that the later test fails when asserting the number of calls on that mock. The original intention of the rbd unit test was to catch the instantiation of the Rados object in test so it set Rados = mock.Mock() so when the code under test called Rados() it actually called Mock(). It worked but it has that huge side effect. Instead of this a proper mocking of the constructor can be done in two steps: rados_inst = mock.Mock() Rados = mock.Mock(return_value=rados_inst) This makes sure that every Rados() call will return rados_inst that is a mock without causing Mock class level side effect. Change-Id: If71620e808744736cb4fe3abda76d81a6335311b Closes-Bug: #1936849 (cherry picked from commit 930b7c992156733fbb4f598488605825d62ebc0c) (cherry picked from commit c958fab901be97999c0d117faa31ab53e52a3371) --- nova/tests/unit/storage/test_rbd.py | 42 ++++++++++++++--------------- 1 file changed, 21 insertions(+), 21 deletions(-) diff --git a/nova/tests/unit/storage/test_rbd.py b/nova/tests/unit/storage/test_rbd.py index 65796ebc1fb..396f22c643a 100644 --- a/nova/tests/unit/storage/test_rbd.py +++ b/nova/tests/unit/storage/test_rbd.py @@ -119,13 +119,15 @@ def setUp(self): rados_patcher = mock.patch.object(rbd_utils, 'rados') self.mock_rados = rados_patcher.start() self.addCleanup(rados_patcher.stop) - self.mock_rados.Rados = mock.Mock - self.mock_rados.Rados.ioctx = mock.Mock() - self.mock_rados.Rados.connect = mock.Mock() - self.mock_rados.Rados.shutdown = mock.Mock() - self.mock_rados.Rados.open_ioctx = mock.Mock() - self.mock_rados.Rados.open_ioctx.return_value = \ - self.mock_rados.Rados.ioctx + self.mock_rados.Rados = mock.Mock() + self.rados_inst = mock.Mock() + self.mock_rados.Rados.return_value = self.rados_inst + self.rados_inst.ioctx = mock.Mock() + self.rados_inst.connect = mock.Mock() + self.rados_inst.shutdown = mock.Mock() + self.rados_inst.open_ioctx = mock.Mock() + self.rados_inst.open_ioctx.return_value = \ + self.rados_inst.ioctx self.mock_rados.Error = Exception rbd_patcher = mock.patch.object(rbd_utils, 'rbd') @@ -339,33 +341,31 @@ def test_rbd_volume_proxy_init(self, mock_connect_from_rados, def test_connect_to_rados_default(self): ret = self.driver._connect_to_rados() - self.mock_rados.Rados.connect.assert_called_once_with( + self.rados_inst.connect.assert_called_once_with( timeout=self.rbd_connect_timeout) - self.assertTrue(self.mock_rados.Rados.open_ioctx.called) - self.assertIsInstance(ret[0], self.mock_rados.Rados) - self.assertEqual(self.mock_rados.Rados.ioctx, ret[1]) - self.mock_rados.Rados.open_ioctx.assert_called_with(self.rbd_pool) + self.assertTrue(self.rados_inst.open_ioctx.called) + self.assertEqual(self.rados_inst.ioctx, ret[1]) + self.rados_inst.open_ioctx.assert_called_with(self.rbd_pool) def test_connect_to_rados_different_pool(self): ret = self.driver._connect_to_rados('alt_pool') - self.mock_rados.Rados.connect.assert_called_once_with( + self.rados_inst.connect.assert_called_once_with( timeout=self.rbd_connect_timeout) - self.assertTrue(self.mock_rados.Rados.open_ioctx.called) - self.assertIsInstance(ret[0], self.mock_rados.Rados) - self.assertEqual(self.mock_rados.Rados.ioctx, ret[1]) - self.mock_rados.Rados.open_ioctx.assert_called_with('alt_pool') + self.assertTrue(self.rados_inst.open_ioctx.called) + self.assertEqual(self.rados_inst.ioctx, ret[1]) + self.rados_inst.open_ioctx.assert_called_with('alt_pool') def test_connect_to_rados_error(self): - self.mock_rados.Rados.open_ioctx.side_effect = self.mock_rados.Error + self.rados_inst.open_ioctx.side_effect = self.mock_rados.Error self.assertRaises(self.mock_rados.Error, self.driver._connect_to_rados) - self.mock_rados.Rados.open_ioctx.assert_called_once_with( + self.rados_inst.open_ioctx.assert_called_once_with( self.rbd_pool) - self.mock_rados.Rados.shutdown.assert_called_once_with() + self.rados_inst.shutdown.assert_called_once_with() def test_connect_to_rados_unicode_arg(self): self.driver._connect_to_rados(u'unicode_pool') - self.mock_rados.Rados.open_ioctx.assert_called_with( + self.rados_inst.open_ioctx.assert_called_with( test.MatchType(str)) def test_ceph_args_none(self): From aaa56240b0311ad47ccccc3b7850ddc5b0a21702 Mon Sep 17 00:00:00 2001 From: Rodrigo Barbieri Date: Wed, 11 Aug 2021 16:03:58 -0300 Subject: [PATCH 24/40] Fix 1vcpu error with multiqueue and vif_type=tap Fix for bug #1893263 introduced a regression where 1 vcpu instances would fail to build when paired with multiqueue-enabled images, in the scenario vif_type=tap. Solution is to not pass multiqueue parameter when instances.get_flavor().vcpus = 1. Closes-bug: #1939604 Change-Id: Iaccf2eeeb6e8bb80c658f51ce9ab4e8eb4093a55 (cherry picked from commit 7fc6fe6fae891eae42b36ccb9d69cd0f6d6db21d) (cherry picked from commit aa5b8d12bcacc01e5f9be45cc1eef24ac9efd2fc) --- nova/tests/unit/virt/libvirt/test_vif.py | 48 ++++++++++++++----- nova/virt/libvirt/vif.py | 3 +- .../notes/bug-1939604-547c729b7741831b.yaml | 5 ++ 3 files changed, 43 insertions(+), 13 deletions(-) create mode 100644 releasenotes/notes/bug-1939604-547c729b7741831b.yaml diff --git a/nova/tests/unit/virt/libvirt/test_vif.py b/nova/tests/unit/virt/libvirt/test_vif.py index b77d704bab6..24df5d5aa74 100644 --- a/nova/tests/unit/virt/libvirt/test_vif.py +++ b/nova/tests/unit/virt/libvirt/test_vif.py @@ -379,6 +379,10 @@ class LibvirtVifTestCase(test.NoDBTestCase): uuid='f0000000-0000-0000-0000-000000000001', project_id=723) + flavor_1vcpu = objects.Flavor(vcpus=1, memory=512, root_gb=1) + + flavor_2vcpu = objects.Flavor(vcpus=2, memory=512, root_gb=1) + bandwidth = { 'quota:vif_inbound_peak': '200', 'quota:vif_outbound_peak': '20', @@ -1063,30 +1067,50 @@ def test_tap_ethernet_vif_driver(self): @mock.patch('nova.privsep.linux_net.device_exists', return_value=True) @mock.patch('nova.privsep.linux_net.set_device_mtu') @mock.patch('nova.privsep.linux_net.create_tap_dev') - def test_plug_tap_kvm_virtio(self, mock_create_tap_dev, mock_set_mtu, - mock_device_exists): + def test_plug_tap_kvm_virtio( + self, mock_create_tap_dev, mock_set_mtu, mock_device_exists): d1 = vif.LibvirtGenericVIFDriver() ins = objects.Instance( id=1, uuid='f0000000-0000-0000-0000-000000000001', + flavor=self.flavor_2vcpu, project_id=723, system_metadata={} ) d1.plug(ins, self.vif_tap) - mock_create_tap_dev.assert_called_once_with('tap-xxx-yyy-zzz', None, - multiqueue=False) + mock_create_tap_dev.assert_called_once_with( + 'tap-xxx-yyy-zzz', None, multiqueue=False) mock_create_tap_dev.reset_mock() d2 = vif.LibvirtGenericVIFDriver() mq_ins = objects.Instance( id=1, uuid='f0000000-0000-0000-0000-000000000001', + flavor=self.flavor_2vcpu, project_id=723, system_metadata={ 'image_hw_vif_multiqueue_enabled': 'True' } ) d2.plug(mq_ins, self.vif_tap) - mock_create_tap_dev.assert_called_once_with('tap-xxx-yyy-zzz', None, - multiqueue=True) + mock_create_tap_dev.assert_called_once_with( + 'tap-xxx-yyy-zzz', None, multiqueue=True) + + @mock.patch('nova.privsep.linux_net.device_exists', return_value=True) + @mock.patch('nova.privsep.linux_net.set_device_mtu') + @mock.patch('nova.privsep.linux_net.create_tap_dev') + def test_plug_tap_mq_ignored_1vcpu( + self, mock_create_tap_dev, mock_set_mtu, mock_device_exists): + + d1 = vif.LibvirtGenericVIFDriver() + mq_ins = objects.Instance( + id=1, uuid='f0000000-0000-0000-0000-000000000001', + image_ref=uuids.image_ref, flavor=self.flavor_1vcpu, + project_id=723, system_metadata={ + 'image_hw_vif_multiqueue_enabled': 'True', + } + ) + d1.plug(mq_ins, self.vif_tap) + mock_create_tap_dev.assert_called_once_with( + 'tap-xxx-yyy-zzz', None, multiqueue=False) @mock.patch('nova.privsep.linux_net.device_exists', return_value=True) @mock.patch('nova.privsep.linux_net.set_device_mtu') @@ -1101,14 +1125,14 @@ def test_plug_tap_mq_ignored_virt_type( d1 = vif.LibvirtGenericVIFDriver() ins = objects.Instance( id=1, uuid='f0000000-0000-0000-0000-000000000001', + flavor=self.flavor_2vcpu, project_id=723, system_metadata={ 'image_hw_vif_multiqueue_enabled': 'True' } ) d1.plug(ins, self.vif_tap) - mock_create_tap_dev.assert_called_once_with('tap-xxx-yyy-zzz', - None, - multiqueue=False) + mock_create_tap_dev.assert_called_once_with( + 'tap-xxx-yyy-zzz', None, multiqueue=False) @mock.patch('nova.privsep.linux_net.device_exists', return_value=True) @mock.patch('nova.privsep.linux_net.set_device_mtu') @@ -1119,15 +1143,15 @@ def test_plug_tap_mq_ignored_vif_model( d1 = vif.LibvirtGenericVIFDriver() ins = objects.Instance( id=1, uuid='f0000000-0000-0000-0000-000000000001', + flavor=self.flavor_2vcpu, project_id=723, system_metadata={ 'image_hw_vif_multiqueue_enabled': 'True', 'image_hw_vif_model': 'e1000', } ) d1.plug(ins, self.vif_tap) - mock_create_tap_dev.assert_called_once_with('tap-xxx-yyy-zzz', - None, - multiqueue=False) + mock_create_tap_dev.assert_called_once_with( + 'tap-xxx-yyy-zzz', None, multiqueue=False) def test_unplug_tap(self): d = vif.LibvirtGenericVIFDriver() diff --git a/nova/virt/libvirt/vif.py b/nova/virt/libvirt/vif.py index 5c87223bafe..67e0771ad93 100644 --- a/nova/virt/libvirt/vif.py +++ b/nova/virt/libvirt/vif.py @@ -666,7 +666,8 @@ def plug_tap(self, instance, vif): vif_model = self.get_vif_model(image_meta=image_meta) # TODO(ganso): explore whether multiqueue works for other vif models # that go through this code path. - multiqueue = (self._requests_multiqueue(image_meta) and + multiqueue = (instance.get_flavor().vcpus > 1 and + self._requests_multiqueue(image_meta) and vif_model == network_model.VIF_MODEL_VIRTIO) nova.privsep.linux_net.create_tap_dev(dev, mac, multiqueue=multiqueue) network = vif.get('network') diff --git a/releasenotes/notes/bug-1939604-547c729b7741831b.yaml b/releasenotes/notes/bug-1939604-547c729b7741831b.yaml new file mode 100644 index 00000000000..e14327c2853 --- /dev/null +++ b/releasenotes/notes/bug-1939604-547c729b7741831b.yaml @@ -0,0 +1,5 @@ +--- +fixes: + - | + Addressed an issue that prevented instances with 1 vcpu using multiqueue + feature from being created successfully when their vif_type is TAP. From 9588cdbfd4649ea53d60303f2d10c5d62a070a07 Mon Sep 17 00:00:00 2001 From: Sean Mooney Date: Mon, 23 Aug 2021 15:37:48 +0100 Subject: [PATCH 25/40] address open redirect with 3 forward slashes Ie36401c782f023d1d5f2623732619105dc2cfa24 was intended to address OSSA-2021-002 (CVE-2021-3654) however after its release it was discovered that the fix only worked for urls with 2 leading slashes or more then 4. This change adresses the missing edgecase for 3 leading slashes and also maintian support for rejecting 2+. Conflicts: nova/tests/unit/console/test_websocketproxy.py NOTE: conflict is due to I58b0382c86d4ef798572edb63d311e0e3e6937bb is missing in Victoria and Ie36401c782f023d1d5f2623732619105dc2cfa24 backport contained conflicts and methods order was swapped. Change-Id: I95f68be76330ff09e5eabb5ef8dd9a18f5547866 co-authored-by: Matteo Pozza Closes-Bug: #1927677 (cherry picked from commit 6fbd0b758dcac71323f3be179b1a9d1c17a4acc5) (cherry picked from commit 47dad4836a26292e9d34e516e1525ecf00be127c) --- nova/console/websocketproxy.py | 7 +--- .../tests/unit/console/test_websocketproxy.py | 34 +++++++++++++++++++ 2 files changed, 35 insertions(+), 6 deletions(-) diff --git a/nova/console/websocketproxy.py b/nova/console/websocketproxy.py index 1410d642608..8512ac62ad7 100644 --- a/nova/console/websocketproxy.py +++ b/nova/console/websocketproxy.py @@ -290,14 +290,9 @@ def send_head(self): if os.path.isdir(path): parts = urlparse.urlsplit(self.path) if not parts.path.endswith('/'): - # redirect browser - doing basically what apache does - new_parts = (parts[0], parts[1], parts[2] + '/', - parts[3], parts[4]) - new_url = urlparse.urlunsplit(new_parts) - # Browsers interpret "Location: //uri" as an absolute URI # like "http://URI" - if new_url.startswith('//'): + if self.path.startswith('//'): self.send_error(HTTPStatus.BAD_REQUEST, "URI must not start with //") return None diff --git a/nova/tests/unit/console/test_websocketproxy.py b/nova/tests/unit/console/test_websocketproxy.py index 4ed2d2d4dc0..3ec73d817af 100644 --- a/nova/tests/unit/console/test_websocketproxy.py +++ b/nova/tests/unit/console/test_websocketproxy.py @@ -660,6 +660,40 @@ def fake_sendall(data): self.assertIn('Error code: 400', self.data) self.assertIn('Message: URI must not start with //', self.data) + def test_reject_open_redirect_3_slashes(self): + # This will test the behavior when an attempt is made to cause an open + # redirect. It should be rejected. + mock_req = mock.MagicMock() + mock_req.makefile().readline.side_effect = [ + b'GET ///example.com/%2F.. HTTP/1.1\r\n', + b'' + ] + + # Collect the response data to verify at the end. The + # SimpleHTTPRequestHandler writes the response data by calling the + # request socket sendall() method. + self.data = b'' + + def fake_sendall(data): + self.data += data + + mock_req.sendall.side_effect = fake_sendall + + client_addr = ('8.8.8.8', 54321) + mock_server = mock.MagicMock() + # This specifies that the server will be able to handle requests other + # than only websockets. + mock_server.only_upgrade = False + + # Constructing a handler will process the mock_req request passed in. + websocketproxy.NovaProxyRequestHandler( + mock_req, client_addr, mock_server) + + # Verify no redirect happens and instead a 400 Bad Request is returned. + self.data = self.data.decode() + self.assertIn('Error code: 400', self.data) + self.assertIn('Message: URI must not start with //', self.data) + @mock.patch('websockify.websocketproxy.select_ssl_version') def test_ssl_min_version_is_not_set(self, mock_select_ssl): websocketproxy.NovaWebSocketProxy() From 0b1fa9b4ae01114299c5225d66e1f6eba25be43e Mon Sep 17 00:00:00 2001 From: Balazs Gibizer Date: Thu, 23 Sep 2021 20:05:45 +0200 Subject: [PATCH 26/40] Reproduce bug 1944759 Add functional tests to reproduce the race between resize_instance() and update_available_resources(). Related-Bug: #1944759 Change-Id: Icb7e3379248fe00f9a94f9860181b5de44902379 (cherry picked from commit 3e4e4489b7a6e9cdefcc6ff02ed99a0a70420fca) (cherry picked from commit e6c6880465824f1e327a54143f32bb5a5816ff6c) (cherry picked from commit 140ae45d98dabd30aef5c0ac075346de4eabcea1) --- .../functional/libvirt/test_numa_servers.py | 121 ++++++++++++++++++ 1 file changed, 121 insertions(+) diff --git a/nova/tests/functional/libvirt/test_numa_servers.py b/nova/tests/functional/libvirt/test_numa_servers.py index 28f8463aeae..90afeb763c0 100644 --- a/nova/tests/functional/libvirt/test_numa_servers.py +++ b/nova/tests/functional/libvirt/test_numa_servers.py @@ -711,6 +711,127 @@ def fake_confirm_migration(*args, **kwargs): server = self._wait_for_state_change(server, 'ACTIVE') + def _assert_pinned_cpus(self, hostname, expected_number_of_pinned): + numa_topology = objects.NUMATopology.obj_from_db_obj( + objects.ComputeNode.get_by_nodename( + self.ctxt, hostname, + ).numa_topology, + ) + self.assertEqual( + expected_number_of_pinned, len(numa_topology.cells[0].pinned_cpus)) + + def _create_server_and_resize_bug_1944759(self): + self.flags( + cpu_dedicated_set='0-3', cpu_shared_set='4-7', group='compute') + self.flags(vcpu_pin_set=None) + + # start services + self.start_compute(hostname='test_compute0') + self.start_compute(hostname='test_compute1') + + flavor_a_id = self._create_flavor( + vcpu=2, extra_spec={'hw:cpu_policy': 'dedicated'}) + server = self._create_server(flavor_id=flavor_a_id) + + src_host = server['OS-EXT-SRV-ATTR:host'] + self._assert_pinned_cpus(src_host, 2) + + # we don't really care what the new flavor is, so long as the old + # flavor is using pinning. We use a similar flavor for simplicity. + flavor_b_id = self._create_flavor( + vcpu=2, extra_spec={'hw:cpu_policy': 'dedicated'}) + + orig_rpc_finish_resize = nova.compute.rpcapi.ComputeAPI.finish_resize + + # Simulate that the finish_resize call overlaps with an + # update_available_resource periodic job + def inject_periodic_to_finish_resize(*args, **kwargs): + self._run_periodics() + return orig_rpc_finish_resize(*args, **kwargs) + + self.stub_out( + 'nova.compute.rpcapi.ComputeAPI.finish_resize', + inject_periodic_to_finish_resize, + ) + + # TODO(stephenfin): The mock of 'migrate_disk_and_power_off' should + # probably be less...dumb + with mock.patch( + 'nova.virt.libvirt.driver.LibvirtDriver' + '.migrate_disk_and_power_off', return_value='{}', + ): + post = {'resize': {'flavorRef': flavor_b_id}} + self.api.post_server_action(server['id'], post) + server = self._wait_for_state_change(server, 'VERIFY_RESIZE') + + dst_host = server['OS-EXT-SRV-ATTR:host'] + + # This is a resource accounting bug, we should have 2 cpus pinned on + # both computes. The source should have it due to the outbound + # migration and the destination due to the instance running there + self._assert_pinned_cpus(src_host, 0) + self._assert_pinned_cpus(dst_host, 2) + + return server, src_host, dst_host + + def test_resize_confirm_bug_1944759(self): + server, src_host, dst_host = ( + self._create_server_and_resize_bug_1944759()) + + # Now confirm the resize + post = {'confirmResize': None} + + # FIXME(gibi): This is bug 1944759 where during resize, on the source + # node the resize_instance() call at the point of calling finish_resize + # overlaps with a update_available_resources() periodic job. This + # causes that the periodic job will not track the migration nor the + # instance and therefore freeing the resource allocation. Then when + # later the resize is confirmed the confirm_resize on the source + # compute also wants to free up the resources, the pinned CPUs, and it + # fails as they are already freed. + exc = self.assertRaises( + client.OpenStackApiException, + self.api.post_server_action, server['id'], post + ) + self.assertEqual(500, exc.response.status_code) + self.assertIn('CPUUnpinningInvalid', str(exc)) + + # confirm failed above but the resource allocation reflects that the + # VM is running on the dest node + self._assert_pinned_cpus(src_host, 0) + self._assert_pinned_cpus(dst_host, 2) + + self._run_periodics() + + # and such allocation situation is stable so as a recovery the VM + # can be reset-state to ACTIVE without problem. + self._assert_pinned_cpus(src_host, 0) + self._assert_pinned_cpus(dst_host, 2) + + def test_resize_revert_bug_1944759(self): + server, src_host, dst_host = ( + self._create_server_and_resize_bug_1944759()) + + # Now revert the resize + post = {'revertResize': None} + + # reverts actually succeeds (not like confirm) but the resource + # allocation is still flaky + self.api.post_server_action(server['id'], post) + self._wait_for_state_change(server, 'ACTIVE') + + # This is a resource accounting bug. After the revert the source host + # should have 2 cpus pinned due to the instance. + self._assert_pinned_cpus(src_host, 0) + self._assert_pinned_cpus(dst_host, 0) + + # running the periodic job will fix the resource accounting + self._run_periodics() + + # this is now correct + self._assert_pinned_cpus(src_host, 2) + self._assert_pinned_cpus(dst_host, 0) + class NUMAServerTestWithCountingQuotaFromPlacement(NUMAServersTest): From 34e0c0205b1053d3bbe064177740aba654997fe0 Mon Sep 17 00:00:00 2001 From: Balazs Gibizer Date: Fri, 24 Sep 2021 15:17:28 +0200 Subject: [PATCH 27/40] Store old_flavor already on source host during resize During resize, on the source host, in resize_instance(), the instance.host and .node is updated to point to the destination host. This indicates to the source host's resource tracker that the allocation of this instance does not need to be tracked as an instance but as an outbound migration instead. However for the source host's resource tracker to do that it, needs to use the instance.old_flavor. Unfortunately the instance.old_flavor is only set during finish_resize() on the destination host. (resize_instance cast to the finish_resize). So it is possible that a running resize_instance() set the instance.host to point to the destination and then before the finish_resize could set the old_flavor an update_available_resources periodic runs on the source host. This causes that the allocation of this instance is not tracked as an instance as the instance.host point to the destination but it is not tracked as a migration either as the instance.old_flavor is not yet set. So the allocation on the source host is simply dropped by the periodic job. When such migration is confirmed the confirm_resize() tries to drop the same resource allocation again but fails as the pinned CPUs of the instance already freed. When such migration is reverted instead, then revert succeeds but the source host resource allocation will not contain the resource allocation of the instance until the next update_available_resources periodic runs and corrects it. This does not affect resources tracked exclusively in placement (e.g. VCPU, MEMORY_MB, DISK_GB) but it does affect NUMA related resource that are still tracked in the resource tracker (e.g. huge pages, pinned CPUs). This patch moves the instance.old_flavor setting to the source node to the same transaction that sets the instance.host to point to the destination host. Hence solving the race condition. Change-Id: Ic0d6c59147abe5e094e8f13e0d3523b178daeba9 Closes-Bug: #1944759 (cherry picked from commit b841e553214be9a732703e2dfed6c97698ef9b71) (cherry picked from commit d4edcd62bae44c01885268a6cf7b7fae92617060) (cherry picked from commit c8b04d183f560a616a79577c4d4ae9465d03e541) --- nova/compute/manager.py | 12 ++++++ .../functional/libvirt/test_numa_servers.py | 40 ++++++------------- 2 files changed, 25 insertions(+), 27 deletions(-) diff --git a/nova/compute/manager.py b/nova/compute/manager.py index 72e88ce6e6e..281f2733aab 100644 --- a/nova/compute/manager.py +++ b/nova/compute/manager.py @@ -5654,6 +5654,14 @@ def _resize_instance(self, context, instance, image, instance.host = migration.dest_compute instance.node = migration.dest_node + # NOTE(gibi): as the instance now tracked on the destination we + # have to make sure that the source compute resource track can + # track this instance as a migration. For that the resource tracker + # needs to see the old_flavor set on the instance. The old_flavor + # setting used to be done on the destination host in finish_resize + # but that is racy with a source host update_available_resource + # periodic run + instance.old_flavor = instance.flavor instance.task_state = task_states.RESIZE_MIGRATED instance.save(expected_task_state=task_states.RESIZE_MIGRATING) @@ -5767,6 +5775,10 @@ def _finish_resize(self, context, instance, migration, disk_info, # to ACTIVE for backwards compatibility old_vm_state = instance.system_metadata.get('old_vm_state', vm_states.ACTIVE) + # NOTE(gibi): this is already set by the resize_instance on the source + # node before calling finish_resize on destination but during upgrade + # it can be that the source node is not having the fix for bug 1944759 + # yet. This assignment can be removed in Z release. instance.old_flavor = old_flavor if old_instance_type_id != new_instance_type_id: diff --git a/nova/tests/functional/libvirt/test_numa_servers.py b/nova/tests/functional/libvirt/test_numa_servers.py index 90afeb763c0..144bad33c82 100644 --- a/nova/tests/functional/libvirt/test_numa_servers.py +++ b/nova/tests/functional/libvirt/test_numa_servers.py @@ -766,10 +766,10 @@ def inject_periodic_to_finish_resize(*args, **kwargs): dst_host = server['OS-EXT-SRV-ATTR:host'] - # This is a resource accounting bug, we should have 2 cpus pinned on - # both computes. The source should have it due to the outbound - # migration and the destination due to the instance running there - self._assert_pinned_cpus(src_host, 0) + # we have 2 cpus pinned on both computes. The source should have it + # due to the outbound migration and the destination due to the + # instance running there + self._assert_pinned_cpus(src_host, 2) self._assert_pinned_cpus(dst_host, 2) return server, src_host, dst_host @@ -781,30 +781,17 @@ def test_resize_confirm_bug_1944759(self): # Now confirm the resize post = {'confirmResize': None} - # FIXME(gibi): This is bug 1944759 where during resize, on the source - # node the resize_instance() call at the point of calling finish_resize - # overlaps with a update_available_resources() periodic job. This - # causes that the periodic job will not track the migration nor the - # instance and therefore freeing the resource allocation. Then when - # later the resize is confirmed the confirm_resize on the source - # compute also wants to free up the resources, the pinned CPUs, and it - # fails as they are already freed. - exc = self.assertRaises( - client.OpenStackApiException, - self.api.post_server_action, server['id'], post - ) - self.assertEqual(500, exc.response.status_code) - self.assertIn('CPUUnpinningInvalid', str(exc)) + self.api.post_server_action(server['id'], post) + self._wait_for_state_change(server, 'ACTIVE') - # confirm failed above but the resource allocation reflects that the - # VM is running on the dest node + # the resource allocation reflects that the VM is running on the dest + # node self._assert_pinned_cpus(src_host, 0) self._assert_pinned_cpus(dst_host, 2) + # and running periodics does not break it either self._run_periodics() - # and such allocation situation is stable so as a recovery the VM - # can be reset-state to ACTIVE without problem. self._assert_pinned_cpus(src_host, 0) self._assert_pinned_cpus(dst_host, 2) @@ -820,15 +807,14 @@ def test_resize_revert_bug_1944759(self): self.api.post_server_action(server['id'], post) self._wait_for_state_change(server, 'ACTIVE') - # This is a resource accounting bug. After the revert the source host - # should have 2 cpus pinned due to the instance. - self._assert_pinned_cpus(src_host, 0) + # After the revert the source host should have 2 cpus pinned due to + # the instance. + self._assert_pinned_cpus(src_host, 2) self._assert_pinned_cpus(dst_host, 0) - # running the periodic job will fix the resource accounting + # running the periodic job will not break it either self._run_periodics() - # this is now correct self._assert_pinned_cpus(src_host, 2) self._assert_pinned_cpus(dst_host, 0) From c531fdcc192afb5af628ac567cb0ff8aa3eab052 Mon Sep 17 00:00:00 2001 From: Balazs Gibizer Date: Mon, 11 Oct 2021 14:41:37 +0200 Subject: [PATCH 28/40] Add a WA flag waiting for vif-plugged event during reboot The libvirt driver power on and hard reboot destroys the domain first and unplugs the vifs then recreate the domain and replug the vifs. However nova does not wait for the network-vif-plugged event before unpause the domain. This can cause that the domain starts running and requesting IP via DHCP before the networking backend finished plugging the vifs. So this patch adds a workaround config option to nova to wait for network-vif-plugged events during hard reboot the same way as nova waits for this event during new instance spawn. This logic cannot be enabled unconditionally as not all neutron networking backend sending plug time events to wait for. Also the logic needs to be vnic_type dependent as ml2/ovs and the in tree sriov backend often deployed together on the same compute. While ml2/ovs sends plug time event the sriov backend does not send it reliably. So the configuration is not just a boolean flag but a list of vnic_types instead. This way the waiting for the plug time event for a vif that is handled by ml2/ovs is possible while the instance has other vifs handled by the sriov backend where no event can be expected. Conflicts: nova/virt/libvirt/driver.py both I73305e82da5d8da548961b801a8e75fb0e8c4cf1 and I0b93bdc12cdce591c7e642ab8830e92445467b9a are not in stable/victoria The stable/victoria specific changes: * The list of accepted vnic_type-s are adapted to what is supported by neutron on this release. So vdpa, accelerator-direct, and accelerator-direct-physical are removed as they are only added in stable/wallaby Change-Id: Ie904d1513b5cf76d6d5f6877545e8eb378dd5499 Closes-Bug: #1946729 (cherry picked from commit 68c970ea9915a95f9828239006559b84e4ba2581) (cherry picked from commit 0c41bfb8c5c60f1cc930ae432e6be460ee2e97ac) (cherry picked from commit 89c4ff5f7b45f1a5bed8b6b9b4586fceaa391bfb) --- .zuul.yaml | 6 +++ nova/conf/workarounds.py | 53 +++++++++++++++++++ nova/tests/unit/virt/libvirt/test_driver.py | 43 ++++++++++++++- nova/virt/libvirt/driver.py | 23 +++++++- ...t-during-hard-reboot-fb491f6a68370bab.yaml | 18 +++++++ 5 files changed, 141 insertions(+), 2 deletions(-) create mode 100644 releasenotes/notes/bug-1946729-wait-for-vif-plugged-event-during-hard-reboot-fb491f6a68370bab.yaml diff --git a/.zuul.yaml b/.zuul.yaml index c00865e5044..aa371db06c1 100644 --- a/.zuul.yaml +++ b/.zuul.yaml @@ -191,6 +191,12 @@ # reduce the number of placement calls in steady state. Added in # Stein. resource_provider_association_refresh: 0 + workarounds: + # This wa is an improvement on hard reboot that cannot be turned + # on unconditionally. But we know that ml2/ovs sends plug time + # events so we can enable this in this ovs job for vnic_type + # normal + wait_for_vif_plugged_event_during_hard_reboot: normal $NOVA_CONF: quota: # Added in Train. diff --git a/nova/conf/workarounds.py b/nova/conf/workarounds.py index 8eadc0b6ec3..4e64d875787 100644 --- a/nova/conf/workarounds.py +++ b/nova/conf/workarounds.py @@ -345,6 +345,59 @@ * :oslo.config:option:`DEFAULT.instances_path` * :oslo.config:option:`image_cache.subdirectory_name` * :oslo.config:option:`update_resources_interval` +"""), + cfg.ListOpt('wait_for_vif_plugged_event_during_hard_reboot', + item_type=cfg.types.String( + choices=[ + "normal", + "direct", + "macvtap", + "baremetal", + "direct-physical", + "virtio-forwarder", + "smart-nic", + ]), + default=[], + help=""" +The libvirt virt driver implements power on and hard reboot by tearing down +every vif of the instance being rebooted then plug them again. By default nova +does not wait for network-vif-plugged event from neutron before it lets the +instance run. This can cause the instance to requests the IP via DHCP before +the neutron backend has a chance to set up the networking backend after the vif +plug. + +This flag defines which vifs nova expects network-vif-plugged events from +during hard reboot. The possible values are neutron port vnic types: + +* normal +* direct +* macvtap +* baremetal +* direct-physical +* virtio-forwarder +* smart-nic + +Adding a ``vnic_type`` to this configuration makes Nova wait for a +network-vif-plugged event for each of the instance's vifs having the specific +``vnic_type`` before unpausing the instance, similarly to how new instance +creation works. + +Please note that not all neutron networking backends send plug time events, for +certain ``vnic_type`` therefore this config is empty by default. + +The ml2/ovs and the networking-odl backends are known to send plug time events +for ports with ``normal`` ``vnic_type`` so it is safe to add ``normal`` to this +config if you are using only those backends in the compute host. + +The neutron in-tree SRIOV backend does not reliably send network-vif-plugged +event during plug time for ports with ``direct`` vnic_type and never sends +that event for port with ``direct-physical`` vnic_type during plug time. For +other ``vnic_type`` and backend pairs, please consult the developers of the +backend. + +Related options: + +* :oslo.config:option:`DEFAULT.vif_plugging_timeout` """), ] diff --git a/nova/tests/unit/virt/libvirt/test_driver.py b/nova/tests/unit/virt/libvirt/test_driver.py index 3a93eaa2a65..7576a6a60d0 100644 --- a/nova/tests/unit/virt/libvirt/test_driver.py +++ b/nova/tests/unit/virt/libvirt/test_driver.py @@ -16288,7 +16288,48 @@ def test_hard_reboot(self, mock_get_mdev, mock_destroy, mock_get_disk_info, accel_info=accel_info) mock_create_guest_with_network.assert_called_once_with(self.context, dummyxml, instance, network_info, block_device_info, - vifs_already_plugged=True) + vifs_already_plugged=True, external_events=[]) + + @mock.patch('oslo_utils.fileutils.ensure_tree', new=mock.Mock()) + @mock.patch('nova.virt.libvirt.LibvirtDriver.get_info') + @mock.patch('nova.virt.libvirt.LibvirtDriver._create_guest_with_network') + @mock.patch('nova.virt.libvirt.LibvirtDriver._get_guest_xml') + @mock.patch('nova.virt.libvirt.LibvirtDriver.destroy', new=mock.Mock()) + @mock.patch( + 'nova.virt.libvirt.LibvirtDriver._get_all_assigned_mediated_devices', + new=mock.Mock(return_value={})) + def test_hard_reboot_wait_for_plug( + self, mock_get_guest_xml, mock_create_guest_with_network, mock_get_info + ): + self.flags( + group="workarounds", + wait_for_vif_plugged_event_during_hard_reboot=["normal"]) + self.context.auth_token = None + instance = objects.Instance(**self.test_instance) + network_info = _fake_network_info(self, num_networks=4) + network_info[0]["vnic_type"] = "normal" + network_info[1]["vnic_type"] = "direct" + network_info[2]["vnic_type"] = "normal" + network_info[3]["vnic_type"] = "direct-physical" + block_device_info = None + return_values = [hardware.InstanceInfo(state=power_state.SHUTDOWN), + hardware.InstanceInfo(state=power_state.RUNNING)] + mock_get_info.side_effect = return_values + mock_get_guest_xml.return_value = mock.sentinel.xml + + drvr = libvirt_driver.LibvirtDriver(fake.FakeVirtAPI(), False) + drvr._hard_reboot( + self.context, instance, network_info, block_device_info) + + mock_create_guest_with_network.assert_called_once_with( + self.context, mock.sentinel.xml, instance, network_info, + block_device_info, + vifs_already_plugged=False, + external_events=[ + ('network-vif-plugged', uuids.vif1), + ('network-vif-plugged', uuids.vif3), + ] + ) @mock.patch('oslo_utils.fileutils.ensure_tree') @mock.patch('oslo_service.loopingcall.FixedIntervalLoopingCall') diff --git a/nova/virt/libvirt/driver.py b/nova/virt/libvirt/driver.py index fbd033690a6..2558a49f7d3 100644 --- a/nova/virt/libvirt/driver.py +++ b/nova/virt/libvirt/driver.py @@ -3383,11 +3383,32 @@ def _hard_reboot(self, context, instance, network_info, # on which vif type we're using and we are working with a stale network # info cache here, so won't rely on waiting for neutron plug events. # vifs_already_plugged=True means "do not wait for neutron plug events" + external_events = [] + vifs_already_plugged = True + event_expected_for_vnic_types = ( + CONF.workarounds.wait_for_vif_plugged_event_during_hard_reboot) + if event_expected_for_vnic_types: + # NOTE(gibi): We unplugged every vif during destroy above and we + # will replug them with _create_guest_with_network. As the + # workaround config has some vnic_types configured we expect + # vif-plugged events for every vif with those vnic_types. + # TODO(gibi): only wait for events if we know that the networking + # backend sends plug time events. For that we need to finish + # https://bugs.launchpad.net/neutron/+bug/1821058 first in Neutron + # then create a driver -> plug-time event mapping in nova. + external_events = [ + ('network-vif-plugged', vif['id']) + for vif in network_info + if vif['vnic_type'] in event_expected_for_vnic_types + ] + vifs_already_plugged = False + # NOTE(efried): The instance should already have a vtpm_secret_uuid # registered if appropriate. self._create_guest_with_network( context, xml, instance, network_info, block_device_info, - vifs_already_plugged=True) + vifs_already_plugged=vifs_already_plugged, + external_events=external_events) self._prepare_pci_devices_for_use( pci_manager.get_instance_pci_devs(instance, 'all')) diff --git a/releasenotes/notes/bug-1946729-wait-for-vif-plugged-event-during-hard-reboot-fb491f6a68370bab.yaml b/releasenotes/notes/bug-1946729-wait-for-vif-plugged-event-during-hard-reboot-fb491f6a68370bab.yaml new file mode 100644 index 00000000000..c3686a99780 --- /dev/null +++ b/releasenotes/notes/bug-1946729-wait-for-vif-plugged-event-during-hard-reboot-fb491f6a68370bab.yaml @@ -0,0 +1,18 @@ +--- +issues: + - | + The libvirt virt driver in Nova implements power on and hard reboot by + destroying the domain first and unpluging the vifs then recreating the + domain and replugging the vifs. However nova does not wait for the + network-vif-plugged event before unpause the domain. This can cause + the domain to start running and requesting IP via DHCP before the + networking backend has finished plugging the vifs. The config option + [workarounds]wait_for_vif_plugged_event_during_hard_reboot has been added, + defaulting to an empty list, that can be used to ensure that the libvirt + driver waits for the network-vif-plugged event for vifs with specific + ``vnic_type`` before it unpauses the domain during hard reboot. This should + only be used if the deployment uses a networking backend that sends such + event for the given ``vif_type`` at vif plug time. The ml2/ovs and the + networking-odl Neutron backend is known to send plug time events for ports + with ``normal`` ``vnic_type``. For more information see + https://bugs.launchpad.net/nova/+bug/1946729 From 28d0059c1f52e51add31bff50f1f6e443c938792 Mon Sep 17 00:00:00 2001 From: Dmitriy Rabotyagov Date: Thu, 30 Sep 2021 14:08:38 +0300 Subject: [PATCH 29/40] Ensure MAC addresses characters are in the same case Currently neutron can report ports to have MAC addresses in upper case when they're created like that. In the meanwhile libvirt configuration file always stores MAC in lower case which leads to KeyError while trying to retrieve migrate_vif. Conflicts: nova/tests/unit/virt/libvirt/test_migration.py Note: conflict is caused by not having six.text_type removal patch I779bd1446dc1f070fa5100ccccda7881fa508d79 in stable/victoria. Original assertion method was preserved to resolve this conflict. Closes-Bug: #1945646 Change-Id: Ie3129ee395427337e9abcef2f938012608f643e1 (cherry picked from commit 6a15169ed9f16672c2cde1d7f27178bb7868c41f) (cherry picked from commit 63a6388f6a0265f84232731aba8aec1bff3c6d18) (cherry picked from commit 6c3d5de659e558e8f6ee353475b54ff3ca7240ee) --- .../tests/unit/virt/libvirt/test_migration.py | 43 ++++++++++++++++++- nova/virt/libvirt/migration.py | 11 ++++- 2 files changed, 51 insertions(+), 3 deletions(-) diff --git a/nova/tests/unit/virt/libvirt/test_migration.py b/nova/tests/unit/virt/libvirt/test_migration.py index d4fba48397d..37bc6ad4a4c 100644 --- a/nova/tests/unit/virt/libvirt/test_migration.py +++ b/nova/tests/unit/virt/libvirt/test_migration.py @@ -991,7 +991,48 @@ def test_update_vif_xml_no_matching_vif(self): doc = etree.fromstring(original_xml) ex = self.assertRaises(KeyError, migration._update_vif_xml, doc, data, get_vif_config) - self.assertIn("CA:FE:DE:AD:BE:EF", six.text_type(ex)) + self.assertIn("ca:fe:de:ad:be:ef", six.text_type(ex)) + + def test_update_vif_xml_lower_case_mac(self): + """Tests that the vif in the migrate data is not found in the existing + guest interfaces. + """ + conf = vconfig.LibvirtConfigGuestInterface() + conf.net_type = "bridge" + conf.source_dev = "qbra188171c-ea" + conf.target_dev = "tapa188171c-ea" + conf.mac_addr = "DE:AD:BE:EF:CA:FE" + conf.model = "virtio" + original_xml = """ + 3de6550a-8596-4937-8046-9d862036bca5 + + + + + + + + + +
+ + + """ % uuids.ovs + expected_xml = """ + 3de6550a-8596-4937-8046-9d862036bca5 + + + + + + +
+ + + """ + self._test_update_vif_xml(conf, original_xml, expected_xml) class MigrationMonitorTestCase(test.NoDBTestCase): diff --git a/nova/virt/libvirt/migration.py b/nova/virt/libvirt/migration.py index 9543adda7ad..e601e46a9ba 100644 --- a/nova/virt/libvirt/migration.py +++ b/nova/virt/libvirt/migration.py @@ -344,14 +344,21 @@ def _update_vif_xml(xml_doc, migrate_data, get_vif_config): instance_uuid = xml_doc.findtext('uuid') parser = etree.XMLParser(remove_blank_text=True) interface_nodes = xml_doc.findall('./devices/interface') - migrate_vif_by_mac = {vif.source_vif['address']: vif + # MAC address stored for port in neutron DB and in domain XML + # might be in different cases, so to harmonize that + # we convert MAC to lower case for dict key. + migrate_vif_by_mac = {vif.source_vif['address'].lower(): vif for vif in migrate_data.vifs} for interface_dev in interface_nodes: mac = interface_dev.find('mac') mac = mac if mac is not None else {} mac_addr = mac.get('address') if mac_addr: - migrate_vif = migrate_vif_by_mac[mac_addr] + # MAC address stored in libvirt should always be normalized + # and stored in lower case. But just to be extra safe here + # we still normalize MAC retrieved from XML to be absolutely + # sure it will be the same with the Neutron provided one. + migrate_vif = migrate_vif_by_mac[mac_addr.lower()] vif = migrate_vif.get_dest_vif() # get_vif_config is a partial function of # nova.virt.libvirt.vif.LibvirtGenericVIFDriver.get_config From e549fec76fd2015e6e21ee5138bf06142a71e71a Mon Sep 17 00:00:00 2001 From: Balazs Gibizer Date: Mon, 6 Dec 2021 16:36:41 +0100 Subject: [PATCH 30/40] Reproduce bug 1953359 This patch adds a functional test that reproduces a race between incoming migration and the update_available_resource periodic Fixes: - Added more memory to mock 'host_info', since the default would not fit the instance. Default was changed in later releases Change-Id: I4be429c56aaa15ee12f448978c38214e741eae63 Related-Bug: #1953359 (cherry picked from commit c59224d715a21998f40f72cf4e37efdc990e4d7e) (cherry picked from commit f0a6d946aaa6c30f826cfced75c2fb06fdb379a8) (cherry picked from commit d8859e4f95f5abb20c844d914f2716cba047630e) --- nova/tests/functional/integrated_helpers.py | 10 ++- .../functional/libvirt/test_numa_servers.py | 82 +++++++++++++++++++ 2 files changed, 91 insertions(+), 1 deletion(-) diff --git a/nova/tests/functional/integrated_helpers.py b/nova/tests/functional/integrated_helpers.py index f0e7e148ebd..fcbbdce9d4f 100644 --- a/nova/tests/functional/integrated_helpers.py +++ b/nova/tests/functional/integrated_helpers.py @@ -385,7 +385,15 @@ def _create_server(self, name=None, image_uuid=None, flavor_id=None, """ # if forcing the server onto a host, we have to use the admin API if not api: - api = self.api if not az else getattr(self, 'admin_api', self.api) + api = self.api if not az and not host else getattr( + self, 'admin_api', self.api) + + if host and not api.microversion: + api.microversion = '2.74' + # with 2.74 networks param needs to use 'none' instead of None + # if no network is needed + if networks is None: + networks = 'none' body = self._build_server( name, image_uuid, flavor_id, networks, az, host) diff --git a/nova/tests/functional/libvirt/test_numa_servers.py b/nova/tests/functional/libvirt/test_numa_servers.py index 144bad33c82..bc8d91e862c 100644 --- a/nova/tests/functional/libvirt/test_numa_servers.py +++ b/nova/tests/functional/libvirt/test_numa_servers.py @@ -818,6 +818,88 @@ def test_resize_revert_bug_1944759(self): self._assert_pinned_cpus(src_host, 2) self._assert_pinned_cpus(dst_host, 0) + def test_resize_dedicated_policy_race_on_dest_bug_1953359(self): + + self.flags(cpu_dedicated_set='0-2', cpu_shared_set=None, + group='compute') + self.flags(vcpu_pin_set=None) + + host_info = fakelibvirt.HostInfo(cpu_nodes=1, cpu_sockets=1, + cpu_cores=2, cpu_threads=1, + kB_mem=15740000) + self.start_compute(host_info=host_info, hostname='compute1') + + extra_spec = { + 'hw:cpu_policy': 'dedicated', + } + flavor_id = self._create_flavor(vcpu=1, extra_spec=extra_spec) + expected_usage = {'DISK_GB': 20, 'MEMORY_MB': 2048, 'PCPU': 1} + + server = self._run_build_test(flavor_id, expected_usage=expected_usage) + + inst = objects.Instance.get_by_uuid(self.ctxt, server['id']) + self.assertEqual(1, len(inst.numa_topology.cells)) + # assert that the pcpu 0 is used on compute1 + self.assertEqual({'0': 0}, inst.numa_topology.cells[0].cpu_pinning_raw) + + # start another compute with the same config + self.start_compute(host_info=host_info, hostname='compute2') + + # boot another instance but now on compute2 so that it occupies the + # pcpu 0 on compute2 + # NOTE(gibi): _run_build_test cannot be used here as it assumes only + # compute1 exists + server2 = self._create_server( + flavor_id=flavor_id, + host='compute2', + ) + inst2 = objects.Instance.get_by_uuid(self.ctxt, server2['id']) + self.assertEqual(1, len(inst2.numa_topology.cells)) + # assert that the pcpu 0 is used + self.assertEqual( + {'0': 0}, inst2.numa_topology.cells[0].cpu_pinning_raw) + + # migrate the first instance from compute1 to compute2 but stop + # migrating at the start of finish_resize. Then start a racing periodic + # update_available_resources. + + def fake_finish_resize(*args, **kwargs): + # start a racing update_available_resource periodic + self._run_periodics() + # we expect it that CPU pinning fails on the destination node + # as the resource_tracker will use the source node numa_topology + # and that does not fit to the dest node as pcpu 0 in the dest + # is already occupied. + + # TODO(stephenfin): The mock of 'migrate_disk_and_power_off' should + # probably be less...dumb + with mock.patch('nova.virt.libvirt.driver.LibvirtDriver' + '.migrate_disk_and_power_off', return_value='{}'): + with mock.patch( + 'nova.compute.manager.ComputeManager.finish_resize' + ) as mock_finish_resize: + mock_finish_resize.side_effect = fake_finish_resize + post = {'migrate': None} + self.admin_api.post_server_action(server['id'], post) + + log = self.stdlog.logger.output + # The resize_claim correctly calculates that the inst1 should be pinned + # to pcpu id 1 instead of 0 + self.assertIn( + 'Computed NUMA topology CPU pinning: usable pCPUs: [[1]], ' + 'vCPUs mapping: [(0, 1)]', + log, + ) + # But the periodic fails as it tries to apply the source topology on + # the dest. This is bug 1953359. + log = self.stdlog.logger.output + self.assertIn('Error updating resources for node compute2', log) + self.assertIn( + 'nova.exception.CPUPinningInvalid: CPU set to pin [0] must be ' + 'a subset of free CPU set [1]', + log, + ) + class NUMAServerTestWithCountingQuotaFromPlacement(NUMAServersTest): From 8d4487465b60cd165dc76dea5a9fdb3c4dbf5740 Mon Sep 17 00:00:00 2001 From: Balazs Gibizer Date: Mon, 6 Dec 2021 16:36:41 +0100 Subject: [PATCH 31/40] Extend the reproducer for 1953359 and 1952915 This patch extends the original reproduction I4be429c56aaa15ee12f448978c38214e741eae63 to cover bug 1952915 as well as they have a common root cause. Change-Id: I57982131768d87e067d1413012b96f1baa68052b Related-Bug: #1953359 Related-Bug: #1952915 (cherry picked from commit 9f296d775d8f58fcbd03393c81a023268c7071cb) (cherry picked from commit 0411962938ae1de39f8dccb03efe4567f82ad671) (cherry picked from commit 94f17be190cce060ba8afcafbade4247b27b86f0) --- .../functional/libvirt/test_numa_servers.py | 62 ++++++++++++++----- 1 file changed, 45 insertions(+), 17 deletions(-) diff --git a/nova/tests/functional/libvirt/test_numa_servers.py b/nova/tests/functional/libvirt/test_numa_servers.py index bc8d91e862c..ea58f619f1a 100644 --- a/nova/tests/functional/libvirt/test_numa_servers.py +++ b/nova/tests/functional/libvirt/test_numa_servers.py @@ -21,6 +21,7 @@ from oslo_log import log as logging import nova +from nova.compute import manager from nova.conf import neutron as neutron_conf from nova import context as nova_context from nova import objects @@ -862,6 +863,7 @@ def test_resize_dedicated_policy_race_on_dest_bug_1953359(self): # migrate the first instance from compute1 to compute2 but stop # migrating at the start of finish_resize. Then start a racing periodic # update_available_resources. + orig_finish_resize = manager.ComputeManager.finish_resize def fake_finish_resize(*args, **kwargs): # start a racing update_available_resource periodic @@ -870,34 +872,60 @@ def fake_finish_resize(*args, **kwargs): # as the resource_tracker will use the source node numa_topology # and that does not fit to the dest node as pcpu 0 in the dest # is already occupied. + log = self.stdlog.logger.output + # The resize_claim correctly calculates that the instance should be + # pinned to pcpu id 1 instead of 0 + self.assertIn( + 'Computed NUMA topology CPU pinning: usable pCPUs: [[1]], ' + 'vCPUs mapping: [(0, 1)]', + log, + ) + # But the periodic fails as it tries to apply the source topology + # on the dest. This is bug 1953359. + log = self.stdlog.logger.output + self.assertIn('Error updating resources for node compute2', log) + self.assertIn( + 'nova.exception.CPUPinningInvalid: CPU set to pin [0] must be ' + 'a subset of free CPU set [1]', + log, + ) + + # now let the resize finishes + return orig_finish_resize(*args, **kwargs) # TODO(stephenfin): The mock of 'migrate_disk_and_power_off' should # probably be less...dumb with mock.patch('nova.virt.libvirt.driver.LibvirtDriver' '.migrate_disk_and_power_off', return_value='{}'): with mock.patch( - 'nova.compute.manager.ComputeManager.finish_resize' - ) as mock_finish_resize: - mock_finish_resize.side_effect = fake_finish_resize + 'nova.compute.manager.ComputeManager.finish_resize', + new=fake_finish_resize, + ): post = {'migrate': None} + # this is expected to succeed but logs are emitted + # from the racing periodic task. See fake_finish_resize + # for the asserts self.admin_api.post_server_action(server['id'], post) - log = self.stdlog.logger.output - # The resize_claim correctly calculates that the inst1 should be pinned - # to pcpu id 1 instead of 0 - self.assertIn( - 'Computed NUMA topology CPU pinning: usable pCPUs: [[1]], ' - 'vCPUs mapping: [(0, 1)]', - log, + server = self._wait_for_state_change(server, 'VERIFY_RESIZE') + + # as the periodic job raced and failed during the resize if we revert + # the instance now then it tries to unpin its cpus from the dest host + # but those was never pinned as the periodic failed. So the unpinning + # will fail too. + post = {'revertResize': {}} + ex = self.assertRaises( + client.OpenStackApiException, + self.admin_api.post_server_action, server['id'], post ) - # But the periodic fails as it tries to apply the source topology on - # the dest. This is bug 1953359. - log = self.stdlog.logger.output - self.assertIn('Error updating resources for node compute2', log) + # This is still bug 1953359. + self.assertEqual(500, ex.response.status_code) + server = self.api.get_server(server['id']) + self.assertEqual('ERROR', server['status']) self.assertIn( - 'nova.exception.CPUPinningInvalid: CPU set to pin [0] must be ' - 'a subset of free CPU set [1]', - log, + 'nova.exception.CPUUnpinningInvalid: CPU set to unpin [1] must be ' + 'a subset of pinned CPU set [0]', + self.stdlog.logger.output, ) From d54bd316b331d439a26a7318ca68cab5f6280ab2 Mon Sep 17 00:00:00 2001 From: Balazs Gibizer Date: Mon, 6 Dec 2021 17:06:51 +0100 Subject: [PATCH 32/40] [rt] Apply migration context for incoming migrations There is a race condition between an incoming resize and an update_available_resource periodic in the resource tracker. The race window starts when the resize_instance RPC finishes and ends when the finish_resize compute RPC finally applies the migration context on the instance. In the race window, if the update_available_resource periodic is run on the destination node, then it will see the instance as being tracked on this host as the instance.node is already pointing to the dest. But the instance.numa_topology still points to the source host topology as the migration context is not applied yet. This leads to CPU pinning error if the source topology does not fit to the dest topology. Also it stops the periodic task and leaves the tracker in an inconsistent state. The inconsistent state only cleanup up after the periodic is run outside of the race window. This patch applies the migration context temporarily to the specific instances during the periodic to keep resource accounting correct. Change-Id: Icaad155e22c9e2d86e464a0deb741c73f0dfb28a Closes-Bug: #1953359 Closes-Bug: #1952915 (cherry picked from commit 32c1044d86a8d02712c8e3abdf8b3e4cff234a9c) (cherry picked from commit 1235dc324ebc1c6ac6dc94da0f45ffffcc546d2c) (cherry picked from commit 5f2f283a75243d2e2629d3c5f7e5ef4b3994972d) --- nova/compute/resource_tracker.py | 35 ++++++++++++++++--- .../functional/libvirt/test_numa_servers.py | 29 +++++---------- 2 files changed, 40 insertions(+), 24 deletions(-) diff --git a/nova/compute/resource_tracker.py b/nova/compute/resource_tracker.py index 5b9e64e2190..685f7992337 100644 --- a/nova/compute/resource_tracker.py +++ b/nova/compute/resource_tracker.py @@ -933,14 +933,41 @@ def _update_available_resource(self, context, resources, startup=False): 'flavor', 'migration_context', 'resources']) - # Now calculate usage based on instance utilization: - instance_by_uuid = self._update_usage_from_instances( - context, instances, nodename) - # Grab all in-progress migrations and error migrations: migrations = objects.MigrationList.get_in_progress_and_error( context, self.host, nodename) + # Check for tracked instances with in-progress, incoming, but not + # finished migrations. For those instance the migration context + # is not applied yet (it will be during finish_resize when the + # migration goes to finished state). We need to manually and + # temporary apply the migration context here when the resource usage is + # updated. See bug 1953359 for more details. + instance_by_uuid = {instance.uuid: instance for instance in instances} + for migration in migrations: + if ( + migration.instance_uuid in instance_by_uuid and + migration.dest_compute == self.host and + migration.dest_node == nodename + ): + # we does not check for the 'post-migrating' migration status + # as applying the migration context for an instance already + # in finished migration status is a no-op anyhow. + instance = instance_by_uuid[migration.instance_uuid] + LOG.debug( + 'Applying migration context for instance %s as it has an ' + 'incoming, in-progress migration %s. ' + 'Migration status is %s', + migration.instance_uuid, migration.uuid, migration.status + ) + # It is OK not to revert the migration context at the end of + # the periodic as the instance is not saved during the periodic + instance.apply_migration_context() + + # Now calculate usage based on instance utilization: + instance_by_uuid = self._update_usage_from_instances( + context, instances, nodename) + self._pair_instances_to_migrations(migrations, instance_by_uuid) self._update_usage_from_migrations(context, migrations, nodename) diff --git a/nova/tests/functional/libvirt/test_numa_servers.py b/nova/tests/functional/libvirt/test_numa_servers.py index ea58f619f1a..db61b5fd4b3 100644 --- a/nova/tests/functional/libvirt/test_numa_servers.py +++ b/nova/tests/functional/libvirt/test_numa_servers.py @@ -880,11 +880,11 @@ def fake_finish_resize(*args, **kwargs): 'vCPUs mapping: [(0, 1)]', log, ) - # But the periodic fails as it tries to apply the source topology - # on the dest. This is bug 1953359. + # We expect that the periodic not fails as bug 1953359 is fixed. log = self.stdlog.logger.output - self.assertIn('Error updating resources for node compute2', log) - self.assertIn( + self.assertIn('Running periodic for compute (compute2)', log) + self.assertNotIn('Error updating resources for node compute2', log) + self.assertNotIn( 'nova.exception.CPUPinningInvalid: CPU set to pin [0] must be ' 'a subset of free CPU set [1]', log, @@ -902,27 +902,16 @@ def fake_finish_resize(*args, **kwargs): new=fake_finish_resize, ): post = {'migrate': None} - # this is expected to succeed but logs are emitted - # from the racing periodic task. See fake_finish_resize - # for the asserts + # this is expected to succeed self.admin_api.post_server_action(server['id'], post) server = self._wait_for_state_change(server, 'VERIFY_RESIZE') - # as the periodic job raced and failed during the resize if we revert - # the instance now then it tries to unpin its cpus from the dest host - # but those was never pinned as the periodic failed. So the unpinning - # will fail too. + # As bug 1953359 is fixed the revert should succeed too post = {'revertResize': {}} - ex = self.assertRaises( - client.OpenStackApiException, - self.admin_api.post_server_action, server['id'], post - ) - # This is still bug 1953359. - self.assertEqual(500, ex.response.status_code) - server = self.api.get_server(server['id']) - self.assertEqual('ERROR', server['status']) - self.assertIn( + self.admin_api.post_server_action(server['id'], post) + self._wait_for_state_change(server, 'ACTIVE') + self.assertNotIn( 'nova.exception.CPUUnpinningInvalid: CPU set to unpin [1] must be ' 'a subset of pinned CPU set [0]', self.stdlog.logger.output, From 8ff36f184dd7aedf9adfdbdf8845504557e2bef5 Mon Sep 17 00:00:00 2001 From: Elod Illes Date: Thu, 14 Apr 2022 20:35:11 +0200 Subject: [PATCH 33/40] [stable-only] Drop lower-constraints job During the PTG the TC discussed the topic and decided to drop the job completely. Since the latest job configuration broke all stable gate for nova (older than yoga) this is needed there to unblock our gates. For dropping the job on master let's wait to the resolution as the gate is not broken there, hence the patch is stable-only. Conflicts: .zuul.yaml lower-constraints.txt NOTE(elod.illes): conflict is due to branch specific settings (job template names, lower constraints changes). Change-Id: I514f6b337ffefef90a0ce9ab0b4afd083caa277e (cherry picked from commit 15b72717f2f3bd79791b913f1b294a19ced47ca7) (cherry picked from commit ba3c5b81abce49fb86981bdcc0013068b54d4f61) (cherry picked from commit 327693af402e4dd0c03fe247c4cee7beaedd2852) --- .zuul.yaml | 1 - lower-constraints.txt | 165 ------------------------------------------ tox.ini | 7 -- 3 files changed, 173 deletions(-) delete mode 100644 lower-constraints.txt diff --git a/.zuul.yaml b/.zuul.yaml index aa371db06c1..b5224367b23 100644 --- a/.zuul.yaml +++ b/.zuul.yaml @@ -417,7 +417,6 @@ - check-requirements - integrated-gate-compute - openstack-cover-jobs - - openstack-lower-constraints-jobs - openstack-python3-victoria-jobs - periodic-stable-jobs - publish-openstack-docs-pti diff --git a/lower-constraints.txt b/lower-constraints.txt deleted file mode 100644 index f41ff689292..00000000000 --- a/lower-constraints.txt +++ /dev/null @@ -1,165 +0,0 @@ -alembic==0.9.8 -amqp==2.5.0 -appdirs==1.4.3 -asn1crypto==0.24.0 -attrs==17.4.0 -automaton==1.14.0 -bandit==1.1.0 -cachetools==2.0.1 -castellan==0.16.0 -cffi==1.14.0 -cliff==2.11.0 -cmd2==0.8.1 -colorama==0.3.9 -coverage==4.0 -cryptography==2.7 -cursive==0.2.1 -dataclasses==0.7 -ddt==1.2.1 -debtcollector==1.19.0 -decorator==4.1.0 -deprecation==2.0 -dogpile.cache==0.6.5 -enum-compat==0.0.2 -eventlet==0.22.0 -extras==1.0.0 -fasteners==0.14.1 -fixtures==3.0.0 -future==0.16.0 -futurist==1.8.0 -gabbi==1.35.0 -gitdb2==2.0.3 -GitPython==2.1.8 -greenlet==0.4.15 -idna==2.6 -iso8601==0.1.11 -Jinja2==2.10 -jmespath==0.9.3 -jsonpatch==1.21 -jsonpath-rw==1.4.0 -jsonpath-rw-ext==1.1.3 -jsonpointer==2.0 -jsonschema==3.2.0 -keystoneauth1==3.16.0 -keystonemiddleware==4.20.0 -kombu==4.6.1 -linecache2==1.0.0 -lxml==4.5.0 -Mako==1.0.7 -MarkupSafe==1.1.1 -microversion-parse==0.2.1 -mock==3.0.0 -msgpack==0.5.6 -msgpack-python==0.5.6 -munch==2.2.0 -mypy==0.761 -netaddr==0.7.18 -netifaces==0.10.4 -networkx==2.1.0 -numpy==1.19.0 -openstacksdk==0.35.0 -os-brick==3.1.0 -os-client-config==1.29.0 -os-resource-classes==0.4.0 -os-service-types==1.7.0 -os-traits==2.4.0 -os-vif==1.14.0 -os-win==4.2.0 -os-xenapi==0.3.4 -osc-lib==1.10.0 -oslo.cache==1.26.0 -oslo.concurrency==3.29.0 -oslo.config==6.8.0 -oslo.context==2.22.0 -oslo.db==4.44.0 -oslo.i18n==3.15.3 -oslo.log==3.36.0 -oslo.messaging==10.3.0 -oslo.middleware==3.31.0 -oslo.policy==3.4.0 -oslo.privsep==1.33.2 -oslo.reports==1.18.0 -oslo.rootwrap==5.8.0 -oslo.serialization==2.21.1 -oslo.service==1.40.1 -oslo.upgradecheck==0.1.1 -oslo.utils==4.5.0 -oslo.versionedobjects==1.35.0 -oslo.vmware==2.17.0 -oslotest==3.8.0 -osprofiler==1.4.0 -ovs==2.10.0 -ovsdbapp==0.15.0 -packaging==20.4 -paramiko==2.7.1 -Paste==2.0.2 -PasteDeploy==1.5.0 -pbr==2.0.0 -pluggy==0.6.0 -ply==3.11 -prettytable==0.7.1 -psutil==3.2.2 -psycopg2==2.8 -py==1.5.2 -pyasn1==0.4.2 -pyasn1-modules==0.2.1 -pycadf==2.7.0 -pycparser==2.18 -pyinotify==0.9.6 -pyroute2==0.5.4 -PyJWT==1.7.0 -PyMySQL==0.8.0 -pyOpenSSL==17.5.0 -pyparsing==2.2.0 -pyperclip==1.6.0 -pypowervm==1.1.15 -pytest==3.4.2 -python-barbicanclient==4.5.2 -python-cinderclient==3.3.0 -python-dateutil==2.5.3 -python-editor==1.0.3 -python-glanceclient==2.8.0 -python-ironicclient==3.0.0 -python-keystoneclient==3.15.0 -python-mimeparse==1.6.0 -python-neutronclient==6.7.0 -python-subunit==1.4.0 -pytz==2018.3 -PyYAML==3.13 -repoze.lru==0.7 -requests==2.23.0 -requests-mock==1.2.0 -requestsexceptions==1.4.0 -retrying==1.3.3 -rfc3986==1.2.0 -Routes==2.3.1 -simplejson==3.13.2 -six==1.11.0 -smmap2==2.0.3 -sortedcontainers==2.1.0 -SQLAlchemy==1.2.19 -sqlalchemy-migrate==0.13.0 -sqlparse==0.2.4 -statsd==3.2.2 -stestr==2.0.0 -stevedore==1.20.0 -suds-jurko==0.6 -taskflow==3.8.0 -Tempita==0.5.2 -tenacity==6.0.0 -testrepository==0.0.20 -testresources==2.0.0 -testscenarios==0.4 -testtools==2.2.0 -tooz==1.58.0 -traceback2==1.4.0 -unittest2==1.1.0 -urllib3==1.22 -vine==1.1.4 -voluptuous==0.11.1 -warlock==1.3.1 -WebOb==1.8.2 -websockify==0.9.0 -wrapt==1.10.11 -wsgi-intercept==1.7.0 -zVMCloudConnector==1.3.0 diff --git a/tox.ini b/tox.ini index be5f18d0130..f3089cd44ed 100644 --- a/tox.ini +++ b/tox.ini @@ -335,10 +335,3 @@ usedevelop = False deps = bindep commands = bindep test - -[testenv:lower-constraints] -usedevelop = False -deps = - -c{toxinidir}/lower-constraints.txt - -r{toxinidir}/test-requirements.txt - -r{toxinidir}/requirements.txt From bc92f05a6d6647e709a23a9d78b49f916874ef85 Mon Sep 17 00:00:00 2001 From: Elod Illes Date: Thu, 28 Apr 2022 17:17:47 +0200 Subject: [PATCH 34/40] [CI] Install dependencies for docs target When tox 'docs' target is called, first it installs the dependencies (listed in 'deps') in 'installdeps' phase, then it installs nova (with its requirements) in 'develop-inst' phase. In the latter case 'deps' is not used so that the constraints defined in 'deps' are not used. This could lead to failures on stable branches when new packages are released that break the build. To avoid this, the simplest solution is to pre-install requirements, i.e. add requirements.txt to 'docs' tox target. Conflicts: tox.ini NOTE(elod.illes): conflict is due to branch specific upper constraints file link. Change-Id: I4471d4488d336d5af0c23028724c4ce79d6a2031 (cherry picked from commit 494e8d7db6f8a3d1a952f657acab353787f57e04) (cherry picked from commit 1ac0d6984a43cddbb5a2f1a2f7bc115fd83517c9) (cherry picked from commit 64cc0848be9bf92d79e6fa7b424668d21321d593) (cherry picked from commit f66a570e946d980162a1313aa5a7e2ce5856a128) --- tox.ini | 1 + 1 file changed, 1 insertion(+) diff --git a/tox.ini b/tox.ini index f3089cd44ed..eca58b5591f 100644 --- a/tox.ini +++ b/tox.ini @@ -182,6 +182,7 @@ description = # to install (test-)requirements.txt for docs. deps = -c{env:TOX_CONSTRAINTS_FILE:https://releases.openstack.org/constraints/upper/victoria} + -r{toxinidir}/requirements.txt -r{toxinidir}/doc/requirements.txt commands = rm -rf doc/build/html doc/build/doctrees From d218250eb53791012f49825140e2592dab89e69c Mon Sep 17 00:00:00 2001 From: Ghanshyam Mann Date: Tue, 12 Oct 2021 12:26:02 -0500 Subject: [PATCH 35/40] Define new functional test tox env for placement gate to run We have placement-nova-tox-functional-py38 job defined and run on placement gate[1] to run the nova functional test excluding api and notification _sample_tests, and db-related tests but that job skip those tests via tox_extra_args which is not right way to do as we currently facing error when tox_extra_args is included in tox siblings task - https://opendev.org/zuul/zuul-jobs/commit/c02c28a982da8d5a9e7b4ca38d30967f6cd1531d - https://zuul.openstack.org/build/a8c186b2c7124856ae32477f10e2b9a4 Let's define a new tox env which can exclude the required test in stestr command itself. Conflicts: tox.ini NOTE(melwitt): The conflict is because change I672904e9bfb45a66a82331063c7d49c4bc0439df (Add functional-py39 testing) is not in Victoria. [1] https://opendev.org/openstack/placement/src/commit/bd5b19c00e1ab293fc157f4699bc4f4719731c25/.zuul.yaml#L83 Change-Id: I20d6339a5203aed058f432f68e2ec1af57030401 (cherry picked from commit 7b063e4d0518af3e57872bc0288a94edcd33c19d) (cherry picked from commit 64f5c1cfb0e7223603c06e22a204716919d05294) (cherry picked from commit baf0d93e0fafcd992d37543aa9df3f6dc248a738) --- tox.ini | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/tox.ini b/tox.ini index eca58b5591f..aae3708a101 100644 --- a/tox.ini +++ b/tox.ini @@ -126,6 +126,16 @@ deps = {[testenv:functional]deps} commands = {[testenv:functional]commands} +[testenv:functional-without-sample-db-tests] +description = + Run functional tests by excluding the API|Notification + sample tests and DB tests. This env is used in + placement-nova-tox-functional-py38 job which is defined and + run in placement. +deps = {[testenv:functional]deps} +commands = + stestr --test-path=./nova/tests/functional run --exclude-regex '((?:api|notification)_sample_tests|functional\.db\.)' {posargs} + [testenv:api-samples] setenv = {[testenv]setenv} From bc5fc2bc688056bc18cf3ae581d8e23592d110da Mon Sep 17 00:00:00 2001 From: Julia Kreger Date: Fri, 2 Jul 2021 12:10:52 -0700 Subject: [PATCH 36/40] [ironic] Minimize window for a resource provider to be lost This patch is based upon a downstream patch which came up in discussion amongst the ironic community when some operators began discussing a case where resource providers had disappeared from a running deployment with several thousand baremetal nodes. Discussion amongst operators and developers ensued and we were able to determine that this was still an issue in the current upstream code and that time difference between collecting data and then reconciling the records was a source of the issue. Per Arun, they have been running this change downstream and had not seen any reoccurances of the issue since the patch was applied. This patch was originally authored by Arun S A G, and below is his original commit mesage. An instance could be launched and scheduled to a compute node between get_uuids_by_host() call and _get_node_list() call. If that happens the ironic node.instance_uuid may not be None but the instance_uuid will be missing from the instance list returned by get_uuids_by_host() method. This is possible because _get_node_list() takes several minutes to return in large baremetal clusters and a lot can happen in that time. This causes the compute node to be orphaned and associated resource provider to be deleted from placement. Once the resource provider is deleted it is never created again until the service restarts. Since resource provider is deleted subsequent boots/rebuilds to the same host will fail. This behaviour is visibile in VMbooter nodes because it constantly launches and deletes instances there by increasing the likelihood of this race condition happening in large ironic clusters. To reduce the chance of this race condition we call _get_node_list() first followed by get_uuids_by_host() method. Change-Id: I55bde8dd33154e17bbdb3c4b0e7a83a20e8487e8 Co-Authored-By: Arun S A G Related-Bug: #1841481 (cherry picked from commit f84d5917c6fb045f03645d9f80eafbc6e5f94bdd) (cherry picked from commit 0c36bd28ebd05ec0b1dbae950a24a2ecf339be00) --- nova/tests/unit/virt/ironic/test_driver.py | 12 ++++++++++++ nova/virt/ironic/driver.py | 16 +++++++++++++++- ...bug-1841481-race-window-f76912d4985770ad.yaml | 13 +++++++++++++ 3 files changed, 40 insertions(+), 1 deletion(-) create mode 100644 releasenotes/notes/minimize-bug-1841481-race-window-f76912d4985770ad.yaml diff --git a/nova/tests/unit/virt/ironic/test_driver.py b/nova/tests/unit/virt/ironic/test_driver.py index 9b8a6a04726..801846c4dd7 100644 --- a/nova/tests/unit/virt/ironic/test_driver.py +++ b/nova/tests/unit/virt/ironic/test_driver.py @@ -3441,6 +3441,9 @@ def _test__refresh_cache(self, instances, nodes, hosts, mock_instances, mock_instances.return_value = instances mock_nodes.return_value = nodes mock_hosts.side_effect = hosts + parent_mock = mock.MagicMock() + parent_mock.attach_mock(mock_nodes, 'get_node_list') + parent_mock.attach_mock(mock_instances, 'get_uuids_by_host') if not can_send_146: mock_can_send.side_effect = ( exception.IronicAPIVersionNotAvailable(version='1.46')) @@ -3453,6 +3456,15 @@ def _test__refresh_cache(self, instances, nodes, hosts, mock_instances, self.driver._refresh_cache() + # assert if get_node_list() is called before get_uuids_by_host() + parent_mock.assert_has_calls( + [ + mock.call.get_node_list(fields=ironic_driver._NODE_FIELDS, + **kwargs), + mock.call.get_uuids_by_host(mock.ANY, self.host) + ] + ) + mock_hash_ring.assert_called_once_with(mock.ANY) mock_instances.assert_called_once_with(mock.ANY, self.host) mock_nodes.assert_called_once_with(fields=ironic_driver._NODE_FIELDS, diff --git a/nova/virt/ironic/driver.py b/nova/virt/ironic/driver.py index aa00ef68395..3bd4d18ecc8 100644 --- a/nova/virt/ironic/driver.py +++ b/nova/virt/ironic/driver.py @@ -788,10 +788,15 @@ def _refresh_hash_ring(self, ctxt): def _refresh_cache(self): ctxt = nova_context.get_admin_context() self._refresh_hash_ring(ctxt) - instances = objects.InstanceList.get_uuids_by_host(ctxt, CONF.host) node_cache = {} def _get_node_list(**kwargs): + # NOTE(TheJulia): This call can take a substantial amount + # of time as it may be attempting to retrieve thousands of + # baremetal nodes. Depending on the version of Ironic, + # this can be as long as 2-10 seconds per every thousand + # nodes, and this call may retrieve all nodes in a deployment, + # depending on if any filter paramters are applied. return self._get_node_list(fields=_NODE_FIELDS, **kwargs) # NOTE(jroll) if partition_key is set, we need to limit nodes that @@ -815,6 +820,15 @@ def _get_node_list(**kwargs): else: nodes = _get_node_list() + # NOTE(saga): As _get_node_list() will take a long + # time to return in large clusters we need to call it before + # get_uuids_by_host() method. Otherwise the instances list we get from + # get_uuids_by_host() method will become stale. + # A stale instances list can cause a node that is managed by this + # compute host to be excluded in error and cause the compute node + # to be orphaned and associated resource provider to be deleted. + instances = objects.InstanceList.get_uuids_by_host(ctxt, CONF.host) + for node in nodes: # NOTE(jroll): we always manage the nodes for instances we manage if node.instance_uuid in instances: diff --git a/releasenotes/notes/minimize-bug-1841481-race-window-f76912d4985770ad.yaml b/releasenotes/notes/minimize-bug-1841481-race-window-f76912d4985770ad.yaml new file mode 100644 index 00000000000..a49d64c6ac5 --- /dev/null +++ b/releasenotes/notes/minimize-bug-1841481-race-window-f76912d4985770ad.yaml @@ -0,0 +1,13 @@ +--- +fixes: + - | + Minimizes a race condition window when using the ``ironic`` virt driver + where the data generated for the Resource Tracker may attempt to compare + potentially stale instance information with the latest known baremetal + node information. While this doesn't completely prevent nor resolve the + underlying race condition identified in + `bug 1841481 `_, + this change allows Nova to have the latest state information, as opposed + to state information which may be out of date due to the time which it may + take to retrieve the status from Ironic. This issue was most observable + on baremetal clusters with several thousand physical nodes. From 35fb52f53fbd3f8290f775760a842d70f583fa67 Mon Sep 17 00:00:00 2001 From: Julia Kreger Date: Fri, 8 Oct 2021 14:35:00 -0700 Subject: [PATCH 37/40] Ignore plug_vifs on the ironic driver When the nova-compute service starts, by default it attempts to startup instance configuration states for aspects such as networking. This is fine in most cases, and makes a lot of sense if the nova-compute service is just managing virtual machines on a hypervisor. This is done, one instance at a time. However, when the compute driver is ironic, the networking is managed as part of the physical machine lifecycle potentially all the way into committed switch configurations. As such, there is no need to attempt to call ``plug_vifs`` on every single instance managed by the nova-compute process which is backed by Ironic. Additionally, using ironic tends to manage far more physical machines per nova-compute service instance then when when operating co-installed with a hypervisor. Often this means a cluster of a thousand machines, with three controllers, will see thousands of un-needed API calls upon service start, which elongates the entire process and negatively impacts operations. In essence, nova.virt.ironic's plug_vifs call now does nothing, and merely issues a debug LOG entry when called. Closes-Bug: #1777608 Change-Id: Iba87cef50238c5b02ab313f2311b826081d5b4ab (cherry picked from commit 7f81cf28bf21ad2afa98accfde3087c83b8e269b) (cherry picked from commit eb6d70f02daa14920a2522e5c734a3775ea2ea7c) (cherry picked from commit f210115bcba3436b957a609cd388a13e6d77a638) --- nova/tests/unit/virt/ironic/test_driver.py | 33 +++++++++++-------- nova/virt/ironic/driver.py | 21 +++++++++--- ...art-port-attachments-3282e9ea051561d4.yaml | 11 +++++++ 3 files changed, 47 insertions(+), 18 deletions(-) create mode 100644 releasenotes/notes/fix-ironic-compute-restart-port-attachments-3282e9ea051561d4.yaml diff --git a/nova/tests/unit/virt/ironic/test_driver.py b/nova/tests/unit/virt/ironic/test_driver.py index 801846c4dd7..7f907336f92 100644 --- a/nova/tests/unit/virt/ironic/test_driver.py +++ b/nova/tests/unit/virt/ironic/test_driver.py @@ -2018,17 +2018,12 @@ def test_plug_vifs_with_port(self, mock_vatt): @mock.patch.object(ironic_driver.IronicDriver, '_plug_vifs') def test_plug_vifs(self, mock__plug_vifs): node_id = 'aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee' - node = _get_cached_node(id=node_id) - - self.mock_conn.get_node.return_value = node instance = fake_instance.fake_instance_obj(self.ctx, node=node_id) network_info = utils.get_test_network_info() self.driver.plug_vifs(instance, network_info) - self.mock_conn.get_node.assert_called_once_with( - node_id, fields=ironic_driver._NODE_FIELDS) - mock__plug_vifs.assert_called_once_with(node, instance, network_info) + mock__plug_vifs.assert_not_called() @mock.patch.object(FAKE_CLIENT.node, 'vif_attach') def test_plug_vifs_multiple_ports(self, mock_vatt): @@ -2162,11 +2157,17 @@ def test_unplug_vifs_no_network_info(self, mock_vdet): self.driver.unplug_vifs(instance, network_info) self.assertFalse(mock_vdet.called) - @mock.patch.object(ironic_driver.IronicDriver, 'plug_vifs') + @mock.patch.object(ironic_driver.IronicDriver, '_plug_vifs') def test_attach_interface(self, mock_pv): - self.driver.attach_interface('fake_context', 'fake_instance', + node_uuid = 'aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee' + node = _get_cached_node(uuid=node_uuid) + instance = fake_instance.fake_instance_obj(self.ctx, + node=node_uuid) + self.mock_conn.get_node.return_value = node + + self.driver.attach_interface('fake_context', instance, 'fake_image_meta', 'fake_vif') - mock_pv.assert_called_once_with('fake_instance', ['fake_vif']) + mock_pv.assert_called_once_with(node, instance, ['fake_vif']) @mock.patch.object(ironic_driver.IronicDriver, 'unplug_vifs') def test_detach_interface(self, mock_uv): @@ -2459,17 +2460,23 @@ def test_get_volume_connector_no_ip_without_mac(self): def test_get_volume_connector_no_ip_no_fixed_ip(self): self._test_get_volume_connector_no_ip(False, no_fixed_ip=True) - @mock.patch.object(ironic_driver.IronicDriver, 'plug_vifs') + @mock.patch.object(ironic_driver.IronicDriver, '_plug_vifs') def test_prepare_networks_before_block_device_mapping(self, mock_pvifs): + node_uuid = 'aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee' + node = _get_cached_node(uuid=node_uuid) + self.mock_conn.get_node.return_value = node instance = fake_instance.fake_instance_obj(self.ctx) network_info = utils.get_test_network_info() self.driver.prepare_networks_before_block_device_mapping(instance, network_info) - mock_pvifs.assert_called_once_with(instance, network_info) + mock_pvifs.assert_called_once_with(node, instance, network_info) - @mock.patch.object(ironic_driver.IronicDriver, 'plug_vifs') + @mock.patch.object(ironic_driver.IronicDriver, '_plug_vifs') def test_prepare_networks_before_block_device_mapping_error(self, mock_pvifs): + node_uuid = 'aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee' + node = _get_cached_node(uuid=node_uuid) + self.mock_conn.get_node.return_value = node instance = fake_instance.fake_instance_obj(self.ctx) network_info = utils.get_test_network_info() mock_pvifs.side_effect = ironic_exception.BadRequest('fake error') @@ -2477,7 +2484,7 @@ def test_prepare_networks_before_block_device_mapping_error(self, ironic_exception.BadRequest, self.driver.prepare_networks_before_block_device_mapping, instance, network_info) - mock_pvifs.assert_called_once_with(instance, network_info) + mock_pvifs.assert_called_once_with(node, instance, network_info) @mock.patch.object(ironic_driver.IronicDriver, 'unplug_vifs') def test_clean_networks_preparation(self, mock_upvifs): diff --git a/nova/virt/ironic/driver.py b/nova/virt/ironic/driver.py index 3bd4d18ecc8..d8140e733b7 100644 --- a/nova/virt/ironic/driver.py +++ b/nova/virt/ironic/driver.py @@ -1623,13 +1623,21 @@ def _unplug_vifs(self, node, instance, network_info): def plug_vifs(self, instance, network_info): """Plug VIFs into networks. + This method is present for compatability. Any call will result + in a DEBUG log entry being generated, and will otherwise be + ignored, as Ironic manages VIF attachments through a node + lifecycle. Please see ``attach_interface``, which is the + proper and current method to utilize. + :param instance: The instance object. :param network_info: Instance network information. """ - # instance.node is the ironic node's UUID. - node = self._get_node(instance.node) - self._plug_vifs(node, instance, network_info) + LOG.debug('VIF plug called for instance %(instance)s on node ' + '%(node)s, however Ironic manages VIF attachments ' + 'for nodes.', + {'instance': instance.uuid, + 'node': instance.node}) def unplug_vifs(self, instance, network_info): """Unplug VIFs from networks. @@ -1660,7 +1668,8 @@ def attach_interface(self, context, instance, image_meta, vif): # event from neutron or by _heal_instance_info_cache periodic task. In # both cases, this is done asynchronously, so the cache may not be up # to date immediately after attachment. - self.plug_vifs(instance, [vif]) + node = self._get_node(instance.node) + self._plug_vifs(node, instance, [vif]) def detach_interface(self, context, instance, vif): """Use hotunplug to remove a network interface from a running instance. @@ -1977,7 +1986,9 @@ def prepare_networks_before_block_device_mapping(self, instance, """ try: - self.plug_vifs(instance, network_info) + node = self._get_node(instance.node) + self._plug_vifs(node, instance, network_info) + except Exception: with excutils.save_and_reraise_exception(): LOG.error("Error preparing deploy for instance " diff --git a/releasenotes/notes/fix-ironic-compute-restart-port-attachments-3282e9ea051561d4.yaml b/releasenotes/notes/fix-ironic-compute-restart-port-attachments-3282e9ea051561d4.yaml new file mode 100644 index 00000000000..2c2c9e78382 --- /dev/null +++ b/releasenotes/notes/fix-ironic-compute-restart-port-attachments-3282e9ea051561d4.yaml @@ -0,0 +1,11 @@ +--- +fixes: + - | + Fixes slow compute restart when using the ``nova.virt.ironic`` compute + driver where the driver was previously attempting to attach VIFS on + start-up via the ``plug_vifs`` driver method. This method has grown + otherwise unused since the introduction of the ``attach_interface`` + method of attaching VIFs. As Ironic manages the attachment of VIFs to + baremetal nodes in order to align with the security requirements of a + physical baremetal node's lifecycle. The ironic driver now ignores calls + to the ``plug_vifs`` method. From b6c877377f58ccaa797af3384b199002726745ea Mon Sep 17 00:00:00 2001 From: Amit Uniyal Date: Wed, 6 Jul 2022 18:20:02 +0000 Subject: [PATCH 38/40] add regression test case for bug 1978983 This change add a repoducer test for evacuating a vm in the powering-off state While backporting to stable/victoria Fixed conflict in functional.integrated_helpers 1 - Added placeholder NOT_SPECIFIED as object Its a default parameter value for _evacuate_server 2 - Updated _evacuate_server defition to allow function to wait, until expected server state reached 3 - Added _start_server and _stop_server 4 - Updated _evacuate_server arguments for test_evacuate as per updated _evacuate_server signature Related-Bug: #1978983 Change-Id: I5540df6c7497956219c06cff6f15b51c2c8bc299 (cherry picked from commit 5904c7f993ac737d68456fc05adf0aaa7a6f3018) (cherry picked from commit 6bd0bf00fca6ac6460d70c855eded3898cfe2401) (cherry picked from commit 1e0af92e17f878ce64bd16e428cb3c10904b0877) (cherry picked from commit b57b0eef218fd7604658842c9277aad782d11b45) --- nova/tests/functional/integrated_helpers.py | 46 +++++++++-- .../regressions/test_bug_1978983.py | 78 +++++++++++++++++++ nova/tests/functional/test_servers.py | 3 +- 3 files changed, 121 insertions(+), 6 deletions(-) create mode 100644 nova/tests/functional/regressions/test_bug_1978983.py diff --git a/nova/tests/functional/integrated_helpers.py b/nova/tests/functional/integrated_helpers.py index fcbbdce9d4f..44afcb0381a 100644 --- a/nova/tests/functional/integrated_helpers.py +++ b/nova/tests/functional/integrated_helpers.py @@ -94,6 +94,12 @@ def router(self): return rpc.ClientRouter(default_client) +# placeholder used as a default parameter value to distinguish between the case +# when the parameter is specified by the caller with None from the case when it +# was not specified +NOT_SPECIFIED = object() + + class InstanceHelperMixin: def _wait_for_server_parameter( @@ -493,12 +499,42 @@ def _unshelve_server(self, server, expected_state='ACTIVE'): self.api.post_server_action(server['id'], {'unshelve': {}}) return self._wait_for_state_change(server, expected_state) - def _evacuate_server(self, server, host, expected_state='ACTIVE'): + def _evacuate_server( + self, server, extra_post_args=None, expected_host=None, + expected_state='ACTIVE', expected_task_state=NOT_SPECIFIED, + expected_migration_status='done'): """Evacuate a server.""" - self.api.post_server_action(server['id'], {'evacuate': {}}) - self._wait_for_server_parameter( - self.server, {'OS-EXT-SRV-ATTR:host': host, - 'status': expected_state}) + api = getattr(self, 'admin_api', self.api) + + post = {'evacuate': {}} + if extra_post_args: + post['evacuate'].update(extra_post_args) + + expected_result = {'status': expected_state} + if expected_host: + expected_result['OS-EXT-SRV-ATTR:host'] = expected_host + if expected_task_state is not NOT_SPECIFIED: + expected_result['OS-EXT-STS:task_state'] = expected_task_state + + api.post_server_action(server['id'], post) + + # NOTE(gibi): The order of waiting for the migration and returning + # a fresh server from _wait_for_server_parameter is important as + # the compute manager sets status of the instance before sets the + # host and finally sets the migration status. So waiting for the + # migration first makes the returned server object more consistent. + self._wait_for_migration_status(server, [expected_migration_status]) + return self._wait_for_server_parameter(server, expected_result) + + def _start_server(self, server): + self.api.post_server_action(server['id'], {'os-start': None}) + return self._wait_for_state_change(server, 'ACTIVE') + + def _stop_server(self, server, wait_for_stop=True): + self.api.post_server_action(server['id'], {'os-stop': None}) + if wait_for_stop: + return self._wait_for_state_change(server, 'SHUTOFF') + return server class PlacementHelperMixin: diff --git a/nova/tests/functional/regressions/test_bug_1978983.py b/nova/tests/functional/regressions/test_bug_1978983.py new file mode 100644 index 00000000000..75260abf371 --- /dev/null +++ b/nova/tests/functional/regressions/test_bug_1978983.py @@ -0,0 +1,78 @@ +# Copyright 2022 Red Hat, Inc. +# All Rights Reserved. +# +# 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. + + +from nova import test +from nova.tests import fixtures as nova_fixtures +from nova.tests.functional.api import client +from nova.tests.functional import fixtures as func_fixtures +from nova.tests.functional import integrated_helpers + + +class EvacuateServerWithTaskState( + test.TestCase, integrated_helpers.InstanceHelperMixin, +): + """Regression test for bug 1978983 + If instance task state is powering-off or not None + instance should be allowed to evacuate. + """ + + def setUp(self): + super().setUp() + # Stub out external dependencies. + self.useFixture(nova_fixtures.NeutronFixture(self)) + self.useFixture(nova_fixtures.GlanceFixture(self)) + self.useFixture(func_fixtures.PlacementFixture()) + self.useFixture(nova_fixtures.HostNameWeigherFixture()) + + # Start nova controller services. + self.start_service('conductor') + self.start_service('scheduler') + + api_fixture = self.useFixture(nova_fixtures.OSAPIFixture( + api_version='v2.1')) + self.api = api_fixture.admin_api + + self.src = self._start_compute(host='host1') + self.dest = self._start_compute(host='host2') + + def test_evacuate_instance(self): + """Evacuating a server + """ + server = self._create_server(networks=[]) + + self.api.microversion = 'latest' + server = self._wait_for_state_change(server, 'ACTIVE') + self.assertEqual('host1', server['OS-EXT-SRV-ATTR:host']) + + # stop host1 compute service + self.src.stop() + + # poweroff instance + self._stop_server(server, wait_for_stop=False) + server = self._wait_for_server_parameter( + server, {'OS-EXT-STS:task_state': 'powering-off'}) + + # FIXME(auniyal): As compute service is down in source node + # instance is stuck at powering-off, evacuation fails with + # msg: Cannot 'evacuate' instance while it is in + # task_state powering-off (HTTP 409) + + ex = self.assertRaises( + client.OpenStackApiException, + self._evacuate_server, + server, + expected_host=self.dest.host) + self.assertEqual(409, ex.response.status_code) diff --git a/nova/tests/functional/test_servers.py b/nova/tests/functional/test_servers.py index 426745cb8ae..90f3f48457d 100644 --- a/nova/tests/functional/test_servers.py +++ b/nova/tests/functional/test_servers.py @@ -8181,7 +8181,8 @@ def test_evacuate_ok(self): arqs = self.cyborg.fake_get_arqs_for_instance(self.server['id']) compute_to_stop, compute_to_evacuate = self._test_evacuate( self.server, self.NUM_HOSTS) - self._evacuate_server(self.server, compute_to_evacuate.host) + self._evacuate_server(self.server, + expected_host=compute_to_evacuate.host) compute_to_stop.start() self.server = self.api.get_server(self.server['id']) arqs_new = self.cyborg.fake_get_arqs_for_instance(self.server['id']) From 3224ceb3fffc57d2375e5163d8ffbbb77529bc38 Mon Sep 17 00:00:00 2001 From: Amit Uniyal Date: Wed, 6 Jul 2022 18:20:02 +0000 Subject: [PATCH 39/40] For evacuation, ignore if task_state is not None ignore instance task state and continue with vm evacutaion. Closes-Bug: #1978983 Change-Id: I5540df6c7497956219c06cff6f15b51c2c8bc29d (cherry picked from commit db919aa15f24c0d74f3c5c0e8341fad3f2392e57) (cherry picked from commit 6d61fccb8455367aaa37ae7bddf3b8befd3c3d88) (cherry picked from commit 8e9aa71e1a4d3074a94911db920cae44334ba2c3) (cherry picked from commit 0b8124b99601e1aba492be8ed564f769438bd93d) --- doc/source/admin/evacuate.rst | 14 +++++++++++ nova/compute/api.py | 4 ++-- .../regressions/test_bug_1978983.py | 23 +++++++------------ ...state-for-evacuation-e000f141d0153638.yaml | 11 +++++++++ 4 files changed, 35 insertions(+), 17 deletions(-) create mode 100644 releasenotes/notes/ignore-instance-task-state-for-evacuation-e000f141d0153638.yaml diff --git a/doc/source/admin/evacuate.rst b/doc/source/admin/evacuate.rst index ef9eccd9312..18796d9c237 100644 --- a/doc/source/admin/evacuate.rst +++ b/doc/source/admin/evacuate.rst @@ -97,3 +97,17 @@ instances up and running. using a pattern you might want to use the ``--strict`` flag which got introduced in version 10.2.0 to make sure nova matches the ``FAILED_HOST`` exactly. + +.. note:: + .. code-block:: bash + + +------+--------+--------------+ + | Name | Status | Task State | + +------+--------+--------------+ + | vm_1 | ACTIVE | powering-off | + +------------------------------+ + + If the instance task state is not None, evacuation will be possible. However, + depending on the ongoing operation, there may be clean up required in other + services which the instance was using, such as neutron, cinder, glance, or + the storage backend. \ No newline at end of file diff --git a/nova/compute/api.py b/nova/compute/api.py index 97868490c34..8b5ba453ce6 100644 --- a/nova/compute/api.py +++ b/nova/compute/api.py @@ -5185,7 +5185,7 @@ def live_migrate_abort(self, context, instance, migration_id, @reject_vtpm_instances(instance_actions.EVACUATE) @block_accelerators(until_service=SUPPORT_ACCELERATOR_SERVICE_FOR_REBUILD) @check_instance_state(vm_state=[vm_states.ACTIVE, vm_states.STOPPED, - vm_states.ERROR]) + vm_states.ERROR], task_state=None) def evacuate(self, context, instance, host, on_shared_storage, admin_password=None, force=None): """Running evacuate to target host. @@ -5212,7 +5212,7 @@ def evacuate(self, context, instance, host, on_shared_storage, context, instance.uuid) instance.task_state = task_states.REBUILDING - instance.save(expected_task_state=[None]) + instance.save(expected_task_state=None) self._record_action_start(context, instance, instance_actions.EVACUATE) # NOTE(danms): Create this as a tombstone for the source compute diff --git a/nova/tests/functional/regressions/test_bug_1978983.py b/nova/tests/functional/regressions/test_bug_1978983.py index 75260abf371..51465900da0 100644 --- a/nova/tests/functional/regressions/test_bug_1978983.py +++ b/nova/tests/functional/regressions/test_bug_1978983.py @@ -13,10 +13,8 @@ # License for the specific language governing permissions and limitations # under the License. - from nova import test from nova.tests import fixtures as nova_fixtures -from nova.tests.functional.api import client from nova.tests.functional import fixtures as func_fixtures from nova.tests.functional import integrated_helpers @@ -44,6 +42,7 @@ def setUp(self): api_fixture = self.useFixture(nova_fixtures.OSAPIFixture( api_version='v2.1')) self.api = api_fixture.admin_api + self.api.microversion = 'latest' self.src = self._start_compute(host='host1') self.dest = self._start_compute(host='host2') @@ -53,26 +52,20 @@ def test_evacuate_instance(self): """ server = self._create_server(networks=[]) - self.api.microversion = 'latest' server = self._wait_for_state_change(server, 'ACTIVE') - self.assertEqual('host1', server['OS-EXT-SRV-ATTR:host']) + self.assertEqual(self.src.host, server['OS-EXT-SRV-ATTR:host']) # stop host1 compute service self.src.stop() + self.api.put_service_force_down(self.src.service_ref.uuid, True) # poweroff instance self._stop_server(server, wait_for_stop=False) server = self._wait_for_server_parameter( server, {'OS-EXT-STS:task_state': 'powering-off'}) - # FIXME(auniyal): As compute service is down in source node - # instance is stuck at powering-off, evacuation fails with - # msg: Cannot 'evacuate' instance while it is in - # task_state powering-off (HTTP 409) - - ex = self.assertRaises( - client.OpenStackApiException, - self._evacuate_server, - server, - expected_host=self.dest.host) - self.assertEqual(409, ex.response.status_code) + # evacuate instance + server = self._evacuate_server( + server, expected_host=self.dest.host + ) + self.assertEqual(self.dest.host, server['OS-EXT-SRV-ATTR:host']) diff --git a/releasenotes/notes/ignore-instance-task-state-for-evacuation-e000f141d0153638.yaml b/releasenotes/notes/ignore-instance-task-state-for-evacuation-e000f141d0153638.yaml new file mode 100644 index 00000000000..46ebf0bd2d0 --- /dev/null +++ b/releasenotes/notes/ignore-instance-task-state-for-evacuation-e000f141d0153638.yaml @@ -0,0 +1,11 @@ +--- +fixes: + - | + If compute service is down in source node and user try to stop + instance, instance gets stuck at powering-off, hence evacuation fails with + msg: Cannot 'evacuate' instance while it is in + task_state powering-off. + It is now possible for evacuation to ignore the vm task state. + For more details see: `bug 1978983`_ + + .. _`bug 1978983`: https://bugs.launchpad.net/nova/+bug/1978983 \ No newline at end of file From b014d67e946114eb3c807d7e8191ed98b05a1fbd Mon Sep 17 00:00:00 2001 From: Kashyap Chamarthy Date: Thu, 28 Jan 2021 16:35:10 +0100 Subject: [PATCH 40/40] libvirt: Add a workaround to skip compareCPU() on destination 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, provide a workaround to skip the CPU comparison check on the destination host before migrating a guest, and let libvirt handle it correctly. This workaround will be removed once Nova replaces the older libvirt APIs with their newer and improved counterparts[1][2]. - - - Note that Nova's libvirt driver calls compareCPU() in another method, _check_cpu_compatibility(); I did not remove its usage yet. As it needs more careful combing of the code, and then: - where possible, remove the usage of compareCPU() altogether, and rely on libvirt doing the right thing under the hood; or - where Nova _must_ do the CPU comparison checks, switch to the better libvirt CPU APIs -- baselineHypervisorCPU() and compareHypervisorCPU() -- that are described here[1]. This is work in progress[2]. [1] https://opendev.org/openstack/nova-specs/commit/70811da221035044e27 [2] https://review.opendev.org/q/topic:bp%252Fcpu-selection-with-hypervisor-consideration Change-Id: I444991584118a969e9ea04d352821b07ec0ba88d Closes-Bug: #1913716 Signed-off-by: Kashyap Chamarthy Signed-off-by: Balazs Gibizer (cherry picked from commit 267a40663cd8d0b94bbc5ebda4ece55a45753b64) --- nova/conf/workarounds.py | 8 +++++++ nova/tests/unit/virt/libvirt/test_driver.py | 19 +++++++++++++++ nova/virt/libvirt/driver.py | 19 ++++++++------- ...-compare-cpu-on-dest-6ae419ddd61fd0f8.yaml | 24 +++++++++++++++++++ 4 files changed, 61 insertions(+), 9 deletions(-) create mode 100644 releasenotes/notes/skip-compare-cpu-on-dest-6ae419ddd61fd0f8.yaml diff --git a/nova/conf/workarounds.py b/nova/conf/workarounds.py index 4e64d875787..efb5aa4940d 100644 --- a/nova/conf/workarounds.py +++ b/nova/conf/workarounds.py @@ -398,6 +398,14 @@ Related options: * :oslo.config:option:`DEFAULT.vif_plugging_timeout` +"""), + 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/tests/unit/virt/libvirt/test_driver.py b/nova/tests/unit/virt/libvirt/test_driver.py index f947641c97f..02459d0292a 100644 --- a/nova/tests/unit/virt/libvirt/test_driver.py +++ b/nova/tests/unit/virt/libvirt/test_driver.py @@ -10951,6 +10951,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/virt/libvirt/driver.py b/nova/virt/libvirt/driver.py index 8242065c89e..9dbaefef9db 100644 --- a/nova/virt/libvirt/driver.py +++ b/nova/virt/libvirt/driver.py @@ -8601,15 +8601,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/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