Skip to content

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.

Sign up for GitHub

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

Open
realrajaryan wants to merge 1 commit into
base: main
Choose a base branch
from
+135 −60
Open

Automatically clean up anonymous volumes for containers with --rm flag #839

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions Sources/ContainerClient/Core/Volume.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,10 +62,18 @@ extension Volume {
/// Reserved label key for marking anonymous volumes
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"
Copy link
Contributor

@jglogan jglogan Nov 14, 2025

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?


/// Whether this is an anonymous volume (detected via label)
public var isAnonymous: Bool {
labels[Self.anonymousLabel] != nil
}

/// The container ID that created this volume
public var createdByContainerID: String? {
labels[Self.createdByLabel]
}
}

/// Error types for volume operations.
Expand Down
9 changes: 6 additions & 3 deletions Sources/ContainerClient/Utility.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,7 +168,7 @@ public struct Utility {
case .filesystem(let fs):
resolvedMounts.append(fs)
case .volume(let parsed):
let volume = try await getOrCreateVolume(parsed: parsed)
let volume = try await getOrCreateVolume(parsed: parsed, containerID: id)
let volumeMount = Filesystem.volume(
name: parsed.name,
format: volume.format,
Expand Down Expand Up @@ -315,8 +315,11 @@ public struct Utility {

/// Gets an existing volume or creates it if it doesn't exist.
/// Shows a warning for named volumes when auto-creating.
private static func getOrCreateVolume(parsed: ParsedVolume) async throws -> Volume {
let labels = parsed.isAnonymous ? [Volume.anonymousLabel: ""] : [:]
private static func getOrCreateVolume(parsed: ParsedVolume, containerID: String) async throws -> Volume {
var labels = parsed.isAnonymous ? [Volume.anonymousLabel: ""] : [:]
if parsed.isAnonymous {
labels[Volume.createdByLabel] = containerID
}

let volume: Volume
do {
Expand Down
64 changes: 60 additions & 4 deletions Sources/Services/ContainerAPIService/Containers/ContainersService.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -434,12 +434,17 @@ public actor ContainersService {
}

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
Copy link
Member

@dcantah dcantah Nov 4, 2025

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

Copy link
Contributor Author

@realrajaryan realrajaryan Nov 5, 2025

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

Copy link
Member

@dcantah dcantah Nov 5, 2025

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?

try await handleContainerExit(id: id, code: code, context: context)
}

// Lock released, safe for XPC calls
if shouldCleanupVolumes {
await self.cleanupAnonymousVolumes(forContainer: id)
}
}

private func handleContainerExit(id: String, code: ExitStatus?, context: AsyncLock.Context) async throws {
private func handleContainerExit(id: String, code: ExitStatus?, context: AsyncLock.Context) async throws -> Bool {
if let code {
self.log.info("Handling container \(id) exit. Code \(code)")
}
Expand All @@ -448,11 +453,11 @@ public actor ContainersService {
do {
state = try self.getContainerState(id: id, context: context)
if state.snapshot.status == .stopped {
return
return false
}
} catch {
// Was auto removed by the background thread, nothing for us to do.
return
return false
}

await self.exitMonitor.stopTracking(id: id)
Expand All @@ -475,7 +480,9 @@ public actor ContainersService {
let options = try getContainerCreationOptions(id: id)
if options.autoRemove {
try await self.cleanup(id: id, context: context)
return true
}
return false
}

private static func fullLaunchdServiceLabel(runtimeName: String, instanceId: String) -> String {
Expand Down Expand Up @@ -511,6 +518,55 @@ public actor ContainersService {
try await self._cleanup(id: id)
}

/// Clean up anonymous volumes created by a container
private func cleanupAnonymousVolumes(forContainer id: String) async {
do {
let allVolumes = try await ClientVolume.list()
let anonymousVolumes = allVolumes.filter { $0.isAnonymous && $0.createdByContainerID == id }

guard !anonymousVolumes.isEmpty else { return }

await withTaskGroup(of: (String, Error?).self) { group in
for volume in anonymousVolumes {
group.addTask {
do {
try await ClientVolume.delete(name: volume.name)
return (volume.name, nil)
} catch {
return (volume.name, error)
}
}
}

for await (volumeName, error) in group {
if let error = error {
self.log.warning(
"Failed to delete anonymous volume",
metadata: [
"volume": "\(volumeName)",
"container": "\(id)",
"error": "\(error)",
])
} else {
self.log.info(
"Deleted anonymous volume",
metadata: [
"volume": "\(volumeName)",
"container": "\(id)",
])
}
}
}
} catch {
self.log.error(
"Failed to query anonymous volumes for cleanup",
metadata: [
"container": "\(id)",
"error": "\(error)",
])
}
}

private func getContainerCreationOptions(id: String) throws -> ContainerCreateOptions {
let path = self.containerRoot.appendingPathComponent(id)
let bundle = ContainerClient.Bundle(path: path)
Expand Down
112 changes: 60 additions & 52 deletions Tests/CLITests/Subcommands/Volumes/TestCLIAnonymousVolumes.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,13 +99,13 @@ class TestCLIAnonymousVolumes: CLITest {
let (_, _, _) = (try? run(arguments: args)) ?? ("", "", 1)
}

@Test func testAnonymousVolumeCreationAndPersistence() async throws {
@Test func testAnonymousVolumeCreationAndAutoCleanup() async throws {
let testName = getTestName()
let containerName = "\(testName)_c1"

defer {
doRemoveIfExists(name: containerName, force: true)
// Clean up anonymous volumes
// Clean up any remaining anonymous volumes
if let volumes = try? getAnonymousVolumeNames() {
volumes.forEach { doVolumeDeleteIfExists(name: $0) }
}
Expand All @@ -129,7 +129,7 @@ class TestCLIAnonymousVolumes: CLITest {

#expect(status == 0, "container run should succeed")

// Give time for container removal to complete
// Give time for cleanup to complete
try await Task.sleep(for: .seconds(1))

// Verify container was removed
Expand All @@ -138,9 +138,9 @@ class TestCLIAnonymousVolumes: CLITest {
.filter { $0.contains(containerName) }
#expect(containers.isEmpty, "container should be removed with --rm")

// Verify anonymous volume persists (no auto-cleanup)
// Verify anonymous volume was auto-cleaned up
let afterCount = try getAnonymousVolumeNames().count
#expect(afterCount == beforeCount + 1, "anonymous volume should persist even with --rm")
#expect(afterCount == beforeCount, "anonymous volume should be auto-cleaned up with --rm")
}

@Test func testAnonymousVolumePersistenceWithoutRm() throws {
Expand Down Expand Up @@ -191,43 +191,6 @@ class TestCLIAnonymousVolumes: CLITest {
doVolumeDeleteIfExists(name: volumeID)
}

@Test func testMultipleAnonymousVolumes() async throws {
let testName = getTestName()
let containerName = "\(testName)_c1"

defer {
doRemoveIfExists(name: containerName, force: true)
// Clean up anonymous volumes
if let volumes = try? getAnonymousVolumeNames() {
volumes.forEach { doVolumeDeleteIfExists(name: $0) }
}
}

let beforeCount = try getAnonymousVolumeNames().count

// Run with multiple anonymous volumes
let (_, _, status) = try run(arguments: [
"run",
"--rm",
"--name",
containerName,
"-v", "/data1",
"-v", "/data2",
"-v", "/data3",
alpine,
"sh", "-c", "ls -d /data*",
])

#expect(status == 0, "container run should succeed")

// Give time for container removal
try await Task.sleep(for: .seconds(1))

// All 3 volumes should persist (no auto-cleanup)
let afterCount = try getAnonymousVolumeNames().count
#expect(afterCount == beforeCount + 3, "all 3 anonymous volumes should persist")
}

@Test func testAnonymousMountSyntax() async throws {
let testName = getTestName()
let containerName = "\(testName)_c1"
Expand Down Expand Up @@ -255,12 +218,12 @@ class TestCLIAnonymousVolumes: CLITest {

#expect(status == 0, "container run with --mount should succeed")

// Give time for container removal
try await Task.sleep(for: .seconds(1))
// Give time for container removal and volume cleanup
try await Task.sleep(for: .seconds(2))

// Anonymous volume should persist (no auto-cleanup)
// Anonymous volume should be auto-cleaned up with --rm
let afterCount = try getAnonymousVolumeNames().count
#expect(afterCount == beforeCount + 1, "anonymous volume should persist")
#expect(afterCount == beforeCount, "anonymous volume should be auto-cleaned up with --rm")
}

@Test func testAnonymousVolumeUUIDFormat() throws {
Expand Down Expand Up @@ -394,15 +357,15 @@ class TestCLIAnonymousVolumes: CLITest {

#expect(status == 0, "container run should succeed")

// Give time for container removal
try await Task.sleep(for: .seconds(1))
// Give time for container removal and volume cleanup
try await Task.sleep(for: .seconds(2))

// Named volume should still exist
let namedExists = try volumeExists(name: namedVolumeName)
#expect(namedExists, "named volume should persist")

let afterAnonCount = try getAnonymousVolumeNames().count
#expect(afterAnonCount == beforeAnonCount + 1, "anonymous volume should persist")
#expect(afterAnonCount == beforeAnonCount, "anonymous volume should be auto-cleaned up with --rm")
}

@Test func testAnonymousVolumeManualDeletion() throws {
Expand Down Expand Up @@ -463,8 +426,8 @@ class TestCLIAnonymousVolumes: CLITest {

#expect(status == 0, "detached container run should succeed")

// Wait for container to exit
try await Task.sleep(for: .seconds(3))
// Wait for container to exit and volume cleanup
try await Task.sleep(for: .seconds(4))

// Container should be removed
let (lsOutput, _, _) = try run(arguments: ["ls", "-a"])
Expand All @@ -473,6 +436,51 @@ class TestCLIAnonymousVolumes: CLITest {
#expect(containers.isEmpty, "container should be auto-removed")

let afterCount = try getAnonymousVolumeNames().count
#expect(afterCount == beforeCount + 1, "anonymous volume should persist")
#expect(afterCount == beforeCount, "anonymous volume should be auto-cleaned up with --rm")
}

@Test func testMultipleAnonymousVolumesAutoCleanupWithRm() async throws {
let testName = getTestName()
let containerName = "\(testName)_c1"

defer {
doRemoveIfExists(name: containerName, force: true)
// Clean up any remaining anonymous volumes
if let volumes = try? getAnonymousVolumeNames() {
volumes.forEach { doVolumeDeleteIfExists(name: $0) }
}
}

let beforeCount = try getAnonymousVolumeNames().count

// Run container with --rm and 5 anonymous volumes
let (_, _, status) = try run(arguments: [
"run",
"--rm",
"--name",
containerName,
"-v", "/data1",
"-v", "/data2",
"-v", "/data3",
"-v", "/data4",
"-v", "/data5",
alpine,
"sh", "-c", "echo 'test' > /data1/file.txt && cat /data1/file.txt",
])

#expect(status == 0, "container run should succeed")

// Wait for cleanup to complete
try await Task.sleep(for: .seconds(2))

// Verify container was removed
let (lsOutput, _, _) = try run(arguments: ["ls", "-a"])
let containers = lsOutput.components(separatedBy: .newlines)
.filter { $0.contains(containerName) }
#expect(containers.isEmpty, "container should be removed with --rm")

// Verify all 5 anon volumes were cleaned up
let afterCount = try getAnonymousVolumeNames().count
#expect(afterCount == beforeCount, "all 5 anonymous volumes should be auto-cleaned up with --rm")
}
}
2 changes: 1 addition & 1 deletion docs/command-reference.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -711,7 +711,7 @@ container run -v $VOL:/data alpine
container volume rm $VOL
```

**Note**: Unlike Docker, anonymous volumes do NOT auto-cleanup with `--rm`. Manual deletion is required.
**Note**: Anonymous volumes auto-cleanup when containers exit with `--rm` flag. Without `--rm`, they persist and require manual deletion.

### `container volume delete (rm)`

Expand Down