volumes: concurrent EFS mount/unmount via per-volume locking

[callaway12]

Summary

• Replace the single global sync.RWMutex in AmazonECSVolumePlugin with a short-lived map lock (mapLock) + per-volume mutexes (volLocks), allowing mount/unmount I/O for different volumes to proceed concurrently
• Refactor ECSVolumeDriver.Create/Remove to release the driver lock before the mount/unmount syscall, since the plugin layer already guarantees per-volume serialization
• Add TestConcurrentMountsDifferentVolumes that verifies 5 parallel mounts complete in wall-clock time close to a single mount, not 5x

Motivation

The EFS volume plugin processes all mount requests serially behind a single write lock. A single EFS mount takes 15–72 seconds (TLS tunnel + NFS4 negotiation). When N concurrent ECS tasks each require 2+ EFS mounts, queued requests block for up to 72 × 2N seconds, exceeding Docker's plugin timeout (~60s) and causing:

CannotCreateContainerError: error while checking if volume exists in driver
"amazon-ecs-volume-plugin": Post "http://plugin.moby.localhost/VolumeDriver.Get":
context deadline exceeded

This is tracked in aws/containers-roadmap#2319.

Production impact: On a c6i.8xlarge running concurrent ECS tasks (EC2 launch type, awsvpc mode) with multiple EFS volumes per task, we observed 37 task failures over 3 months directly caused by this serialization, with failure frequency accelerating as instance uptime increased.

Design

Operation	Before	After
Mount()	Global write lock held for entire mount I/O	Global RLock for map lookup → per-volume Mutex for mount I/O
Unmount()	Global write lock held for entire unmount I/O	Global RLock for map lookup → per-volume Mutex for unmount I/O
Create()	Global write lock (metadata only — fast)	Global write lock (unchanged, no I/O)
Remove()	Global write lock	Global write lock (unchanged, infrequent)
List/Get/Path	Global read lock	Global read lock (unchanged)

Invariants preserved:

• Operations on the same volume remain serialized (per-volume mutex)
• The volumes map is protected during all reads/writes
• StateManager.save() has its own mutex for file persistence
• types.Volume.AddMount/RemoveMount is called under the per-volume lock (as documented: "caller is responsible for holding any locks")

Test plan

• All existing unit tests pass with updated lock field initialization
• New TestConcurrentMountsDifferentVolumes: 5 volumes with 50ms simulated mount latency complete in <200ms (proves concurrency), would take ≥250ms if serial
• Integration test on ECS instance with concurrent EFS-backed tasks

Fixes aws/containers-roadmap#2319

[amogh09]

Unfortunately our CI doesn't use go test -race on ecs-init module. Can you please run the new unit test locally with -race? It is failing for me.

[amogh09]

I have updated our CI to run ecs-init unit tests with -race. Can you please rebase against dev branch?

[callaway12]

Thanks for the review! Rebased onto latest dev and addressed all comments. Here's a summary of the changes:

Review feedback addressed:

1. Split getOrCreateVolLock() into 3 methods (getVolLock, createVolLock, getOrCreateVolLock) with double-checked locking to eliminate the race of writing the map under RLock
2. Added TestConcurrentMountsSameVolume — verifies driver.Create() is called exactly once when 5 goroutines mount the same volume concurrently
3. Replaced wall-clock assertion with sync.WaitGroup barrier to deterministically prove concurrent overlap

Additional race conditions fixed (found via go test -race):
4. StateManager race — recordVolume() was mutating VolState.Volumes map outside the lock, then save() acquired the lock and called json.MarshalIndent which reads the map concurrently. Fixed by extending the lock scope to cover both map mutation and serialization
5. Remove() coordination — Remove() now acquires the per-volume lock before proceeding, which drains any in-flight Mount/Unmount operations. It also performs slow I/O (IsMounted, volDriver.Remove) outside mapLock to avoid blocking other volumes
6. seelog format bug — Fixed Errorf("Error saving state of volume %s", r.Name, err) (err silently dropped) → Errorf("Error saving state of volume %s: %v", r.Name, err)

All tests pass with go test -race -count=20.

[singholt]

Thanks for your contribution, @callaway12.

The implementation itself looks fine. However, I have concerns with the design/approach. Introducing fine-grained locking meaningfully grows the concurrency surface of this volume plugin, and makes maintainability/extensibility harder.

As an alternative, I put together a rough sketch of an actor based model: singholt@40c72a7. Each volume would get a worker goroutine that processes its mount/unmount/remove operations from a mailbox. Same-volume operations would be ordered; and different volume operations would run concurrently because they're independent goroutines. There's no shared mutable volume state to lock during I/O, so the parallel-map and lifecycle issue simply don't arise.

