csi: register device presence at connect time, not just via poll#337
Open
boddumanohar wants to merge 3 commits into
Open
csi: register device presence at connect time, not just via poll#337boddumanohar wants to merge 3 commits into
boddumanohar wants to merge 3 commits into
Conversation
initiatorNVMf.Connect() resolves and confirms the device is live but never recorded it in devicePresentMap/deviceToLvolIDMap. Those maps were only ever populated by the background MonitorConnection poll loop (reconnectSubsystems), which discovers connected devices on its own ~3s+jitter cadence. If a device connects and then loses all its paths faster than one poll interval, it was never marked present, so the guardian's gone-device detection had nothing to diff against when it vanished — the loss was silently missed forever, and the guardian never got a chance to restart the affected pod. Register presence synchronously right after Connect() confirms the device path, closing that bootstrap race independent of poll timing. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…try, not nvme list The previous commit patched a narrow race (registering device presence synchronously in Connect()) on top of the existing design, where reconnectSubsystems discovered connections by scanning `nvme list` and diffing successive snapshots against history. That discover-then-diff approach has a deeper problem: it can only ever detect a lost connection if some earlier poll happened to observe it as present first. Any gap that beats the poll cadence — not just the connect/disconnect race, but also e.g. the csi-node pod itself restarting and losing its in-memory maps — reintroduces the same class of silent miss. Replace it with a model driven by the guardian's own publish/unpublish registry (already populated synchronously at NodeStageVolume/ NodePublishVolume/NodeUnpublishVolume time, with no polling involved). That registry is now extended to also record the exact subsystem NQN a lvol was published under, and reconnectSubsystems checks each tracked lvol's live NVMe-oF state directly against a single `nvme list-subsys` snapshot each poll — no historical diffing, so no dependency on catching a narrow window at all. This also removes the `isManagedLvol` PV lookup gate added for the same purpose in a previous change: it's now redundant, since the guardian's registry only ever contains lvols this driver's own NodePublishVolume handled — benchmark/foreign volumes were never eligible in the first place. As a byproduct, the `devicePath` parameter threaded through recoverPathsWithANA/reconcileOptimizedPath/reconcileNonOptimizedPaths was already dead (flagged unused by the linter) and is removed too. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
boddumanohar
changed the title
5 tasks
…wn registry, not nvme list" This reverts commit 92083c6.
boddumanohar
changed the title
geoffrey1330
approved these changes
Jul 16, 2026
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.
Summary
Investigating a flaky
SPDKCSI-RECONNECT-GUARDIANe2e failure (guardian never restarted a pod after total NVMe-oF path loss), cross-referencing the k8s pod logs, the SPDK target logs, and the raw ginkgo run (simplyBlockDeployGCP run 29477941151) gave a precise, confirmed timeline for the failing run:07:25:42.147–07:25:42.284— volume staged and connected (csi-node pod log)07:25:44.423— test issuesnvme disconnect -n <NQN>to induce total path loss (ginkgo step log)07:25:44.531–.688— SPDK target confirms both paths' queues torn downThe device was alive for only ~2.3 seconds before losing all paths — shorter than the csi-node's
MonitorConnectionpoll cadence (3s base + up to 500ms jitter). The connection loss itself was real and permanent (never reconnected before the guardian could act), and the storage cluster was healthy throughout — this isn't an infra flake. Yet the csi-node's ownGuardian: MarkBrokenLvol/... is no longer presentdetection never fired for this lvol, while it did fire correctly for other lvols earlier in the same run.Root cause:
initiatorNVMf.Connect()(called fromNodeStageVolume) resolves and confirms the device is live, but never records it indevicePresentMap/deviceToLvolIDMap. Those maps are populated only by the backgroundMonitorConnectionpoll loop (reconnectSubsystems), on its own ~3s+jitter cadence. If a device connects and then loses all paths faster than one poll cycle — as happened here — it's never marked "present", so the guardian's gone-device detection has nothing to diff against when it vanishes: the loss is silently missed forever, and the guardian never gets a chance to restart the pod.This PR's approach
The minimal, targeted fix: register presence synchronously in
Connect()right after the device path is confirmed, instead of waiting for the next poll to discover it. This closes the specific race with a small, low-risk change (16 lines, one file).See #338 for an alternative, more thorough fix to the same root cause: instead of patching the race, it replaces
reconnectSubsystems's entire discover-then-diff-history model with one driven by the guardian's own already-race-free publish/unpublish registry. That's a materially larger change with its own trade-offs (detailed in that PR's description) — opening both so the two approaches can be compared and one chosen.Test plan
go build ./...go test ./pkg/... ./cmd/...SPDKCSI-RECONNECT-GUARDIANe2e a few times to confirm improved reliability (no existing unit test harness coversConnect()/reconnectSubsystems, since both shell out to realnvme//sysstate)🤖 Generated with Claude Code