From 4cf0147e86039e8fa2232c3de6d45f1405a515d0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Harald=20Jens=C3=A5s?= Date: Thu, 28 Apr 2022 09:22:14 +0200 Subject: [PATCH] Exclude current conductor from offline_conductors In some cases the current conductor may have failed to updated the heartbeat timestamp due to failure of resource starvation. When this occurs the dbapi get_offline_conductors method will include the current conductor in its return value. In this scenario the conductor may end up forcefully remove node reservations or allocations from itself, triggering takeover which fail on-going operations. This change adds a wrapper to exclude the current conductor. The wrapper will log a warning to raise the issue. Related-Bug: #1970484 Stroy: 2010016 Task: 45204 Change-Id: I6a8f38934b475f792433be6f0882540b82ca26c1 --- ironic/conductor/manager.py | 6 ++++-- ironic/conductor/utils.py | 21 +++++++++++++++++++ ironic/tests/unit/conductor/test_utils.py | 10 +++++++++ ...m-offline-conductors-2e2ef401a8b7d7e8.yaml | 12 +++++++++++ 4 files changed, 47 insertions(+), 2 deletions(-) create mode 100644 releasenotes/notes/exclude-current-conductor-from-offline-conductors-2e2ef401a8b7d7e8.yaml diff --git a/ironic/conductor/manager.py b/ironic/conductor/manager.py index d112248523..7f6470e387 100644 --- a/ironic/conductor/manager.py +++ b/ironic/conductor/manager.py @@ -1604,7 +1604,8 @@ class ConductorManager(base_manager.BaseConductorManager): :param context: request context. """ - offline_conductors = self.dbapi.get_offline_conductors() + offline_conductors = utils.exclude_current_conductor( + self.host, self.dbapi.get_offline_conductors()) if not offline_conductors: return @@ -3436,7 +3437,8 @@ class ConductorManager(base_manager.BaseConductorManager): :param context: request context. """ - offline_conductors = self.dbapi.get_offline_conductors(field='id') + offline_conductors = utils.exclude_current_conductor( + self.conductor.id, self.dbapi.get_offline_conductors(field='id')) for conductor_id in offline_conductors: filters = {'state': states.ALLOCATING, 'conductor_affinity': conductor_id} diff --git a/ironic/conductor/utils.py b/ironic/conductor/utils.py index 4a0d68a5d8..b418d9d0a9 100644 --- a/ironic/conductor/utils.py +++ b/ironic/conductor/utils.py @@ -1671,3 +1671,24 @@ def update_image_type(context, node): 'image_type', images.IMAGE_TYPE_WHOLE_DISK if iwdi else images.IMAGE_TYPE_PARTITION) return True + + +def exclude_current_conductor(current_conductor, offline_conductors): + """Wrapper to exclude current conductor from offline_conductors + + In some cases the current conductor may have failed to update + the heartbeat timestamp due to failure or resource starvation. + When this occurs the dbapi get_offline_conductors method will + include the current conductor in its return value. + + :param current_conductor: id or hostname of the current conductor + :param offline_conductors: List of offline conductors. + :return: List of offline conductors, excluding current conductor + """ + if current_conductor in offline_conductors: + LOG.warning('Current conductor %s will be excluded from offline ' + 'conductors. Conductor heartbeat has failed to update the ' + 'database timestamp. This is sign of resource starvation.', + current_conductor) + + return [x for x in offline_conductors if x != current_conductor] diff --git a/ironic/tests/unit/conductor/test_utils.py b/ironic/tests/unit/conductor/test_utils.py index 5363fe8013..7d8e70a1fa 100644 --- a/ironic/tests/unit/conductor/test_utils.py +++ b/ironic/tests/unit/conductor/test_utils.py @@ -1921,6 +1921,16 @@ class MiscTestCase(db_base.DbTestCase): conductor_utils.restore_power_state_if_needed(task, power_state) self.assertEqual(0, power_action_mock.call_count) + @mock.patch.object(conductor_utils.LOG, 'warning', autospec=True) + def test_exclude_current_conductor(self, mock_log): + current_conductor = 'foo' + offline_conductos = ['foo', 'bar'] + result = conductor_utils.exclude_current_conductor(current_conductor, + offline_conductos) + self.assertTrue(mock_log.called) + self.assertIn('bar', result) + self.assertNotIn('foo', result) + class ValidateInstanceInfoTraitsTestCase(tests_base.TestCase): diff --git a/releasenotes/notes/exclude-current-conductor-from-offline-conductors-2e2ef401a8b7d7e8.yaml b/releasenotes/notes/exclude-current-conductor-from-offline-conductors-2e2ef401a8b7d7e8.yaml new file mode 100644 index 0000000000..c7f3e4acc7 --- /dev/null +++ b/releasenotes/notes/exclude-current-conductor-from-offline-conductors-2e2ef401a8b7d7e8.yaml @@ -0,0 +1,12 @@ +--- +fixes: + - | + Fixes an issue where a conductor would attempt local takeover. In case of + heartbeat failure due to resource starvation, the current conductor was + detected as offline when querying the database. In this scenario the + conductor would forcibly remove reservations of it's own and initiate + takeover. Current conductor is now excluded from the list of offline + conductors, so that local takeover does not occur for this case. A warning + is logged to highlight the potential resource starvation issue. + See bug: `2010016 `_. +