From 783598eec147223f41cc31e3488119bf0aeed656 Mon Sep 17 00:00:00 2001 From: Takashi Kajinami Date: Thu, 4 Mar 2021 22:27:25 +0900 Subject: [PATCH] Clean up allocations left by evacuation when deleting service When a compute node goes down and all instances on the compute node are evacuated, allocation records about these instance are still left in the source compute node until nova-compute service is again started on the node. However if a compute node is completely broken, it is not possible to start the service again. In this situation deleting nova-compute service for the compute node doesn't delete its resource provider record, and even if a user tries to delete the resource provider, the delete request is rejected because allocations are still left on that node. This change ensures that remaining allocations left by successful evacuations are cleared when deleting a nova-compute service, to avoid any resource provider record left even if a compute node can't be recovered. Migration records are still left in 'done' status to trigger clean-up tasks in case the compute node is recovered later. Conflicts: nova/scheduler/client/report.py Closes-Bug: #1829479 Change-Id: I3ce6f6275bfe09d43718c3a491b3991a804027bd (cherry picked from commit e5a34fffdf97fcda7d0abfdc9e23485479ca2c4f) (cherry picked from commit 037e588788e60d7b51ebe2cbb0787b3008f402fd) --- nova/scheduler/client/report.py | 32 +++++++-- nova/tests/functional/test_nova_manage.py | 65 ------------------- nova/tests/functional/wsgi/test_services.py | 25 +++---- .../unit/scheduler/client/test_report.py | 34 +++++++++- .../notes/bug-1829479-cd2db21526965e6d.yaml | 8 +++ 5 files changed, 74 insertions(+), 90 deletions(-) create mode 100644 releasenotes/notes/bug-1829479-cd2db21526965e6d.yaml diff --git a/nova/scheduler/client/report.py b/nova/scheduler/client/report.py index 229b6d03223..964cc1162b1 100644 --- a/nova/scheduler/client/report.py +++ b/nova/scheduler/client/report.py @@ -36,6 +36,7 @@ from nova import exception from nova.i18n import _ from nova import objects +from nova.objects import fields from nova import utils @@ -2218,6 +2219,22 @@ def get_allocations_for_provider_tree(self, context, nodename): return {consumer: self.get_allocs_for_consumer(context, consumer) for consumer in consumers} + def _remove_allocations_for_evacuated_instances(self, context, + compute_node): + filters = { + 'source_compute': compute_node.host, + 'status': ['done'], + 'migration_type': fields.MigrationType.EVACUATION, + } + evacuations = objects.MigrationList.get_by_filters(context, filters) + + for evacuation in evacuations: + if not self.remove_provider_tree_from_instance_allocation( + context, evacuation.instance_uuid, compute_node.uuid): + LOG.error("Failed to clean allocation of evacuated " + "instance on the source node %s", + compute_node.uuid, instance=evacuation.instance) + def delete_resource_provider(self, context, compute_node, cascade=False): """Deletes the ResourceProvider record for the compute_node. @@ -2236,16 +2253,19 @@ def delete_resource_provider(self, context, compute_node, cascade=False): # Delete any allocations for this resource provider. # Since allocations are by consumer, we get the consumers on this # host, which are its instances. - # NOTE(mriedem): This assumes the only allocations on this node - # are instances, but there could be migration consumers if the - # node is deleted during a migration or allocations from an - # evacuated host (bug 1829479). Obviously an admin shouldn't - # do that but...you know. I guess the provider deletion should fail - # in that case which is what we'd want to happen. instance_uuids = objects.InstanceList.get_uuids_by_host_and_node( context, host, nodename) for instance_uuid in instance_uuids: self.delete_allocation_for_instance(context, instance_uuid) + + # When an instance is evacuated, its allocation remains in + # the source compute node until the node recovers again. + # If the broken compute never recovered but instead it is + # decommissioned, then we should delete the allocations of + # successfully evacuated instances during service delete. + self._remove_allocations_for_evacuated_instances(context, + compute_node) + # Ensure to delete resource provider in tree by top-down # traversable order. rps_to_refresh = self.get_providers_in_tree(context, rp_uuid) diff --git a/nova/tests/functional/test_nova_manage.py b/nova/tests/functional/test_nova_manage.py index 96d0b6f11e2..c33efb2cbff 100644 --- a/nova/tests/functional/test_nova_manage.py +++ b/nova/tests/functional/test_nova_manage.py @@ -1573,71 +1573,6 @@ def test_audit_orphaned_allocations_from_confirmed_resize(self): self.assertIn('Processed 1 allocation.', output) self.assertEqual(4, ret) - # TODO(sbauza): Remove this test once bug #1829479 is fixed - def test_audit_orphaned_allocations_from_deleted_compute_evacuate(self): - """Evacuate a server and the delete the source node so that it will - leave a source allocation that the audit command will find. - """ - - source_hostname = self.compute1.host - dest_hostname = self.compute2.host - - source_rp_uuid = self._get_provider_uuid_by_host(source_hostname) - dest_rp_uuid = self._get_provider_uuid_by_host(dest_hostname) - - server = self._boot_and_check_allocations(self.flavor, source_hostname) - - # Stop the service and fake it down - self.compute1.stop() - source_service_id = self.admin_api.get_services( - host=source_hostname, binary='nova-compute')[0]['id'] - self.admin_api.put_service(source_service_id, {'forced_down': 'true'}) - - # evacuate the instance to the target - self._evacuate_server( - server, {'host': dest_hostname}, expected_host=dest_hostname) - - # Now the instance is gone, we can delete the compute service - self.admin_api.api_delete('/os-services/%s' % source_service_id) - - # Since the compute is deleted, we should have in theory a single - # allocation against the destination resource provider, but evacuated - # instances are not having their allocations deleted. See bug #1829479. - # We have two allocations for the same consumer, source and destination - self._check_allocation_during_evacuate( - self.flavor, server['id'], source_rp_uuid, dest_rp_uuid) - - # Now, run the audit command that will find this orphaned allocation - ret = self.cli.audit(verbose=True) - output = self.output.getvalue() - self.assertIn( - 'Allocations for consumer UUID %(consumer_uuid)s on ' - 'Resource Provider %(rp_uuid)s can be deleted' % - {'consumer_uuid': server['id'], - 'rp_uuid': source_rp_uuid}, - output) - self.assertIn('Processed 1 allocation.', output) - self.assertEqual(3, ret) - - # Now we want to delete the orphaned allocation that is duplicate - ret = self.cli.audit(delete=True, verbose=True) - - # We finally should only have the target allocations - self.assertFlavorMatchesUsage(dest_rp_uuid, self.flavor) - self.assertRequestMatchesUsage({'VCPU': 0, - 'MEMORY_MB': 0, - 'DISK_GB': 0}, source_rp_uuid) - - output = self.output.getvalue() - self.assertIn( - 'Deleted allocations for consumer UUID %(consumer_uuid)s on ' - 'Resource Provider %(rp_uuid)s' % - {'consumer_uuid': server['id'], - 'rp_uuid': source_rp_uuid}, - output) - self.assertIn('Processed 1 allocation.', output) - self.assertEqual(4, ret) - class TestDBArchiveDeletedRows(integrated_helpers._IntegratedTestBase): """Functional tests for the "nova-manage db archive_deleted_rows" CLI.""" diff --git a/nova/tests/functional/wsgi/test_services.py b/nova/tests/functional/wsgi/test_services.py index e5132471306..8da8ea96aed 100644 --- a/nova/tests/functional/wsgi/test_services.py +++ b/nova/tests/functional/wsgi/test_services.py @@ -120,8 +120,7 @@ def test_evacuate_then_delete_compute_service(self): """Tests a scenario where a server is created on a host, the host goes down, the server is evacuated to another host, and then the source host compute service is deleted. After that the deleted - compute service is restarted. Related placement resources are checked - throughout. + compute service is restarted and starts successfully. """ # Create our source host that we will evacuate *from* later. host1 = self._start_compute('host1') @@ -152,23 +151,15 @@ def test_evacuate_then_delete_compute_service(self): services = self.admin_api.get_services( binary='nova-compute', host='host1') self.assertEqual(0, len(services), services) - # FIXME(mriedem): This is bug 1829479 where the compute service is - # deleted but the resource provider is not because there are still - # allocations against the provider from the evacuated server. + # Then the resource provider is also deleted. resp = self.placement.get('/resource_providers/%s' % rp_uuid) - self.assertEqual(200, resp.status) - self.assertFlavorMatchesUsage(rp_uuid, flavor) - # Try to restart the host1 compute service to create a new resource - # provider. + self.assertEqual(404, resp.status) + # Try to restart the host1 compute service to create a new service + # and a new resource provider. self.restart_compute_service(host1) - # FIXME(mriedem): This is bug 1817833 where restarting the now-deleted - # compute service attempts to create a new resource provider with a - # new uuid but the same name which results in a conflict. The service - # does not die, however, because _update_available_resource_for_node - # catches and logs but does not re-raise the error. - log_output = self.stdlog.logger.output - self.assertIn('Error updating resources for node host1.', log_output) - self.assertIn('Failed to create resource provider host1', log_output) + # Make sure the compute service record for host1 is recreated. + service = self.admin_api.get_services( + binary='nova-compute', host='host1')[0] def test_migrate_confirm_after_deleted_source_compute(self): """Tests a scenario where a server is cold migrated and while in diff --git a/nova/tests/unit/scheduler/client/test_report.py b/nova/tests/unit/scheduler/client/test_report.py index da1f0d7892c..c4d7449584f 100644 --- a/nova/tests/unit/scheduler/client/test_report.py +++ b/nova/tests/unit/scheduler/client/test_report.py @@ -3184,15 +3184,41 @@ def test_refresh_associations_disabled(self): class TestAllocations(SchedulerReportClientTestCase): + @mock.patch("nova.scheduler.client.report.SchedulerReportClient." + "remove_provider_tree_from_instance_allocation") + @mock.patch('nova.objects.MigrationList.get_by_filters') + def test_remove_allocations_for_evacuated_instances(self, + mock_get_migrations, mock_rm_pr_tree): + cn = objects.ComputeNode(uuid=uuids.cn, host="fake_host", + hypervisor_hostname="fake_hostname", ) + migrations = [mock.Mock(instance_uuid=uuids.inst1, status='done'), + mock.Mock(instance_uuid=uuids.inst2, status='done')] + mock_get_migrations.return_value = migrations + mock_rm_pr_tree.return_value = True + self.client._remove_allocations_for_evacuated_instances(self.context, + cn) + mock_get_migrations.assert_called_once_with( + self.context, + {'source_compute': cn.host, 'status': ['done'], + 'migration_type': 'evacuation'}) + mock_rm_pr_tree.assert_has_calls( + [mock.call(self.context, uuids.inst1, cn.uuid), + mock.call(self.context, uuids.inst2, cn.uuid)]) + # status of migrations should be kept + for migration in migrations: + self.assertEqual('done', migration.status) + @mock.patch('nova.scheduler.client.report.SchedulerReportClient.' 'get_providers_in_tree') @mock.patch("nova.scheduler.client.report.SchedulerReportClient." "delete") + @mock.patch("nova.scheduler.client.report.SchedulerReportClient." + "_remove_allocations_for_evacuated_instances") @mock.patch("nova.scheduler.client.report.SchedulerReportClient." "delete_allocation_for_instance") @mock.patch("nova.objects.InstanceList.get_uuids_by_host_and_node") def test_delete_resource_provider_cascade(self, mock_by_host, - mock_del_alloc, mock_delete, mock_get_rpt): + mock_del_alloc, mock_evacuate, mock_delete, mock_get_rpt): cn = objects.ComputeNode(uuid=uuids.cn, host="fake_host", hypervisor_hostname="fake_hostname", ) mock_by_host.return_value = [uuids.inst1, uuids.inst2] @@ -3208,6 +3234,7 @@ def test_delete_resource_provider_cascade(self, mock_by_host, mock_by_host.assert_called_once_with( self.context, cn.host, cn.hypervisor_hostname) self.assertEqual(2, mock_del_alloc.call_count) + mock_evacuate.assert_called_once_with(self.context, cn) exp_url = "/resource_providers/%s" % uuids.cn mock_delete.assert_called_once_with( exp_url, global_request_id=self.context.global_id) @@ -3217,11 +3244,13 @@ def test_delete_resource_provider_cascade(self, mock_by_host, 'get_providers_in_tree') @mock.patch("nova.scheduler.client.report.SchedulerReportClient." "delete") + @mock.patch("nova.scheduler.client.report.SchedulerReportClient." + "_remove_allocations_for_evacuated_instances") @mock.patch("nova.scheduler.client.report.SchedulerReportClient." "delete_allocation_for_instance") @mock.patch("nova.objects.InstanceList.get_uuids_by_host_and_node") def test_delete_resource_provider_no_cascade(self, mock_by_host, - mock_del_alloc, mock_delete, mock_get_rpt): + mock_del_alloc, mock_evacuate, mock_delete, mock_get_rpt): self.client._association_refresh_time[uuids.cn] = mock.Mock() cn = objects.ComputeNode(uuid=uuids.cn, host="fake_host", hypervisor_hostname="fake_hostname", ) @@ -3236,6 +3265,7 @@ def test_delete_resource_provider_no_cascade(self, mock_by_host, mock_delete.return_value = resp_mock self.client.delete_resource_provider(self.context, cn) mock_del_alloc.assert_not_called() + mock_evacuate.assert_not_called() exp_url = "/resource_providers/%s" % uuids.cn mock_delete.assert_called_once_with( exp_url, global_request_id=self.context.global_id) diff --git a/releasenotes/notes/bug-1829479-cd2db21526965e6d.yaml b/releasenotes/notes/bug-1829479-cd2db21526965e6d.yaml new file mode 100644 index 00000000000..5e7a4c51d25 --- /dev/null +++ b/releasenotes/notes/bug-1829479-cd2db21526965e6d.yaml @@ -0,0 +1,8 @@ +--- +fixes: + - | + `Bug #1829479 `_: Now + deleting a nova-compute service removes allocations of successfully + evacuated instances. This allows the associated resource provider to be + deleted automatically even if the nova-compute service cannot recover after + all instances on the node have been successfully evacuated.