fix: prevent cluster activation from wedging in in_activation#1166
Open
wmousa wants to merge 10 commits into
Open
fix: prevent cluster activation from wedging in in_activation#1166wmousa wants to merge 10 commits into
wmousa wants to merge 10 commits into
Conversation
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
| # 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 = {} |
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.
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.
wmousa
force-pushed
the
main-fix-stuck-activation
branch
from
48b825b to
79d6da8
Compare
…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.
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
2 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.
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.