Detect and repair stale NVMe-oF subsystems and controllers

[noctarius]

Detect and repair stale NVMe-oF subsystems and controllers

Generalises three separate stale-controller fixes — f99b5b57, 498c2c5f, and the
device-discovery rework in #399 — into one diagnosis/repair layer in atlas-lib/nvmeof,
and rewires the CSI node plugin onto it.

Stacked on #430, now merged: the shared connector machinery and CLIConnector came from
there, and this PR only consumes them.

The problem

All three fixes were chasing the same shape of bug: a connect succeeds at one layer of
the NVMe object tree while the layer below it is unusable, and the check that gates the
retry sits at the higher layer — so nothing looks missing and the retry spins forever.

• A subsystem attached with live controllers that exports no namespace at all.
  nvme connect is satisfied, nvme list-subsys shows the NQN, and no block device ever
  appears — so every NodeStageVolume skips the connect and times out on device
  discovery, which kubelet retries forever. That is the multi-hour FIO pod hang.
• A controller that is live and contributes no path to the namespace. The
  device-scoped view omits it, so the volume runs a path short, but nvme connect refuses
  with "already connected" because nothing is missing at the controller level. In
  K8sNativeResilientFailoverTest iteration 28 that produced 106 retries over 11 minutes
  for one volume, not one of which reached the target; the volume ran at 2 of 3 paths and
  lost all I/O when the outage took the other two.
• Two kernel subsystem instances answering one NQN, where a lookup can return the wrong
  block device.

What's in it

Inspect (inspect.go) — diagnosis, read-only, no NVMe commands issued. Returns typed
Defects: NoNamespace, NamespaceMissing, ControllerNotContributing, AmbiguousHead,
StaleEndpoint. Each carries the teardown Scope, the controllers to release already in
teardown order, and CoTenants — the namespaces that would be left with no usable path.

Detection is positive, from state the kernel publishes, not inferred from a wait that
ran out or from the text of an nvme-cli error. 498c2c5f used a 30s timeout as its stale
signal, which cannot tell "slow" from "broken"; "live controller, zero namespaces" is
decidable on the first look.

Repair + Repairer (repair.go) — Repair is the bare mechanism, taking a
one-method ControllerDetacher so a caller whose connect path lives elsewhere need not
supply a whole Connector. Repairer.Attach adds policy: narrowest scope first, and never
at the cost of another volume's block device, the caller's own device, or a repair that
just ran.

CSI driver — nvmerepair.go diagnoses through Inspect and tears down through
Repair over #430's CLIConnector, so detection, teardown ordering and the disconnect
itself are all shared; what stays local is policy alone. Two seams in initiator.go:
Connect splits into connectOnce plus a repair retry, and recoverPathsWithANA calls
the monitor hook after its reconciles.

Reading sysfs instead of shelling out also removes a process per monitor tick (the previous
approach forked nvme list-subsys twice per volume plus once per co-tenant, at a 3s
cadence) and removes the reverse-engineering of udev's _<nsid> link naming, which was the
most fragile part.

Verified against a live cluster

Identity and the two premises the detectors rest on were checked on a running cluster
(kernel 5.14.0-503.14.1.el9_5, nvme-cli 2.16), not assumed:

• model is the master lvol UUID space-padded to 40 bytes; serial is ha; ANA legs live
  at /sys/class/nvme/<ctrl>/nvme<subsys>c<ctrl>n<nsid>/ana_state; cntlid is 1..N on the
  primary storage node and 1000+ on the secondary.
• Host-wide nvme list-subsys carries no ANAState at all — only the device-scoped
  form does. The code uses each accordingly.
• The by-id globs resolve correctly on both single- and multi-namespace subsystems.

