Fix stuck activation rebased 1 by wmousa · Pull Request #1179 · simplyblock/sbcli

wmousa · 2026-07-16T13:01:44Z

No description provided.

add_device_mig_task_for_node fell through to an implicit None return when no sub-tasks were created, inconsistent with its explicit True/False contract elsewhere. run_lvol_sync_ops fallback except silently swallowed a failure in _defer with no trace. Both flagged by github-code-quality[bot] on PR #1179.

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

The /shutdown endpoint answered 202 and evaluated its safety guards (active migration tasks, active restart task, concurrent peer restart/shutdown, node state) only in the fire-and-forget background thread. A refusal was invisible to the caller: during a MachineConfig rollout the k8s operator took the 202 as "shutdown in progress" and switched to polling for the node to go offline - forever. The blocking migrations finished minutes later, but nothing re-issued the shutdown, stalling the node drain until manual intervention (2026-07-06). Extract the guards into check_node_shutdown_preconditions(), a read-only validator (force downgrades refusals to warnings, matching the previous behavior), call it synchronously in the endpoint and return 409 with the refusal reason. shutdown_storage_node performs the same validation through the shared helper, so CLI and other callers keep the same protection. A 409 also makes the operator re-issue the shutdown on each reconcile instead of entering its poll-forever state, so drains now proceed automatically once the blocking condition clears.

…idator

TASK_LEASE_TTL_SEC was reduced to 180s for fast ownership transfer, but runners only refreshed a lease on task writes. Long-blocking work (add_node can run for many minutes without touching its task) let a live owner's lease go stale mid-execution: a second runner host - e.g. the new pod during a rolling update - would claim the task and double-drive it. For node-add that is destructive: the re-entrant cleanup kills the in-flight add's SPDK and deletes its half-created node record. Add tasks_controller.task_lease_heartbeat(), a context manager that refreshes this host's lease every TASK_LEASE_HEARTBEAT_SEC and stops itself if the lease is lost, and wrap task execution at every claim site (node_add, restart, migration, lvol_migration, port_allow, cluster_expand). Leases are per task, so concurrent adds of different nodes remain fully parallel; takeover now occurs only on actual owner death.

Applying the CPU topology during add_node reboots the node, and two independent defects turned that expected reboot into failed or stalled adds (2026-07-06, workers 0/4): - load_kernel_module wrote its modules-load.d persistence into the storage-node agent container's ephemeral filesystem, where the host's systemd-modules-load never sees it. vfio-pci/uio_pci_generic were therefore gone after the reboot and bind_device_to_spdk answered 500 on every retry. Write through the /host/etc/modules-load.d hostPath mount when present (added to the CSI chart's storage-node daemonset), falling back to the local path for docker/bare-metal deployments. - The node-add runner was blind to the reboot it triggered: clean failed attempts retried after a flat 10s, burning max_retry against a node that is down for 5-8 minutes, while excepting attempts grew the exponential backoff so the runner kept sleeping long after the node was back. After a failed attempt whose node agent address is unreachable, wait for it to answer again (poll 15s, bounded by NODE_REBOOT_WAIT_MAX_SEC=900, matching the topology job's reboot budget) without consuming retries, then retry promptly with the backoff reset. An existing add task declares the node is meant to join, so waiting out unreachability is the correct convergence behavior. Requires the matching CSI chart change mounting the host's /etc/modules-load.d at /host/etc/modules-load.d in the agent.

When CPU topology is enabled, adding a node reboots it � killing the agent that serves spdk_process_start and, if the tasks-runner pod is co-located on that storage node, the pod driving the add too (taking every parallel add with it). The reboot is unavoidable, so recovery must be clean. Two gaps prevented that: - add_node's stale-record cleanup only matched on SSD-PCIe overlap, so an attempt interrupted before SSD assignment left an orphaned IN_CREATION StorageNode. The lease-based retry then built a DUPLICATE node and the total_mem loop double-counted the orphan's hugepages. Add an idempotent re-entry cleanup keyed on api_endpoint+socket that kills any half-started SPDK and drops the stale record before the hugepage/mesh work runs. - The guaranteed topology reboot makes spdk_process_start fail and add_node return False, which process_task counted as a consumed retry. The runner now detects (via agent reachability) that the failure was a reboot and rolls the retry back, so the expected reboot never erodes the max_retry budget; it then waits for the agent to return and retries promptly. Combined with the existing task-lease takeover, an add interrupted by any reboot � including the co-located-pod case � resumes and converges automatically with no duplicate nodes and no wasted retries.

