Merge "Remove locks before RPC bus is started" into stable/rocky

ironic/common/rpc_service.py

@@ -44,9 +44,11 @@ class RPCService(service.Service):
         super(RPCService, self).start()
         admin_context = context.get_admin_context()
 
+        serializer = objects_base.IronicObjectSerializer(is_server=True)
+        # Perform preparatory actions before starting the RPC listener
+        self.manager.prepare_host()
         target = messaging.Target(topic=self.topic, server=self.host)
         endpoints = [self.manager]
-        serializer = objects_base.IronicObjectSerializer(is_server=True)
         self.rpcserver = rpc.get_server(target, endpoints, serializer)
         self.rpcserver.start()
 

ironic/conductor/base_manager.py

@@ -77,6 +77,33 @@ class BaseConductorManager(object):
         self.sensors_notifier = rpc.get_sensors_notifier()
         self._started = False
         self._shutdown = None
+        self.dbapi = None
+
+    def prepare_host(self):
+        """Prepares host for initialization
+
+        Removes existing database entries involved with node locking for nodes
+        in a transitory power state and nodes that are presently locked by
+        the hostname of this conductor.
+
+        Under normal operation, this is also when the initial database
+        connectivity is established for the conductor's normal operation.
+        """
+        # NOTE(TheJulia) We need to clear locks early on in the process
+        # of starting where the database shows we still hold them.
+        # This must be done before we re-register our existence in the
+        # conductors table and begin accepting new requests via RPC as
+        # if we do not then we may squash our *new* locks from new work.
+
+        if not self.dbapi:
+            LOG.debug('Initializing database client for %s.', self.host)
+            self.dbapi = dbapi.get_instance()
+        LOG.debug('Removing stale locks from the database matching '
+                  'this conductor\'s hostname: %s', self.host)
+        # clear all target_power_state with locks by this conductor
+        self.dbapi.clear_node_target_power_state(self.host)
+        # clear all locks held by this conductor before registering
+        self.dbapi.clear_node_reservations_for_conductor(self.host)
 
     def init_host(self, admin_context=None):
         """Initialize the conductor host.
@@ -94,7 +121,8 @@ class BaseConductorManager(object):
                                  'conductor manager'))
         self._shutdown = False
 
-        self.dbapi = dbapi.get_instance()
+        if not self.dbapi:
+            self.dbapi = dbapi.get_instance()
 
         self._keepalive_evt = threading.Event()
         """Event for the keepalive thread."""
@@ -142,10 +170,6 @@ class BaseConductorManager(object):
 
         self._collect_periodic_tasks(admin_context)
 
-        # clear all target_power_state with locks by this conductor
-        self.dbapi.clear_node_target_power_state(self.host)
-        # clear all locks held by this conductor before registering
-        self.dbapi.clear_node_reservations_for_conductor(self.host)
         try:
             # Register this conductor with the cluster
             self.conductor = objects.Conductor.register(

ironic/tests/unit/common/test_rpc_service.py

@@ -35,13 +35,14 @@ class TestRPCService(base.TestCase):
         mgr_class = "ConductorManager"
         self.rpc_svc = rpc_service.RPCService(host, mgr_module, mgr_class)
 
+    @mock.patch.object(manager.ConductorManager, 'prepare_host', autospec=True)
     @mock.patch.object(oslo_messaging, 'Target', autospec=True)
     @mock.patch.object(objects_base, 'IronicObjectSerializer', autospec=True)
     @mock.patch.object(rpc, 'get_server', autospec=True)
     @mock.patch.object(manager.ConductorManager, 'init_host', autospec=True)
     @mock.patch.object(context, 'get_admin_context', autospec=True)
     def test_start(self, mock_ctx, mock_init_method,
-                   mock_rpc, mock_ios, mock_target):
+                   mock_rpc, mock_ios, mock_target, mock_prepare_method):
         mock_rpc.return_value.start = mock.MagicMock()
         self.rpc_svc.handle_signal = mock.MagicMock()
         self.rpc_svc.start()
@@ -49,5 +50,6 @@ class TestRPCService(base.TestCase):
         mock_target.assert_called_once_with(topic=self.rpc_svc.topic,
                                             server="fake_host")
         mock_ios.assert_called_once_with(is_server=True)
+        mock_prepare_method.assert_called_once_with(self.rpc_svc.manager)
         mock_init_method.assert_called_once_with(self.rpc_svc.manager,
                                                  mock_ctx.return_value)

ironic/tests/unit/conductor/mgr_utils.py

@@ -142,6 +142,7 @@ class ServiceSetUpMixin(object):
             self.service.init_host()
         else:
             with mock.patch.object(periodics, 'PeriodicWorker', autospec=True):
+                self.service.prepare_host()
                 self.service.init_host()
         self.addCleanup(self._stop_service)
 

ironic/tests/unit/conductor/test_base_manager.py

@@ -30,6 +30,7 @@ from ironic.conductor import base_manager
 from ironic.conductor import manager
 from ironic.conductor import notification_utils
 from ironic.conductor import task_manager
+from ironic.db import api as dbapi
 from ironic.drivers import fake_hardware
 from ironic.drivers import generic
 from ironic.drivers.modules import fake
@@ -247,6 +248,15 @@ class StartStopTestCase(mgr_utils.ServiceSetUpMixin, db_base.DbTestCase):
         self.service.del_host()
         self.assertTrue(self.service._shutdown)
 
+    @mock.patch.object(dbapi, 'get_instance', autospec=True)
+    def test_start_dbapi_single_call(self, mock_dbapi):
+        self._start_service()
+        # NOTE(TheJulia): This seems like it should only be 1, but
+        # the hash ring initailization pulls it's own database connection
+        # instance, which is likely a good thing, thus this is 2 instead of
+        # 3 without reuse of the database connection.
+        self.assertEqual(2, mock_dbapi.call_count)
+
 
 class CheckInterfacesTestCase(mgr_utils.ServiceSetUpMixin, db_base.DbTestCase):
     def test__check_enabled_interfaces_success(self):

releasenotes/notes/remove-locks-first-d12ac27106f800f8.yaml

@@ -0,0 +1,9 @@
+---
+fixes:
+  - |
+    Fixes an issue where ``ironic-conductor`` initialization could return a
+    ``NodeNotLocked`` error for requests requiring locks when the conductor
+    was starting. This was due to the conductor removing locks after
+    beginning accepting new work. The lock removal has been moved to after
+    the Database connectivity has been established but before the RPC bus
+    is initialized.