From 374fd2791fcd76653d517ee354bb33a164b86c9a Mon Sep 17 00:00:00 2001
From: Matt Riedemann <mriedem.os@gmail.com>
Date: Wed, 29 May 2019 16:35:04 -0400
Subject: [PATCH] Optimize NovaHelper.get_compute_node_by_hostname

The get_compute_node_by_hostname method is given a
compute service hostname and then does two queries to
find the matching hypervisor (compute node) with details:

1. List hypervisors with details and find the one that
   matches the given compute service hostname.

2. Using that node, search for hypervisors with the
   matching hypervisor_hostname.

There are two issues here:

1. The first query is inefficient in that it has to list
   all hypervisors in the deployment to try and match the
   one with the compute service hostname client side.

2. The second query is a fuzzy match on the server side [1]
   so even though we have matched on the node we want,
   get_compute_node_by_name can still return more than
   one hypervisor which will result in the helper method
   raising ComputeNodeNotFound. Consider having compute
   hosts with names compute1, compute10, compute11, compute100,
   and so on. The fuzzy match on compute1 would return all of
   those hypervisors.

For non-ironic nodes in nova, the compute service host and
hypervisor should be 1:1, meaning the hypervisor.service['host']
should be the same as hypervisor.hypervisor_hostname. Knowing
this, we can simplify the code to search just on the given
compute service hostname and if we get more than one result, it
is because of the fuzzy match and we can then do our client-side
filtering on the compute service hostname.

[1] https://github.com/openstack/nova/blob/d4f58f5eb/nova/db/sqlalchemy/api.py#L676

Change-Id: I84f387982f665d7cc11bffe8ec390cc7e7ed5278
---
 watcher/common/nova_helper.py            | 32 ++++++++++++------------
 watcher/tests/common/test_nova_helper.py | 22 +++++++++++++---
 2 files changed, 35 insertions(+), 19 deletions(-)

diff --git a/watcher/common/nova_helper.py b/watcher/common/nova_helper.py
index 7d9800421..0bc66e47a 100644
--- a/watcher/common/nova_helper.py
+++ b/watcher/common/nova_helper.py
@@ -61,23 +61,23 @@ class NovaHelper(object):
                                             detailed=detailed)
 
     def get_compute_node_by_hostname(self, node_hostname):
-        """Get compute node by hostname"""
-        # TODO(mriedem): This method could be optimized if
-        # GET /os-hypervisors/detail had a host filter parameter.
-        try:
-            hypervisors = [hv for hv in self.get_compute_node_list()
-                           if hv.service['host'] == node_hostname]
-            if len(hypervisors) != 1:
-                # TODO(hidekazu)
-                # this may occur if ironic driver is used
-                raise exception.ComputeNodeNotFound(name=node_hostname)
-            else:
-                compute_nodes = self.get_compute_node_by_name(
-                    hypervisors[0].hypervisor_hostname, detailed=True)
-                if len(compute_nodes) != 1:
-                    raise exception.ComputeNodeNotFound(name=node_hostname)
+        """Get compute node by hostname
 
-                return compute_nodes[0]
+        :param node_hostname: Compute service hostname
+        :returns: novaclient.v2.hypervisors.Hypervisor object if found
+        :raises: ComputeNodeNotFound if no hypervisor is found for the compute
+            service hostname or there was an error communicating with nova
+        """
+        try:
+            # This is a fuzzy match on hypervisor_hostname so we could get back
+            # more than one compute node. If so, match on the compute service
+            # hostname.
+            compute_nodes = self.get_compute_node_by_name(
+                node_hostname, detailed=True)
+            for cn in compute_nodes:
+                if cn.service['host'] == node_hostname:
+                    return cn
+            raise exception.ComputeNodeNotFound(name=node_hostname)
         except Exception as exc:
             LOG.exception(exc)
             raise exception.ComputeNodeNotFound(name=node_hostname)
diff --git a/watcher/tests/common/test_nova_helper.py b/watcher/tests/common/test_nova_helper.py
index d69fbdfe6..d15cd6d7e 100644
--- a/watcher/tests/common/test_nova_helper.py
+++ b/watcher/tests/common/test_nova_helper.py
@@ -121,9 +121,6 @@ class TestNovaHelper(base.TestCase):
         hypervisor_id = utils.generate_uuid()
         hypervisor_name = "fake_hypervisor_1"
         hypervisor = self.fake_hypervisor(hypervisor_id, hypervisor_name)
-        self.fake_nova_hypervisor_list(
-            nova_util,
-            fake_list=[hypervisor])
         nova_util.nova.hypervisors.search.return_value = [hypervisor]
         # verify that the compute node can be obtained normally by name
         self.assertEqual(
@@ -145,6 +142,25 @@ class TestNovaHelper(base.TestCase):
             nova_util.get_compute_node_by_hostname,
             hypervisor_name)
 
+    def test_get_compute_node_by_hostname_multiple_matches(self, *mocks):
+        # Tests a scenario where get_compute_node_by_name returns multiple
+        # hypervisors and we have to pick the exact match based on the given
+        # compute service hostname.
+        nova_util = nova_helper.NovaHelper()
+        nodes = []
+        # compute1 is a substring of compute10 to trigger the fuzzy match.
+        for hostname in ('compute1', 'compute10'):
+            node = mock.MagicMock()
+            node.id = utils.generate_uuid()
+            node.hypervisor_hostname = hostname
+            node.service = {'host': hostname}
+            nodes.append(node)
+        # We should get back exact matches based on the service host.
+        nova_util.nova.hypervisors.search.return_value = nodes
+        for index, name in enumerate(['compute1', 'compute10']):
+            result = nova_util.get_compute_node_by_hostname(name)
+            self.assertIs(nodes[index], result)
+
     def test_get_instance_list(self, *args):
         nova_util = nova_helper.NovaHelper()
         # Call it once with no filters.