fix(k8s): the three faults that blocked node_add on a fresh install by noctarius · Pull Request #1378 · simplyblock/sbcli

noctarius · 2026-09-21T05:46:49Z

Three faults on the control plane's Kubernetes path, each of which made a
node add fail while reporting something other than its own cause. They were
found together on a fresh OpenShift install and are independent of one another.

1. The storage CRs were read at v1alpha1 (`ed14e78c7`)

The operator's CRD redesign made v1alpha2 the stored version and the control
plane kept asking for v1alpha1. A v1alpha1 read of a v1alpha2-stored object
needs the conversion webhook, and a fresh install deliberately runs none —
everything is written at the storage version, so nothing needs converting.

conversion webhook for storage.simplyblock.io/v1alpha2, Kind=StorageCluster
failed: ... service "simplyblock-operator-conversion-webhook-service" not found

patch_cr_node_status returns False when its read fails and the caller turns
that into Node add result: False, so on a cluster whose operator and CRDs were
both correct every node_add failed, retried eleven times, and gave up.

2. A failed `sn configure` exited 0 (`6ccbed944`)

generate_automated_deployment_config reports failure by returning
(False, False) — no device matched the filters, the sockets did not validate,
the memory did not add up. main discarded that return, so the process exited 0.

Its caller is an init container, so exiting 0 told Kubernetes the node was
configured and the pod started anyway — on whatever sn_config_file the host
already had, because a generation that fails writes nothing. On a cluster
reinstalled over an earlier deployment that file was the earlier deployment's,
naming lblk devices, and the node_add that read it was refused for a device
class nobody had asked for rather than for the configure that never ran twenty
minutes earlier.

The result is now checked and a failure exits 1, and the stale file is discarded
before the generation that replaces it — after the pod-present check, whose file
belongs to a running pod, and before the generation whose failure is the case
this exists for. A missing file is a first install, not an error.

3. A deduplicated `node_add` answered 500 (`9ba41b36d`)

ValueError: Failed to create add-node task
→ the control plane answered 500: Internal Server Error

_validate_new_task_node_add is load-bearing and stays: without it a retried
post creates a second independent FN_NODE_ADD for one host, both are
dispatched, and two threads race that host's config-slot logic (2026-07-23, six
nodes created for a four-slot host).

What it reported was the problem. _add_task answers a duplicate with False
and the v2 endpoint raises ValueError on falsy, so the guard working exactly as
designed reached the caller as a server error. The operator re-posts an add
whenever the task window it polls comes back empty, which happens whenever the
control plane cannot be read for a moment — seventeen 500s in two minutes on
worker-5, all for a host whose add was already queued and running.

A task that already exists is the answer to "add this host", so it is returned
instead, which is what ensure_node_restart_task in the same module already does
with its own repeat. The other _add_task callers keep answering False for
their own duplicates; whether False is right anywhere is a wider question than
this fault.

Testing

Three unit-test files, each written before its fix and shown red against the
unfixed tree: tests/unit/test_storage_cluster_cr_version.py,
test_node_configure_failure_is_fatal.py, test_add_node_task_is_idempotent.py.

pytest tests/unit after rebasing onto current main: 2659 passed, 1 failed.
The failure is test_spdk_proxy_unit.py::TestMetricsEndpoint::test_metrics_are_served_to_an_authorized_caller,
which fails identically on origin/main with this branch absent — pre-existing,
not from these commits.

Not verified on a live cluster yet; that needs the operator image rebuilt and a
redeploy.

Not addressed

Co-tenancy — two storage nodes of different clusters on one worker sharing
/etc/simplyblock/sn_config_file — is left as a TODO in node_configure.py. The
discard in (2) makes a redeployment safe against its own stale file; it does not
make two concurrent tenants safe against each other.

🤖 Generated with Claude Code

The operator's CRD redesign made v1alpha2 the stored version, and the control plane kept asking for v1alpha1. A v1alpha1 request against a v1alpha2-stored object needs the conversion webhook, and a fresh install deliberately runs none: everything is written at the storage version, so nothing needs converting. The two only collide when something asks for the old version, and then every such read fails. conversion webhook for storage.simplyblock.io/v1alpha2, Kind=StorageCluster failed: ... service "simplyblock-operator-conversion-webhook-service" not found That is not cosmetic. patch_cr_node_status returns False when its read fails and the caller turns that into "Node add result: False", so on a cluster whose operator and CRDs were both correct every node_add failed, retried eleven times, and gave up. maxParallelNodeAdds moved twice over: off the retired StorageNodeSet and onto StorageCluster, and from the top of the spec into the storageNodes block. It seeds the storage MachineConfigPool's initial maxUnavailable, so reading it from where it no longer is returned None everywhere and serialized the first-time CPU-topology reboots of every fresh OpenShift cluster. Both spellings are read, newest first, so a control plane talking to either generation of operator finds it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

