Use instance_info.display_name for existing hostname matching

When profile matching is used on a nova based undercloud and the node
names match the role hostname convention then the resulting exported
instances can reveal mixed name/hostname:

  instances:
  - hostname: controller-0
    name: controller-2
  - hostname: controller-1
    name: controller-1
  - hostname: controller-2
    name: controller-0

In this case, check_existing incorrectly uses the node name when
matching the hostname, when the actual hostname is in
instance_info.display_name.

This change fixes this by first matching hostname to existing nodes
instance_info.display_name, falling back to node name if there is no
match.

Since there is no way to lookup nodes by instance_info.display_name,
all nodes are queried to build a display_name->uuid dictionary.

Change-Id: I0674b9f3e4f8d5075cfd1aa9df9d43a2b3c99c8a
Story: 2010018
Task: 45211
rhbz: #2079935

tripleo_ansible/ansible_plugins/module_utils/baremetal_deploy.py

@@ -14,6 +14,7 @@
 # License for the specific language governing permissions and limitations
 # under the License.
 
+import collections
 from copy import deepcopy as dcopy
 import os
 
@@ -418,9 +419,39 @@ def check_existing(instances, provisioner, baremetal):
     not_found = []
     found = []
     unmanaged = []
+    existing_by_hostname = collections.defaultdict(list)
+    existing_by_name = collections.defaultdict(list)
+    if baremetal:
+        for node in baremetal.nodes(associated=True, fields=['uuid', 'name', 'instance_info']):
+            existing_by_name[node.name].append(node.uuid)
+            display_name = node.instance_info.get('display_name')
+            if display_name:
+                existing_by_hostname[display_name].append(node.uuid)
+
     for request in instances:
 
-        ident = request.get('name', request['hostname'])
+        hostname = request['hostname']
+        name = request.get('name')
+        hostname_matches = len(existing_by_hostname[hostname])
+        name_matches = name and len(existing_by_name[name]) or 0
+        if hostname_matches == 0:
+            if name_matches == 0:
+                # try the hostname, there may be an allocation with that name
+                ident = hostname
+            elif name_matches == 1:
+                ident = name
+            else:
+                message = ('There is more than one existing node with name=%s. '
+                           'Replace this name with a specific node uuid to match the desired instance.'
+                           % hostname)
+                raise BaremetalDeployException(message)
+        elif hostname_matches == 1:
+            ident = existing_by_hostname[hostname][0]
+        else:
+            message = ('There is more than one existing instance with instance_info.display_name=%s. '
+                       'Replace this hostname with a specific node uuid to match the desired instance.'
+                       % hostname)
+            raise BaremetalDeployException(message)
 
         if not request.get('managed', True):
             unmanaged.append(request)

tripleo_ansible/tests/plugins/module_utils/test_baremetal_deploy.py

@@ -1040,6 +1040,9 @@ class TestCheckExistingInstances(base.TestCase):
     def test_success(self):
         pr = mock.Mock()
         baremetal = mock.Mock()
+        baremetal.nodes.return_value = [mock.MagicMock(
+            uuid='aaaa', instance_info={'display_name': 'host2'})]
+
         instances = [
             {'hostname': 'host1',
              'image': {'href': 'overcloud-full'}},
@@ -1049,8 +1052,43 @@ class TestCheckExistingInstances(base.TestCase):
              'capabilities': {'answer': '42'},
              'image': {'href': 'overcloud-full'}}
         ]
-        existing = mock.MagicMock(hostname='host2', allocation=None)
-        existing.uuid = 'aaaa'
+        existing = mock.MagicMock(uuid='aaaa', hostname='host2', allocation=None)
+        pr.show_instance.side_effect = [
+            sdk_exc.ResourceNotFound(""),
+            metalsmith.exceptions.Error(""),
+            existing,
+        ]
+        found, not_found, unmanaged = bd.check_existing(instances, pr,
+                                                        baremetal)
+
+        self.assertEqual([existing], found)
+        self.assertEqual([{
+            'hostname': 'host1',
+            'image': {'href': 'overcloud-full'},
+        }, {
+            'hostname': 'host3',
+            'image': {'href': 'overcloud-full'},
+        }], not_found)
+        pr.show_instance.assert_has_calls([
+            mock.call(host) for host in ['host1', 'host3', 'aaaa']
+        ])
+
+    def test_match_name_only(self):
+        pr = mock.Mock()
+        baremetal = mock.Mock()
+        baremetal.nodes.return_value = [mock.MagicMock(
+            uuid='aaaa', instance_info={})]
+
+        instances = [
+            {'hostname': 'host1',
+             'image': {'href': 'overcloud-full'}},
+            {'hostname': 'host3',
+             'image': {'href': 'overcloud-full'}},
+            {'hostname': 'host2', 'resource_class': 'compute',
+             'capabilities': {'answer': '42'},
+             'image': {'href': 'overcloud-full'}}
+        ]
+        existing = mock.MagicMock(uuid='aaaa', hostname='host2', allocation=None)
         pr.show_instance.side_effect = [
             sdk_exc.ResourceNotFound(""),
             metalsmith.exceptions.Error(""),
@@ -1071,9 +1109,86 @@ class TestCheckExistingInstances(base.TestCase):
             mock.call(host) for host in ['host1', 'host3', 'host2']
         ])
 
