Named Volumes #362
Named Volumes #362
Conversation
realrajaryan
force-pushed
the
users/realrajaryan/named-volumes
branch
from
ce8c885 to
32360ad
Compare
realrajaryan
force-pushed
the
users/realrajaryan/named-volumes
branch
2 times, most recently
from
7f696c0 to
1886b21
Compare
realrajaryan
force-pushed
the
users/realrajaryan/named-volumes
branch
from
1886b21 to
a20913f
Compare
realrajaryan
force-pushed
the
users/realrajaryan/named-volumes
branch
from
8082c9d to
f84e06b
Compare
realrajaryan
force-pushed
the
users/realrajaryan/named-volumes
branch
2 times, most recently
from
a2ae64b to
bc485a7
Compare
jglogan
left a comment
jglogan
left a comment
There was a problem hiding this comment.
lgtm - on to the sandbox work!
realrajaryan
force-pushed
the
users/realrajaryan/named-volumes
branch
2 times, most recently
from
0e34a15 to
a637e3b
Compare
realrajaryan
force-pushed
the
users/realrajaryan/named-volumes
branch
2 times, most recently
from
6d6be85 to
12ef2af
Compare
realrajaryan
force-pushed
the
users/realrajaryan/named-volumes
branch
from
06da2d9 to
9a8d691
Compare
realrajaryan
changed the title
| } | ||
|
|
||
| public func list() async throws -> [Volume] { | ||
| try await store.list() |
There was a problem hiding this comment.
Does this need to be in the lock?
There was a problem hiding this comment.
I suppose it could be but the store is an actor with no awaits in it
|
|
||
| public func delete(name: String) async throws { | ||
| try await lock.withLock { _ in | ||
| try await self._delete(name: name) |
There was a problem hiding this comment.
This is very much a nit - but for code clarity
should we make it such that the _foo methods takes in an object of type AsyncLock.Context
So these invocations become
try await lock.withLock { ctx in
try await self._delete(name: name, lock: ctx)
}
Dont need to make this update in this PR, it can be done as a quick follow up
adityaramani
left a comment
adityaramani
left a comment
There was a problem hiding this comment.
Just had one question
realrajaryan
force-pushed
the
users/realrajaryan/named-volumes
branch
from
8cf2da1 to
4990c52
Compare
| } | ||
|
|
||
| // Check if volume is in use by any container atomically | ||
| let volumeInUse = try await containersService.withContainerList { containers in |
There was a problem hiding this comment.
Shouldn't the volume delete itself be executed in the block so that there's no chance of a reference being created to the volume after this block exits? Right now it seems that if another thread is creating a container that attaches the volume at the same time we're at line 197, there's a race still.
| let volumeInUse = try await containersService.withContainerList { containers in | ||
| for container in containers { | ||
| for mount in container.configuration.mounts { | ||
| if mount.isVolume && mount.volumeName == name { | ||
| return true | ||
| } | ||
| } | ||
| } | ||
| return false | ||
| } | ||
|
|
||
| if volumeInUse { | ||
| throw VolumeError.volumeInUse(name) | ||
| } | ||
|
|
||
| try await store.delete(name) | ||
| try removeVolumeDirectory(for: name) |
There was a problem hiding this comment.
These lines should be inside the closure itself (for whatever reason John's comment isn't popping up in this view, but I'm mirroring his comment)
realrajaryan
force-pushed
the
users/realrajaryan/named-volumes
branch
from
4990c52 to
6432bac
Compare
jglogan
left a comment
jglogan
left a comment
There was a problem hiding this comment.
Need to fix build errors after pulling volume delete into closure.
realrajaryan
force-pushed
the
users/realrajaryan/named-volumes
branch
15 times, most recently
from
52dcb0e to
3a12bb0
Compare
realrajaryan
force-pushed
the
users/realrajaryan/named-volumes
branch
from
3a12bb0 to
8ff6472
Compare
4 participants
Closes #339.
This change adds named volume support to container, providing volume management CLI commands -
create, delete, list and inspect. The implementation uses EXT4 block-based persistent storage with a newVolumesServiceactor for thread-safe operations, integrates seamlessly with the existing container mount system through a new.volumefilesystem type, and provides atomic volume operations with XPC-based API communication. Volumes are stored in isolated directories with configurable sizes (default 512GB) and include proper cleanup and container usage tracking for safe deletion.Example Usage: