openstack/nova
Code Issues Proposed changes

Fix fill_metadata usage for the ImagePropertiesWeigher

Browse Source 
When using the weigher, we need to target the right cell context for the
existing instances in the host.
fill_metadata was also having an issue as we need to pass the dict value
from the updated dict by keying the instance uuid, not the whole dict of
updated instances.

Change-Id: I18260095ed263da4204f21de27f866568843804e
Closes-Bug: #2125935
Signed-off-by: Sylvain Bauza <sbauza@redhat.com>
This commit is contained in:
Sylvain Bauza
2025-09-30 16:08:57 +02:00
parent 04afc452b3
commit 98885344bd
7 changed files with 118 additions and 18 deletions
Download Patch File Download Diff File Expand all files Collapse all files

8
doc/source/admin/scheduling.rst
View File

@@ -1104,6 +1104,14 @@ provided in the option value.
The resulted host weight would then be multiplied by the value of
:oslo.config:option:`filter_scheduler.image_props_weight_multiplier`.
.. note::
The weigher compares the values of the image properties as strings. If some
image properties are lists (eg. hw_numa_nodes), then if the values are
ordered differently, then the weigher will consider them as different
values.
Utilization-aware scheduling
----------------------------

3
nova/objects/instance.py
View File

@@ -1605,7 +1605,8 @@ class InstanceList(base.ObjectListBase, base.NovaObject):
for inst in [i for i in self if i.uuid in updated]:
# Patch up our instances with system_metadata from the fill
# operation
inst.system_metadata = utils.instance_sys_meta(updated)
inst.system_metadata = utils.instance_sys_meta(
updated[inst.uuid])
@base.remotable_classmethod
def get_uuids_by_host(cls, context, host):

14
nova/scheduler/weights/image_props.py
View File

@@ -74,10 +74,18 @@ class ImagePropertiesWeigher(weights.BaseHostWeigher):
# context so we can access all of them, not only the ones from the
# request.
ctxt = nova_context.get_admin_context()
insts = objects.InstanceList(ctxt,
objects=host_state.instances.values())
# system_metadata isn't loaded yet, let's do this.
insts.fill_metadata()
# Given all instances are in the same host, we can target the same
# cell.
try:
cell_mapping = objects.CellMapping.get_by_uuid(
ctxt, host_state.cell_uuid)
except exception.CellMappingNotFound:
return weight
with nova_context.target_cell(ctxt, cell_mapping) as cell_ctxt:
insts = objects.InstanceList(cell_ctxt,
objects=host_state.instances.values())
insts.fill_metadata()
for inst in insts:
try:

15
nova/tests/functional/regressions/test_bug_2125935.py
View File

@@ -83,14 +83,12 @@ class TestImagePropsWeigher(integrated_helpers._IntegratedTestBase):
networks='none',
)
# The weigher is called but it says that there are no existing
# instances on both the hosts with the same image props, while we are
# sure that the values should be 1.0 for both hosts.
# FIXME(sbauza): That's due to bug/2125935
# now the weigher sees that host1 has an instance with the same image
# props
mock_debug.assert_any_call(
"%s: raw weights %s",
"ImagePropertiesWeigher",
{('host2', 'host2'): 0.0, ('host1', 'host1'): 0.0})
{('host2', 'host2'): 1.0, ('host1', 'host1'): 1.0})
mock_debug.reset_mock()
# server3 is now on the same host than host1 as the weigh multiplier
# makes the scheduler to pack instances sharing the same image props.
@@ -100,13 +98,10 @@ class TestImagePropsWeigher(integrated_helpers._IntegratedTestBase):
name='inst4',
networks='none',
)
# FIXME(sbauza): Same issue. The values should be 2.0 for host1 and 1.0
# for host2. That said, due to the fact the HostState is refreshed
# already for the previous schedule, the system metadata is existing
# for this instance so that's why the weight is 1.0 for host1.
# eventually, the weigher sees the two existing instances on host1
mock_debug.assert_any_call(
"%s: raw weights %s",
"ImagePropertiesWeigher",
{('host2', 'host2'): 0.0, ('host1', 'host1'): 1.0})
{('host2', 'host2'): 1.0, ('host1', 'host1'): 2.0})
# server4 is now packed with server1 and server3.
self.assertEqual('host1', server4['OS-EXT-SRV-ATTR:host'])

13
nova/tests/unit/objects/test_instance.py
View File

