From a70508ad2399bb392c08147cb630b924df2b4320 Mon Sep 17 00:00:00 2001 From: Olivier Lambert Date: Tue, 14 Jul 2026 22:44:29 +0200 Subject: [PATCH] Fix permanent VDI activation lock leak after host crash The 'activating' VDI sm_config key used by blktap2 as a pool-wide activation lock is released in a finally clause, which protects against exceptions but not against the host dying mid-activation (fence, power loss, watchdog). The leaked key then fails every subsequent activation of that VDI pool-wide with MAP_DUPLICATE_KEY, and blocks GC relinking. Unlike the sibling host_ attach keys, it carried no owner and no cleanup path ever removed it: xe cannot touch it (sm-config is read-only in the CLI) and only raw VDI.remove_from_sm_config calls could recover. Encode the owning host ref as the key's value (instead of 'True') so staleness can be arbitrated, and clean the key up in the two places that already give host_ keys their crash-recovery treatment: - resetvdis.reset_sr, run on every sr_attach, now removes an 'activating' entry owned by the re-attaching host, so a crashed host cleans up after itself as soon as it replugs its PBDs; - resetvdis.reset_vdi now removes the entry when --force is given or when the recorded owner is no longer part of the pool, making 'resetvdis single ' a supported remedy for stale locks left by hosts that never come back. Legacy bare 'True' values written by older code cannot be attributed to an owner and are deliberately only cleared by --force. --- libs/sm/blktap2.py | 6 +- libs/sm/resetvdis.py | 25 +++++++ tests/test_blktap2.py | 14 ++-- tests/test_resetvdis.py | 145 ++++++++++++++++++++++++++++++++++++++++ 4 files changed, 182 insertions(+), 8 deletions(-) create mode 100644 tests/test_resetvdis.py diff --git a/libs/sm/blktap2.py b/libs/sm/blktap2.py index 00992b9aa..55fbcd017 100644 --- a/libs/sm/blktap2.py +++ b/libs/sm/blktap2.py @@ -1344,8 +1344,12 @@ def _add_tag(self, vdi_uuid, writable): util.SMlog("Paused or host_ref key found [%s]" % sm_config) return False try: + # Store the host ref as the value so that a lock leaked by a + # host crash mid-activation can be attributed to its owner and + # cleaned up when that host re-attaches the SR (see + # resetvdis.reset_sr) self._session.xenapi.VDI.add_to_sm_config( - vdi_ref, 'activating', 'True') + vdi_ref, 'activating', host_ref) except XenAPI.Failure as e: if e.details[0] == 'MAP_DUPLICATE_KEY' and not writable: # Someone else is activating - a retry might succeed diff --git a/libs/sm/resetvdis.py b/libs/sm/resetvdis.py index cfce82033..609ff951c 100644 --- a/libs/sm/resetvdis.py +++ b/libs/sm/resetvdis.py @@ -49,6 +49,13 @@ def reset_sr(session, host_uuid, sr_uuid, is_sr_master): if sm_config.get(host_key): util.SMlog("Clearing attached status for VDI %s" % vdi_uuid) session.xenapi.VDI.remove_from_sm_config(vdi_ref, host_key) + if sm_config.get("activating") == host_ref: + # this host crashed while activating the VDI: the flag would + # otherwise stay behind forever, failing all subsequent + # activations with MAP_DUPLICATE_KEY + util.SMlog("Clearing stale activating status for VDI %s" % + vdi_uuid) + session.xenapi.VDI.remove_from_sm_config(vdi_ref, "activating") if is_sr_master and sm_config.get("paused"): util.SMlog("Clearing paused status for VDI %s" % vdi_uuid) session.xenapi.VDI.remove_from_sm_config(vdi_ref, "paused") @@ -119,6 +126,24 @@ def reset_vdi(session, vdi_uuid, force, term_output=True, writable=True): if term_output: print(msg) + activating = sm_config.get("activating") + if activating: + clear_activating = force + if not clear_activating and activating.startswith("OpaqueRef:"): + # the owning host is encoded in the value: if it is no longer + # part of the pool, the flag is necessarily stale + try: + session.xenapi.host.get_record(activating) + except XenAPI.Failure: + clear_activating = True + + if clear_activating: + session.xenapi.VDI.remove_from_sm_config(vdi_ref, "activating") + msg = "Cleared activating flag for %s" % vdi_uuid + util.SMlog(msg) + if term_output: + print(msg) + if not host_ref: msg = "VDI %s is not marked as attached anywhere, nothing to do" \ % vdi_uuid diff --git a/tests/test_blktap2.py b/tests/test_blktap2.py index c5870de6c..34bf9694c 100644 --- a/tests/test_blktap2.py +++ b/tests/test_blktap2.py @@ -243,7 +243,7 @@ def test_activate(self, mock_tapdisk, mock_nbd_link, self.vdi.activate(self.sr_uuid, self.vdi_uuid, True, {}) self.mock_session.xenapi.VDI.add_to_sm_config.assert_has_calls( - [mock.call('vref1', 'activating', 'True'), + [mock.call('vref1', 'activating', 'href1'), mock.call('vref1', 'host_href1', "RW")], any_order=True) self.mock_session.xenapi.VDI.remove_from_sm_config.assert_has_calls( @@ -272,7 +272,7 @@ def test_activate_relink_retry( self.vdi.activate(self.sr_uuid, self.vdi_uuid, True, {}) self.mock_session.xenapi.VDI.add_to_sm_config.assert_has_calls( - [mock.call('vref1', 'activating', 'True'), + [mock.call('vref1', 'activating', 'href1'), mock.call('vref1', 'host_href1', "RW")], any_order=True) @@ -297,7 +297,7 @@ def test_activate_pause_retry( self.vdi.activate(self.sr_uuid, self.vdi_uuid, True, {}) self.mock_session.xenapi.VDI.add_to_sm_config.assert_has_calls( - [mock.call('vref1', 'activating', 'True'), + [mock.call('vref1', 'activating', 'href1'), mock.call('vref1', 'host_href1', "RW")], any_order=True) @@ -324,7 +324,7 @@ def test_activate_paused_while_tagging( self.vdi.activate(self.sr_uuid, self.vdi_uuid, True, {}) self.mock_session.xenapi.VDI.add_to_sm_config.assert_has_calls( - [mock.call('vref1', 'activating', 'True'), + [mock.call('vref1', 'activating', 'href1'), mock.call('vref1', 'host_href1', "RW")], any_order=True) self.mock_session.xenapi.VDI.remove_from_sm_config.assert_has_calls( @@ -355,7 +355,7 @@ def test_activate_relink_while_tagging( self.vdi.activate(self.sr_uuid, self.vdi_uuid, True, {}) self.mock_session.xenapi.VDI.add_to_sm_config.assert_has_calls( - [mock.call('vref1', 'activating', 'True'), + [mock.call('vref1', 'activating', 'href1'), mock.call('vref1', 'host_href1', "RW")], any_order=True) self.mock_session.xenapi.VDI.remove_from_sm_config.assert_has_calls( @@ -393,8 +393,8 @@ def test_activate_ro_already_activating_retry( self.vdi.activate(self.sr_uuid, self.vdi_uuid, False, {}) self.mock_session.xenapi.VDI.add_to_sm_config.assert_has_calls( - [mock.call('vref1', 'activating', 'True'), - mock.call('vref1', 'activating', 'True'), + [mock.call('vref1', 'activating', 'href1'), + mock.call('vref1', 'activating', 'href1'), mock.call('vref1', 'host_href1', "RO")]) self.mock_session.xenapi.VDI.remove_from_sm_config.assert_has_calls( [mock.call('vref1', 'activating')]) diff --git a/tests/test_resetvdis.py b/tests/test_resetvdis.py new file mode 100644 index 000000000..f420f6d74 --- /dev/null +++ b/tests/test_resetvdis.py @@ -0,0 +1,145 @@ +import unittest +import unittest.mock as mock + +import XenAPI + +from sm import resetvdis + +HOST_UUID = "3fa1e58f-2545-4b3b-8e07-c53a6bab0a41" +HOST_REF = "OpaqueRef:host1" +OTHER_HOST_REF = "OpaqueRef:host2" +SR_UUID = "3dd7d05e-8b7f-4d64-8801-ee2ee0b3ff81" +SR_REF = "OpaqueRef:sr1" +VDI_UUID = "b0dc7f19-4ee6-4d05-acd9-4a1b0f0e6be7" +VDI_REF = "OpaqueRef:vdi1" + + +@mock.patch('sm.resetvdis.util.SMlog', autospec=True) +class TestResetSr(unittest.TestCase): + def setUp(self): + cleanup_patcher = mock.patch('sm.resetvdis.cleanup', autospec=True) + cleanup_patcher.start() + lock_patcher = mock.patch('sm.resetvdis.lock.Lock', autospec=True) + lock_patcher.start() + + self.addCleanup(mock.patch.stopall) + + self.session = mock.MagicMock() + self.session.xenapi.host.get_by_uuid.return_value = HOST_REF + self.session.xenapi.SR.get_by_uuid.return_value = SR_REF + + def set_vdis(self, sm_config): + self.session.xenapi.VDI.get_all_records_where.return_value = { + VDI_REF: {"uuid": VDI_UUID, "sm_config": sm_config}} + + def test_reset_sr_clears_host_key(self, mock_log): + self.set_vdis({"host_%s" % HOST_REF: "RW"}) + + resetvdis.reset_sr(self.session, HOST_UUID, SR_UUID, False) + + self.session.xenapi.VDI.remove_from_sm_config.assert_called_once_with( + VDI_REF, "host_%s" % HOST_REF) + + def test_reset_sr_clears_activating_owned_by_host(self, mock_log): + self.set_vdis({"activating": HOST_REF}) + + resetvdis.reset_sr(self.session, HOST_UUID, SR_UUID, False) + + self.session.xenapi.VDI.remove_from_sm_config.assert_called_once_with( + VDI_REF, "activating") + + def test_reset_sr_keeps_activating_owned_by_other_host(self, mock_log): + self.set_vdis({"activating": OTHER_HOST_REF}) + + resetvdis.reset_sr(self.session, HOST_UUID, SR_UUID, False) + + self.session.xenapi.VDI.remove_from_sm_config.assert_not_called() + + def test_reset_sr_keeps_legacy_activating(self, mock_log): + # written by a host running a version predating owner encoding + self.set_vdis({"activating": "True"}) + + resetvdis.reset_sr(self.session, HOST_UUID, SR_UUID, False) + + self.session.xenapi.VDI.remove_from_sm_config.assert_not_called() + + def test_reset_sr_clears_paused_on_master(self, mock_log): + self.set_vdis({"paused": "true"}) + + resetvdis.reset_sr(self.session, HOST_UUID, SR_UUID, True) + + self.session.xenapi.VDI.remove_from_sm_config.assert_called_once_with( + VDI_REF, "paused") + + +@mock.patch('sm.resetvdis.util.SMlog', autospec=True) +class TestResetVdi(unittest.TestCase): + def setUp(self): + self.session = mock.MagicMock() + self.session.xenapi.VDI.get_by_uuid.return_value = VDI_REF + + def set_vdi(self, sm_config): + self.session.xenapi.VDI.get_record.return_value = { + "uuid": VDI_UUID, "SR": SR_REF, "sm_config": sm_config} + + def test_reset_vdi_force_clears_activating(self, mock_log): + self.set_vdi({"activating": HOST_REF}) + + clean = resetvdis.reset_vdi(self.session, VDI_UUID, force=True, + term_output=False) + + self.assertTrue(clean) + self.session.xenapi.VDI.remove_from_sm_config.assert_called_once_with( + VDI_REF, "activating") + + def test_reset_vdi_force_clears_legacy_activating(self, mock_log): + self.set_vdi({"activating": "True"}) + + clean = resetvdis.reset_vdi(self.session, VDI_UUID, force=True, + term_output=False) + + self.assertTrue(clean) + self.session.xenapi.VDI.remove_from_sm_config.assert_called_once_with( + VDI_REF, "activating") + + def test_reset_vdi_keeps_activating_of_valid_host(self, mock_log): + self.set_vdi({"activating": HOST_REF}) + self.session.xenapi.host.get_record.return_value = { + "uuid": HOST_UUID, "name_label": "host1"} + + resetvdis.reset_vdi(self.session, VDI_UUID, force=False, + term_output=False) + + self.session.xenapi.VDI.remove_from_sm_config.assert_not_called() + + def test_reset_vdi_clears_activating_of_invalid_host(self, mock_log): + self.set_vdi({"activating": HOST_REF}) + self.session.xenapi.host.get_record.side_effect = XenAPI.Failure( + ["HANDLE_INVALID", "host", HOST_REF]) + + resetvdis.reset_vdi(self.session, VDI_UUID, force=False, + term_output=False) + + self.session.xenapi.VDI.remove_from_sm_config.assert_called_once_with( + VDI_REF, "activating") + + def test_reset_vdi_keeps_legacy_activating_without_force(self, mock_log): + # a legacy 'True' value cannot be attributed to a host, so only + # --force may clear it + self.set_vdi({"activating": "True"}) + + resetvdis.reset_vdi(self.session, VDI_UUID, force=False, + term_output=False) + + self.session.xenapi.VDI.remove_from_sm_config.assert_not_called() + + def test_reset_vdi_term_output_prints_activating(self, mock_log): + self.set_vdi({"activating": HOST_REF}) + + with mock.patch('builtins.print') as mock_print: + resetvdis.reset_vdi(self.session, VDI_UUID, force=True) + + self.session.xenapi.VDI.remove_from_sm_config.assert_called_once_with( + VDI_REF, "activating") + printed = "".join(str(c) for c in mock_print.call_args_list) + self.assertIn("activating", printed)