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)