From 13513e6232366a82a8a81c01f9ec8210f6da3a30 Mon Sep 17 00:00:00 2001 From: Roman Podoliaka Date: Wed, 27 Jul 2016 19:46:16 +0300 Subject: [PATCH] ironic_host_manager: fix population of instances info on start IronicHostManager currently overrides the _init_instance_info() method of the base class and unconditionally skips population of instances information for all compute nodes, even if they are not Ironic ones. If there are compute nodes with the hypervisor_type different from Ironic in the same cloud. the instances info will be missing in nova-scheduler (if IronicHostManager is configured as a host manager impl in nova.conf), which will effectively break instance affinity filters like DifferentHostFilter or SameHostFilter, that check set intersections of instances running on a particular host and the ones passed as a hint for nova-scheduler in a boot request. IronicHostManager should use the method implementation of the base class for non-ironic compute nodes. Ib1ddb44d71f7b085512c1f3fc0544f7b00c754fe fixed the problem with scheduling, this change is needed to make sure we also populate the instances info on start of nova-scheduler. Closes-Bug: #1606496 Co-Authored-By: Timofei Durakov (cherry-picked from cc64a45d98d7576a78a853cc3da8109c31f4b75d) Change-Id: I9d8d2dc99773df4097c178d924d182a0d1971bcc --- nova/scheduler/host_manager.py | 21 ++++++---- nova/scheduler/ironic_host_manager.py | 12 +++++- .../tests/unit/scheduler/test_host_manager.py | 24 ++++++++++++ .../scheduler/test_ironic_host_manager.py | 39 +++++++++++++++++-- nova/tests/unit/scheduler/test_scheduler.py | 6 ++- 5 files changed, 87 insertions(+), 15 deletions(-) diff --git a/nova/scheduler/host_manager.py b/nova/scheduler/host_manager.py index 4623035b5286..ab0ccf032cec 100644 --- a/nova/scheduler/host_manager.py +++ b/nova/scheduler/host_manager.py @@ -389,20 +389,27 @@ class HostManager(object): if aggregate.id in self.host_aggregates_map[host]: self.host_aggregates_map[host].remove(aggregate.id) - def _init_instance_info(self): + def _init_instance_info(self, compute_nodes=None): """Creates the initial view of instances for all hosts. As this initial population of instance information may take some time, we don't wish to block the scheduler's startup while this completes. The async method allows us to simply mock out the _init_instance_info() method in tests. + + :param compute_nodes: a list of nodes to populate instances info for + if is None, compute_nodes will be looked up in database """ - def _async_init_instance_info(): - context = context_module.get_admin_context() + def _async_init_instance_info(compute_nodes): + context = context_module.RequestContext() LOG.debug("START:_async_init_instance_info") self._instance_info = {} - compute_nodes = objects.ComputeNodeList.get_all(context).objects + + if not compute_nodes: + compute_nodes = objects.ComputeNodeList.get_all( + context).objects + LOG.debug("Total number of compute nodes: %s", len(compute_nodes)) # Break the queries into batches of 10 to reduce the total number # of calls to the DB. @@ -416,8 +423,8 @@ class HostManager(object): filters = {"host": [curr_node.host for curr_node in curr_nodes], "deleted": False} - result = objects.InstanceList.get_by_filters(context, - filters) + result = objects.InstanceList.get_by_filters( + context.elevated(), filters) instances = result.objects LOG.debug("Adding %s instances for hosts %s-%s", len(instances), start_node, end_node) @@ -433,7 +440,7 @@ class HostManager(object): LOG.debug("END:_async_init_instance_info") # Run this async so that we don't block the scheduler start-up - utils.spawn_n(_async_init_instance_info) + utils.spawn_n(_async_init_instance_info, compute_nodes) def _choose_host_filters(self, filter_cls_names): """Since the caller may specify which filters to use we need diff --git a/nova/scheduler/ironic_host_manager.py b/nova/scheduler/ironic_host_manager.py index 969aaa63cf6a..1228bbcd73e5 100644 --- a/nova/scheduler/ironic_host_manager.py +++ b/nova/scheduler/ironic_host_manager.py @@ -23,6 +23,8 @@ subdivided into multiple instances. """ from nova.compute import hv_type import nova.conf +from nova import context as context_module +from nova import objects from nova.scheduler import host_manager CONF = nova.conf.CONF @@ -91,9 +93,15 @@ class IronicHostManager(host_manager.HostManager): else: return host_manager.HostState(host, node) - def _init_instance_info(self): + def _init_instance_info(self, compute_nodes=None): """Ironic hosts should not pass instance info.""" - pass + context = context_module.RequestContext() + if not compute_nodes: + compute_nodes = objects.ComputeNodeList.get_all(context).objects + + non_ironic_computes = [c for c in compute_nodes + if not self._is_ironic_compute(c)] + super(IronicHostManager, self)._init_instance_info(non_ironic_computes) def _get_instance_info(self, context, compute): """Ironic hosts should not pass instance info.""" diff --git a/nova/tests/unit/scheduler/test_host_manager.py b/nova/tests/unit/scheduler/test_host_manager.py index a043d8e08151..d6e7b13898fc 100644 --- a/nova/tests/unit/scheduler/test_host_manager.py +++ b/nova/tests/unit/scheduler/test_host_manager.py @@ -114,6 +114,30 @@ class HostManagerTestCase(test.NoDBTestCase): exp_filters = {'deleted': False, 'host': [u'host1', u'host2']} mock_get_by_filters.assert_called_once_with(mock.ANY, exp_filters) + @mock.patch.object(nova.objects.InstanceList, 'get_by_filters') + @mock.patch.object(nova.objects.ComputeNodeList, 'get_all') + def test_init_instance_info_compute_nodes(self, mock_get_all, + mock_get_by_filters): + cn1 = objects.ComputeNode(host='host1') + cn2 = objects.ComputeNode(host='host2') + inst1 = objects.Instance(host='host1', uuid=uuids.instance_1) + inst2 = objects.Instance(host='host1', uuid=uuids.instance_2) + inst3 = objects.Instance(host='host2', uuid=uuids.instance_3) + mock_get_by_filters.return_value = objects.InstanceList( + objects=[inst1, inst2, inst3]) + hm = self.host_manager + hm._instance_info = {} + hm._init_instance_info([cn1, cn2]) + self.assertEqual(len(hm._instance_info), 2) + fake_info = hm._instance_info['host1'] + self.assertIn(uuids.instance_1, fake_info['instances']) + self.assertIn(uuids.instance_2, fake_info['instances']) + self.assertNotIn(uuids.instance_3, fake_info['instances']) + exp_filters = {'deleted': False, 'host': [u'host1', u'host2']} + mock_get_by_filters.assert_called_once_with(mock.ANY, exp_filters) + # should not be called if the list of nodes was passed explicitly + self.assertFalse(mock_get_all.called) + def test_default_filters(self): default_filters = self.host_manager.default_filters self.assertEqual(1, len(default_filters)) diff --git a/nova/tests/unit/scheduler/test_ironic_host_manager.py b/nova/tests/unit/scheduler/test_ironic_host_manager.py index a5b868ebd42e..3f5c375324ae 100644 --- a/nova/tests/unit/scheduler/test_ironic_host_manager.py +++ b/nova/tests/unit/scheduler/test_ironic_host_manager.py @@ -43,7 +43,8 @@ class FakeFilterClass2(filters.BaseHostFilter): class IronicHostManagerTestCase(test.NoDBTestCase): """Test case for IronicHostManager class.""" - @mock.patch.object(host_manager.HostManager, '_init_instance_info') + @mock.patch.object(ironic_host_manager.IronicHostManager, + '_init_instance_info') @mock.patch.object(host_manager.HostManager, '_init_aggregates') def setUp(self, mock_init_agg, mock_init_inst): super(IronicHostManagerTestCase, self).setUp() @@ -127,11 +128,39 @@ class IronicHostManagerTestCase(test.NoDBTestCase): # we return exactly what the base class implementation returned self.assertIs(expected_rv, rv) + @mock.patch.object(host_manager.HostManager, '_init_instance_info') + @mock.patch.object(objects.ComputeNodeList, 'get_all') + def test_init_instance_info(self, mock_get_all, + mock_base_init_instance_info): + cn1 = objects.ComputeNode(**{'hypervisor_type': 'ironic'}) + cn2 = objects.ComputeNode(**{'hypervisor_type': 'qemu'}) + cn3 = objects.ComputeNode(**{'hypervisor_type': 'qemu'}) + mock_get_all.return_value.objects = [cn1, cn2, cn3] + + self.host_manager._init_instance_info() + # ensure we filter out ironic nodes before calling the base class impl + mock_base_init_instance_info.assert_called_once_with([cn2, cn3]) + + @mock.patch.object(host_manager.HostManager, '_init_instance_info') + @mock.patch.object(objects.ComputeNodeList, 'get_all') + def test_init_instance_info_compute_nodes(self, mock_get_all, + mock_base_init_instance_info): + cn1 = objects.ComputeNode(**{'hypervisor_type': 'ironic'}) + cn2 = objects.ComputeNode(**{'hypervisor_type': 'qemu'}) + + self.host_manager._init_instance_info(compute_nodes=[cn1, cn2]) + + # check we don't try to get nodes list if it was passed explicitly + self.assertFalse(mock_get_all.called) + # ensure we filter out ironic nodes before calling the base class impl + mock_base_init_instance_info.assert_called_once_with([cn2]) + class IronicHostManagerChangedNodesTestCase(test.NoDBTestCase): """Test case for IronicHostManager class.""" - @mock.patch.object(host_manager.HostManager, '_init_instance_info') + @mock.patch.object(ironic_host_manager.IronicHostManager, + '_init_instance_info') @mock.patch.object(host_manager.HostManager, '_init_aggregates') def setUp(self, mock_init_agg, mock_init_inst): super(IronicHostManagerChangedNodesTestCase, self).setUp() @@ -277,7 +306,8 @@ class IronicHostManagerChangedNodesTestCase(test.NoDBTestCase): class IronicHostManagerTestFilters(test.NoDBTestCase): """Test filters work for IronicHostManager.""" - @mock.patch.object(host_manager.HostManager, '_init_instance_info') + @mock.patch.object(ironic_host_manager.IronicHostManager, + '_init_instance_info') @mock.patch.object(host_manager.HostManager, '_init_aggregates') def setUp(self, mock_init_agg, mock_init_inst): super(IronicHostManagerTestFilters, self).setUp() @@ -314,7 +344,8 @@ class IronicHostManagerTestFilters(test.NoDBTestCase): self.assertEqual(1, len(default_filters)) self.assertIsInstance(default_filters[0], FakeFilterClass1) - @mock.patch.object(host_manager.HostManager, '_init_instance_info') + @mock.patch.object(ironic_host_manager.IronicHostManager, + '_init_instance_info') @mock.patch.object(host_manager.HostManager, '_init_aggregates') def test_host_manager_default_filters_uses_baremetal(self, mock_init_agg, mock_init_inst): diff --git a/nova/tests/unit/scheduler/test_scheduler.py b/nova/tests/unit/scheduler/test_scheduler.py index cbf7182ee38e..9c2406e691b1 100644 --- a/nova/tests/unit/scheduler/test_scheduler.py +++ b/nova/tests/unit/scheduler/test_scheduler.py @@ -191,7 +191,8 @@ class SchedulerInitTestCase(test.NoDBTestCase): manager = self.driver_cls().host_manager self.assertIsInstance(manager, host_manager.HostManager) - @mock.patch.object(host_manager.HostManager, '_init_instance_info') + @mock.patch.object(ironic_host_manager.IronicHostManager, + '_init_instance_info') @mock.patch.object(host_manager.HostManager, '_init_aggregates') def test_init_using_ironic_hostmanager(self, mock_init_agg, @@ -211,7 +212,8 @@ class SchedulerInitTestCase(test.NoDBTestCase): # NOTE(Yingxin): Loading full class path is deprecated and should be # removed in the N release. @mock.patch.object(driver.LOG, 'warning') - @mock.patch.object(host_manager.HostManager, '_init_instance_info') + @mock.patch.object(ironic_host_manager.IronicHostManager, + '_init_instance_info') @mock.patch.object(host_manager.HostManager, '_init_aggregates') def test_init_using_classpath_to_hostmanager(self, mock_init_agg,