Add support for anonymous volumes #768
Conversation
| /// Volume storage management utilities. | ||
| public struct VolumeStorage { | ||
| public static let volumeNamePattern = "^[A-Za-z0-9][A-Za-z0-9_.-]*$" | ||
| public static let anonymousVolumePattern = "^anon-[0-9a-hjkmnp-tv-z]{26}$" |
There was a problem hiding this comment.
Why not just use plain UUIDv4? Containers do the same if no --name is provided, and it keeps the PR simpler.
|
|
||
| // Add reserved label for anonymous volumes to persist the flag | ||
| var finalLabels = labels | ||
| if isAnonymous { |
There was a problem hiding this comment.
What purpose do these labels serve?
| labels: [String: String] = [:], | ||
| options: [String: String] = [:], | ||
| sizeInBytes: UInt64? = nil, | ||
| isAnonymous: Bool = false, |
There was a problem hiding this comment.
I think I'd prefer that we don't do what Docker does, at least for now:
Just like named volumes, anonymous volumes persist even if you remove the container that uses them, except if you use the --rm flag when creating the container, in which case the anonymous volume associated with the container is destroyed.
IMO it introduces a bunch of server-side complexity that we can add later should we need it. Perhaps we can approach the problem of pruning such volumes creatively to make up for the lack of the --rm anonymous volume feature.
There was a problem hiding this comment.
I always wondered what the use case is/was with respect to anonymous not being tied to workload.
There was a problem hiding this comment.
Is the complexity for the server that much though? I think we're (as we've found 😄) somewhat beholden to the behavior of other tools regardless, and Docker's behavior somewhat makes sense to me here. I think the outcome of not following this --rm behavior is a little more devious than others (like not supporting some flag they did, which is minor in most cases) as if folks have gotten used to --rm being the "nuke this thing please" button, if we didn't follow suit they may be surprised after 100 container runs with -v /whatever to find they have a bunch of volumes filling up their disk.
There was a problem hiding this comment.
It's not whether it's too complex for the server. I'd rather do it as two smaller PRs if we're going to do it.
I also don't like us having to drop a bunch of extra fields in our core structs for anonymous and have to broaden the API when we're already using reserved labels for resource scope/role hints.
Let's get the basic stuff in place and do the cleanup in a follow-up PR.
I always wondered what the use case is/was with respect to anonymous not being tied to workload.
"I don't care about the volume name but I want the data around for later."
I agree that it's a bit of a tenuous use case.
realrajaryan
force-pushed
the
users/realrajaryan/anon-volumes
branch
from
aaea539 to
66cc250
Compare
| /// Volume storage management utilities. | ||
| public struct VolumeStorage { | ||
| public static let volumeNamePattern = "^[A-Za-z0-9][A-Za-z0-9_.-]*$" | ||
| public static let anonymousVolumePattern = "^anon-[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$" |
There was a problem hiding this comment.
My preference is for no anon-. Then unnamed containers and volumes follow the same naming convention.
|
|
||
| extension Volume { | ||
| /// Reserved label key for marking anonymous volumes | ||
| public static let anonymousLabel = "com.apple.container.volume.anonymous" |
There was a problem hiding this comment.
Let's discuss using either com.apple.container.resource.role=anonymous, or com.apple.container.resource.anonymous like you're using it now.
Sources/ContainerClient/Parser.swift
Outdated
| case 1: | ||
| throw ContainerizationError(.invalidArgument, message: "anonymous volumes are not supported") | ||
| // Anonymous volume: -v /path | ||
| // Generate a ULID-based name for the anonymous volume |
There was a problem hiding this comment.
It's a UUID now, could just say "Generate a random name"
| resolvedMounts.append(fs) | ||
| case .volume(let parsed): | ||
| do { | ||
| // Try to inspect the volume first |
There was a problem hiding this comment.
Could we just check whether the volume is anonymous here, and if so create it, instead of all the retroactive checking in the exception handler?
| return | ||
| } | ||
|
|
||
| // Sort volumes by creation time (newest first) |
There was a problem hiding this comment.
For the future, we should probably sort everything this way.
|
|
||
| extension Volume { | ||
| var asRow: [String] { | ||
| let volumeType = self.isAnonymous ? "anonymous" : "named" |
There was a problem hiding this comment.
This seems fine. If we have this, anon- on the name really does seem superfluous.
realrajaryan
force-pushed
the
users/realrajaryan/anon-volumes
branch
from
24551ee to
fced10e
Compare
4 participants
Type of Change
Motivation and Context
Adds support for anonymous volumes. Users can now create volumes without explicit naming using
-v /pathand--mount type=volume,dst=/pathsyntax.Usage:
Testing