+    def test_duplicate_display_names(self):
+        pr = mock.Mock()
+        baremetal = mock.Mock()
+        baremetal.nodes.return_value = [
+            mock.MagicMock(uuid='aaaa', instance_info={'display_name': 'host1'}),
+            mock.MagicMock(uuid='bbbb', instance_info={'display_name': 'host1'}),
+            mock.MagicMock(uuid='cccc', instance_info={'display_name': 'host1'})
+        ]
+        instances = [
+            {'hostname': 'host1',
+             'image': {'href': 'overcloud-full'}},
+        ]
+        exc = self.assertRaises(
+            bd.BaremetalDeployException, bd.check_existing,
+            instances, pr, baremetal)
+
+        self.assertIn("more than one existing instance", str(exc))
+        pr.show_instance.assert_not_called()
+
+    def test_duplicate_names(self):
+        pr = mock.Mock()
+        baremetal = mock.Mock()
+        nodes = [
+            mock.MagicMock(uuid='aaaa', instance_info={'display_name': 'host1'}),
+            mock.MagicMock(uuid='bbbb', instance_info={'display_name': 'host2'}),
+            mock.MagicMock(uuid='cccc', instance_info={'display_name': 'host3'})
+        ]
+        nodes[0].name = 'node1'
+        nodes[1].name = 'node1'
+        nodes[2].name = 'node1'
+        baremetal.nodes.return_value = nodes
+        instances = [
+            {'hostname': 'host4',
+             'name': 'node1',
+             'image': {'href': 'overcloud-full'}},
+        ]
+        exc = self.assertRaises(
+            bd.BaremetalDeployException, bd.check_existing,
+            instances, pr, baremetal)
+
+        self.assertIn("more than one existing node", str(exc))
+        pr.show_instance.assert_not_called()
+
+    def test_name_hostname_swapped(self):
+        pr = mock.Mock()
+        baremetal = mock.Mock()
+        baremetal.nodes.return_value = [
+            mock.MagicMock(uuid='aaaa', instance_info={'display_name': 'host3'}),
+            mock.MagicMock(uuid='bbbb', instance_info={'display_name': 'host2'}),
+            mock.MagicMock(uuid='cccc', instance_info={'display_name': 'host1'})
+        ]
+
+        instances = [
+            {'hostname': 'host3', 'name': 'host1',
+             'image': {'href': 'overcloud-full'}},
+            {'hostname': 'host2', 'name': 'host2',
+             'image': {'href': 'overcloud-full'}},
+            {'hostname': 'host1', 'name': 'host3',
+             'image': {'href': 'overcloud-full'}},
+        ]
+        existing = [
+            mock.MagicMock(uuid='aaaa', hostname='host3', allocation=None),
+            mock.MagicMock(uuid='aaaa', hostname='host2', allocation=None),
+            mock.MagicMock(uuid='aaaa', hostname='host1', allocation=None),
+        ]
+        pr.show_instance.side_effect = existing
+        found, not_found, unmanaged = bd.check_existing(instances, pr,
+                                                        baremetal)
+
+        self.assertEqual(existing, found)
+        self.assertEqual([], not_found)
+        pr.show_instance.assert_has_calls([
+            mock.call(host) for host in ['aaaa', 'bbbb', 'cccc']
+        ])
+
     def test_existing_no_allocation(self):
         pr = mock.Mock()
         baremetal = mock.Mock()
+        baremetal.nodes.return_value = [mock.MagicMock(
+            uuid='aaaa', name="server2", instance_info={'display_name': 'host2'})]
         instances = [
             {'name': 'server2', 'resource_class': 'compute',
              'hostname': 'host2',
@@ -1081,9 +1196,8 @@ class TestCheckExistingInstances(base.TestCase):
              'image': {'href': 'overcloud-full'}}
         ]
         existing = mock.MagicMock(
-            hostname='host2', allocation=None,
+            uuid='aaaa', hostname='host2', allocation=None,
             state=metalsmith.InstanceState.ACTIVE)
