From c7a6673fd5621d1c121c20376634ec49644fae59 Mon Sep 17 00:00:00 2001 From: Nikola Dipanov Date: Wed, 17 Feb 2016 19:27:36 +0000 Subject: [PATCH] RT: aborting claims clears instance host and NUMA info When the claim is aborted, this information is no longer correct for the instance, so we clear it to avoid inconsistencies. Change-Id: I83a5f06adb22c21392d5fc867728181ea4b0454d Resolves-bug: 1545675 --- nova/compute/resource_tracker.py | 24 ++++++++---- .../unit/compute/test_resource_tracker.py | 12 +++++- nova/tests/unit/compute/test_tracker.py | 38 ++++++++++++++++--- 3 files changed, 60 insertions(+), 14 deletions(-) diff --git a/nova/compute/resource_tracker.py b/nova/compute/resource_tracker.py index c1345dbdac9d..c2650fde9d1c 100644 --- a/nova/compute/resource_tracker.py +++ b/nova/compute/resource_tracker.py @@ -178,7 +178,7 @@ class ResourceTracker(object): if self.disabled: # compute_driver doesn't support resource tracking, just # set the 'host' and node fields and continue the build: - self._set_instance_host_and_node(context, instance_ref) + self._set_instance_host_and_node(instance_ref) return claims.NopClaim() # sanity checks: @@ -206,7 +206,7 @@ class ResourceTracker(object): # that numa_topology is saved while under COMPUTE_RESOURCE_SEMAPHORE # so that the resource audit knows about any cpus we've pinned. instance_ref.numa_topology = claim.claimed_numa_topology - self._set_instance_host_and_node(context, instance_ref) + self._set_instance_host_and_node(instance_ref) # Mark resources in-use and update stats self._update_usage_from_instance(context, instance_ref) @@ -326,14 +326,15 @@ class ResourceTracker(object): migration.status = 'pre-migrating' migration.save() - def _set_instance_host_and_node(self, context, instance): + def _set_instance_host_and_node(self, instance, clear=False): """Tag the instance as belonging to this host. This should be done while the COMPUTE_RESOURCES_SEMAPHORE is held so the resource claim will not be lost if the audit process starts. """ - instance.host = self.host - instance.launched_on = self.host - instance.node = self.nodename + instance.host = None if clear else self.host + if not clear: + instance.launched_on = self.host + instance.node = None if clear else self.nodename instance.save() @utils.synchronized(COMPUTE_RESOURCE_SEMAPHORE) @@ -341,8 +342,15 @@ class ResourceTracker(object): """Remove usage from the given instance.""" # flag the instance as deleted to revert the resource usage # and associated stats: - instance['vm_state'] = vm_states.DELETED - self._update_usage_from_instance(context, instance) + real_vm_state = instance.vm_state + instance.vm_state = vm_states.DELETED + try: + self._update_usage_from_instance(context, instance) + finally: + instance.vm_state = real_vm_state + + instance.clear_numa_topology() + self._set_instance_host_and_node(instance, clear=True) self._update(context.elevated()) diff --git a/nova/tests/unit/compute/test_resource_tracker.py b/nova/tests/unit/compute/test_resource_tracker.py index 5d668fe078ef..2a1a18628a3e 100644 --- a/nova/tests/unit/compute/test_resource_tracker.py +++ b/nova/tests/unit/compute/test_resource_tracker.py @@ -735,12 +735,22 @@ class TrackerTestCase(BaseTrackerTestCase): def test_set_instance_host_and_node(self): inst = objects.Instance() with mock.patch.object(inst, 'save') as mock_save: - self.tracker._set_instance_host_and_node(self.context, inst) + self.tracker._set_instance_host_and_node(inst) mock_save.assert_called_once_with() self.assertEqual(self.tracker.host, inst.host) self.assertEqual(self.tracker.nodename, inst.node) self.assertEqual(self.tracker.host, inst.launched_on) + def test_set_instance_host_and_node_clear(self): + inst = objects.Instance() + with mock.patch.object(inst, 'save') as mock_save: + self.tracker._set_instance_host_and_node(inst) + self.tracker._set_instance_host_and_node(inst, clear=True) + self.assertEqual(2, mock_save.call_count) + self.assertIsNone(inst.host) + self.assertIsNone(inst.node) + self.assertEqual(self.tracker.host, inst.launched_on) + class SchedulerClientTrackerTestCase(BaseTrackerTestCase): diff --git a/nova/tests/unit/compute/test_tracker.py b/nova/tests/unit/compute/test_tracker.py index d989d4aabef8..eab96ed7fc4d 100644 --- a/nova/tests/unit/compute/test_tracker.py +++ b/nova/tests/unit/compute/test_tracker.py @@ -1294,8 +1294,15 @@ class TestInstanceClaim(BaseTestCase): self.assertEqual(0, self.rt.compute_node.memory_mb_used) self.assertEqual(0, self.rt.compute_node.running_vms) - @mock.patch.object(self.instance, 'save') - def _doit(mock_save): + mock_save = mock.MagicMock() + mock_clear_numa = mock.MagicMock() + + @mock.patch.object(self.instance, 'save', mock_save) + @mock.patch.object(self.instance, 'clear_numa_topology', + mock_clear_numa) + @mock.patch.object(objects.Instance, 'obj_clone', + return_value=self.instance) + def _doit(mock_clone): with self.rt.instance_claim(self.ctx, self.instance, None): # Raise an exception. Just make sure below that the abort() # method of the claim object was called (and the resulting @@ -1303,6 +1310,10 @@ class TestInstanceClaim(BaseTestCase): raise test.TestingException() self.assertRaises(test.TestingException, _doit) + self.assertEqual(2, mock_save.call_count) + mock_clear_numa.assert_called_once_with() + self.assertIsNone(self.instance.host) + self.assertIsNone(self.instance.node) # Assert that the resources claimed by the Claim() constructor # are returned to the resource tracker due to the claim's abort() @@ -1317,15 +1328,32 @@ class TestInstanceClaim(BaseTestCase): pci_mock.return_value = objects.InstancePCIRequests(requests=[]) disk_used = self.instance.root_gb + self.instance.ephemeral_gb - with mock.patch.object(self.instance, 'save'): - claim = self.rt.instance_claim(self.ctx, self.instance, None) + @mock.patch.object(objects.Instance, 'obj_clone', + return_value=self.instance) + @mock.patch.object(self.instance, 'save') + def _claim(mock_save, mock_clone): + return self.rt.instance_claim(self.ctx, self.instance, None) + claim = _claim() self.assertEqual(disk_used, self.rt.compute_node.local_gb_used) self.assertEqual(self.instance.memory_mb, self.rt.compute_node.memory_mb_used) self.assertEqual(1, self.rt.compute_node.running_vms) - claim.abort() + mock_save = mock.MagicMock() + mock_clear_numa = mock.MagicMock() + + @mock.patch.object(self.instance, 'save', mock_save) + @mock.patch.object(self.instance, 'clear_numa_topology', + mock_clear_numa) + def _abort(): + claim.abort() + + _abort() + mock_save.assert_called_once_with() + mock_clear_numa.assert_called_once_with() + self.assertIsNone(self.instance.host) + self.assertIsNone(self.instance.node) self.assertEqual(0, self.rt.compute_node.local_gb_used) self.assertEqual(0, self.rt.compute_node.memory_mb_used)