From 980013d7008aff3f83d9de646417f387b29dc1df Mon Sep 17 00:00:00 2001 From: Matt Riedemann Date: Mon, 8 Oct 2018 17:33:49 -0400 Subject: [PATCH] Properly track local root disk usage during moves Change I0839470c4bcfb16590a0d87b306d683b059bf8a9 fixed root disk usage tracking for volume-backed instances when performing an instance_claim which happens during the initial server create and unshelve. However, root disk reporting is still wrong for volume-backed instances during move claims (resize and evacuate) because a move claim calls _update_usage_from_migration which passes a Flavor object to ResourceTracker._get_usage_dict() and that method didn't have "is_bfv" logic in that scenario. This fixes the bug by always passing the instance object to the _get_usage_dict() method so we can determine if it's volume-backed and if so report the root_gb usage as 0. The related functional regression test is updated appropriately to show the bug is fixed for volume-backed instances. Change-Id: Ia19264ae7c88bb03ed3118795d4011ceb62ef92c Closes-Bug: #1796737 (cherry picked from commit a99722bb854d2ba99c3abc2d2738ba281148fde4) --- nova/compute/resource_tracker.py | 32 +++++++++----- .../tests/functional/test_boot_from_volume.py | 19 +++----- .../unit/compute/test_resource_tracker.py | 44 ++++++++++++++----- 3 files changed, 60 insertions(+), 35 deletions(-) diff --git a/nova/compute/resource_tracker.py b/nova/compute/resource_tracker.py index 3a21689e2d9c..2ccd6aaf96ab 100644 --- a/nova/compute/resource_tracker.py +++ b/nova/compute/resource_tracker.py @@ -457,7 +457,7 @@ class ResourceTracker(object): numa_topology = self._get_migration_context_resource( 'numa_topology', instance, prefix=prefix) usage = self._get_usage_dict( - instance_type, numa_topology=numa_topology) + instance_type, instance, numa_topology=numa_topology) self._drop_pci_devices(instance, nodename, prefix) self._update_usage(usage, nodename, sign=-1) @@ -1066,7 +1066,7 @@ class ResourceTracker(object): if itype: cn = self.compute_nodes[nodename] usage = self._get_usage_dict( - itype, numa_topology=numa_topology) + itype, instance, numa_topology=numa_topology) if self.pci_tracker and sign: self.pci_tracker.update_pci_for_instance( context, instance, sign=sign) @@ -1176,8 +1176,8 @@ class ResourceTracker(object): self.reportclient.update_instance_allocation(context, cn, instance, sign) # new instance, update compute node resource usage: - self._update_usage(self._get_usage_dict(instance), nodename, - sign=sign) + self._update_usage(self._get_usage_dict(instance, instance), + nodename, sign=sign) # Stop tracking removed instances in the is_bfv cache. This needs to # happen *after* calling _get_usage_dict() since that relies on the @@ -1486,13 +1486,16 @@ class ResourceTracker(object): # them. In that case - just get the instance flavor. return instance.flavor - def _get_usage_dict(self, object_or_dict, **updates): + def _get_usage_dict(self, object_or_dict, instance, **updates): """Make a usage dict _update methods expect. Accepts a dict or an Instance or Flavor object, and a set of updates. Converts the object to a dict and applies the updates. :param object_or_dict: instance or flavor as an object or just a dict + :param instance: nova.objects.Instance for the related operation; this + is needed to determine if the instance is + volume-backed :param updates: key-value pairs to update the passed object. Currently only considers 'numa_topology', all other keys are ignored. @@ -1500,15 +1503,20 @@ class ResourceTracker(object): :returns: a dict with all the information from object_or_dict updated with updates """ - usage = {} - if isinstance(object_or_dict, objects.Instance): + + def _is_bfv(): # Check to see if we have the is_bfv value cached. - if object_or_dict.uuid in self.is_bfv: - is_bfv = self.is_bfv[object_or_dict.uuid] + if instance.uuid in self.is_bfv: + is_bfv = self.is_bfv[instance.uuid] else: is_bfv = compute_utils.is_volume_backed_instance( - object_or_dict._context, object_or_dict) - self.is_bfv[object_or_dict.uuid] = is_bfv + instance._context, instance) + self.is_bfv[instance.uuid] = is_bfv + return is_bfv + + usage = {} + if isinstance(object_or_dict, objects.Instance): + is_bfv = _is_bfv() usage = {'memory_mb': object_or_dict.flavor.memory_mb, 'swap': object_or_dict.flavor.swap, 'vcpus': object_or_dict.flavor.vcpus, @@ -1518,6 +1526,8 @@ class ResourceTracker(object): 'numa_topology': object_or_dict.numa_topology} elif isinstance(object_or_dict, objects.Flavor): usage = obj_base.obj_to_primitive(object_or_dict) + if _is_bfv(): + usage['root_gb'] = 0 else: usage.update(object_or_dict) diff --git a/nova/tests/functional/test_boot_from_volume.py b/nova/tests/functional/test_boot_from_volume.py index e5098a1dcc85..7804fbe9dc6e 100644 --- a/nova/tests/functional/test_boot_from_volume.py +++ b/nova/tests/functional/test_boot_from_volume.py @@ -23,9 +23,9 @@ class BootFromVolumeTest(integrated_helpers.InstanceHelperMixin, response = self.admin_api.api_get('/os-hypervisors/statistics') return response.body['hypervisor_statistics'] - def _verify_zero_local_gb_used(self, expected=0): + def _verify_zero_local_gb_used(self): stats = self._get_hypervisor_stats() - self.assertEqual(expected, stats['local_gb_used']) + self.assertEqual(0, stats['local_gb_used']) def _verify_instance_flavor_not_zero(self, instance_uuid): # We are trying to avoid saving instance records with root_gb=0 @@ -88,8 +88,7 @@ class BootFromVolumeTest(integrated_helpers.InstanceHelperMixin, self._wait_for_state_change(self.api, created_server, 'VERIFY_RESIZE') # Check that hypervisor local disk reporting is still 0 - # FIXME(mriedem): Expect 0 once bug 1796737 is fixed. - self._verify_zero_local_gb_used(expected=16384) + self._verify_zero_local_gb_used() # Check that instance has not been saved with 0 root_gb self._verify_instance_flavor_not_zero(server_id) # Check that request spec has not been saved with 0 root_gb @@ -101,8 +100,7 @@ class BootFromVolumeTest(integrated_helpers.InstanceHelperMixin, self._wait_for_state_change(self.api, created_server, 'ACTIVE') # Check that hypervisor local disk reporting is still 0 - # FIXME(mriedem): Expect 0 once bug 1796737 is fixed. - self._verify_zero_local_gb_used(expected=8192) + self._verify_zero_local_gb_used() # Check that instance has not been saved with 0 root_gb self._verify_instance_flavor_not_zero(server_id) # Check that request spec has not been saved with 0 root_gb @@ -115,8 +113,7 @@ class BootFromVolumeTest(integrated_helpers.InstanceHelperMixin, 'SHELVED_OFFLOADED') # Check that hypervisor local disk reporting is still 0 - # FIXME(mriedem): Expect 0 once bug 1796737 is fixed. - self._verify_zero_local_gb_used(expected=8192) + self._verify_zero_local_gb_used() # Check that instance has not been saved with 0 root_gb self._verify_instance_flavor_not_zero(server_id) # Check that request spec has not been saved with 0 root_gb @@ -128,8 +125,7 @@ class BootFromVolumeTest(integrated_helpers.InstanceHelperMixin, self._wait_for_state_change(self.api, created_server, 'ACTIVE') # Check that hypervisor local disk reporting is still 0 - # FIXME(mriedem): Expect 0 once bug 1796737 is fixed. - self._verify_zero_local_gb_used(expected=8192) + self._verify_zero_local_gb_used() # Check that instance has not been saved with 0 root_gb self._verify_instance_flavor_not_zero(server_id) # Check that request spec has not been saved with 0 root_gb @@ -144,8 +140,7 @@ class BootFromVolumeTest(integrated_helpers.InstanceHelperMixin, self._wait_for_state_change(self.api, created_server, 'ACTIVE') # Check that hypervisor local disk reporting is still 0 - # FIXME(mriedem): Expect 0 once bug 1796737 is fixed. - self._verify_zero_local_gb_used(expected=8192) + self._verify_zero_local_gb_used() # Check that instance has not been saved with 0 root_gb self._verify_instance_flavor_not_zero(server_id) # Check that request spec has not been saved with 0 root_gb diff --git a/nova/tests/unit/compute/test_resource_tracker.py b/nova/tests/unit/compute/test_resource_tracker.py index 0fb8efc4ff5d..af8566e0f7e8 100644 --- a/nova/tests/unit/compute/test_resource_tracker.py +++ b/nova/tests/unit/compute/test_resource_tracker.py @@ -726,6 +726,8 @@ class TestUpdateAvailableResources(BaseTestCase): self.assertTrue(obj_base.obj_equal_prims(expected_resources, actual_resources)) + @mock.patch('nova.compute.utils.is_volume_backed_instance', + return_value=False) @mock.patch('nova.objects.InstancePCIRequests.get_by_instance', return_value=objects.InstancePCIRequests(requests=[])) @mock.patch('nova.objects.PciDeviceList.get_by_compute_node', @@ -736,7 +738,8 @@ class TestUpdateAvailableResources(BaseTestCase): @mock.patch('nova.objects.InstanceList.get_by_host_and_node') def test_no_instances_source_migration(self, get_mock, get_inst_mock, migr_mock, get_cn_mock, pci_mock, - instance_pci_mock): + instance_pci_mock, + mock_is_volume_backed_instance): # We test the behavior of update_available_resource() when # there is an active migration that involves this compute node # as the source host not the destination host, and the resource @@ -788,6 +791,8 @@ class TestUpdateAvailableResources(BaseTestCase): self.assertTrue(obj_base.obj_equal_prims(expected_resources, actual_resources)) + @mock.patch('nova.compute.utils.is_volume_backed_instance', + return_value=False) @mock.patch('nova.objects.InstancePCIRequests.get_by_instance', return_value=objects.InstancePCIRequests(requests=[])) @mock.patch('nova.objects.PciDeviceList.get_by_compute_node', @@ -798,7 +803,8 @@ class TestUpdateAvailableResources(BaseTestCase): @mock.patch('nova.objects.InstanceList.get_by_host_and_node') def test_no_instances_dest_migration(self, get_mock, get_inst_mock, migr_mock, get_cn_mock, pci_mock, - instance_pci_mock): + instance_pci_mock, + mock_is_volume_backed_instance): # We test the behavior of update_available_resource() when # there is an active migration that involves this compute node # as the destination host not the source host, and the resource @@ -847,6 +853,8 @@ class TestUpdateAvailableResources(BaseTestCase): self.assertTrue(obj_base.obj_equal_prims(expected_resources, actual_resources)) + @mock.patch('nova.compute.utils.is_volume_backed_instance', + return_value=False) @mock.patch('nova.objects.InstancePCIRequests.get_by_instance', return_value=objects.InstancePCIRequests(requests=[])) @mock.patch('nova.objects.PciDeviceList.get_by_compute_node', @@ -857,7 +865,8 @@ class TestUpdateAvailableResources(BaseTestCase): @mock.patch('nova.objects.InstanceList.get_by_host_and_node') def test_no_instances_dest_evacuation(self, get_mock, get_inst_mock, migr_mock, get_cn_mock, pci_mock, - instance_pci_mock): + instance_pci_mock, + mock_is_volume_backed_instance): # We test the behavior of update_available_resource() when # there is an active evacuation that involves this compute node # as the destination host not the source host, and the resource @@ -2232,6 +2241,8 @@ class TestResize(BaseTestCase): def test_instance_build_resize_confirm(self): self._test_instance_build_resize() + @mock.patch('nova.compute.utils.is_volume_backed_instance', + return_value=False) @mock.patch('nova.objects.Service.get_minimum_version', return_value=22) @mock.patch('nova.pci.stats.PciDeviceStats.support_requests', @@ -2246,7 +2257,8 @@ class TestResize(BaseTestCase): @mock.patch('nova.objects.ComputeNode.save') def test_resize_claim_dest_host_with_pci(self, save_mock, get_mock, migr_mock, get_cn_mock, pci_mock, pci_req_mock, pci_claim_mock, - pci_dev_save_mock, pci_supports_mock, version_mock): + pci_dev_save_mock, pci_supports_mock, version_mock, + mock_is_volume_backed_instance): # Starting from an empty destination compute node, perform a resize # operation for an instance containing SR-IOV PCI devices on the # original host. @@ -2373,6 +2385,8 @@ class TestResize(BaseTestCase): mock_pci_free_device.assert_called_once_with( pci_dev, mock.ANY) + @mock.patch('nova.compute.utils.is_volume_backed_instance', + return_value=False) @mock.patch('nova.objects.Service.get_minimum_version', return_value=22) @mock.patch('nova.objects.InstancePCIRequests.get_by_instance', @@ -2384,7 +2398,8 @@ class TestResize(BaseTestCase): @mock.patch('nova.objects.InstanceList.get_by_host_and_node') @mock.patch('nova.objects.ComputeNode.save') def test_resize_claim_two_instances(self, save_mock, get_mock, migr_mock, - get_cn_mock, pci_mock, instance_pci_mock, version_mock): + get_cn_mock, pci_mock, instance_pci_mock, version_mock, + mock_is_volume_backed_instance): # Issue two resize claims against a destination host with no prior # instances on it and validate that the accounting for resources is # correct. @@ -2740,7 +2755,7 @@ class TestUpdateUsageFromInstance(BaseTestCase): mock_check_bfv.return_value = True # Make sure the cache is empty. self.assertNotIn(self.instance.uuid, self.rt.is_bfv) - result = self.rt._get_usage_dict(self.instance) + result = self.rt._get_usage_dict(self.instance, self.instance) self.assertEqual(0, result['root_gb']) mock_check_bfv.assert_called_once_with( self.instance._context, self.instance) @@ -2750,7 +2765,7 @@ class TestUpdateUsageFromInstance(BaseTestCase): # Now run _get_usage_dict again to make sure we don't call # is_volume_backed_instance. mock_check_bfv.reset_mock() - result = self.rt._get_usage_dict(self.instance) + result = self.rt._get_usage_dict(self.instance, self.instance) self.assertEqual(0, result['root_gb']) mock_check_bfv.assert_not_called() @@ -2760,7 +2775,8 @@ class TestUpdateUsageFromInstance(BaseTestCase): mock_check_bfv.return_value = False instance_with_swap = self.instance.obj_clone() instance_with_swap.flavor.swap = 10 - result = self.rt._get_usage_dict(instance_with_swap) + result = self.rt._get_usage_dict( + instance_with_swap, instance_with_swap) self.assertEqual(10, result['swap']) @mock.patch('nova.compute.utils.is_volume_backed_instance') @@ -2773,7 +2789,8 @@ class TestUpdateUsageFromInstance(BaseTestCase): _NODENAME) mock_update_usage.assert_called_once_with( - self.rt._get_usage_dict(self.instance), _NODENAME, sign=1) + self.rt._get_usage_dict(self.instance, self.instance), + _NODENAME, sign=1) @mock.patch('nova.compute.utils.is_volume_backed_instance') @mock.patch('nova.compute.resource_tracker.ResourceTracker.' @@ -2792,7 +2809,8 @@ class TestUpdateUsageFromInstance(BaseTestCase): # The instance should have been removed from the is_bfv cache. self.assertNotIn(self.instance.uuid, self.rt.is_bfv) mock_update_usage.assert_called_once_with( - self.rt._get_usage_dict(self.instance), _NODENAME, sign=-1) + self.rt._get_usage_dict(self.instance, self.instance), + _NODENAME, sign=-1) @mock.patch('nova.compute.utils.is_volume_backed_instance') @mock.patch('nova.compute.resource_tracker.ResourceTracker.' @@ -2804,7 +2822,8 @@ class TestUpdateUsageFromInstance(BaseTestCase): _NODENAME) mock_update_usage.assert_called_once_with( - self.rt._get_usage_dict(self.instance), _NODENAME, sign=1) + self.rt._get_usage_dict(self.instance, self.instance), + _NODENAME, sign=1) @mock.patch('nova.compute.utils.is_volume_backed_instance') @mock.patch('nova.compute.resource_tracker.ResourceTracker.' @@ -2819,7 +2838,8 @@ class TestUpdateUsageFromInstance(BaseTestCase): self.instance, _NODENAME, True) mock_update_usage.assert_called_once_with( - self.rt._get_usage_dict(self.instance), _NODENAME, sign=-1) + self.rt._get_usage_dict(self.instance, self.instance), + _NODENAME, sign=-1) @mock.patch('nova.objects.Instance.get_by_uuid') def test_remove_deleted_instances_allocations_deleted_instance(self,