diff options
author | Zuul <zuul@review.opendev.org> | 2020-08-11 12:08:49 +0000 |
---|---|---|
committer | Gerrit Code Review <review@openstack.org> | 2020-08-11 12:08:50 +0000 |
commit | 918478b4e14876dbc5dd9a6f40951979751276fd (patch) | |
tree | 718bcebb19f0102d7833084bdd80236a91438e89 | |
parent | aa99108e85a71189b43e50bf0638ef2848606455 (diff) | |
parent | 5a429ce8888c882189b36214bcfba2bfd72a6d3b (diff) | |
download | ironic-918478b4e14876dbc5dd9a6f40951979751276fd.tar.gz |
Merge "Remove locks before RPC bus is started" into stable/queens
-rw-r--r-- | ironic/common/rpc_service.py | 4 | ||||
-rw-r--r-- | ironic/conductor/base_manager.py | 34 | ||||
-rw-r--r-- | ironic/tests/unit/common/test_rpc_service.py | 4 | ||||
-rw-r--r-- | ironic/tests/unit/conductor/mgr_utils.py | 1 | ||||
-rw-r--r-- | ironic/tests/unit/conductor/test_base_manager.py | 10 | ||||
-rw-r--r-- | releasenotes/notes/remove-locks-first-d12ac27106f800f8.yaml | 9 |
6 files changed, 55 insertions, 7 deletions
diff --git a/ironic/common/rpc_service.py b/ironic/common/rpc_service.py index a2aebb56c..13bdde8c5 100644 --- a/ironic/common/rpc_service.py +++ b/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() diff --git a/ironic/conductor/base_manager.py b/ironic/conductor/base_manager.py index e43edb8a4..0de77e309 100644 --- a/ironic/conductor/base_manager.py +++ b/ironic/conductor/base_manager.py @@ -76,6 +76,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. @@ -95,7 +122,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.""" @@ -163,10 +191,6 @@ class BaseConductorManager(object): "configured.") raise exception.ConfigInvalid(msg) - # 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( diff --git a/ironic/tests/unit/common/test_rpc_service.py b/ironic/tests/unit/common/test_rpc_service.py index 755df4f81..b4e857bf8 100644 --- a/ironic/tests/unit/common/test_rpc_service.py +++ b/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) diff --git a/ironic/tests/unit/conductor/mgr_utils.py b/ironic/tests/unit/conductor/mgr_utils.py index c531bcfe6..35c191556 100644 --- a/ironic/tests/unit/conductor/mgr_utils.py +++ b/ironic/tests/unit/conductor/mgr_utils.py @@ -194,6 +194,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) diff --git a/ironic/tests/unit/conductor/test_base_manager.py b/ironic/tests/unit/conductor/test_base_manager.py index a128aa8f2..d2d285885 100644 --- a/ironic/tests/unit/conductor/test_base_manager.py +++ b/ironic/tests/unit/conductor/test_base_manager.py @@ -28,6 +28,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 import objects @@ -290,6 +291,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): diff --git a/releasenotes/notes/remove-locks-first-d12ac27106f800f8.yaml b/releasenotes/notes/remove-locks-first-d12ac27106f800f8.yaml new file mode 100644 index 000000000..a7e0cb958 --- /dev/null +++ b/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. |