Do a precise node lookup for unprovision

A hostname or name is passed to the unprovision call, and the node is
looked up by allocation name (hostname), falling back to node name.

This can result in the wrong node being unprovisioned in the following
scenario:
* node and hostname naming scheme are identical
* node and hostname names are mixed because allocation placement
  doesn't take the node name into account
* a node has already been unprovisioned by hostname (allocation)
  lookup
* unprovision is performed a second time, this time matching the
  hostname with a different node's name
* the wrong node is unprovisioned

The unprovision_node method can also take a node object instead of a
string identifier, so this change does a precise node lookup before
calling it. This means only hostnames are used for allocation lookups,
and node names for node lookups.

Change-Id: I9507f8d30c871ae62a250148789393695d59183a
Resolves: rhbz#2092444

metalsmith/test/test_metalsmith_instances.py

@@ -18,6 +18,7 @@ from unittest import mock
 
 from metalsmith_ansible.ansible_plugins.modules \
     import metalsmith_instances as mi
+from openstack import exceptions as os_exc
 
 from metalsmith import exceptions as exc
 
@@ -265,7 +266,17 @@ class TestMetalsmithInstances(unittest.TestCase):
     @mock.patch('metalsmith.instance_config.CloudInitConfig', autospec=True)
     def test_unprovision(self, mock_config, mock_detect):
 
-        provisioner = mock.Mock()
+        mock_node1 = mock.Mock(name='node-1')
+        mock_node2 = mock.Mock(name='node-2')
+        mock_allocation1 = mock.Mock(name='overcloud-controller-1',
+                                     node_id='aaaa')
+        connection = mock.Mock()
+        provisioner = mock.Mock(connection=connection)
+
+        connection.baremetal.get_allocation.side_effect = [
+            mock_allocation1, os_exc.ResourceNotFound()]
+        connection.baremetal.get_node.side_effect = [
+            mock_node1, mock_node2, os_exc.ResourceNotFound()]
         instances = [{
             'name': 'node-1',
             'hostname': 'overcloud-controller-1',
@@ -275,9 +286,23 @@ class TestMetalsmithInstances(unittest.TestCase):
             'name': 'node-2',
             'image': {'href': 'overcloud-full'},
             'state': 'absent'
+        }, {
+            'name': 'node-3',
+            'hostname': 'overcloud-controller-3',
+            'image': {'href': 'overcloud-full'},
+            'state': 'absent'
         }]
         self.assertTrue(mi.unprovision(provisioner, instances))
         provisioner.unprovision_node.assert_has_calls([
-            mock.call('overcloud-controller-1'),
-            mock.call('node-2')
+            mock.call(mock_node1),
+            mock.call(mock_node2)
+        ])
+        connection.baremetal.get_allocation.assert_has_calls([
+            mock.call('overcloud-controller-1'),
+            mock.call('overcloud-controller-3')
+        ])
+        connection.baremetal.get_node.assert_has_calls([
+            mock.call('aaaa'),
+            mock.call('node-2'),
+            mock.call('node-3')
         ])

metalsmith_ansible/ansible_plugins/modules/metalsmith_instances.py

@@ -30,6 +30,7 @@ except ImportError:
 import metalsmith
 from metalsmith import instance_config
 from metalsmith import sources
+from openstack import exceptions as os_exc
 
 import yaml
 
@@ -374,9 +375,30 @@ def _provision_instance(provisioner, instance, nodes, timeout, wait):
 
 
 def unprovision(provisioner, instances):
+    connection = provisioner.connection
     for instance in instances:
-        provisioner.unprovision_node(instance.get('hostname',
-                                                  instance.get('name')))
+        hostname = instance.get('hostname')
+        node = None
+        if hostname:
+            try:
+                allocation = connection.baremetal.get_allocation(hostname)
+                node = connection.baremetal.get_node(allocation.node_id)
+            except os_exc.ResourceNotFound:
+                # Allocation for this hostname doesn't exist, so attempt
+                # to lookup by node name
+                pass
+
+        name = instance.get('name')
+        if not node and name:
+            try:
+                node = connection.baremetal.get_node(name)
+            except os_exc.ResourceNotFound:
+                # Node with this name doesn't exist, so there is no
+                # node to unprovision
+                pass
+
+        if node:
+            provisioner.unprovision_node(node)
     return True