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
This commit is contained in:
Matt Riedemann 2018-10-08 17:33:49 -04:00
parent cb36ca76df
commit a99722bb85
3 changed files with 60 additions and 35 deletions

View File

@ -459,7 +459,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)
@ -1084,7 +1084,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)
@ -1190,8 +1190,8 @@ class ResourceTracker(object):
instance,
sign=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
@ -1444,13 +1444,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.
@ -1458,15 +1461,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,
@ -1476,6 +1484,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)

View File

@ -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

View File

@ -755,6 +755,8 @@ class TestUpdateAvailableResources(BaseTestCase):
actual_resources))
update_mock.assert_called_once()
@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',
@ -765,7 +767,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
@ -818,6 +821,8 @@ class TestUpdateAvailableResources(BaseTestCase):
actual_resources))
update_mock.assert_called_once()
@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',
@ -828,7 +833,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
@ -878,6 +884,8 @@ class TestUpdateAvailableResources(BaseTestCase):
actual_resources))
update_mock.assert_called_once()
@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',
@ -888,7 +896,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
@ -2266,6 +2275,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',
@ -2280,7 +2291,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.
@ -2407,6 +2419,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',
@ -2418,7 +2432,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.
@ -2774,7 +2789,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)
@ -2784,7 +2799,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()
@ -2794,7 +2809,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')
@ -2807,7 +2823,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.'
@ -2826,7 +2843,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.'
@ -2838,7 +2856,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.'
@ -2853,7 +2872,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,