…ceful path check_node_shutdown_preconditions can only refuse a graceful shutdown; with force=True every guard downgrades to a warning and returns allowed, so calling it unconditionally was dead work that also broke the mocked endpoint test (MagicMock unpacks to 0 values). Move it inside the existing `if not force` block. fix(restart): reuse pre-status node read instead of re-fetching The transferable-ownership block re-read the node via get_storage_node_by_id immediately after the pre_status read � a wasted FDB round-trip that also consumed an extra read the restart-wrapper's tests count against its contract, breaking the orphan-RESTARTING cleanup path. Reuse the already-read node.

…to FTT on activate Deleting/creating the storage MachineConfigPool with a hardcoded maxUnavailable=1 serialized every first-time CPU-topology reboot pool-wide: with N nodes, a node added early could sit behind all the others' reboots before its own, so node-add's wait-for-reboot was unbounded and unpredictable. No tuning of the job's wait fixes this � the delay lives in MCO's serialized rollout, not the job. Make the reboot concurrency phase-aware instead: - During node-add (pre-activation, nodes carry no data) the MCP is created with maxUnavailable = PARALLEL_ADD_NUMBER, so the first-time topology reboots roll in parallel waves instead of a one-at-a-time queue. - At the end of cluster_activate (success path, before flipping to ACTIVE) the pool is narrowed to cluster.max_fault_tolerance, so any later MachineConfig/KubeletConfig rollout never reboots more storage nodes at once than the data plane can absorb. Post-activation expansion keeps this value (later adds only append to the nodeSelector), so it stays safe on a live cluster. Changes: - utils.set_storage_mcp_max_unavailable(): patch spec.maxUnavailable on the storage-<first6> MCP; floors at 1 (0 wedges the pool), OpenShift-only (no-ops on k3s / when the pool is absent), never raises. - cluster_ops._cluster_activate(): narrow to max_fault_tolerance before ACTIVE. - oc_storage_cpu_topology.yaml.j2: create MCP with maxUnavailable={{ MCP_MAX_UNAVAILABLE }}. - kubernetes.py: thread MCP_MAX_UNAVAILABLE from env PARALLEL_ADD_NUMBER (default 1). Default PARALLEL_ADD_NUMBER=1 preserves current one-at-a-time behavior; set it to the parallel-add count to enable parallel bring-up. Use max_fault_tolerance-1 for the steady-state value if you want headroom for an unplanned failure concurrent with a rollout.

…arallelNodeAdds During initial node-add the CPU-topology MachineConfigPool was created with a hardcoded maxUnavailable=1, so first-time kubelet/topology reboots rolled one node at a time � a node added early sat behind every other node's reboot. Pace those reboots by the configured parallel-add count instead, then narrow the pool once the cluster serves IO. The parallel-add count is the operator-facing StorageNodeSet CRD field spec.maxParallelNodeAdds. Read it straight from the CR on the control-plane side (add_node, which already has cr_name/cr_namespace/cr_plural and broad k8s RBAC) and thread the resolved value to the node agent, which stamps it onto the MCP at creation time. No operator/deployment change required. - utils.get_max_parallel_node_adds_from_cr(): read spec.maxParallelNodeAdds from the StorageNodeSet CR; returns None on 404 / non-OpenShift / unset field so callers fall back; never raises. - add_node: read the CR value and pass mcp_max_unavailable through snode_client.spdk_process_start to the agent. - agent (kubernetes.py): new SPDKParams.mcp_max_unavailable; MCP maxUnavailable source order = CR value -> MAX_PARALLEL_NODE_ADDS env -> NODE_ADD_MAX_PARALLEL. - oc_storage_cpu_topology.yaml.j2: MCP maxUnavailable now templated ({{ MCP_MAX_UNAVAILABLE }}) instead of hardcoded 1. - cluster_activate: restore MCP maxUnavailable to the cluster's fault tolerance (max_fault_tolerance) once the cluster is active, for safe steady-state rollouts. - constants.NODE_ADD_MAX_PARALLEL: single default, shared by the node-add runner cap and the agent fallback. - docker.py SPDKParams: accept and ignore the field (no MCP off OpenShift).

