Fix/graceful shutdown (port of #1303 to main) - #1305
Open
wmousa wants to merge 2 commits into
Open
Conversation
cluster_grace_shutdown iterated every node record with no status filter at all and force-shut-down each one. shutdown_storage_node drives in_shutdown -> offline, so a node that had been REMOVED came back as a plain offline member. Live 2026-09-03: a single graceful-shutdown resurrected all four nodes removed earlier that day, and the records had to be repaired by hand before the cluster could be activated again. That is not cosmetic. failure_domain_host_map skips only STATUS_REMOVED, so the resurrected records immediately start occupying failure-domain host slots again -- the cluster went from 8 hosts at 2/2/2/2 to 12 at 3/3/3/3 -- and a later activation or startup then acts on nodes whose devices are already failed_and_migrated and which own no lvstore. REMOVED and IN_REMOVAL are now skipped; IN_REMOVAL because node_removal_orchestrate has already shut that node down and owns the rest of its lifecycle. PENDING_REMOVAL deliberately is not skipped: the node is still up and serving then, so a full-cluster shutdown must stop it like any other member. The test asserts on which nodes were swept rather than on a return value -- the function returns None, so behaviour is only observable through the calls it makes. Verified by mutation: emptying the status tuple fails two of the three cases.
…starts auto_restart_disabled is enforced at ENQUEUE time only (tasks_controller.add_node_to_auto_restart, "the single chokepoint for every auto-restart queue path"); tasks_runner_restart never consults it. So an FN_NODE_RESTART row queued BEFORE the flag was set survives, executes unconditionally, and its ONLINE transition clears the flag again -- the deliberate-stop intent destroyed by the very task it was meant to prevent. Live 2026-09-03: a cluster graceful-shutdown returned successfully having left s7457 and zdgtb ONLINE, because two such rows fired seconds after the sweep passed them. Both had to be shut down by hand. shutdown_storage_node now reaps those rows where it commits the intent, mirroring what set_node_status already does on the opposite transition (ONLINE cancels obsolete restart rows). This also gives the runner its dequeue-side check for free -- it already honours task.canceled -- with no new field, and without breaking ensure_node_restart_task, which deliberately bypasses the flag because an explicit `sn restart` is the operator intervention the flag waits for. A restart queued after that point is exactly that and is left alone. cancel_pending_node_restart_tasks gains exclude_task_id, and that exclusion is load-bearing rather than defensive: the restart runner drives shutdown_storage_node as its own kill step (tasks_runner_restart.py:542, passing current_restart_task_id), so a blanket cancel would abort the very restart doing the shutting down -- turning a fix for a rare race into a failure on every node restart. The suspend-recovery path is unaffected; it passes keep_auto_restart=True and never reaches this block. cluster_grace_shutdown additionally verifies its own end state instead of assuming it. The sweep is serial, so a node it already passed can come back behind it; stragglers get one more shutdown, and anything still not offline is logged by name. It does not raise -- an operator following up needs the list, not a traceback. Mutation-verified: dropping the exclusion fails the own-task test, disabling the settle pass fails the comes-back test.
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.
Port of #1303 (merged to
R26.3asab1b9d351) ontomain.Both commits cherry-picked cleanly; the applied content is byte-identical to what merged into R26.3 (verified by diffing the two patches).
1. graceful-shutdown must not resurrect removed nodes
cluster_grace_shutdowniterated every node record with no status filter and force-shut-down each one.shutdown_storage_nodedrivesin_shutdown -> offline, so a node that had beenREMOVEDcame back as a plain offline member.Live 2026-09-03: a single graceful-shutdown resurrected all four nodes removed earlier that day, and the records had to be repaired by hand before the cluster could be activated again. Not cosmetic —
failure_domain_host_mapskips onlySTATUS_REMOVED, so the resurrected records immediately reoccupy FD host slots (the cluster went from 8 hosts at 2/2/2/2 to 12 at 3/3/3/3), and a later activation acts on nodes whose devices are alreadyfailed_and_migrated.REMOVEDandIN_REMOVALare now skipped.PENDING_REMOVALdeliberately is not: the node is still up and serving, so a full-cluster shutdown must stop it like any other member.2. graceful-shutdown must not lose the race with queued restarts
auto_restart_disabledis enforced at enqueue time only;tasks_runner_restartnever consults it. A restart row queued before the flag was set survives, executes, and its ONLINE transition clears the flag — the deliberate-stop intent destroyed by the task it was meant to prevent. Live: the command returned successfully having left two nodes ONLINE.shutdown_storage_nodenow reaps those rows where it commits the intent, mirroring whatset_node_statusalready does on the ONLINE transition. That also gives the runner a dequeue-side check for free (it already honourstask.canceled), with no new model field.cancel_pending_node_restart_tasksgainsexclude_task_id, and it is load-bearing, not defensive: the restart runner drivesshutdown_storage_nodeas its own kill step (tasks_runner_restart.py:542, passingcurrent_restart_task_id), so a blanket cancel would abort the very restart doing the shutting down — turning a fix for a rare race into a failure on every node restart. The suspend-recovery path is unaffected (keep_auto_restart=True, never reaches the block).cluster_grace_shutdownadditionally verifies its own end state: stragglers get one more shutdown, anything still not offline is logged by name. It does not raise — an operator following up needs the list, not a traceback.Verification on this branch
test_service_entrypoints.py(tasks-runner-lvol-migration,tasks-runner-restart) are pre-existing on cleanorigin/main— reproduced identically on an untouched worktree, unrelated to these commits🤖 Generated with Claude Code