diff --git a/doc/source/admin/scheduling.rst b/doc/source/admin/scheduling.rst index b93b12fdf4..72f23e11df 100644 --- a/doc/source/admin/scheduling.rst +++ b/doc/source/admin/scheduling.rst @@ -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 ---------------------------- diff --git a/nova/objects/instance.py b/nova/objects/instance.py index f3f10bb623..b387b8e0fe 100644 --- a/nova/objects/instance.py +++ b/nova/objects/instance.py @@ -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): diff --git a/nova/scheduler/weights/image_props.py b/nova/scheduler/weights/image_props.py index c28e908576..12284080b0 100644 --- a/nova/scheduler/weights/image_props.py +++ b/nova/scheduler/weights/image_props.py @@ -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: diff --git a/nova/tests/functional/regressions/test_bug_2125935.py b/nova/tests/functional/regressions/test_bug_2125935.py index 7308e75a36..45810c0e23 100644 --- a/nova/tests/functional/regressions/test_bug_2125935.py +++ b/nova/tests/functional/regressions/test_bug_2125935.py @@ -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']) diff --git a/nova/tests/unit/objects/test_instance.py b/nova/tests/unit/objects/test_instance.py index 0e041ecd37..b97964c222 100644 --- a/nova/tests/unit/objects/test_instance.py +++ b/nova/tests/unit/objects/test_instance.py @@ -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, diff --git a/nova/tests/unit/scheduler/weights/test_weights_image_props.py b/nova/tests/unit/scheduler/weights/test_weights_image_props.py index 107b074468..f9c789494c 100644 --- a/nova/tests/unit/scheduler/weights/test_weights_image_props.py +++ b/nova/tests/unit/scheduler/weights/test_weights_image_props.py @@ -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) diff --git a/releasenotes/notes/bug-2125935-de97147d5ff960b5.yaml b/releasenotes/notes/bug-2125935-de97147d5ff960b5.yaml new file mode 100644 index 0000000000..e07482f531 --- /dev/null +++ b/releasenotes/notes/bug-2125935-de97147d5ff960b5.yaml @@ -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