All four defects that can be forced were then reproduced with kernel NVMe-oF targets
(nvmet, tooling in atlas-lib/hack/nvmet from #430), including
ControllerNotContributing via two targets sharing an NQN with disjoint cntlid ranges —
the host merges them and one controller ends up with an empty leg set. Each state was
captured and is committed as a replayable snapshot.

Tests

• 109 tests in atlas-lib/nvmeof (41 specific to this PR), 92% statement coverage;
  17 in csi-driver/pkg/util for the policy layer.
• testdata/sysfs/*.tsv — four sysfs snapshots replayed through the real resolver: a
  healthy production node (4 subsystems, 8 namespaces, one a 5-namespace shared subsystem)
  plus the three forced defects. Sanitized with consistent substitution so the
  relationships under test survive.
• The healthy snapshot is the false-positive guard, and it is the assertion that
  matters most: a repair tears down live data paths, so a diagnosis that fires on a healthy
  fabric is worse than no diagnosis. Hand-built fixtures cannot rule that out — they encode
  what we believe sysfs looks like, which is the belief under test.

Two decisions worth a second opinion

The NoNamespace co-tenant gate departs from detach.go's precedent. DetachDevice
gates on IsMultiNamespace (can the subsystem be shared) rather than on current
co-tenants, because a namespace can join between check and disconnect. This gates on the
actual blast radius instead: for NoNamespace the subsystem exports zero namespaces, so
there is no co-tenant device to destroy, and gating on "shareable" would leave that volume
permanently stuck — the exact bug being fixed. The race detach.go warns about is benign
here, since a co-tenant joining a subsystem that exports nothing wants it torn down too.
This is the one place the two files disagree.

The cooldown key is constrained from both sides. It must be narrow enough that two
outstanding repairs do not collide — applying one would otherwise mask the other — and
stable enough to survive a repair that did not stick, since a key the teardown itself
changes sees a brand-new repair every time. Neither the controller id nor the kernel
subsystem id qualifies in general, because a repair re-creates both:

Scope / kind	Key subject	Why
Controller	fabric endpoint	the name changes on re-create, the endpoint does not; keeps three broken paths as three repairs
Subsystem, NoNamespace / NamespaceMissing	(nothing)	at most one per NQN, and the teardown may renumber the instance
Subsystem, AmbiguousHead	instance id	the only defect reported several times per NQN; stable in the way that matters, since a failed repair leaves that same head in place

The AmbiguousHead row was a bug Copilot caught on the earlier combined PR: without it,
repairing the first stale head marked the rest as handled and left them attached.

Known limitations

• The auto-repair path has no automated end-to-end coverage. Forcing
  ControllerNotContributing at a published endpoint needs a storage node to stop
  exporting a namespace on one of its published listeners, which the real control plane
  will not do on request. Detection of that state is pinned by the committed snapshot; the
  repair mechanism is shared with the defects that are covered. Closing the gap needs a
  simulated control plane — see follow-ups.
• Defects are logged but not otherwise surfaced. f99b5b57's incident report names "no
  operator-visible signal" as part of the damage; counters would fix that and would also be
  the natural e2e assertion surface. Not in this PR.
• AmbiguousHead is reported but cannot be forced deterministically — the kernel rejects a
  duplicate-cntlid controller rather than creating a second subsystem. Unit tests only,
  which is also where Copilot found the bug above; that correlation is not a coincidence.

Follow-ups (separate branches)

• feat/csi-e2e-stale-controller — Ginkgo specs for the two properties reachable against
  real simplyblock: a healthy fabric is never disturbed, and an unpublished controller is
  never torn down while a lost path still recovers.
• Integration tier: minimal k3s + simulated control plane + nvmet, which is what makes the
  auto-repair path, the cooldown and the co-tenant refusal testable end to end.
• Migrating the driver's connect path onto ConnectPaths. Held for fix: forward hostNQN/DHCHAP secrets on connect and fix allowed-node scheduling #417, which forwards
  DHCHAP secrets on connect — nvmeof.Target has no DHCHAP fields, so that is a
  prerequisite rather than just a conflict.

[Copilot]

Pull request overview

Introduces a generalized NVMe-oF diagnosis + repair layer in atlas-lib/nvmeof (sysfs-based defect detection plus controller/subsystem teardown), and wires the CSI node plugin to use it to break “already connected but unusable” retry loops (both at NodeStage attach time and in the connection monitor).

Changes:

• Add nvmeof.Inspect (typed defect detection from sysfs) and nvmeof.Repair / nvmeof.Repairer (teardown mechanism + policy/cooldown orchestration).
• Add CSI-side policy/adapter (nvmerepair.go) and integrate attach/monitor hooks in initiator.go.
• Add snapshot-based sysfs tests + nvmet lab tooling and sysfs fixtures to make defect states replayable in CI.

Reviewed changes

Copilot reviewed 18 out of 18 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
csi-driver/pkg/util/nvmerepair.go	CSI-side repair policy wrapper around nvmeof.Inspect/nvmeof.Repair, with cooldown and safety gates.
csi-driver/pkg/util/nvmerepair_test.go	Unit tests for CSI-side policy selection, cooldown keying, and teardown ordering.
csi-driver/pkg/util/initiator.go	Split connect into connectOnce + repair retry; add monitor hook after ANA reconciliation; minor ANA constant cleanup.
csi-driver/e2e/nvmet/nvmet-lab.sh	nvmet-based lab script to force/verify defect states against a real kernel NVMe-oF target.
csi-driver/e2e/nvmet/capture-sysfs.sh	Script to dump/sanitize/reconstruct NVMe sysfs state into replayable snapshots.
atlas-lib/nvmeof/testdata/sysfs/no-namespace.tsv	Replayable sysfs snapshot fixture for “live controllers, zero namespaces” defect.
atlas-lib/nvmeof/testdata/sysfs/namespace-missing.tsv	Replayable sysfs snapshot fixture for “namespace missing but others present” defect.
atlas-lib/nvmeof/testdata/sysfs/controller-not-contributing.tsv	Replayable sysfs snapshot fixture for “live controller contributes no ANA leg” defect.
atlas-lib/nvmeof/inspect.go	Sysfs-based defect detection (NoNamespace, NamespaceMissing, ControllerNotContributing, AmbiguousHead, StaleEndpoint).
atlas-lib/nvmeof/inspect_test.go	Unit tests for defect detection, including blast-radius/co-tenant calculations.
atlas-lib/nvmeof/repair.go	Repair mechanism + Repairer.Attach policy loop with cooldown, scope caps, and disruption protection.
atlas-lib/nvmeof/repair_test.go	Unit tests for repair ordering, cooldown behavior, refusal gates, and attach/repair loop semantics.
atlas-lib/nvmeof/realsysfs_test.go	Snapshot replay tests to guard against false positives and to assert forced-defect verdicts.
atlas-lib/nvmeof/fabrics.go	Add DisconnectController to support controller-scope teardowns.
atlas-lib/nvmeof/connector.go	Extend Connector interface with DisconnectController.
atlas-lib/nvmeof/detach_test.go	Update connector test double to satisfy the extended interface.
atlas-lib/nvmeof/doc.go	Package documentation update describing the new inspect/repair layering and rationale.

---

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