Merge "Fix fill_metadata usage for the ImagePropertiesWeigher"

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

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):

nova/scheduler/weights/image_props.py

@@ -74,9 +74,17 @@ 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.
+        # 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:

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
+        # now the weigher sees that host1 has an instance with the same image
-        # instances on both the hosts with the same image props, while we are
+        # props
-        # sure that the values should be 1.0 for both hosts.
-        # FIXME(sbauza): That's due to bug/2125935
         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
+        # eventually, the weigher sees the two existing instances on host1
-        # 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.
         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'])

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
+        # Other instances should have system_metadata loaded directly from the
-        self.assertIn('system_metadata', insts[0])
+        # DB
-        self.assertEqual({}, insts[0].system_metadata)
+        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,

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)

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