Fix permanent VDI activation lock leak after host crash

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

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

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')])

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)