-
Notifications
You must be signed in to change notification settings - Fork 530
Automatically clean up anonymous volumes for containers with --rm flag #839
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Automatically clean up anonymous volumes for containers with --rm flag #839
Conversation
ef8edc7 to
b1d37b9
Compare
|
|
||
| private func handleContainerExit(id: String, code: ExitStatus? = nil) async throws { | ||
| try await self.lock.withLock { [self] context in | ||
| let shouldCleanupVolumes = try await self.lock.withLock { [self] context in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Curious why we don't just do the cleanup in handleContainerExit itself
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Once the lock is released if autoRemove was true, the container bundle has already been deleted by cleanup(), so we can't call getContainerCreationOptions() again to check the flag, the file is gone. So we need to capture the decision while the container still exists (inside the lock) then act on it outside the lock
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What I mean is if cleanup() is called we know that autoRemove was supplied, so why not just do it in there?
| public static let anonymousLabel = "com.apple.container.resource.anonymous" | ||
|
|
||
| /// Reserved label key for storing the container ID that created this volume | ||
| public static let createdByLabel = "com.apple.container.resource.created-by" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My concern with this approach is that the ID of a container isn't really a unique ID across all time. In this case we might be able to get by with it but in general if you store the ID test, that could be one container on one day and a totally different container a week later.
Would I make more sense for us, when a container is removed in the API server, to quickly scan all containers for volume references, find all unreferenced volumes, and then prune those that are marked anonymous? Given what you have in cleanupAnonymousVolumes() this might be a simple change?
Type of Change
Motivation and Context
Adds automatic cleanup of anonymous volumes when containers started with the
--rmflag exit, preventing orphaned volumes from accumulating and consuming disk space.Testing