openstack
/
nova
Code Issues Proposed changes

Improve efficiency of Migration.instance property 

In the resource tracker, we get a list of migrations and rely on the
Migration.instance property to look up the related instance. This
doesn't actually cache the instance, which means repeated calls will
continue to trigger RPC and DB hits. Further, in resource tracker,
we have already pulled a list of instances and then proceed to re-query
them through lazy-loads from the migration objects, even though we
already have them. To add insult to injury, neither this first list
of instances, nor the ones pulled from Migration.instance contain
instances with the 'flavor' or 'migration_context' properties
populated, which means we trigger *two* more lazy loads to pull those.

This patch does a few things:
 - Makes the Migration object cache its instance property
 - Makes the resource tracker request instances with flavors and the
   migration_context populated
 - Stitches the instance list into the migration list to avoid the
   two subsequent lazy loads of Instance and Instance.flavor

Change-Id: Ifc7dcde8a659710acecb1967da15c632c69d675c
Partial-Bug: #1540526
This commit is contained in:
Dan Smith 2016-02-01 11:55:15 -08:00
parent 2e96f7f2d7
commit 7a9942c592
5 changed files with 72 additions and 8 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

20
nova/compute/resource_tracker.py
View File

@ -31,7 +31,7 @@ from nova.compute import resources as ext_resources
from nova.compute import task_states
from nova.compute import vm_states
from nova import exception
from nova.i18n import _, _LI, _LW
from nova.i18n import _, _LE, _LI, _LW
from nova import objects
from nova.objects import base as obj_base
from nova.objects import migration as migration_obj
@ -491,6 +491,20 @@ class ResourceTracker(object):
self._update_available_resource(context, resources)
def _pair_instances_to_migrations(self, migrations, instances):
instance_by_uuid = {inst.uuid: inst for inst in instances}
for migration in migrations:
try:
migration.instance = instance_by_uuid[migration.instance_uuid]
except KeyError:
# NOTE(danms): If this happens, we don't set it here, and
# let the code either fail or lazy-load the instance later
# which is what happened before we added this optimization.
# This _should_ not be possible, of course.
LOG.error(_LE('Migration for instance %(uuid)s refers to '
'another host\'s instance!'),
{'uuid': migration.instance_uuid})
@utils.synchronized(COMPUTE_RESOURCE_SEMAPHORE)
def _update_available_resource(self, context, resources):
@ -516,7 +530,8 @@ class ResourceTracker(object):
instances = objects.InstanceList.get_by_host_and_node(
context, self.host, self.nodename,
expected_attrs=['system_metadata',
'numa_topology'])
'numa_topology',
'flavor', 'migration_context'])
# Now calculate usage based on instance utilization:
self._update_usage_from_instances(context, instances)
@ -525,6 +540,7 @@ class ResourceTracker(object):
migrations = objects.MigrationList.get_in_progress_by_host_and_node(
context, self.host, self.nodename)
self._pair_instances_to_migrations(migrations, instances)
self._update_usage_from_migrations(context, migrations)
# Detect and account for orphaned instances that may exist on the

9
nova/objects/migration.py
View File