…e-adds Two robustness fixes for the OpenShift CPU-topology node-add flow. 1) Start SPDK only after the topology reboot has actually landed on the node Previously the agent waited only for the node to be uncordoned, which is true immediately because MCO cordons/reboots asynchronously (queued by the MCP maxUnavailable). SPDK therefore came up before cpuManagerPolicy=static was in effect (unpinned) and was then killed by the reboot. The agent now waits until the node has converged on the storage MCP's config (state=Done, uncordoned) and only starts SPDK after that. Convergence is compared by rendered-config CONTENT HASH (rendered-<pool>-<hash>), not the full name, so a node still under an older pool with byte-identical config (e.g. a previous cluster id) passes immediately instead of hanging. Missing MCP is handled (404 -> degrade to uncordon wait, no crash); non-OpenShift / non-topology keeps the original behavior. Adds get_k8s_custom_objects_client. The topology Job now ensures MCP/MC/KubeletConfig membership on EVERY run instead of skipping it when the marker file exists, so a stale marker can no longer leave a node off the pool config. The marker now only decides whether to wait for the first reboot or just confirm NUMA-plugin readiness. 2) Idempotent node-add: never build a duplicate storage node add_node already cleaned up a prior attempt stuck IN_CREATION, but once that attempt went ONLINE a redundant re-add (e.g. the operator re-posting after its pod was recreated by a topology reboot) created a SECOND record with new ports that could never come up and stranded IN_CREATION (observed on worker-1, 2026-07-10). add_node now aborts as an idempotent no-op when a healthy node already occupies the slot. The slot key is (api_endpoint, socket, cpu_mask) so nodesPerSocket>1 (multiple nodes per host/socket, distinguished only by CPU core group) is preserved and co-socket siblings are never mistaken for duplicates � this also fixes a latent bug where the old (api_endpoint, socket) cleanup could kill a legitimate co-socket sibling.

…remove) Re-creating a cluster mints a new UUID, so the topology job creates a new storage-<newid> MachineConfigPool while the previous cluster's storage-<oldid> pool still lists the same workers. A node then matches two custom pools and MCO refuses it � "node ... belongs to 2 custom roles, cannot proceed" � freezing its rollout (observed on ocp.simplyblock.ai: storage-d52a10 + storage-13c525 both claiming all 6 workers). After adding the node to its own pool, the job now removes that node's hostname from every other storage-* pool's nodeSelector. Add-before-remove is deliberate: the node is never in zero custom pools, so it never falls back to the worker pool and never takes an extra reboot. Only nodeSelectors are edited � the old pool and its rendered config are left intact � so the node simply moves between identical-content renders (rendered-storage-<oldid> -> rendered-storage-<newid>, same digest) with no reboot and no orphaned-render wedge; the stale pool shrinks to empty on its own. Removal runs in a retry loop so the small parallel-add fan-out (sibling jobs patching the same stale pool) converges under last-write-wins. Pool naming stays cluster-id-based, so all name-based lookups (set_storage_mcp_max_unavailable, the reboot-gate, cluster_activate narrow) keep resolving the current cluster's pool, which now solely owns the nodes.

…eep on fail) Stale in_creation cleanup during add_node killed the SPDK pod best-effort and removed the storage-node record regardless. When the kill failed (agent briefly unreachable during the CPU-topology reboot), the pod was left running with no record � an orphan holding the node's CPU/hugepages, so the retry's SPDK pod stayed Pending and the node was stuck in_creation (worker-3, 2026-07-13). Both add-time cleanup sites now retry spdk_process_kill and only drop the record (snode_delete + remove) once the kill is confirmed; if it never succeeds they keep the record and fail the add for retry. This preserves the invariant that a running SPDK pod always has a DB record, so orphans can't form and the duplicate-guard (which locates pods via the record's rpc_port) can always clean the pair. Naturally nodesPerSocket>1-safe: a live node always keeps its record.

…ing a pool The add-before-remove migration removed nodes from the old storage MCP's nodeSelector one by one. Removing the last node patched values to [], and an In operator with empty values is an invalid label selector ("values set can't be empty") � MCO's node controller then fails cluster-wide and marks every node Degraded, freezing any node still migrating off the old pool's render (all 6 workers degraded, worker-5 stuck on the old render; da6996/cca232, 2026-07-13). When removing a pool's last node, retire it with a sentinel value that matches no host instead of emptying the list. The pool becomes inert (0 nodes) but stays a valid object, so nodes on its identical-content render migrate to their new pool cleanly.

… fix lease-task lookup Two defects in the transferable-restart-ownership mechanism (6482c7a), both observed in the 2026-07-17 single_node_outage_ha run: 1. Self-collision: restart_storage_node creates/claims a NODE_RESTART task as its resume-on-driver-death token, then _restart_storage_node_impl active-task guard found that very task and returned False - every manual "sn restart" failed inline ("Restart task found: <own task>") and degraded into waiting ~10s for the task runner (which passes only because it calls with force=True). Thread the claimed task id into the impl and exempt exactly that task; a task whose lease is held by another live host still blocks. 2. ensure_node_restart_task returned JobSchedule.get_id() (the composite cluster/date/uuid FDB key) for an existing task, which get_task_by_id (bare-uuid lookup) cannot resolve - KeyError "Task .../.../... not found", so claim/heartbeat setup failed and the lease was never engaged on re-entry. Return task.uuid.