Its a bigger refactor than what you propose, but I think its the right long-term solution, and not too cumbersome to work on with AI-assisted development. Please let me know what you think. Once we are aligned, I can refine my "rough sketch" into a reviewable PR, test it and get feedback internally from my team as well.

[callaway12]

Thanks for the feedback and the actor-model sketch, @singholt. The design direction makes sense.

That said, this issue is actively causing production task failures for us (37 failures over 3 months and accelerating). Could you share a rough timeline for when you'd expect the actor-based PR to be reviewable? If it's going to take a while, would you be open to merging the current per-volume locking as a stopgap and then refactoring to the actor model in a follow-up?

Happy to help with the actor-model implementation as well if that speeds things up.

[singholt]

Thanks for the feedback and the actor-model sketch, @singholt. The design direction makes sense.

That said, this issue is actively causing production task failures for us (37 failures over 3 months and accelerating). Could you share a rough timeline for when you'd expect the actor-based PR to be reviewable? If it's going to take a while, would you be open to merging the current per-volume locking as a stopgap and then refactoring to the actor model in a follow-up?

Happy to help with the actor-model implementation as well if that speeds things up.

Thanks. That sounds good to me. Let me get back to you on whether there are any other workarounds for mitigation of your issue. Apologies on that. If there aren't any feasible workarounds, I am good with merging this PR and including it in the next agent release.

[singholt]

Thanks for the feedback and the actor-model sketch, @singholt. The design direction makes sense.
That said, this issue is actively causing production task failures for us (37 failures over 3 months and accelerating). Could you share a rough timeline for when you'd expect the actor-based PR to be reviewable? If it's going to take a while, would you be open to merging the current per-volume locking as a stopgap and then refactoring to the actor model in a follow-up?
Happy to help with the actor-model implementation as well if that speeds things up.

Thanks. That sounds good to me. Let me get back to you on whether there are any other workarounds for mitigation of your issue. Apologies on that. If there aren't any feasible workarounds, I am good with merging this PR and including it in the next agent release.

I looked into if setting a larger volume operation timeout is supported via a Docker daemon configuration. Unfortunately, I don't think there's an available option. Other workarounds include bin-packing fewer tasks (with EFS vols) per instance, but this is not a good enough workaround to directly address the problem at hand. I will review the PR next.

[TheanLim]

Hi @callaway12, are you still able to address the review feedback? If you're blocked or don't have bandwidth, let us know — happy to help push it through. Thanks for the contribution!

[singholt]

Thanks for getting back on this. We are working on testing this change internally through some manual load testing.

[singholt]

Suggested change

	// Re-check that the volume still exists after acquiring the per-volume lock,
	// as Remove() may have deleted it while we were waiting.
	a.mapLock.RLock()
	if _, ok := a.volumes[r.Name]; !ok {
	a.mapLock.RUnlock()
	return nil, fmt.Errorf("volume %s was removed", r.Name)
	}
	a.mapLock.RUnlock()
	// Re-check under the map lock that the volume still exists AND that the lock we
	// are holding is still the one guarding it. A concurrent Remove()+Create() of the
	// same name replaces both the *types.Volume and its per-volume lock, so a bare
	// name-existence check would pass while `vol`/`volMu` point at the removed volume.
	// Re-fetch the live volume and verify lock identity; treat a mismatch as removal.
	a.mapLock.RLock()
	current, ok := a.volumes[r.Name]
	if !ok || a.getVolLock(r.Name) != volMu {
	a.mapLock.RUnlock()
	return nil, fmt.Errorf("volume %s was removed", r.Name)
	}
	vol = current
	a.mapLock.RUnlock()

[singholt]

Same applies to Unmount

[callaway12]

Applied. Two things I ended up doing beyond the suggestion after walking through the code:

• Also re-fetched volDriver from the current vol.Type, since a Remove+Create that changes the driver type on the same name would otherwise route the I/O through the pre-Remove driver. Rare, but the identity check made it cheap to close.
• Applied the same phase-2 identity check to Remove. A second concurrent Remove racing with a Create can cleanup/removeVolume the newly-created volume. It's a much narrower window in practice (didn't reproduce in 60k iterations of a stress test), but it's the same shape as the Mount/Unmount case and the fix costs nothing.

Added TestRemoveRecreateMountRace that catches the original Mount race by observing driver.Create's arguments against the live map entry — it fails on the previous code and passes with the identity check in place.

Ran go test -race -count=100 on the concurrency-sensitive tests, all green. Done in 6ac5a3c.

[callaway12]

Addressed together with the Mount thread — same phase-2 identity re-check pattern applied to Unmount (and Remove) in 6ac5a3c.