nvmeof: Add GroupLock to coordinate stage and unstage operations and e2e tests by gadididi · Pull Request #6210 · ceph/ceph-csi

gadididi · 2026-03-30T11:32:18Z

Describe what this PR does

This PR adds a GroupLock to prevent race conditions between stage and unstage operations that could lead to premature NVMe controller disconnects.
Added group mutual lock into NodeStageVolume() and NodeUnStageVolume() , because these 2 operations cannot run together. but few calls of same type can run together.

The Problem

Without coordination, a stage operation can connect to NVMe controllers while an unstage operation is simultaneously checking if it's safe to disconnect them. This creates a race:

Stage operation connects to controllers
Unstage operation checks cache, sees no devices
Unstage operation disconnects controllers
Stage operation tries to mount, but device is already disconnected

Result: Stage fails with "device not found" errors.

The Solution

Add a GroupLock that allows:

Multiple stage operations to run concurrently (Group A)
Multiple unstage operations to run concurrently (Group B)
But prevents stage and unstage from running at the same time

Three Levels of Locking

(after the PR: nvmeof: Add mount cache and locking for safe nvme disconnect will be merged)

The code will have three levels of locks working together:

Level 1: volumeLocks (per-volume mutex)
this already exists in the code.

Scope: Single volume
Purpose: Prevents concurrent operations on the same volume
Example: Two NodeStageVolume calls for vol-123 cannot run at the same time

Level 2: stageUnstageLock (GroupLock)
the current PR introduces it.

Scope: All volumes, operation type
Purpose: Prevents stage and unstage from interfering with each other
Allows: Multiple stages together, multiple unstages together
Prevents: Stage and unstage at the same time

Level 3: mountCache.mu (cache mutex)
this PR nvmeof: Add mount cache and locking for safe nvme disconnect

Scope: Cache data structure
Purpose: Protects cache reads and writes
Duration: Nanoseconds (just for map operations)

Lock Acquisition Order

Both NodeStageVolume and NodeUnstageVolume follow the same order:

volumeLocks.Lock(volumeID) // Lock this specific volume
stageUnstageLock.AcquireGroup() // Lock operation type (A or B)
mountCache operations // Cache mutex acquired internally as needed
stageUnstageLock.ReleaseGroup() // Release operation type
volumeLocks.Release(volumeID) // Release volume lock

There are unit tests for group locking here: https://github.com/ceph/ceph-csi/blob/devel/internal/util/lock/group_lock_test.go

Also, e2e tests were added.

Checklist:

Commit Message Formatting: Commit titles and messages follow
guidelines in the developer
guide.
Reviewed the developer guide on Submitting a Pull
Request
Pending release
notes
updated with breaking and/or notable changes for the next major release.
Documentation has been updated, if necessary.
Unit tests have been added, if necessary.
Integration tests have been added, if necessary.

Show available bot commands

These commands are normally not required, but in case of issues, leave any of
the following bot commands in an otherwise empty comment in this PR:

/retest ci/centos/<job-name>: retest the <job-name> after unrelated
failure (please report the failure too!)

Copilot

Pull request overview

Adds a cross-volume “group mutual exclusion” lock to the NVMe-oF node server to prevent NodeStageVolume and NodeUnstageVolume from running concurrently, avoiding races that can lead to premature NVMe controller disconnects.

Changes:

Introduces a stageUnstageLock (lock.GroupLock) field on the NVMe-oF NodeServer.
Wraps NodeStageVolume() with Group A acquire/release and NodeUnstageVolume() with Group B acquire/release.
Adds the internal/util/lock import to support the new locking behavior.

Copilot

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

gadididi · 2026-03-31T09:49:32Z

/test ci/centos/mini-e2e/k8s-1.35/nvmeof

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

gadididi · 2026-03-31T10:55:51Z

/test ci/centos/mini-e2e/k8s-1.35/nvmeof

nixpanic

lgtm, thanks!

gadididi · 2026-04-01T09:45:23Z

/test ci/centos/mini-e2e/k8s-1.35/nvmeof

Copilot

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.

gadididi · 2026-04-01T09:59:12Z

/test ci/centos/mini-e2e/k8s-1.35/nvmeof

mergify · 2026-04-13T12:06:23Z

Merge Queue Status

✅ Entered queue — 2026-04-13 12:06 UTC · Rule: default
✅ Checks started · in-place
🚫 Left the queue — 2026-04-13 12:46 UTC · at 5aa179784f9012b45fd514d01cfb2be44ce736a1

This pull request spent 39 minutes 51 seconds in the queue, with no time running CI.

Required conditions to merge

#approved-reviews-by >= 2 [🛡 GitHub branch protection]
- nvmeof: Add GroupLock to coordinate stage and unstage operations and e2e tests #6210
#changes-requested-reviews-by = 0 [🛡 GitHub branch protection]
- nvmeof: Add GroupLock to coordinate stage and unstage operations and e2e tests #6210