add_device_mig_task_for_node fell through to an implicit None return when no sub-tasks were created, inconsistent with its explicit True/False contract elsewhere. run_lvol_sync_ops fallback except silently swallowed a failure in _defer with no trace. Both flagged by github-code-quality[bot] on PR #1179.

…rting the whole host add_node() loops over every config slot generated for a host (nodes_per_socket * sockets_to_use, e.g. 4 for 2 sockets x 2 nodes each), but its two "this slot already has a healthy node" checks returned "Success" immediately instead of continuing to the next slot. The very first slot found already-provisioned made the function report the entire host done and quit without ever looking at the remaining slots. Confirmed via logs on a fresh redeploy: a call for one host matched its first config entry (socket 0) as already-online 11ms in and returned, never touching the other three slots (two more on socket 0, two on socket 1). Repeated retries kept re-confirming the same first slot, producing multiple competing SPDK pods all requesting numa-align/numa-0 while numa-1 stayed empty - oversubscribing socket 0s CPU cores and tripping kubelet TopologyAffinityError on the pod that finally hit the limit. Both "already added" branches now continue to the next node_config entry; the loops existing final return (already correctly placed outside the loop) becomes the single true completion point once every slot has been checked.

…race Two node_add tasks could end up targeting the same host (e.g. a caller retrying its HTTP request), and the task-runner concurrency guard only deduped by task uuid, not target host - its concurrency model assumed different tasks always target different nodes with no shared state, which stopped holding once a host can have multiple storage-node slots (nodesPerSocket > 1). Both tasks got dispatched to separate worker threads and ran add_node() milliseconds apart, racing the same hosts config-slot classify-then-create logic and producing more storage-node records than the host has slots for (2026-07-23: 6 records for a 4-slot host). Two-layer fix: - tasks_controller._add_task now dedups FN_NODE_ADD by function_params[node_addr], refusing to create a second task while one is already pending/running for that host (matches the existing pattern for FN_NODE_RESTART and the migration task types). - tasks_runner_node_add tracks in-flight node_addr values alongside in-flight task uuids, so even a duplicate task that already exists cannot be dispatched to a second worker while another is driving the same host.

add_device_mig_task_for_node fell through to an implicit None return when no sub-tasks were created, inconsistent with its explicit True/False contract elsewhere. run_lvol_sync_ops fallback except silently swallowed a failure in _defer with no trace. Both flagged by github-code-quality[bot] on PR #1179.

wmousa force-pushed the fix-stuck-activation-rebased-1 branch 4 times, most recently from 4f781d6 to 0e4af90 Compare July 20, 2026 08:17

github-code-quality Bot found potential problems Jul 20, 2026

View reviewed changes

Comment thread simplyblock_core/controllers/tasks_controller.py Fixed

Comment thread simplyblock_core/controllers/tasks_controller.py Fixed

wmousa force-pushed the fix-stuck-activation-rebased-1 branch 3 times, most recently from 392fb87 to a37f157 Compare July 22, 2026 08:25

wmousa marked this pull request as ready for review July 22, 2026 08:57

wmousa force-pushed the fix-stuck-activation-rebased-1 branch from 3cd84a7 to d1a7a73 Compare July 23, 2026 15:02

wmousa force-pushed the fix-stuck-activation-rebased-1 branch from d1a7a73 to 26e9fa8 Compare July 24, 2026 07:46

wmousa force-pushed the fix-stuck-activation-rebased-1 branch from 26e9fa8 to 62001a1 Compare July 24, 2026 09:17

wmousa and others added 14 commits July 24, 2026 13:01

Add migration tasks before node status is set to Online

fd57e2a

Fix: Instantiate db_controller = DBController() at the top of the val…

dde1f3c

…idator

wmousa added 5 commits July 24, 2026 13:02

wmousa force-pushed the fix-stuck-activation-rebased-1 branch from da4a961 to 866db2d Compare July 24, 2026 11:02

Hamdy-khader approved these changes Jul 24, 2026

View reviewed changes

wmousa merged commit 7f48ce6 into main Jul 24, 2026
8 of 11 checks passed

wmousa deleted the fix-stuck-activation-rebased-1 branch July 24, 2026 11:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix stuck activation rebased 1#1179

Fix stuck activation rebased 1#1179
wmousa merged 19 commits into
mainfrom
fix-stuck-activation-rebased-1

wmousa commented Jul 16, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

wmousa commented Jul 16, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants