From a57527e4b0e44227863ce26ce66eb106eff8a766 Mon Sep 17 00:00:00 2001 From: jwhur <57657645+JaewonHur@users.noreply.github.com> Date: Tue, 27 Jan 2026 11:15:22 -0800 Subject: [PATCH] Refactor container lifecycle functions to perform scoped rollback on failure (#1080) - Closes #977. - Closes #1058. - Prevents unexpected removal of containers on bootstrapping and starting failures, by reorganizing error handling for container `run`, `start`, and `exec` so that error handling only unwinds that which was done in the current scope. - Relies on apple/containerization#495. --- Package.resolved | 6 +- Package.swift | 2 +- .../Container/ContainerRun.swift | 1 + .../Server/Containers/ContainersService.swift | 125 +++++++++--------- .../Server/ReservedVmnetNetwork.swift | 2 +- .../Subcommands/Containers/TestCLIExec.swift | 20 +++ .../Subcommands/Run/TestCLIRunLifecycle.swift | 29 ++++ Tests/CLITests/Utilities/CLITest.swift | 7 +- 8 files changed, 121 insertions(+), 71 deletions(-) diff --git a/Package.resolved b/Package.resolved index 33f6a178..035e298e 100644 --- a/Package.resolved +++ b/Package.resolved @@ -1,5 +1,5 @@ { - "originHash" : "6b2a03b8a5a190690d707d7c5454783bfed5c0c8b44490ef50ef4651038b88f9", + "originHash" : "ea5432dec5056581c1236ddc433fa8813eed012017e53b9e3a40abb648f2676b", "pins" : [ { "identity" : "async-http-client", @@ -15,8 +15,8 @@ "kind" : "remoteSourceControl", "location" : "https://github.com/apple/containerization.git", "state" : { - "revision" : "26f3dcc796c89baf729827770c011cd694debcbf", - "version" : "0.21.1" + "revision" : "fd62f311b480d1fb5ef243a65927da54b4eecfae", + "version" : "0.23.1" } }, { diff --git a/Package.swift b/Package.swift index d0bfc1b2..6aefeeb4 100644 --- a/Package.swift +++ b/Package.swift @@ -23,7 +23,7 @@ import PackageDescription let releaseVersion = ProcessInfo.processInfo.environment["RELEASE_VERSION"] ?? "0.0.0" let gitCommit = ProcessInfo.processInfo.environment["GIT_COMMIT"] ?? "unspecified" let builderShimVersion = "0.7.0" -let scVersion = "0.21.1" +let scVersion = "0.23.1" let package = Package( name: "container", diff --git a/Sources/ContainerCommands/Container/ContainerRun.swift b/Sources/ContainerCommands/Container/ContainerRun.swift index 3836d97b..f366acf7 100644 --- a/Sources/ContainerCommands/Container/ContainerRun.swift +++ b/Sources/ContainerCommands/Container/ContainerRun.swift @@ -161,6 +161,7 @@ extension Application { exitCode = try await io.handleProcess(process: process, log: log) } catch { + try? await container.delete() if error is ContainerizationError { throw error } diff --git a/Sources/Services/ContainerAPIService/Server/Containers/ContainersService.swift b/Sources/Services/ContainerAPIService/Server/Containers/ContainersService.swift index 64037384..30742b46 100644 --- a/Sources/Services/ContainerAPIService/Server/Containers/ContainersService.swift +++ b/Sources/Services/ContainerAPIService/Server/Containers/ContainersService.swift @@ -268,20 +268,21 @@ public actor ContainersService { /// Bootstrap the init process of the container. public func bootstrap(id: String, stdio: [FileHandle?]) async throws { self.log.debug("\(#function)") - do { - try await self.lock.withLock { context in - var state = try await self.getContainerState(id: id, context: context) + try await self.lock.withLock { context in + var state = try await self.getContainerState(id: id, context: context) - // We've already bootstrapped this container. Ideally we should be able to - // return some sort of error code from the sandbox svc to check here, but this - // is also a very simple check and faster than doing an rpc to get the same result. - if state.client != nil { - return - } + // We've already bootstrapped this container. Ideally we should be able to + // return some sort of error code from the sandbox svc to check here, but this + // is also a very simple check and faster than doing an rpc to get the same result. + if state.client != nil { + return + } - let path = self.containerRoot.appendingPathComponent(id) - let bundle = ContainerResource.Bundle(path: path) - let config = try bundle.configuration + let path = self.containerRoot.appendingPathComponent(id) + let bundle = ContainerResource.Bundle(path: path) + let config = try bundle.configuration + + do { try Self.registerService( plugin: self.runtimePlugins.first { $0.name == config.runtimeHandler }!, loader: self.pluginLoader, @@ -303,14 +304,17 @@ public actor ContainersService { state.client = sandboxClient await self.setContainerState(id, state, context: context) - } - } catch { - do { - try await _cleanup(id: id) } catch { - self.log.error("failed to cleanup container \(id) after bootstrap failure: \(error)") + let label = Self.fullLaunchdServiceLabel( + runtimeName: config.runtimeHandler, + instanceId: id + ) + + await self.exitMonitor.stopTracking(id: id) + try? ServiceManager.deregister(fullServiceLabel: label) + + throw error } - throw error } } @@ -324,21 +328,12 @@ public actor ContainersService { self.log.debug("\(#function)") let state = try self._getContainerState(id: id) - do { - let client = try state.getClient() - try await client.createProcess( - processID, - config: config, - stdio: stdio - ) - } catch { - do { - try await _cleanup(id: id) - } catch { - self.log.error("failed to cleanup container \(id) after start failure: \(error)") - } - throw error - } + let client = try state.getClient() + try await client.createProcess( + processID, + config: config, + stdio: stdio + ) } /// Start a process in a container. This can either be a process created via @@ -347,43 +342,43 @@ public actor ContainersService { public func startProcess(id: String, processID: String) async throws { self.log.debug("\(#function)") - do { - try await self.lock.withLock { context in - var state = try await self.getContainerState(id: id, context: context) + try await self.lock.withLock { context in + var state = try await self.getContainerState(id: id, context: context) - let isInit = Self.isInitProcess(id: id, processID: processID) - if state.snapshot.status == .running && isInit { - return - } - - let client = try state.getClient() - try await client.startProcess(processID) - - if isInit { - let log = self.log - let waitFunc: ExitMonitor.WaitHandler = { - log.info("registering container \(id) with exit monitor") - let code = try await client.wait(id) - log.info("container \(id) finished in exit monitor, exit code \(code)") - - return code - } - try await self.exitMonitor.track(id: id, waitingOn: waitFunc) - - let sandboxSnapshot = try await client.state() - state.snapshot.status = .running - state.snapshot.networks = sandboxSnapshot.networks - state.snapshot.startedDate = Date() - await self.setContainerState(id, state, context: context) - } + let isInit = Self.isInitProcess(id: id, processID: processID) + if state.snapshot.status == .running && isInit { + return } - } catch { + + let client = try state.getClient() + try await client.startProcess(processID) + + guard isInit else { + return + } + do { - try await _cleanup(id: id) + let log = self.log + let waitFunc: ExitMonitor.WaitHandler = { + log.info("registering container \(id) with exit monitor") + let code = try await client.wait(id) + log.info("container \(id) finished in exit monitor, exit code \(code)") + + return code + } + try await self.exitMonitor.track(id: id, waitingOn: waitFunc) + + let sandboxSnapshot = try await client.state() + state.snapshot.status = .running + state.snapshot.networks = sandboxSnapshot.networks + state.snapshot.startedDate = Date() + await self.setContainerState(id, state, context: context) } catch { - self.log.error("failed to cleanup container \(id) after start failure: \(error)") + await self.exitMonitor.stopTracking(id: id) + try? await client.stop(options: ContainerStopOptions.default) + + throw error } - throw error } } diff --git a/Sources/Services/ContainerNetworkService/Server/ReservedVmnetNetwork.swift b/Sources/Services/ContainerNetworkService/Server/ReservedVmnetNetwork.swift index 0815c183..f1c74730 100644 --- a/Sources/Services/ContainerNetworkService/Server/ReservedVmnetNetwork.swift +++ b/Sources/Services/ContainerNetworkService/Server/ReservedVmnetNetwork.swift @@ -181,7 +181,7 @@ public final class ReservedVmnetNetwork: Network { let prefixIpv6Bytes = withUnsafeBytes(of: prefixAddr.__u6_addr.__u6_addr8) { Array($0) } - let prefixIpv6Addr = IPv6Address(prefixIpv6Bytes) + let prefixIpv6Addr = try IPv6Address(prefixIpv6Bytes) let runningV6Subnet = try CIDRv6(prefixIpv6Addr, prefix: prefix) log.info( diff --git a/Tests/CLITests/Subcommands/Containers/TestCLIExec.swift b/Tests/CLITests/Subcommands/Containers/TestCLIExec.swift index e175f5df..16353253 100644 --- a/Tests/CLITests/Subcommands/Containers/TestCLIExec.swift +++ b/Tests/CLITests/Subcommands/Containers/TestCLIExec.swift @@ -107,4 +107,24 @@ class TestCLIExecCommand: CLITest { return } } + + @Test func testExecOnExitingContainer() throws { + do { + let name = getTestName() + try doLongRun(name: name, containerArgs: ["sh"], autoRemove: false) + defer { + try? doRemove(name: name) + } + // Give time for container process to exit due to no stdin + sleep(1) + + try doStart(name: name) + do { + _ = try doExec(name: name, cmd: ["sleep", "infinity"]) + } catch CLIError.executionFailed(let message) { + #expect(message.contains("is not running")) + } + #expect(try getContainerStatus(name) == "stopped") + } + } } diff --git a/Tests/CLITests/Subcommands/Run/TestCLIRunLifecycle.swift b/Tests/CLITests/Subcommands/Run/TestCLIRunLifecycle.swift index 7a5fc971..0862848a 100644 --- a/Tests/CLITests/Subcommands/Run/TestCLIRunLifecycle.swift +++ b/Tests/CLITests/Subcommands/Run/TestCLIRunLifecycle.swift @@ -14,6 +14,7 @@ // limitations under the License. //===----------------------------------------------------------------------===// +import ContainerizationError import Testing class TestCLIRunLifecycle: CLITest { @@ -83,4 +84,32 @@ class TestCLIRunLifecycle: CLITest { try self.doStop(name: name) } } + + @Test func testStartPortBindFails() async throws { + let port = UInt16.random(in: 50000..<60000) + + let name = getTestName() + try self.doCreate(name: name, ports: ["\(port)"]) + defer { + try? self.doRemove(name: name) + } + + let server = "\(name)-server" + try doLongRun( + name: server, + image: "docker.io/library/python:alpine", + args: ["--publish", "\(port):\(port)"], + containerArgs: ["python3", "-m", "http.server", "\(port)"] + ) + defer { + try? doStop(name: server) + } + + #expect(throws: CLIError.self) { + try doStart(name: name) + } + + let status = try getContainerStatus(name) + #expect(status == "stopped") + } } diff --git a/Tests/CLITests/Utilities/CLITest.swift b/Tests/CLITests/Utilities/CLITest.swift index 7f94cf37..59299464 100644 --- a/Tests/CLITests/Utilities/CLITest.swift +++ b/Tests/CLITests/Utilities/CLITest.swift @@ -269,7 +269,8 @@ class CLITest { image: String? = nil, args: [String]? = nil, volumes: [String] = [], - networks: [String] = [] + networks: [String] = [], + ports: [String] = [] ) throws { let image = image ?? alpine let args: [String] = args ?? ["sleep", "infinity"] @@ -288,6 +289,10 @@ class CLITest { arguments += ["--network", network] } + for port in ports { + arguments += ["--publish", "\(port):\(port)"] + } + arguments += [image] + args let (_, _, error, status) = try run(arguments: arguments)