Reason

The merge conditions cannot be satisfied due to failing checks

Hint

You may have to fix your CI before adding the pull request to the queue again.
If you update this pull request, to fix the CI, it will automatically be requeued once the queue conditions match again.
If you think this was a flaky issue instead, you can requeue the pull request, without updating it, by posting a @mergifyio queue comment.

added group mutual lock into `NodeStageVolume()` and `NodeUnStageVolume()` , because these 2 operations cannot run together. but few calls of same type can run together. Signed-off-by: gadi-didi <gadi.didi@ibm.com>

Add e2e tests to validate nvmeof NodeServer GroupLock implementation under concurrent NodeStage (Group A) and NodeUnstage (Group B) operations. The tests ensure no deadlock occurs when multiple PVCs and Pods are created and deleted simultaneously. New helper file (nvmeof_helper.go) provides reusable functions for concurrent PVC/Pod operations with proper error tracking. Two test cases cover: 1) sequential concurrent batches (create all, then delete all) 2) mixed operations with pre-created batch to guarantee continuous Group A/B switching.. Signed-off-by: gadi-didi <gadi.didi@ibm.com>

ceph-csi-bot · 2026-04-13T12:07:01Z

/test ci/centos/upgrade-tests-cephfs

ceph-csi-bot · 2026-04-13T12:07:01Z

/test ci/centos/k8s-e2e-external-storage/1.35

ceph-csi-bot · 2026-04-13T12:07:02Z

/test ci/centos/mini-e2e-helm/k8s-1.35

ceph-csi-bot · 2026-04-13T12:07:02Z

/test ci/centos/upgrade-tests-rbd

ceph-csi-bot · 2026-04-13T12:07:03Z

/test ci/centos/k8s-e2e-external-storage/1.33

ceph-csi-bot · 2026-04-13T12:07:03Z

/test ci/centos/mini-e2e/k8s-1.35

ceph-csi-bot · 2026-04-13T12:07:03Z

/test ci/centos/k8s-e2e-external-storage/1.34

ceph-csi-bot · 2026-04-13T12:07:03Z

/test ci/centos/mini-e2e-helm/k8s-1.33

ceph-csi-bot · 2026-04-13T12:07:04Z

/test ci/centos/mini-e2e-helm/k8s-1.34

ceph-csi-bot · 2026-04-13T12:07:04Z

/test ci/centos/mini-e2e/k8s-1.33

ceph-csi-bot · 2026-04-13T12:07:04Z

/test ci/centos/mini-e2e/k8s-1.34

gadididi · 2026-04-13T14:21:08Z

@Mergifyio rebase

mergify · 2026-04-13T14:21:32Z

rebase

☑️ Nothing to do, the required conditions are not met

Details

any of:
- #commits-behind > 0 [📌 rebase requirement]
- -linear-history [📌 rebase requirement]
-closed [📌 rebase requirement]
-conflict [📌 rebase requirement]
queue-position = -1 [📌 rebase requirement]

nixpanic · 2026-04-13T15:00:24Z

/retest ci/centos/k8s-e2e-external-storage/1.34

nixpanic · 2026-04-13T15:01:03Z

/retest ci/centos/k8s-e2e-external-storage/1.34

Failed with something related to CephFS:

[FAIL] External Storage [Driver: cephfs.csi.ceph.com] [Testpattern: Dynamic PV (default fs)] provisioning [It] should provision storage with pvc data source

logs

mergify · 2026-04-13T16:14:34Z

Merge Queue Status

✅ Entered queue — 2026-04-13 16:14 UTC · Rule: default
✅ Checks skipped · PR is already up-to-date
✅ Merged — 2026-04-13 16:14 UTC · at 5aa179784f9012b45fd514d01cfb2be44ce736a1

This pull request spent 21 seconds in the queue, including 4 seconds running CI.

Required conditions to merge

#approved-reviews-by >= 2 [🛡 GitHub branch protection]
- nvmeof: Add GroupLock to coordinate stage and unstage operations and e2e tests #6210
#changes-requested-reviews-by = 0 [🛡 GitHub branch protection]
- nvmeof: Add GroupLock to coordinate stage and unstage operations and e2e tests #6210

gadididi requested review from Madhu-1, Copilot and nixpanic March 30, 2026 11:32

gadididi self-assigned this Mar 30, 2026

gadididi added the component/nvme-of Issues and PRs related to NVMe-oF. label Mar 30, 2026

Copilot started reviewing on behalf of gadididi March 30, 2026 11:32 View session

Copilot AI reviewed Mar 30, 2026

View reviewed changes

Comment thread internal/nvmeof/nodeserver/nodeserver.go Outdated

gadididi force-pushed the nvmeof/add_locking_nodeserver branch from 3b71e31 to fe2a852 Compare March 30, 2026 11:38

gadididi requested a review from Copilot March 30, 2026 11:38

