From 5459c30ed4eaea7983533e3a2ad3d207f5895dfe Mon Sep 17 00:00:00 2001
From: Steve Baker <sbaker@redhat.com>
Date: Mon, 20 Jun 2022 09:52:44 +1200
Subject: [PATCH] metalsmith_instances module, unprovision by hostname

The node lookup for unprovision first attempts the allocation name
(hostname)[1] but the metalsmith_instances unprovision passes the node
name.

This means in cases where the node naming scheme and the allocation
naming scheme are the same, the wrong node may be unprovisioned.

This change switches to passing the hostname for unprovision, only
passing the node name if the hostname is missing from the instances
entry.

[1] https://opendev.org/openstack/metalsmith/src/branch/master/metalsmith/_provisioner.py#L653

Change-Id: Ie6b989f8d67c03606be37310777175cfb8d9303e
Resolves: rhbz#2092444
---
 metalsmith/test/test_metalsmith_instances.py  | 21 +++++++++++++++++++
 .../modules/metalsmith_instances.py           |  3 ++-
 2 files changed, 23 insertions(+), 1 deletion(-)

diff --git a/metalsmith/test/test_metalsmith_instances.py b/metalsmith/test/test_metalsmith_instances.py
index 7a0578c..8616295 100644
--- a/metalsmith/test/test_metalsmith_instances.py
+++ b/metalsmith/test/test_metalsmith_instances.py
@@ -260,3 +260,24 @@ class TestMetalsmithInstances(unittest.TestCase):
             mock.call(1),
             mock.call(2)
         ])
+
+    @mock.patch('metalsmith.sources.detect', autospec=True)
+    @mock.patch('metalsmith.instance_config.CloudInitConfig', autospec=True)
+    def test_unprovision(self, mock_config, mock_detect):
+
+        provisioner = mock.Mock()
+        instances = [{
+            'name': 'node-1',
+            'hostname': 'overcloud-controller-1',
+            'image': {'href': 'overcloud-full'},
+            'state': 'absent'
+        }, {
+            'name': 'node-2',
+            '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')
+        ])
diff --git a/metalsmith_ansible/ansible_plugins/modules/metalsmith_instances.py b/metalsmith_ansible/ansible_plugins/modules/metalsmith_instances.py
index 4aad599..9268e65 100644
--- a/metalsmith_ansible/ansible_plugins/modules/metalsmith_instances.py
+++ b/metalsmith_ansible/ansible_plugins/modules/metalsmith_instances.py
@@ -375,7 +375,8 @@ def _provision_instance(provisioner, instance, nodes, timeout, wait):
 
 def unprovision(provisioner, instances):
     for instance in instances:
-        provisioner.unprovision_node(instance.get('name'))
+        provisioner.unprovision_node(instance.get('hostname',
+                                                  instance.get('name')))
     return True