-        existing.uuid = 'aaaa'
         pr.show_instance.return_value = existing
         baremetal.get_allocation.side_effect = sdk_exc.ResourceNotFound
 
@@ -1094,11 +1208,13 @@ class TestCheckExistingInstances(base.TestCase):
 
         self.assertEqual([], not_found)
         self.assertEqual([existing], found)
-        pr.show_instance.assert_has_calls([mock.call('server2'),
-                                           mock.call(existing.uuid)])
+        pr.show_instance.assert_has_calls([mock.call('aaaa'),
+                                           mock.call('aaaa')])
 
     def test_hostname_mismatch(self):
         pr = mock.Mock()
+        baremetal = mock.Mock()
+        baremetal.nodes.return_value = []
         instances = [
             {'hostname': 'host1',
              'image': {'href': 'overcloud-full'}},
@@ -1106,7 +1222,7 @@ class TestCheckExistingInstances(base.TestCase):
         pr.show_instance.return_value.hostname = 'host2'
         exc = self.assertRaises(
             bd.BaremetalDeployException, bd.check_existing,
-            instances, pr, mock.Mock())
+            instances, pr, baremetal)
 
         self.assertIn("hostname host1 was not found", str(exc))
         pr.show_instance.assert_called_once_with('host1')
@@ -1114,15 +1230,16 @@ class TestCheckExistingInstances(base.TestCase):
     def test_hostname_mismatch_but_instance_info_display_name_correct(self):
         pr = mock.Mock()
         baremetal = mock.Mock()
+        baremetal.nodes.return_value = [mock.MagicMock(
+            uuid='aaaa', instance_info={'display_name': 'correct_hostname'})]
         instances = [
             {'name': 'bm_node1', 'resource_class': 'baremetal',
              'hostname': 'correct_hostname',
              'image': {'href': 'overcloud-full'}},
         ]
         existing = mock.MagicMock(
-            name='bm_node1', hostname='wrong_hostname', allocation=None,
+            uuid='aaaa', name='bm_node1', hostname='wrong_hostname', allocation=None,
             state=metalsmith.InstanceState.ACTIVE)
-        existing.uuid = 'aaaa'
         pr.show_instance.return_value = existing
         baremetal.get_node.return_value.instance_info = {
             'display_name': 'correct_hostname'}
@@ -1138,21 +1255,24 @@ class TestCheckExistingInstances(base.TestCase):
         self.assertEqual([], not_found)
         self.assertEqual([existing], found)
         self.assertEqual(2, pr.show_instance.call_count)
-        pr.show_instance.assert_has_calls([mock.call('bm_node1'),
-                                           mock.call(existing.uuid)])
+        pr.show_instance.assert_has_calls([mock.call('aaaa'),
+                                           mock.call('aaaa')])
 
     def test_hostname_mismatch_and_instance_info_display_name_mismatch(self):
         pr = mock.Mock()
         baremetal = mock.Mock()
+        nodes = [mock.MagicMock(
+            uuid='aaaa', instance_info={'display_name': 'mismatching_hostname'})]
+        baremetal.nodes.return_value = nodes
+        nodes[0].name = 'bm_node1'
         instances = [
             {'name': 'bm_node1', 'resource_class': 'baremetal',
              'hostname': 'correct_hostname',
              'image': {'href': 'overcloud-full'}},
         ]
         existing = mock.MagicMock(
-            name='bm_node1', hostname='wrong_hostname', allocation=None,
+            uuid='aaaa', name='bm_node1', hostname='wrong_hostname', allocation=None,
             state=metalsmith.InstanceState.ACTIVE)
-        existing.uuid = 'aaaa'
         pr.show_instance.return_value = existing
         baremetal.get_node.return_value.instance_info = {
             'display_name': 'mismatching_hostname'}
@@ -1178,6 +1298,8 @@ class TestCheckExistingInstances(base.TestCase):
 
     def test_unexpected_error(self):
         pr = mock.Mock()
+        baremetal = mock.Mock()
+        baremetal.nodes.return_value = []
         instances = [
             {'image': {'href': 'overcloud-full'},
              'hostname': 'host%d' % i} for i in range(3)
@@ -1185,7 +1307,7 @@ class TestCheckExistingInstances(base.TestCase):
         pr.show_instance.side_effect = RuntimeError('boom')
         exc = self.assertRaises(
             bd.BaremetalDeployException, bd.check_existing,
-            instances, pr, mock.Mock())
+            instances, pr, baremetal)
 
         self.assertIn("for host0", str(exc))
         self.assertIn("RuntimeError: boom", str(exc))