Skip to content

Commit 2865ff0

Browse files
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_<ref> 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_<ref> 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 <uuid>' 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. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 parent 471cf7e commit 2865ff0

4 files changed

Lines changed: 182 additions & 8 deletions

File tree

libs/sm/blktap2.py

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1344,8 +1344,12 @@ def _add_tag(self, vdi_uuid, writable):
13441344
util.SMlog("Paused or host_ref key found [%s]" % sm_config)
13451345
return False
13461346
try:
1347+
# Store the host ref as the value so that a lock leaked by a
1348+
# host crash mid-activation can be attributed to its owner and
1349+
# cleaned up when that host re-attaches the SR (see
1350+
# resetvdis.reset_sr)
13471351
self._session.xenapi.VDI.add_to_sm_config(
1348-
vdi_ref, 'activating', 'True')
1352+
vdi_ref, 'activating', host_ref)
13491353
except XenAPI.Failure as e:
13501354
if e.details[0] == 'MAP_DUPLICATE_KEY' and not writable:
13511355
# Someone else is activating - a retry might succeed

libs/sm/resetvdis.py

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,13 @@ def reset_sr(session, host_uuid, sr_uuid, is_sr_master):
4949
if sm_config.get(host_key):
5050
util.SMlog("Clearing attached status for VDI %s" % vdi_uuid)
5151
session.xenapi.VDI.remove_from_sm_config(vdi_ref, host_key)
52+
if sm_config.get("activating") == host_ref:
53+
# this host crashed while activating the VDI: the flag would
54+
# otherwise stay behind forever, failing all subsequent
55+
# activations with MAP_DUPLICATE_KEY
56+
util.SMlog("Clearing stale activating status for VDI %s" %
57+
vdi_uuid)
58+
session.xenapi.VDI.remove_from_sm_config(vdi_ref, "activating")
5259
if is_sr_master and sm_config.get("paused"):
5360
util.SMlog("Clearing paused status for VDI %s" % vdi_uuid)
5461
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):
119126
if term_output:
120127
print(msg)
121128

129+
activating = sm_config.get("activating")
130+
if activating:
131+
clear_activating = force
132+
if not clear_activating and activating.startswith("OpaqueRef:"):
133+
# the owning host is encoded in the value: if it is no longer
134+
# part of the pool, the flag is necessarily stale
135+
try:
136+
session.xenapi.host.get_record(activating)
137+
except XenAPI.Failure:
138+
clear_activating = True
139+
140+
if clear_activating:
141+
session.xenapi.VDI.remove_from_sm_config(vdi_ref, "activating")
142+
msg = "Cleared activating flag for %s" % vdi_uuid
143+
util.SMlog(msg)
144+
if term_output:
145+
print(msg)
146+
122147
if not host_ref:
123148
msg = "VDI %s is not marked as attached anywhere, nothing to do" \
124149
% vdi_uuid

