Skip to content

APIServer: Make container deletes a little more robust #567

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

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

APIServer: Make container deletes a little more robust #567

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
105 changes: 77 additions & 28 deletions Sources/APIServer/Containers/ContainersService.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,11 @@ actor ContainersService {
throw ContainerizationError(.exists, message: "hostname(s) already exist: \(conflictingHostnames)")
}

self.containers[configuration.id] = ContainerSnapshot(configuration: configuration, status: .stopped, networks: [])
self.containers[configuration.id] = ContainerSnapshot(
configuration: configuration,
status: .stopped,
networks: []
)

let runtimePlugin = self.runtimePlugins.filter {
$0.name == configuration.runtimeHandler
Expand Down Expand Up @@ -207,36 +211,63 @@ actor ContainersService {
/// Delete a container and its resources.
public func delete(id: String, force: Bool) async throws {
self.log.debug("\(#function)")
let item = try self._get(id: id)
switch item.status {
case .running:
if !force {

try await lock.withLock { context in
let item = try await self.get(id: id, context: context)
switch item.status {
case .running:
let autoRemove = try await self.getContainerCreationOptions(id: id).autoRemove
// Check the state from the sb services point of view.
let client = SandboxClient(
id: item.configuration.id,
runtime: item.configuration.runtimeHandler
)
let ctrState = try await client.state()
// Someone asked to delete a container before the process exited XPC event
// came through. The sandbox process says the container is dead, but our
// state here isn't updated yet. We are perfectly fine to delete the container,
// so let's update our state and do just that.
if ctrState.status == .stopped {
let snapshot = ContainerSnapshot(
configuration: item.configuration,
status: .stopped,
networks: []
)
await self.setContainer(id, snapshot, context: context)
if !autoRemove {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

perhaps a one-line explanation of why we don't invoke this for the auto-remove case? who's responsible for cleanup?

try await self.cleanup(id: id, item: item, context: context)
}
return
}

if !force {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: perhaps L243-L258 in an else, and then 237-240 can go away in favor of 259-262?

throw ContainerizationError(
.invalidState,
message: "container \(id) is running and can not be deleted"
)
}

let opts = ContainerStopOptions(
timeoutInSeconds: 5,
signal: SIGKILL
)
try await self._stop(
id: id,
runtimeHandler: item.configuration.runtimeHandler,
options: opts
)
if autoRemove {
return
}
try await self.cleanup(id: id, item: item, context: context)
case .stopped:
try await self.cleanup(id: id, item: item, context: context)
default:
throw ContainerizationError(
.invalidState,
message: "container \(id) is \(item.status) and can not be deleted"
message: "container \(id) is in state \(item.status) and can not be deleted"
)
}
let autoRemove = try getContainerCreationOptions(id: id).autoRemove
let opts = ContainerStopOptions(
timeoutInSeconds: 5,
signal: SIGKILL
)
try await self._stop(
id: id,
runtimeHandler: item.configuration.runtimeHandler,
options: opts
)
if autoRemove {
return
}
try self._cleanup(id: id, item: item)
case .stopping:
throw ContainerizationError(
.invalidState,
message: "container \(id) is \(item.status) and can not be deleted"
)
default:
try self._cleanup(id: id, item: item)
}
}

Expand Down Expand Up @@ -284,6 +315,21 @@ actor ContainersService {
let snapshot = ContainerSnapshot(configuration: item.configuration, status: .stopped, networks: [])
await self.setContainer(id, snapshot, context: context)

do {
let client = SandboxClient(
id: item.configuration.id,
runtime: item.configuration.runtimeHandler
)
try await client.shutdown()
} catch {
self.log.error(
"Failed to shut down SandboxService after process exit",
metadata: [
"id": .string(id),
"error": .string(String(describing: error)),
])
}

let options = try getContainerCreationOptions(id: id)
if options.autoRemove {
try self.cleanup(id: id, item: item, context: context)
Expand All @@ -303,7 +349,10 @@ actor ContainersService {
self.log.info("Handling container \(id) Start.")
do {
let currentSnapshot = try self.get(id: id, context: context)
let client = SandboxClient(id: currentSnapshot.configuration.id, runtime: currentSnapshot.configuration.runtimeHandler)
let client = SandboxClient(
id: currentSnapshot.configuration.id,
runtime: currentSnapshot.configuration.runtimeHandler
)
let sandboxSnapshot = try await client.state()
let snapshot = ContainerSnapshot(configuration: currentSnapshot.configuration, status: .running, networks: sandboxSnapshot.networks)
await self.setContainer(id, snapshot, context: context)
Expand Down
9 changes: 9 additions & 0 deletions Sources/ContainerClient/SandboxClient.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,6 +175,15 @@ extension SandboxClient {
return fh
}

package func shutdown() async throws {
let request = XPCMessage(route: SandboxRoutes.shutdown.rawValue)

let client = createClient()
defer { client.close() }

_ = try await client.send(request)
}

private func createClient() -> XPCClient {
XPCClient(service: machServiceLabel)
}
Expand Down
2 changes: 2 additions & 0 deletions Sources/ContainerClient/SandboxRoutes.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,4 +35,6 @@ public enum SandboxRoutes: String {
case exec = "com.apple.container.sandbox/exec"
/// Dial a vsock port in the sandbox.
case dial = "com.apple.container.sandbox/dial"
/// Shutdown the sandbox service process.
case shutdown = "com.apple.container.sandbox/shutdown"
}
5 changes: 4 additions & 1 deletion Sources/ContainerXPC/XPCClient.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,10 @@ extension XPCClient {
group.addTask {
try await Task.sleep(for: responseTimeout)
let route = message.string(key: XPCMessage.routeKey) ?? "nil"
throw ContainerizationError(.internalError, message: "XPC timeout for request to \(self.service)/\(route)")
throw ContainerizationError(
.internalError,
message: "XPC timeout for request to \(self.service)/\(route)"
)
}
}

Expand Down
1 change: 1 addition & 0 deletions Sources/Helpers/RuntimeLinux/RuntimeLinuxHelper.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,7 @@ struct RuntimeLinuxHelper: AsyncParsableCommand {
SandboxRoutes.wait.rawValue: server.wait,
SandboxRoutes.start.rawValue: server.startProcess,
SandboxRoutes.dial.rawValue: server.dial,
SandboxRoutes.shutdown.rawValue: server.shutdown,
],
log: log
)
Expand Down
Loading