From 1e8b17ac46a9052234c9f56da42a2a4bb8250216 Mon Sep 17 00:00:00 2001 From: chenke Date: Fri, 28 Jun 2019 14:43:57 +0800 Subject: [PATCH] Reduce the query time of the instances when call get_instance_list() The problem is that watcher is passing limit=-1 to novaclient when listing servers which will always make at least two API calls to be sure it's done paging: https://github.com/openstack/python-novaclient/blob/13.0.1/novaclient/v2/servers.py#L896 If we can determine before we list servers that there are only a certain number where the number of servers is less than 1000. For example: 4, we should just pass the limit=len(servers) to novaclient and avoid the second call for paging which takes extra time and yields no results. Change-Id: I797ad934a0f8496dbcbf65798e28b0443f238137 Closes-Bug: #1834679 --- watcher/common/nova_helper.py | 12 +++++++--- .../decision_engine/model/collector/nova.py | 12 +++++++++- .../decision_engine/cluster/test_nova_cdmc.py | 24 ++++++++++++++++++- 3 files changed, 43 insertions(+), 5 deletions(-) diff --git a/watcher/common/nova_helper.py b/watcher/common/nova_helper.py index 6c0a3c8f7..7f8665ccf 100644 --- a/watcher/common/nova_helper.py +++ b/watcher/common/nova_helper.py @@ -82,19 +82,25 @@ class NovaHelper(object): LOG.exception(exc) raise exception.ComputeNodeNotFound(name=node_hostname) - def get_instance_list(self, filters=None): + def get_instance_list(self, filters=None, limit=-1): """List servers for all tenants with details. This always gets servers with the all_tenants=True filter. - :param filters: dict of additional filters + :param filters: dict of additional filters (optional). + :param limit: Maximum number of servers to return (optional). + If limit == -1, all servers will be returned, + note that limit == -1 will have a performance + penalty. For details, please see: + https://bugs.launchpad.net/watcher/+bug/1834679 :returns: list of novaclient Server objects """ search_opts = {'all_tenants': True} if filters: search_opts.update(filters) + # TODO(chenker) Add marker param to list Server objects. return self.nova.servers.list(search_opts=search_opts, - limit=-1) + limit=limit) def get_instance_by_uuid(self, instance_uuid): return [instance for instance in diff --git a/watcher/decision_engine/model/collector/nova.py b/watcher/decision_engine/model/collector/nova.py index 750fb4266..c2cabcce0 100644 --- a/watcher/decision_engine/model/collector/nova.py +++ b/watcher/decision_engine/model/collector/nova.py @@ -324,8 +324,18 @@ class ModelBuilder(object): return host = node.service["host"] compute_node = self.model.get_node_by_uuid(host) + filters = {'host': host} + limit = len(instances) if len(instances) <= 1000 else -1 # Get all servers on this compute host. - instances = self.nova_helper.get_instance_list({'host': host}) + # Note that the advantage of passing the limit parameter is + # that it can speed up the call time of novaclient. 1000 is + # the default maximum number of return servers provided by + # compute API. If we need to request more than 1000 servers, + # we can set limit=-1. For details, please see: + # https://bugs.launchpad.net/watcher/+bug/1834679 + instances = self.nova_helper.get_instance_list( + filters=filters, + limit=limit) for inst in instances: # Add Node instance = self._build_instance_node(inst) diff --git a/watcher/tests/decision_engine/cluster/test_nova_cdmc.py b/watcher/tests/decision_engine/cluster/test_nova_cdmc.py index f4569bb59..a2b53fbce 100644 --- a/watcher/tests/decision_engine/cluster/test_nova_cdmc.py +++ b/watcher/tests/decision_engine/cluster/test_nova_cdmc.py @@ -114,4 +114,26 @@ class TestNovaClusterDataModelCollector(base.TestCase): m_nova_helper.get_compute_node_by_name.assert_called_once_with( minimal_node['hypervisor_hostname'], servers=True, detailed=True) m_nova_helper.get_instance_list.assert_called_once_with( - {'host': fake_compute_node.service['host']}) + filters={'host': fake_compute_node.service['host']}, limit=1) + + +class TestModelBuilder(base.TestCase): + + @mock.patch.object(nova_helper, 'NovaHelper', mock.MagicMock()) + def test_add_instance_node(self): + model_builder = nova.ModelBuilder(osc=mock.MagicMock()) + model_builder.model = mock.MagicMock() + mock_node = mock.MagicMock() + mock_host = mock_node.service["host"] + mock_instances = [mock.MagicMock()] + model_builder.add_instance_node(mock_node, mock_instances) + # verify that when len(instances) <= 1000, limit == len(instance). + model_builder.nova_helper.get_instance_list.assert_called_once_with( + filters={'host': mock_host}, limit=1) + + # verify that when len(instances) > 1000, limit == -1. + mock_instance = mock.Mock() + mock_instances = [mock_instance] * 1001 + model_builder.add_instance_node(mock_node, mock_instances) + model_builder.nova_helper.get_instance_list.assert_called_with( + filters={'host': mock_host}, limit=-1)