From ae3a14e65c81f48472040a086075b63ca1ce76ac Mon Sep 17 00:00:00 2001
From: Julia Kreger <juliaashleykreger@gmail.com>
Date: Fri, 6 Mar 2020 12:25:33 -0800
Subject: [PATCH] Make reservation checks caseless

If a conductor hostname is changed while reservations are
issued to a conductor with one hostname, such as 'hostName'
and then the process is restarted with 'hostname', then the
queries would not match the node and effectively the nodes
would become inaccessible until the reservation is cleared.

This patch changes the queries to use a case insenitive
like search, better known as ilike.

Special thanks to the nova team for finding a bug in the hash
ring calculation in the ironic virt driver where a customer
changed their nova-compute hostname, which inspired this
patch.

Change-Id: Ib7da925ba5ca6a82ba542e0f4ae4cf7f0d070835
---
 ironic/db/sqlalchemy/api.py                              | 9 +++++----
 ironic/tests/unit/db/test_conductor.py                   | 8 ++++++++
 ...aseless-conductor-restart-check-f70005fbf65f6bb6.yaml | 5 +++++
 3 files changed, 18 insertions(+), 4 deletions(-)
 create mode 100644 releasenotes/notes/caseless-conductor-restart-check-f70005fbf65f6bb6.yaml

diff --git a/ironic/db/sqlalchemy/api.py b/ironic/db/sqlalchemy/api.py
index f072856bbc..95baf75fde 100644
--- a/ironic/db/sqlalchemy/api.py
+++ b/ironic/db/sqlalchemy/api.py
@@ -999,9 +999,9 @@ class Connection(api.Connection):
         nodes = []
         with _session_for_write():
             query = (model_query(models.Node)
-                     .filter_by(reservation=hostname))
+                     .filter(models.Node.reservation.ilike(hostname)))
             nodes = [node['uuid'] for node in query]
-            query.update({'reservation': None})
+            query.update({'reservation': None}, synchronize_session=False)
 
         if nodes:
             nodes = ', '.join(nodes)
@@ -1014,13 +1014,14 @@ class Connection(api.Connection):
         nodes = []
         with _session_for_write():
             query = (model_query(models.Node)
-                     .filter_by(reservation=hostname))
+                     .filter(models.Node.reservation.ilike(hostname)))
             query = query.filter(models.Node.target_power_state != sql.null())
             nodes = [node['uuid'] for node in query]
             query.update({'target_power_state': None,
                           'last_error': _("Pending power operation was "
                                           "aborted due to conductor "
-                                          "restart")})
+                                          "restart")},
+                         synchronize_session=False)
 
         if nodes:
             nodes = ', '.join(nodes)
diff --git a/ironic/tests/unit/db/test_conductor.py b/ironic/tests/unit/db/test_conductor.py
index ebde348172..1ae38de88b 100644
--- a/ironic/tests/unit/db/test_conductor.py
+++ b/ironic/tests/unit/db/test_conductor.py
@@ -179,13 +179,16 @@ class DbConductorTestCase(base.DbTestCase):
         node1 = self.dbapi.create_node({'reservation': 'hostname1'})
         node2 = self.dbapi.create_node({'reservation': 'hostname2'})
         node3 = self.dbapi.create_node({'reservation': None})
+        node4 = self.dbapi.create_node({'reservation': 'hostName1'})
         self.dbapi.clear_node_reservations_for_conductor('hostname1')
         node1 = self.dbapi.get_node_by_id(node1.id)
         node2 = self.dbapi.get_node_by_id(node2.id)
         node3 = self.dbapi.get_node_by_id(node3.id)
+        node4 = self.dbapi.get_node_by_id(node4.id)
         self.assertIsNone(node1.reservation)
         self.assertEqual('hostname2', node2.reservation)
         self.assertIsNone(node3.reservation)
+        self.assertIsNone(node4.reservation)
 
     def test_clear_node_target_power_state(self):
         node1 = self.dbapi.create_node({'reservation': 'hostname1',
@@ -194,16 +197,21 @@ class DbConductorTestCase(base.DbTestCase):
                                         'target_power_state': 'power on'})
         node3 = self.dbapi.create_node({'reservation': None,
                                         'target_power_state': 'power on'})
+        node4 = self.dbapi.create_node({'reservation': 'hostName1',
+                                        'target_power_state': 'power on'})
         self.dbapi.clear_node_target_power_state('hostname1')
         node1 = self.dbapi.get_node_by_id(node1.id)
         node2 = self.dbapi.get_node_by_id(node2.id)
         node3 = self.dbapi.get_node_by_id(node3.id)
+        node4 = self.dbapi.get_node_by_id(node4.id)
         self.assertIsNone(node1.target_power_state)
         self.assertIn('power operation was aborted', node1.last_error)
         self.assertEqual('power on', node2.target_power_state)
         self.assertIsNone(node2.last_error)
         self.assertEqual('power on', node3.target_power_state)
         self.assertIsNone(node3.last_error)
+        self.assertIsNone(node4.target_power_state)
+        self.assertIn('power operation was aborted', node4.last_error)
 
     @mock.patch.object(timeutils, 'utcnow', autospec=True)
     def test_get_active_hardware_type_dict_one_host_no_ht(self, mock_utcnow):
diff --git a/releasenotes/notes/caseless-conductor-restart-check-f70005fbf65f6bb6.yaml b/releasenotes/notes/caseless-conductor-restart-check-f70005fbf65f6bb6.yaml
new file mode 100644
index 0000000000..a085a04c32
--- /dev/null
+++ b/releasenotes/notes/caseless-conductor-restart-check-f70005fbf65f6bb6.yaml
@@ -0,0 +1,5 @@
+---
+fixes:
+  - |
+    Fixes an issue where a node may be locked from changes if a conductor's
+    hostname case is changed before restarting the conductor service.