@ -119,7 +119,14 @@ class Migration(base.NovaPersistentObject, base.NovaObject,
@property
def instance(self):
return objects.Instance.get_by_uuid(self._context, self.instance_uuid)
if not hasattr(self, '_cached_instance'):
self._cached_instance = objects.Instance.get_by_uuid(
self._context, self.instance_uuid)
return self._cached_instance
@instance.setter
def instance(self, instance):
self._cached_instance = instance
@base.NovaObjectRegistry.register

36
nova/tests/unit/compute/test_resource_tracker.py
View File

@ -40,6 +40,7 @@ from nova.objects import pci_device_pool
from nova import rpc
from nova import test
from nova.tests.unit.pci import fakes as pci_fakes
from nova.tests import uuidsentinel
from nova.virt import driver
@ -1006,19 +1007,46 @@ class InstanceClaimTestCase(BaseTrackerTestCase):
"fakenode")
@mock.patch('nova.objects.MigrationList.get_in_progress_by_host_and_node')
def test_instances_with_live_migrations(self, mock_migration_list):
@mock.patch('nova.objects.InstanceList.get_by_host_and_node')
def test_instances_with_live_migrations(self, mock_instance_list,
mock_migration_list):
instance = self._fake_instance_obj()
migration = objects.Migration(context=self.context,
migration_type='live-migration',
instance_uuid=instance.uuid)
mock_migration_list.return_value = [migration]
self.tracker.update_available_resource(self.context)
self.assertEqual(0, self.tracker.compute_node['memory_mb_used'])
self.assertEqual(0, self.tracker.compute_node['local_gb_used'])
mock_instance_list.return_value = [instance]
with mock.patch.object(self.tracker, '_pair_instances_to_migrations'
) as mock_pair:
self.tracker.update_available_resource(self.context)
self.assertTrue(mock_pair.called)
self.assertEqual(
instance.uuid,
mock_pair.call_args_list[0][0][0][0].instance_uuid)
self.assertEqual(instance.uuid,
mock_pair.call_args_list[0][0][1][0].uuid)
self.assertEqual(
['system_metadata', 'numa_topology', 'flavor',
'migration_context'],
mock_instance_list.call_args_list[0][1]['expected_attrs'])
self.assertEqual(FAKE_VIRT_MEMORY_MB + FAKE_VIRT_MEMORY_OVERHEAD,
self.tracker.compute_node['memory_mb_used'])
self.assertEqual(ROOT_GB + EPHEMERAL_GB,
self.tracker.compute_node['local_gb_used'])
mock_migration_list.assert_called_once_with(self.context,
"fakehost",
"fakenode")
def test_pair_instances_to_migrations(self):
migrations = [objects.Migration(instance_uuid=uuidsentinel.instance1),
objects.Migration(instance_uuid=uuidsentinel.instance2)]
instances = [objects.Instance(uuid=uuidsentinel.instance2),
objects.Instance(uuid=uuidsentinel.instance1)]
self.tracker._pair_instances_to_migrations(migrations, instances)
order = [uuidsentinel.instance1, uuidsentinel.instance2]
for i, migration in enumerate(migrations):
self.assertEqual(order[i], migration.instance.uuid)
@mock.patch('nova.compute.claims.Claim')
@mock.patch('nova.objects.Instance.save')
def test_claim_saves_numa_topology(self, mock_save, mock_claim):

4
nova/tests/unit/compute/test_tracker.py
View File

@ -452,7 +452,9 @@ class TestUpdateAvailableResources(BaseTestCase):
'fake-node',
expected_attrs=[
'system_metadata',
'numa_topology'])
'numa_topology',
'flavor',
'migration_context'])
get_cn_mock.assert_called_once_with(mock.sentinel.ctx, 'fake-host',
'fake-node')
migr_mock.assert_called_once_with(mock.sentinel.ctx, 'fake-host',

11
nova/tests/unit/objects/test_migration.py
View File

@ -22,6 +22,7 @@ from nova import objects
from nova.objects import migration
from nova.tests.unit import fake_instance
from nova.tests.unit.objects import test_objects
from nova.tests import uuidsentinel
NOW = timeutils.utcnow().replace(microsecond=0)
@ -142,6 +143,16 @@ class _TestMigrationObject(object):
self.mox.ReplayAll()
self.assertEqual(mig.instance.host, fake_inst['host'])
def test_instance_setter(self):
migration = objects.Migration(instance_uuid=uuidsentinel.instance)
inst = objects.Instance(uuid=uuidsentinel.instance)
with mock.patch('nova.objects.Instance.get_by_uuid') as mock_get:
migration.instance = inst
migration.instance
self.assertFalse(mock_get.called)
self.assertEqual(inst, migration._cached_instance)
self.assertEqual(inst, migration.instance)
def test_get_unconfirmed_by_dest_compute(self):
ctxt = context.get_admin_context()
fake_migration = fake_db_migration()