tests/test_blktap2.py

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -243,7 +243,7 @@ def test_activate(self, mock_tapdisk, mock_nbd_link,
243243
self.vdi.activate(self.sr_uuid, self.vdi_uuid, True, {})
244244

245245
self.mock_session.xenapi.VDI.add_to_sm_config.assert_has_calls(
246-
[mock.call('vref1', 'activating', 'True'),
246+
[mock.call('vref1', 'activating', 'href1'),
247247
mock.call('vref1', 'host_href1', "RW")],
248248
any_order=True)
249249
self.mock_session.xenapi.VDI.remove_from_sm_config.assert_has_calls(
@@ -272,7 +272,7 @@ def test_activate_relink_retry(
272272
self.vdi.activate(self.sr_uuid, self.vdi_uuid, True, {})
273273

274274
self.mock_session.xenapi.VDI.add_to_sm_config.assert_has_calls(
275-
[mock.call('vref1', 'activating', 'True'),
275+
[mock.call('vref1', 'activating', 'href1'),
276276
mock.call('vref1', 'host_href1', "RW")],
277277
any_order=True)
278278

@@ -297,7 +297,7 @@ def test_activate_pause_retry(
297297

298298
self.vdi.activate(self.sr_uuid, self.vdi_uuid, True, {})
299299
self.mock_session.xenapi.VDI.add_to_sm_config.assert_has_calls(
300-
[mock.call('vref1', 'activating', 'True'),
300+
[mock.call('vref1', 'activating', 'href1'),
301301
mock.call('vref1', 'host_href1', "RW")],
302302
any_order=True)
303303

@@ -324,7 +324,7 @@ def test_activate_paused_while_tagging(
324324
self.vdi.activate(self.sr_uuid, self.vdi_uuid, True, {})
325325

326326
self.mock_session.xenapi.VDI.add_to_sm_config.assert_has_calls(
327-
[mock.call('vref1', 'activating', 'True'),
327+
[mock.call('vref1', 'activating', 'href1'),
328328
mock.call('vref1', 'host_href1', "RW")],
329329
any_order=True)
330330
self.mock_session.xenapi.VDI.remove_from_sm_config.assert_has_calls(
@@ -355,7 +355,7 @@ def test_activate_relink_while_tagging(
355355
self.vdi.activate(self.sr_uuid, self.vdi_uuid, True, {})
356356

357357
self.mock_session.xenapi.VDI.add_to_sm_config.assert_has_calls(
358-
[mock.call('vref1', 'activating', 'True'),
358+
[mock.call('vref1', 'activating', 'href1'),
359359
mock.call('vref1', 'host_href1', "RW")],
360360
any_order=True)
361361
self.mock_session.xenapi.VDI.remove_from_sm_config.assert_has_calls(
@@ -393,8 +393,8 @@ def test_activate_ro_already_activating_retry(
393393
self.vdi.activate(self.sr_uuid, self.vdi_uuid, False, {})
394394

395395
self.mock_session.xenapi.VDI.add_to_sm_config.assert_has_calls(
396-
[mock.call('vref1', 'activating', 'True'),
397-
mock.call('vref1', 'activating', 'True'),
396+
[mock.call('vref1', 'activating', 'href1'),
397+
mock.call('vref1', 'activating', 'href1'),
398398
mock.call('vref1', 'host_href1', "RO")])
399399
self.mock_session.xenapi.VDI.remove_from_sm_config.assert_has_calls(
400400
[mock.call('vref1', 'activating')])

tests/test_resetvdis.py

Lines changed: 145 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,145 @@
1+
import unittest
2+
import unittest.mock as mock
3+
4+
import XenAPI
5+
6+
from sm import resetvdis
7+
8+
HOST_UUID = "3fa1e58f-2545-4b3b-8e07-c53a6bab0a41"
9+
HOST_REF = "OpaqueRef:host1"
10+
OTHER_HOST_REF = "OpaqueRef:host2"
11+
SR_UUID = "3dd7d05e-8b7f-4d64-8801-ee2ee0b3ff81"
12+
SR_REF = "OpaqueRef:sr1"
13+
VDI_UUID = "b0dc7f19-4ee6-4d05-acd9-4a1b0f0e6be7"
14+
VDI_REF = "OpaqueRef:vdi1"
15+
16+
17+
@mock.patch('sm.resetvdis.util.SMlog', autospec=True)
18+
class TestResetSr(unittest.TestCase):
19+
def setUp(self):
20+
cleanup_patcher = mock.patch('sm.resetvdis.cleanup', autospec=True)
21+
cleanup_patcher.start()
22+
lock_patcher = mock.patch('sm.resetvdis.lock.Lock', autospec=True)
23+
lock_patcher.start()
24+
25+
self.addCleanup(mock.patch.stopall)
26+
27+
self.session = mock.MagicMock()
28+
self.session.xenapi.host.get_by_uuid.return_value = HOST_REF
29+
self.session.xenapi.SR.get_by_uuid.return_value = SR_REF
30+
31+
def set_vdis(self, sm_config):
32+
self.session.xenapi.VDI.get_all_records_where.return_value = {
33+
VDI_REF: {"uuid": VDI_UUID, "sm_config": sm_config}}
34+
35+
def test_reset_sr_clears_host_key(self, mock_log):
36+
self.set_vdis({"host_%s" % HOST_REF: "RW"})
37+
38+
resetvdis.reset_sr(self.session, HOST_UUID, SR_UUID, False)
39+
40+
self.session.xenapi.VDI.remove_from_sm_config.assert_called_once_with(
41+
VDI_REF, "host_%s" % HOST_REF)
42+
43+
def test_reset_sr_clears_activating_owned_by_host(self, mock_log):
44+
self.set_vdis({"activating": HOST_REF})
45+
46+
resetvdis.reset_sr(self.session, HOST_UUID, SR_UUID, False)
47+
48+
self.session.xenapi.VDI.remove_from_sm_config.assert_called_once_with(
49+
VDI_REF, "activating")
50+
51+
def test_reset_sr_keeps_activating_owned_by_other_host(self, mock_log):
52+
self.set_vdis({"activating": OTHER_HOST_REF})
53+
54+
resetvdis.reset_sr(self.session, HOST_UUID, SR_UUID, False)
55+
56+
self.session.xenapi.VDI.remove_from_sm_config.assert_not_called()
57+
58+
def test_reset_sr_keeps_legacy_activating(self, mock_log):
59+
# written by a host running a version predating owner encoding
60+
self.set_vdis({"activating": "True"})
61+
62+
resetvdis.reset_sr(self.session, HOST_UUID, SR_UUID, False)
63+
64+
self.session.xenapi.VDI.remove_from_sm_config.assert_not_called()
65+
66+
def test_reset_sr_clears_paused_on_master(self, mock_log):
67+
self.set_vdis({"paused": "true"})
68+
69+
resetvdis.reset_sr(self.session, HOST_UUID, SR_UUID, True)
70+
71+
self.session.xenapi.VDI.remove_from_sm_config.assert_called_once_with(
72+
VDI_REF, "paused")
73+
74+
75+
@mock.patch('sm.resetvdis.util.SMlog', autospec=True)
76+
class TestResetVdi(unittest.TestCase):
77+
def setUp(self):
78+
self.session = mock.MagicMock()
79+
self.session.xenapi.VDI.get_by_uuid.return_value = VDI_REF
80+
81+
def set_vdi(self, sm_config):
82+
self.session.xenapi.VDI.get_record.return_value = {
83+
"uuid": VDI_UUID, "SR": SR_REF, "sm_config": sm_config}
84+
85+
def test_reset_vdi_force_clears_activating(self, mock_log):
86+
self.set_vdi({"activating": HOST_REF})
87+
88+
clean = resetvdis.reset_vdi(self.session, VDI_UUID, force=True,
89+
term_output=False)
90+
91+
self.assertTrue(clean)
92+
self.session.xenapi.VDI.remove_from_sm_config.assert_called_once_with(
93+
VDI_REF, "activating")
94+
95+
def test_reset_vdi_force_clears_legacy_activating(self, mock_log):
96+
self.set_vdi({"activating": "True"})
97+
98+
clean = resetvdis.reset_vdi(self.session, VDI_UUID, force=True,
99+
term_output=False)
100+
101+
self.assertTrue(clean)
102+
self.session.xenapi.VDI.remove_from_sm_config.assert_called_once_with(
103+
VDI_REF, "activating")
104+
105+
def test_reset_vdi_keeps_activating_of_valid_host(self, mock_log):
106+
self.set_vdi({"activating": HOST_REF})
107+
self.session.xenapi.host.get_record.return_value = {
108+
"uuid": HOST_UUID, "name_label": "host1"}
109+
110+
resetvdis.reset_vdi(self.session, VDI_UUID, force=False,
111+
term_output=False)
112+
113+
self.session.xenapi.VDI.remove_from_sm_config.assert_not_called()
114+
115+
def test_reset_vdi_clears_activating_of_invalid_host(self, mock_log):
116+
self.set_vdi({"activating": HOST_REF})
117+
self.session.xenapi.host.get_record.side_effect = XenAPI.Failure(
118+
["HANDLE_INVALID", "host", HOST_REF])
119+
120+
resetvdis.reset_vdi(self.session, VDI_UUID, force=False,
121+
term_output=False)
122+
123+
self.session.xenapi.VDI.remove_from_sm_config.assert_called_once_with(
124+
VDI_REF, "activating")
125+
126+
def test_reset_vdi_keeps_legacy_activating_without_force(self, mock_log):
127+
# a legacy 'True' value cannot be attributed to a host, so only
128+
# --force may clear it
129+
self.set_vdi({"activating": "True"})
130+
131+
resetvdis.reset_vdi(self.session, VDI_UUID, force=False,
132+
term_output=False)
133+
134+
self.session.xenapi.VDI.remove_from_sm_config.assert_not_called()
135+
136+
def test_reset_vdi_term_output_prints_activating(self, mock_log):
137+
self.set_vdi({"activating": HOST_REF})
138+
139+
with mock.patch('builtins.print') as mock_print:
140+
resetvdis.reset_vdi(self.session, VDI_UUID, force=True)
141+
142+
self.session.xenapi.VDI.remove_from_sm_config.assert_called_once_with(
143+
VDI_REF, "activating")
144+
printed = "".join(str(c) for c in mock_print.call_args_list)
145+
self.assertIn("activating", printed)

0 commit comments

Comments
 (0)