…ng behind generate_automated_deployment_config reports failure by returning (False, False): no device matched the filters, the sockets did not validate, the memory did not add up. main discarded that return, so the process exited 0. Its caller is an init container. Exiting 0 tells Kubernetes the node was configured, so the pod started anyway -- on whatever /etc/simplyblock/sn_config_file the host already had, because a generation that fails writes nothing. On a cluster reinstalled over an earlier deployment that file was the earlier deployment's, naming lblk devices, and the node_add that read it was refused: The node config carries 'lblk_devices' but this cluster runs in nvme device mode; re-run 'sn configure' without --lblk or create the cluster with --device-mode lblk The message names the device class, which nobody had asked for, rather than the configure that never ran twenty minutes earlier. Two nodes that never reached node_add still carried that file, dated the previous evening, which is what identified it as inherited rather than produced. So the result is checked and a failure exits 1, and the file is discarded before the generation that replaces it. The order is the substance: after the pod-present check, which skips generation entirely and whose file belongs to a running pod, and before the generation whose failure is the case this exists for. A missing file is a first install rather than an error. Why this was possible: failure was reported by a return value in a function whose caller had no reason to look at one, and the only evidence of it was a log line in an init container nobody reads while the pod is Running. The state it fell back to was indistinguishable from state this deployment had produced, because the file carries no record of which deployment wrote it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

…a 500 ValueError: Failed to create add-node task → the control plane answered 500: Internal Server Error _validate_new_task_node_add is load-bearing and stays. Without it a retried post creates a second independent FN_NODE_ADD for one host, both are dispatched, and two threads race that host's config-slot logic — 2026-07-23, six nodes created for a four-slot host. What it reported was the problem. _add_task answers a duplicate with False, and the v2 endpoint raises ValueError on falsy, so the guard working exactly as designed reached the caller as a server error. The operator re-posts an add whenever the task window it polls comes back empty, which happens whenever the control plane cannot be read for a moment, and every one of those retries was answered with a 500 on a host whose add was already queued and running (2026-09-20, worker-5: seventeen in two minutes). A task that already exists is the answer to "add this host". It is returned instead, which is what ensure_node_restart_task in the same module already does with its own repeat, and it makes the endpoint idempotent under exactly the retry it is written to expect. The other _add_task callers keep answering False for their own duplicates. This is scoped to the one whose caller raises on it; whether False is the right answer anywhere is a wider question than this fault. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Copilot

Copilot review overview

🟡 Changes recommended

The new unit tests/documentation currently model an incorrect return contract for generate_automated_deployment_config, and the global CR_VERSION bump risks breaking flows still referencing storagenodesets without a per-kind version strategy.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1 · 2 · 1

Open (4)

CR version bump breaks StorageNodeSet plural resource paths · New
Deployment config tests use incorrect tuple return contract · New
Deployment config mock uses incorrect tuple return value · New
Document deployment config failure as boolean, not tuple · New

What changed in this PR

Fixes three independent Kubernetes-path faults that prevented node_add from succeeding on fresh installs by aligning CRD version access, making node configuration failures fatal in init containers, and making add-node task creation idempotent.

Changes:

Update the control plane to read the stored StorageCluster CR version (v1alpha2) and the relocated maxParallelNodeAdds field.
Ensure node_configure deletes stale host configs before regeneration and exits non-zero if config generation fails.
Make add_node_add_task return the already-queued task UUID on duplicates (idempotent behavior), with new unit coverage.

File	Description
tests/unit/test_storage_cluster_cr_version.py	Adds unit coverage for CR stored-version reads and `maxParallelNodeAdds` lookup path/plural.
tests/unit/test_node_configure_failure_is_fatal.py	Adds unit coverage to ensure failed config generation exits non-zero and stale configs are discarded.
tests/unit/test_add_node_task_is_idempotent.py	Adds unit coverage ensuring add-node task creation is idempotent for the same host.
simplyblock_web/node_configure.py	Discards stale node config before regeneration and exits 1 when generation fails.
simplyblock_web/api/internal/storage_node/kubernetes.py	Updates comments to reflect StorageCluster CR source for parallel-add knob.
simplyblock_core/utils/__init__.py	Reads `maxParallelNodeAdds` from `spec.storageNodes.*` (fallback to legacy spec path) using CR group/version constants.
simplyblock_core/storage_node_ops.py	Updates comments to reflect StorageCluster CR source for parallel-add knob.
simplyblock_core/controllers/tasks_controller.py	Returns existing queued add-node task UUID instead of falsey duplicate result.
simplyblock_core/constants.py	Bumps `CR_VERSION` constant to `v1alpha2`.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

generate_automated_deployment_config returns a bool. The guard added in 2b4f39c also handled a (False, False) pair, and its tests mocked only that pair, so the branch the production call reaches was never exercised: removing the tuple handling leaves both failure cases asserting DID NOT RAISE SystemExit. A test that only covers a shape the callee cannot return is a test of the mock. The claim came from the device-filter helpers inside the generator, which do answer (False, False). The generator collapses that into a single False before returning, so no caller ever sees the pair. The earlier commit message and the test docstring both state otherwise and are wrong. The guard itself was right either way -- `not False` and `not (False, False)` reach the same exit for the real contract, because the tuple branch was only ever dead code. What was wrong is that nothing proved it. With the mocks at the real contract, restoring the ignored return turns all three cases red again. None is kept alongside False: a generator refactored to fall off the end without returning is the same thing to this caller, and it is the shape such a change would most plausibly introduce. Reported by Copilot on #1378. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

mxsrc

Generally I like this. I'd like it more to be three PRs :) I have a few comments where I think this should be improved.

mxsrc · 2026-09-21T12:41:55Z

+        except FileNotFoundError:
+            continue  # A first install has none, which is the ordinary case.
+        except OSError as e:
+            logger.warning(f"The previous node configuration {path} could not be removed: {e}")


Shouldn't we fail here?

It's ok as a best effort. Typically it'll be overridden. Or do you mean the OSError specifically?

A cleanup shouldn't be best-effort imho, if it fails to remove the file the updated write will also fail, just that it's less clear where the failure originated from.

But that is the OSError, the FileNotFoundError would be ok.

Yeah, FileNotFoundError is (unexpected) success.

mxsrc · 2026-09-21T12:46:02Z

+    existing = _validate_new_task_node_add(
+        cluster_id, (function_params or {}).get("node_addr"))
+    if existing:
+        return existing


With the current function name I feel an existing task should constitute a failure. We do not check that the parameters of the other task match, so this might not do what the caller expects.

Maybe the v2 API should return a conflict then? Or any other idea? A 500 is just plainly wrong 😅

Yeah, that would be one way. I think a 409 failure would be fine to have, it gives the caller the chance to verify themselves with a reference to the existing task whether that's a duplicate. Alternatively we could also check whether the task is in fact identical and does not just affect the same node, and return an 204 with a reference, and a 409 otherwise.

Personally, the 204 is nicer obviously 😁

Yeah, and it shouldn't be too hard to do..

noctarius and others added 3 commits September 21, 2026 07:43

Copilot AI lite review requested due to automatic review settings September 21, 2026 05:46

Copilot started reviewing on behalf of noctarius September 21, 2026 05:47 View session

Copilot AI reviewed Sep 21, 2026

View reviewed changes

Comment thread simplyblock_core/constants.py

Comment thread simplyblock_web/node_configure.py Outdated

Comment thread tests/unit/test_node_configure_failure_is_fatal.py Outdated

Comment thread tests/unit/test_node_configure_failure_is_fatal.py Outdated

mxsrc requested changes Sep 21, 2026

View reviewed changes

Conversation

noctarius commented Sep 21, 2026

1. The storage CRs were read at v1alpha1 (ed14e78c7)

2. A failed sn configure exited 0 (6ccbed944)

3. A deduplicated node_add answered 500 (9ba41b36d)

Testing

Not addressed

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mxsrc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

1. The storage CRs were read at v1alpha1 (`ed14e78c7`)

2. A failed `sn configure` exited 0 (`6ccbed944`)

3. A deduplicated `node_add` answered 500 (`9ba41b36d`)