@@ -2066,6 +2066,8 @@ class TestInstanceListObject(test_objects._LocalTest,
'host': 'foo'}
for i in range(5):
# persist some metadata directly in the DB
values['system_metadata'] = {'BTTF2': '2015'}
db.instance_create(self.context,
dict(values))
@@ -2089,12 +2091,17 @@ class TestInstanceListObject(test_objects._LocalTest,
self.assertEqual(0, len([i for i in insts
if 'system_metadata' not in i]))
#  Inst 1 is now updated with the new metadata
self.assertEqual({'BTTF1': '1955'}, insts[1].system_metadata)
# Inst 2 should have not had its in-memory copy clobbered
self.assertEqual({'bttf3': '1885'}, insts[2].system_metadata)
# Inst 1 should have system_metadata loaded, but empty
self.assertIn('system_metadata', insts[0])
self.assertEqual({}, insts[0].system_metadata)
# Other instances should have system_metadata loaded directly from the
# DB
for i in [0, 3, 4]:
self.assertIn('system_metadata', insts[i])
self.assertEqual({'BTTF2': '2015'}, insts[i].system_metadata)
def test_fill_metadata_nop(self):
insts = objects.InstanceList([objects.Instance(uuid=uuids.inst,

75
nova/tests/unit/scheduler/weights/test_weights_image_props.py
View File

@@ -16,6 +16,8 @@ from unittest import mock
from oslo_utils.fixture import uuidsentinel as uuids
from nova import context as nova_context
from nova import exception
from nova import objects
from nova.scheduler import weights
from nova.scheduler.weights import image_props
@@ -36,7 +38,7 @@ PROP_LIN_PC = objects.ImageMeta(
properties=objects.ImageMetaProps(os_distro='linux', hw_machine_type='pc'))
class ImagePropertiesWeigherTestCase(test.NoDBTestCase):
class _ImagePropertiesWeigherBase(test.NoDBTestCase):
def setUp(self):
super().setUp()
@@ -90,6 +92,9 @@ class ImagePropertiesWeigherTestCase(test.NoDBTestCase):
return [fakes.FakeHostState(host, node, values)
for host, node, values in host_values]
class ImagePropertiesWeigherTestCase(_ImagePropertiesWeigherBase):
@mock.patch('nova.objects.InstanceList.fill_metadata')
def test_multiplier_default(self, mock_fm):
hostinfo_list = self._get_all_hosts()
@@ -194,3 +199,71 @@ class ImagePropertiesWeigherTestCase(test.NoDBTestCase):
self.assertEqual('host3', weights[0].obj.host)
mock_fm.assert_has_calls([mock.call(), mock.call(),
mock.call(), mock.call()])
class TestTargetCellCalled(_ImagePropertiesWeigherBase):
# Using real cell infrastructure instead of SingleCellSimple fixture
# as we need to verify set_target_cell calls are made
USES_DB_SELF = True
@mock.patch('nova.objects.InstanceList.fill_metadata')
@mock.patch.object(nova_context, 'set_target_cell')
@mock.patch('nova.objects.CellMapping.get_by_uuid')
@mock.patch.object(nova_context.RequestContext, 'from_dict')
@mock.patch.object(nova_context, 'get_admin_context')
def test_target_cell_called(self, mock_get_admin_context, mock_from_dict,
mock_get_by_uuid, mock_target_cell, mock_fm):
fake_context = nova_context.RequestContext(is_admin=True)
fake_cell_ctx = nova_context.RequestContext(is_admin=True)
# let's simulate that we set a cell context as target_cell calls
# from_dict internally
mock_from_dict.return_value = fake_cell_ctx
mock_get_admin_context.return_value = fake_context
self.flags(image_props_weight_multiplier=1.0, group='filter_scheduler')
fake_cell_mapping = objects.CellMapping(uuid=uuids.cell1)
mock_get_by_uuid.side_effect = [fake_cell_mapping, fake_cell_mapping,
# host3 won't have a cell mapping
exception.CellMappingNotFound(
uuid=uuids.cell2)]
# Just create three hosts with only one instance, we just want to test
# the calls to target_cell and fill_metadata.
hostinfo_list = [
fakes.FakeHostState('host1', 'node1',
{'instances': {uuids.inst1: objects.Instance(
uuid=uuids.inst1)},
'cell_uuid': uuids.cell1}),
fakes.FakeHostState('host2', 'node2',
{'instances': {}, 'cell_uuid': uuids.cell1}),
# host3 is in a different cell
fakes.FakeHostState('host3', 'node3',
{'instances': {}, 'cell_uuid': uuids.cell2}),
]
request_spec = objects.RequestSpec(image=PROP_WIN_PC)
weights = self.weight_handler.get_weighed_objects(
self.weighers, hostinfo_list, weighing_properties=request_spec)
mock_get_by_uuid.assert_has_calls([
mock.call(fake_context, uuids.cell1),
mock.call(fake_context, uuids.cell1),
mock.call(fake_context, uuids.cell2)])
# Now we see that set_target_cell is called with the cell context only
# twice given host3 does not have a cell mapping
mock_target_cell.assert_has_calls(
[mock.call(fake_cell_ctx, fake_cell_mapping),
mock.call(fake_cell_ctx, fake_cell_mapping)])
# Ensure that fill_metadata was called with the correct context
def _fake_fill_metadata(_self):
self.assertEqual(fake_cell_ctx, _self._context)
mock_fm.side_effect = _fake_fill_metadata
# Double check that we called it only twice
self.assertEqual(2, mock_fm.call_count)
# host1 wins but we return all three hosts.
self.assertEqual(3, len(weights))
self.assertEqual('host1', weights[0].obj.host)

8
releasenotes/notes/bug-2125935-de97147d5ff960b5.yaml Normal file
View File

@@ -0,0 +1,8 @@
---
fixes:
- |
Previously the ImagePropertiesWeigher was not running correctly as it wasn't
targeting the correct cell context for getting the system metadata of the
instances. This was fixed in bug `#2125935`_.
.. _`#2125935`: https://bugs.launchpad.net/nova/+bug/2125935