feat(node-removal): online node removal as inverse of cluster expansion#1104
Open
schmidt-scaled wants to merge 37 commits into
Open
feat(node-removal): online node removal as inverse of cluster expansion#1104schmidt-scaled wants to merge 37 commits into
schmidt-scaled wants to merge 37 commits into
Conversation
Add a background, task-driven node-removal orchestration that mirrors
add_node/cluster expansion in reverse. `sn remove` now validates and
queues an FN_NODE_REMOVAL task; tasks_runner_node_removal drives the
idempotent, resumable flow:
shutdown -> in_removal -> rewire LVS replicas -> remove/fail/migrate
devices -> removed
Preconditions (enforced before queueing): target ONLINE, every other
non-removed node ONLINE, FTT headroom OK, no LVols/snapshots on the node
(operator migrates those separately), and every secondary/tertiary
replica the node hosts for other primaries has a host-disjoint
relocation target.
LVS rewiring:
* Case A - the node's own primary LVS: tear down its (empty)
secondary/tertiary replicas on the peers that host them.
* Case B - replicas this node hosts for other primaries: re-host each
on a fresh, anti-affinity-valid node (get_secondary_nodes /
get_secondary_nodes_2 + recreate_lvstore_on_non_leader) so those
primaries keep their fault tolerance.
Devices are driven online -> removed -> failed (queuing failure
migration on the surviving online nodes) and the flow waits for
failed_and_migrated before flipping the node to removed.
Changes:
* base_model: new STATUS_IN_REMOVAL (code 13)
* job_schedule: new FN_NODE_REMOVAL
* tasks_controller: add_node_removal_task + dedup + getter; skip
IN_REMOVAL nodes when fanning out device-migration tasks
* storage_node_ops: rewrite remove_storage_node + node_removal_orchestrate
and Case A/B + device helpers
* services/tasks_runner_node_removal.py: new runner service
* docker-compose-swarm.yml: register TasksNodeRemovalRunner
* tests/test_node_removal.py: 21 unit tests
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
mxsrc
requested changes
Jun 17, 2026
mxsrc
left a comment
mxsrc
left a comment
Contributor
There was a problem hiding this comment.
The new functions that are being introduced use return-value based error reporting, since they are new I feel they should use exceptions for the error handling. Other than that I think the code looks good, I just have a few comments on two locations. It requires changes in the helm-chart to add the container there as well.
| return cands[0] if cands else None | ||
|
|
||
|
|
||
| def node_removal_orchestrate(node_id, force_remove=False): |
Contributor
There was a problem hiding this comment.
I think this should be moved to the task module, including its helpers. The tasks should be as self-contained as possible, this code should not be executed from anywhere else.
# Conflicts: # simplyblock_cli/cli.py # simplyblock_core/controllers/tasks_controller.py # simplyblock_core/scripts/docker-compose-swarm.yml # simplyblock_core/storage_node_ops.py
| try: | ||
| sec = db_controller.get_storage_node_by_id(primary.secondary_node_id) | ||
| exclude_mgmt_ips.append(sec.mgmt_ip) | ||
| except KeyError: |
…node removal process
…d improve JM reassignment logic in node removal process
Hamdy-khader
approved these changes
Jun 30, 2026
…vol allocation status instead of being deprecated placeholders
# Conflicts: # simplyblock_core/storage_node_ops.py
…and persist state in DB
…hecks and consolidating logic
Activation of a fresh cluster died mid-flight with "RuntimeError: Node
not found" from patch_cr_node_status and left the cluster permanently
in_activation, since the activation thread had no failure handling.
Root cause: patch_cr_node_status replaces the CR's whole status.nodes
list via read-modify-write with no concurrency control. During
activation, many writers (node registration, monitor health patches,
port events) patch the same list concurrently, so a reader can observe
a list momentarily missing a node and a writer can silently drop a
concurrent update.
- patch_cr_node_status: retry when the node is not (yet) present, send
resourceVersion with the status patch so conflicting writes 409 and
retry on fresh data instead of clobbering each other, and log+return
False instead of raising - CR status mirroring must never crash the
caller
- cluster_activate: wrap the implementation so any unhandled failure
reverts in_activation to the prior status before re-raising, keeping
the cluster retryable
- create_lvstore call in activation: add the same error handling as the
sibling recreate_lvstore path
Cluster activation and node restarts could both wedge permanently when
the driving process died or raced concurrent writers:
- cluster_activate died on a transient "Node not found" from
patch_cr_node_status and left the cluster stuck in in_activation.
The CR status.nodes list is rewritten wholesale by concurrent
writers, so reads can transiently miss a node and writes can drop
concurrent updates.
- A node whose restart was interrupted (tasks-runner pod evicted while
its host drains, node crash) stayed orphaned in RESTARTING with no
task and no owner. The k8s operator holds its drain slot until the
node is online, deadlocking MachineConfig rollouts cluster-wide
(observed 2026-07-04: every MCO reboot froze the rollout until
manual intervention).
Changes:
- patch_cr_node_status: retry on transient not-found, compare-and-swap
via resourceVersion (409 -> reread and retry), log-and-return instead
of raising - CR status mirroring must never crash business logic
- cluster_activate: wrap the implementation so any unhandled failure
reverts in_activation to the prior status, keeping retry possible
- create_lvstore call in activation: same error handling as the
sibling recreate_lvstore path
- restart_storage_node: every restart now ensures a persistent
NODE_RESTART task, claims its lease, and heartbeats it (30s) while
driving the restart; TASK_LEASE_TTL_SEC drops 1200->180 so a live
tasks-runner takes over a dead driver's restart within ~3 minutes
- tasks_runner_restart: watchdog that detects nodes stuck in
RESTARTING/IN_SHUTDOWN with no owning task, verifies SPDK is dead
(_reset_if_transient), resets to OFFLINE and queues an auto-restart
mxsrc
requested changes
Jul 7, 2026
mxsrc
left a comment
mxsrc
left a comment
Contributor
There was a problem hiding this comment.
Looks largely good, I think there's some friction with merging to main that should be addressed first.
| @@ -0,0 +1,405 @@ | |||
| # coding=utf-8 | |||
Contributor
There was a problem hiding this comment.
This file needs to be sorted into either tests/unit or tests/integration, otherwise it's silently ignored. It looks to me more like an integration test, in that case instead of mocking the DB, it should rely on the one provided by the integration tests. You can check recent changes to the other integration tests for how this may look.
| SIMPLY_BLOCK_VERSION=19.2.34 | ||
|
|
||
| SIMPLY_BLOCK_DOCKER_IMAGE=public.ecr.aws/simply-block/simplyblock:main | ||
| SIMPLY_BLOCK_DOCKER_IMAGE=public.ecr.aws/simply-block/simplyblock:feature-node-removal |
Contributor
There was a problem hiding this comment.
This must not be merged to main.
|
|
||
| def _run(): | ||
| try: | ||
|
|
| environment: | ||
| SIMPLYBLOCK_LOG_LEVEL: "$LOG_LEVEL" | ||
|
|
||
| TasksNodeRemovalRunner: |
Contributor
There was a problem hiding this comment.
We should rename this to TasksNodeRemoveRunner for consistency.
| FN_FAILED_DEV_MIG = "failed_device_migration" | ||
| FN_NEW_DEV_MIG = "new_device_migration" | ||
| FN_NODE_ADD = "node_add" | ||
| FN_NODE_REMOVAL = "node_removal" |
Contributor
There was a problem hiding this comment.
We should rename this to FN_NODE_REMOVE = "node_remove" for consistency.
Comment on lines
+108
to
+110
| # When attaching this JM to a node, override the device name on that node. | ||
| # This is needed when a JM device is removed and needed to be replaced, but the name must be the same. | ||
| override_name_on_node: dict[str, str] = {} # node_id: new_name |
Contributor
There was a problem hiding this comment.
Ideally we'd provide a single helper JMDevice.controller_name(node_id: UUID) -> str that is used to centrally determine the controller name and is called from storage_node_ops. The derivation logic is duplicated and prone to be missed in future changes.
# Conflicts: # simplyblock_core/storage_node_ops.py
# Conflicts: # simplyblock_core/cluster_ops.py # simplyblock_core/services/tasks_runner_restart.py
| # holds no task and looks ownerless to this check) may be driving it, and | ||
| # resetting under it would kill the SPDK it just started — so wait long | ||
| # enough for any legitimate restart to finish. | ||
| _transitional_first_seen: dict = {} |
Updated health check conditions to account for nodes with suspended status, ensuring correct health reporting.
… typo - Added `STATUS_SUSPENDED` to `report_health` condition for accurate health reporting. - Fixed typo in `set_node_health_check` function call.
… typo - Added `STATUS_SUSPENDED` to `report_health` condition for accurate health reporting. - Fixed typo in `set_node_health_check` function call.
- Added `STATUS_SUSPENDED` to permissible node statuses in connection logic and health checks.
# Conflicts: # simplyblock_core/storage_node_ops.py
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
5 participants
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.
Summary
Implements an online storage-node removal function that works as the inverse of cluster expansion (
add_node). It is background, task-driven, idempotent and resumable —sn removevalidates preconditions then queues anFN_NODE_REMOVALtask; the newtasks_runner_node_removalservice drives the flow:This replaces the old
remove_storage_nodesemantics (which only handled an already-offline node and did no LVS rewiring).Behavior
Preconditions (enforced before anything is queued):
online;online;_check_ftt_allows_node_removal);LVS rewiring:
get_secondary_nodes/get_secondary_nodes_2+recreate_lvstore_on_non_leader) so those primaries keep their fault tolerance. Bookkeeping back-ref on the removed node is cleared only after a successful rebuild, so retries resume cleanly.Devices: each data device is driven
online → removed → failed(queuing failure-migration on the surviving online nodes), then the flow waits forfailed_and_migratedbefore flipping the node toremoved.Changes
models/base_model.py: newSTATUS_IN_REMOVAL(code 13)models/job_schedule.py: newFN_NODE_REMOVALcontrollers/tasks_controller.py:add_node_removal_task+ dedup branch +get_active_node_removal_task; skipIN_REMOVALnodes when fanning out device-migration tasks (their SPDK is dead)storage_node_ops.py: rewriteremove_storage_node(validate + queue) +node_removal_orchestrate+ Case A/B + device-decommission/finalize helpersservices/tasks_runner_node_removal.py: new runner service (lease-aware, suspend-and-revisit for the long migration wait)scripts/docker-compose-swarm.yml: registerTasksNodeRemovalRunnersimplyblock_cli/cli.py: updatedsn removehelp;--force-removenow only cancels active taskstests/test_node_removal.py: 21 unit testsTesting
tests/test_node_removal.py— 21 tests (preconditions, relocation feasibility/picking, Case A/B bookkeeping incl. idempotency/resume, device completion gate, status mapping). ✅Notes / review focus
recreate_lvstore_on_non_leaderwith the primary as the online leader. Worth a careful look from someone close to the LVS restart/recreate machinery.🤖 Generated with Claude Code