fix(lvol): never drop the FDB record before the teardown is confirmed… - #1383
Merged
Merged
Conversation
…#1380) Backport of PR #1380 to R26.3. R26.3 field report: four lvols existed as bdevs in SPDK with no record in FDB. One had been created and deleted per the cluster log but never passed through in_deletion; two showed a create and no delete at all; the fourth left no cluster-log trace whatsoever. Every path behind that shares one shape -- the record removal did not depend on the data-plane teardown having happened: * _remove_bdev_stack returned a constant True. A failed removal was logged and the entry stamped status='deleted' anyway, so delete_lvol_from_node reported success and lvol_monitor erased the record over a live bdev. It now returns False and leaves the entry unmarked so a retry re-attempts it. * The "already gone?" probe was `if not rpc_client.get_bdevs(name)`. get_bdevs returns None both for -ENODEV and for a non-200 from the SPDK proxy, so one transient hiccup during a mass delete skipped the delete entirely and recorded it as done, leaving nothing but an INFO line. RPCClient.bdev_get is tri-state; an unknown answer now falls through to the delete rather than skipping it, since the probe was only an optimisation. -19 from the delete itself counts as confirmation. * delete_lvol_from_node now raises PreconditionError for a deferred teardown (skip/queue) and RuntimeError for a failed one, returning normally only once the teardown is confirmed complete -- callers that used to check a truthy/falsy return now catch the exception instead. * process_lvol_delete_finish skipped the leader's sync delete outright when the leader was merely unreachable or restarting, logged a failure and stepped over it, and removed the record regardless -- although that sync delete is the ONLY operation that removes the blob metadata and unregisters the bdev. It is now gated on DONE plus a fresh probe confirming the bdev is absent, and on no live peer still owing its leg. Record-only retirement of landing volumes (empty bdev_stack) short- circuits before the gate: their blob is meant to survive as the converted snapshot. * The in_deletion guard before release_lvol_ns_slot existed on exactly one of nine create/clone rollback sites. rollback_create_record() is now the single owner, and _fail_after_bdev persists the deletion intent BEFORE any teardown RPC, so a rollback whose RPCs raise can no longer leave a record the caller then erases. Two other create/clone rollback sites that called delete_lvol_from_node with LVol/StorageNode objects instead of ids (silently deleting nothing, per the old function's `except KeyError: return True`) now pass ids and route through rollback_create_record too. * delete_lvol's missing-node force branch erased the record without a single RPC and without an event -- for an HA volume the blob lives on every LVS member, whose node records usually still exist. It now tears down on the surviving peers and logs loudly when it cannot confirm. * lvol_events._lvol_event dropped the event when the node record was unreadable, so a volume whose node was gone produced no create, delete or status entry at all. It now falls back to the pool's cluster. Reconciliation: storage_node_ops.auto_repair already compared SPDK's inventory against the database, but only as an operator-invoked CLI command that prints to stdout, so nothing ever ran it and every leak above was permanent and invisible. The blob-level comparison moves to find_orphan_lvstore_blobs(), and lvol_monitor runs a detect-only sweep (LVOL_MONITOR_ORPHAN_CHECK, once a day) that logs and raises a cluster event. It never deletes: a blob it cannot correlate is the one whose ownership it understands least. Two pre-existing bugs in process_lvol_delete_finish, independent of the above and failing CI's integration tier on their own: * post_lvol_delete_rebalance indexed db.get_cluster_capacity(...)[0] unguarded; a delete finishing without pre_lvol_delete_rebalance() having run first computed diff from the epoch and IndexErrored on an empty result. Now guards on an empty records list. * The primary-not-yet-confirmed-absent branch returned before the peer sync-delete loop ran at all, so an already-cleared peer was never recorded in sync_deleted_nodes. The peer loop, its bookkeeping, and the del-sync-lock reset now always run; only the final record removal is gated on the primary's confirmation. Tests: tests/unit/test_lvol_teardown_confirmation.py covers the pure logic (the tri-state probe, _remove_bdev_stack's honest result). tests/ integration/test_lvol_delete_record_gate.py covers the record-lifetime gate, the rollback guard and the orphan sweep against real FDB. Also fixes two mypy findings unrelated to this fix but blocking this PR's type-checker gate: simplyblock_core/services/tasks_runner_node_add.py's module-level `_inflight`/`_inflight_addrs` sets lacked element type annotations, which a newer mypy on this branch now flags. R26.3 lacks several main-only features the original PR's diff assumed as context (the HA replica is_primary sync-delete path in _fail_after_bdev, and rolling back every placed node -- not just the target -- in the migration/fail-over clone path). Those hunks are adapted to R26.3's simpler node model rather than ported wholesale: this backport applies only the teardown-confirmation gate, not those unrelated main-only features. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Hamdy-khader
force-pushed
the
R26.3-fix-lvol-record-teardown-gate
branch
from
September 22, 2026 18:21
5b7b750 to
76565e5
Compare
R26.3's CI runs on Python 3.9 (tox.ini basepython, and the pip/mypy workflow jobs). `def bdev_get(self, name) -> dict | None:` parses fine everywhere but evaluates `dict | None` as a real expression at function- definition time, and `type.__or__` doesn't exist before 3.10 -- TypeError: unsupported operand type(s) for |: 'type' and 'NoneType'. Because rpc_client.py is imported transitively by nearly every module, this broke collection for the type checker (238 files) and for pytest across ~150 unrelated test files that never touch RPCClient directly -- the traceback in each case bottoms out at this one line. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
fix(lvol): never drop the FDB record before the teardown is confirmed (#1380)
R26.3 field report: four lvols existed as bdevs in SPDK with no record in
FDB. One had been created and deleted per the cluster log but never passed
through in_deletion; two showed a create and no delete at all; the fourth
left no cluster-log trace whatsoever.
Every path behind that shares one shape -- the record removal did not
depend on the data-plane teardown having happened:
_remove_bdev_stack returned a constant True. A failed removal was logged
and the entry stamped status='deleted' anyway, so delete_lvol_from_node
reported success and lvol_monitor erased the record over a live bdev.
It now returns False and leaves the entry unmarked so a retry re-attempts
it.
The "already gone?" probe was
if not rpc_client.get_bdevs(name).get_bdevs returns None both for -ENODEV and for a non-200 from the SPDK
proxy, so one transient hiccup during a mass delete skipped the delete
entirely and recorded it as done, leaving nothing but an INFO line.
_bdev_present() is tri-state on get_bdevs_2 (which existed for exactly
this and was never wired in); an unknown answer now falls through to the
delete rather than skipping it, since the probe was only an optimisation.
-19 from the delete itself counts as confirmation.
delete_lvol_from_node answered True for nodes it had decided not to touch
(check_non_leader_for_operation -> "skip"/"queue"). It now returns
NodeTeardown DONE/DEFERRED/FAILED, of which only DONE is truthy, so the
legacy
if not ret:call sites keep working with the stricter meaning.process_lvol_delete_finish skipped the leader's sync delete outright when
the leader was merely unreachable or restarting, logged a failure and
stepped over it, and removed the record regardless -- although that sync
delete is the ONLY operation that removes the blob metadata and
unregisters the bdev. It is now gated on DONE plus a fresh probe
confirming the bdev is absent, and on no live peer still owing its leg
(sync_delete_on_peer's return value was discarded). Anything less keeps
the record in_deletion for the next pass. Record-only retirement of
landing volumes (empty bdev_stack) short-circuits before the gate: their
blob is meant to survive as the converted snapshot.
The in_deletion guard before release_lvol_ns_slot existed on exactly one
of nine create/clone rollback sites. rollback_create_record() is now the
single owner, and _fail_after_bdev persists the deletion intent BEFORE
any teardown RPC, so a rollback whose RPCs raise can no longer leave a
record the caller then erases.
delete_lvol's missing-node force branch erased the record without a
single RPC and without an event -- for an HA volume the blob lives on
every LVS member, whose node records usually still exist. It now tears
down on the surviving peers and logs loudly when it cannot confirm.
lvol_events._lvol_event dropped the event when the node record was
unreadable, so a volume whose node was gone produced no create, delete or
status entry at all. That is why three of the four had no trace. It now
falls back to the pool's cluster.
Reconciliation: storage_node_ops.auto_repair already compared SPDK's
inventory against the database, but only as an operator-invoked CLI command
that prints to stdout, so nothing ever ran it and every leak above was
permanent and invisible (the bdev is re-registered from lvstore metadata on
the next node restart). The blob-level comparison moves to
find_orphan_lvstore_blobs(), and lvol_monitor runs a detect-only sweep
(LVOL_MONITOR_ORPHAN_CHECK, every 30 min) that logs and raises a cluster
event. It never deletes: a blob it cannot correlate is the one whose
ownership it understands least.
Tests: tests/unit/test_lvol_teardown_confirmation.py covers the pure logic
(NodeTeardown truthiness, the tri-state probe, _remove_bdev_stack's honest
result). tests/integration/test_lvol_delete_record_gate.py covers the
record-lifetime gate, the rollback guard and the orphan sweep against real
FDB.
Co-Authored-By: Claude Opus 5 (1M context) noreply@anthropic.com
delete_lvol_from_node now returns a plain bool again: True only for a
confirmed teardown, False for a deferred (skip/queue) or a failed one alike
-- callers can no longer tell those two apart from the return value. Two call
sites that used the DEFERRED/FAILED distinction to avoid raising or
re-triggering leader failover on a merely-deferred teardown lose that
nuance and now treat both as a failure.
Co-Authored-By: Claude Opus 5 (1M context) noreply@anthropic.com
Addresses mxsrc's review comments on PR #1380:
teardown (skip/queue) and RuntimeError for a failed one, returning
normally only once the teardown is confirmed complete. This restores the
deferred-vs-failed distinction lost when NodeTeardown was removed, using
the exception machinery the codebase already has instead of a bespoke
enum, and matches the "raise specific exceptions, never return
None/booleans for errors" convention.
that returns None for "gone" and raises RPCConnectionError/RPCHTTPError/
RPCProtocolError/RPCRemoteError for a genuine failure. _remove_bdev_stack
and lvol_bdev_absent_on_node now use it directly, replacing the
module-local _bdev_present tri-state helper and get_bdevs_2.
volume's pool instead of falling back from the (possibly missing) node
record — the pool reliably exists for the whole LVol lifecycle.
lvol_monitor.py) for the raise-not-return contract, and the affected
unit/integration tests.
Co-Authored-By: Claude Opus 5 (1M context) noreply@anthropic.com
Both were pre-dating this branch (present since the original commit
aeb3361 / the merge-from-main at 243acf9) and failing CI's
integration tier independently of the NodeTeardown work:
post_lvol_delete_rebalance indexed db.get_cluster_capacity(...)[0]
unguarded. lvol_del_start_time defaults to 0.0, so a delete finishing
without pre_lvol_delete_rebalance() having run first (or after a
monitor restart) computes diff from the epoch, requests a capacity
window over that whole span, and IndexErrors on an empty result.
Now guards on an empty records list.
The primary-not-yet-confirmed-absent branch returned before the peer
sync-delete loop ran at all, so a peer already cleared while the
primary catches up was never recorded in sync_deleted_nodes -- the
next pass would re-walk an already-clean peer blob tree. The peer
loop, its bookkeeping, and the del-sync-lock reset now always run;
only the final record removal is gated on the primary's confirmation.
Verified via a full tox integration run (1300 tests) on a dev box with
real Docker/FDB -- both fixed tests pass and nothing else regresses.
Co-Authored-By: Claude Opus 5 (1M context) noreply@anthropic.com
Co-authored-by: wmousa eng.ledoo@gmail.com
Co-authored-by: Claude Opus 5 (1M context) noreply@anthropic.com