Copilot started reviewing on behalf of gadididi March 30, 2026 11:39 View session

Copilot AI reviewed Mar 30, 2026

View reviewed changes

gadididi force-pushed the nvmeof/add_locking_nodeserver branch from fe2a852 to 96907dc Compare March 30, 2026 11:48

gadididi requested a review from Copilot March 31, 2026 09:45

Copilot started reviewing on behalf of gadididi March 31, 2026 09:46 View session

Copilot AI reviewed Mar 31, 2026

View reviewed changes

Comment thread e2e/nvmeof_helper.go Outdated

Comment thread internal/nvmeof/nodeserver/nodeserver.go

gadididi force-pushed the nvmeof/add_locking_nodeserver branch from 2928da4 to c2c15b1 Compare March 31, 2026 10:27

gadididi requested a review from Copilot March 31, 2026 10:28

Copilot started reviewing on behalf of gadididi March 31, 2026 10:29 View session

Copilot AI reviewed Mar 31, 2026

View reviewed changes

nixpanic reviewed Mar 31, 2026

View reviewed changes

Comment thread internal/nvmeof/nodeserver/nodeserver.go

Comment thread e2e/nvmeof_helper.go Outdated

gadididi force-pushed the nvmeof/add_locking_nodeserver branch from a214c1b to 6519c04 Compare April 1, 2026 09:44

gadididi requested a review from Copilot April 1, 2026 09:44

Copilot started reviewing on behalf of gadididi April 1, 2026 09:45 View session

Copilot AI reviewed Apr 1, 2026

View reviewed changes

Comment thread e2e/nvmeof_helper.go Outdated

Comment thread e2e/nvmeof_helper.go Outdated

Comment thread e2e/nvmeof_helper.go Outdated

Comment thread e2e/nvmeof_helper.go

Comment thread e2e/nvmeof_helper.go Outdated

Comment thread e2e/nvmeof_helper.go Outdated

gadididi force-pushed the nvmeof/add_locking_nodeserver branch from 6519c04 to 9094db7 Compare April 1, 2026 09:57

gadididi requested a review from Copilot April 1, 2026 09:58

mergify Bot added the queued label Apr 13, 2026

gadididi added 2 commits April 13, 2026 12:06

nvmeof: Adding group mutual lock into nodeserver

fb6a064

added group mutual lock into `NodeStageVolume()` and `NodeUnStageVolume()` , because these 2 operations cannot run together. but few calls of same type can run together. Signed-off-by: gadi-didi <gadi.didi@ibm.com>

ceph-csi-bot force-pushed the nvmeof/add_locking_nodeserver branch from f04bd14 to 5aa1797 Compare April 13, 2026 12:06

mergify Bot added the ok-to-test Label to trigger E2E tests label Apr 13, 2026

ceph-csi-bot added ci/in-progress/e2e This label acts like a guard and prevents Mergify from adding the `ok-to-test` label again. and removed ok-to-test Label to trigger E2E tests labels Apr 13, 2026

mergify Bot added dequeued and removed ci/in-progress/e2e This label acts like a guard and prevents Mergify from adding the `ok-to-test` label again. queued labels Apr 13, 2026

mergify Bot merged commit 199e62f into ceph:devel Apr 13, 2026
44 checks passed

mergify Bot removed the dequeued label Apr 13, 2026

gadididi deleted the nvmeof/add_locking_nodeserver branch April 14, 2026 05:04

Conversation

gadididi commented Mar 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Describe what this PR does

The Problem

The Solution

Three Levels of Locking

Lock Acquisition Order

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

gadididi commented Mar 31, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

gadididi commented Mar 31, 2026

Uh oh!

nixpanic left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

gadididi commented Apr 1, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gadididi commented Apr 1, 2026

Uh oh!

mergify Bot commented Apr 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merge Queue Status

Reason

Hint

Uh oh!

ceph-csi-bot commented Apr 13, 2026

Uh oh!

ceph-csi-bot commented Apr 13, 2026

Uh oh!

ceph-csi-bot commented Apr 13, 2026

Uh oh!

ceph-csi-bot commented Apr 13, 2026

Uh oh!

ceph-csi-bot commented Apr 13, 2026

Uh oh!

ceph-csi-bot commented Apr 13, 2026

Uh oh!

ceph-csi-bot commented Apr 13, 2026

Uh oh!

ceph-csi-bot commented Apr 13, 2026

Uh oh!

ceph-csi-bot commented Apr 13, 2026

Uh oh!

ceph-csi-bot commented Apr 13, 2026

Uh oh!

ceph-csi-bot commented Apr 13, 2026

Uh oh!

gadididi commented Apr 13, 2026

gadididi commented Mar 30, 2026 •

edited

Loading

mergify Bot commented Apr 13, 2026 •

edited

Loading

mergify Bot commented Apr 13, 2026 •

edited

Loading