From d9aaa4412eb2e5c64362d80aecd6979be41e2af0 Mon Sep 17 00:00:00 2001 From: Chris George Date: Sat, 2 May 2026 14:28:16 -0700 Subject: [PATCH 1/2] feat(api): add RestartPolicy + --restart flag (data-shape only) Adds a new public 'RestartPolicy' type, a 'restartPolicy' field on 'ContainerCreateOptions', and a '--restart' CLI flag on 'container run'. Motivation ---------- External orchestrators that drive the API server (the canonical use case is a Compose-spec orchestrator implementing 'service.restart: always|on-failure|unless-stopped|no') need to declare a restart policy at container creation time. Today there is no contract: the client cannot tell the daemon what to do when a container exits, and even if the client polls for exits and re-launches itself, that behavior cannot survive a client restart. Surfacing the policy at the API boundary lets a future restart manager honor the policy without another wire-format break. Scope of this PR (deliberately minimal) --------------------------------------- This PR is data-shape + CLI parsing only. It adds: - RestartPolicy { mode: { no, always, on-failure, unless-stopped }, maxRetries }, a Sendable Codable struct. - 'restartPolicy: RestartPolicy?' optional field on ContainerCreateOptions, defaulted nil at all existing construction sites. - '--restart ' option on Flags.Management. - 'parseRestartPolicy(_:)' helper on Application.Run that converts the string spec to RestartPolicy?, with on-failure[:N] support. It does NOT add a restart manager. The daemon stores the policy on ContainerCreateOptions and never observes container exits to enforce it. Wiring the actual observer + re-launch loop is a deferred follow-up that will land as a separate PR. Why ship a record-only field? ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ A restart manager is non-trivial: it needs to subscribe to exit notifications, track per-container retry counts, distinguish 'we asked it to stop' from 'it crashed', integrate with auto-remove, and handle daemon restarts. We would rather have that discussion separately, against a concrete companion design issue, than couple it to the SDK shape PR. Reserving the SDK shape now lets downstream tools start coding against the field with the documented 'not yet enforced' guarantee inline; flipping the implementation on later does not require another SDK shape break. Wire compatibility ------------------ ContainerCreateOptions is marshaled as Codable JSON over XPC. Adding an optional field is forward-compatible: - Older clients reading from a newer server: ignore the new key. - Newer clients reading from an older server: decode restartPolicy as nil. Files ----- - Sources/ContainerResource/Container/RestartPolicy.swift (new): the RestartPolicy struct + Mode enum, with each case and the 'data-shape only' caveat documented inline. - Sources/ContainerResource/Container/ContainerCreateOptions.swift: new optional field + defaulted init parameter; existing construction sites compile unchanged. - Sources/Services/ContainerAPIService/Client/Flags.swift: new '--restart ' Option on Flags.Management. - Sources/ContainerCommands/Container/ContainerRun.swift: parseRestartPolicy(_:) helper + invocation in run() before building ContainerCreateOptions. Verification ------------ Full 'swift build' clean on macOS 26 / Apple silicon (release config, all targets including downstream consumers of ContainerCreateOptions). --- .../Container/ContainerRun.swift | 24 +++++++++- .../Container/ContainerCreateOptions.swift | 13 +++++- .../Container/RestartPolicy.swift | 44 +++++++++++++++++++ .../ContainerAPIService/Client/Flags.swift | 6 +++ 4 files changed, 85 insertions(+), 2 deletions(-) create mode 100644 Sources/ContainerResource/Container/RestartPolicy.swift diff --git a/Sources/ContainerCommands/Container/ContainerRun.swift b/Sources/ContainerCommands/Container/ContainerRun.swift index 2957fe473..dc137656a 100644 --- a/Sources/ContainerCommands/Container/ContainerRun.swift +++ b/Sources/ContainerCommands/Container/ContainerRun.swift @@ -108,7 +108,10 @@ extension Application { progress.set(description: "Starting container") - let options = ContainerCreateOptions(autoRemove: managementFlags.remove) + let options = ContainerCreateOptions( + autoRemove: managementFlags.remove, + restartPolicy: Self.parseRestartPolicy(managementFlags.restart) + ) try await client.create( configuration: ck.0, options: options, @@ -176,5 +179,24 @@ extension Application { } throw ArgumentParser.ExitCode(exitCode) } + + static func parseRestartPolicy(_ raw: String?) -> RestartPolicy? { + guard let raw, !raw.isEmpty else { return nil } + switch raw { + case "no": + return .none + case "always": + return RestartPolicy(mode: .always) + case "unless-stopped": + return RestartPolicy(mode: .unlessStopped) + default: + if raw.hasPrefix("on-failure") { + let parts = raw.split(separator: ":", maxSplits: 1) + let retries = parts.count > 1 ? Int(parts[1]) ?? 0 : 0 + return RestartPolicy(mode: .onFailure, maxRetries: retries) + } + return nil + } + } } } diff --git a/Sources/ContainerResource/Container/ContainerCreateOptions.swift b/Sources/ContainerResource/Container/ContainerCreateOptions.swift index fe75577b0..e3424fa8b 100644 --- a/Sources/ContainerResource/Container/ContainerCreateOptions.swift +++ b/Sources/ContainerResource/Container/ContainerCreateOptions.swift @@ -19,10 +19,21 @@ public struct ContainerCreateOptions: Codable, Sendable { public let autoRemove: Bool /// Override the rootFs with this one other than the image-cloned version public let rootFsOverride: Filesystem? + /// Declarative restart policy recorded at creation time. + /// + /// Today this is data-shape only — the daemon stores the policy but does + /// not observe exits and re-launch automatically. A restart-manager + /// follow-up will honor this field at runtime. + public let restartPolicy: RestartPolicy? - public init(autoRemove: Bool, rootFsOverride: Filesystem? = nil) { + public init( + autoRemove: Bool, + rootFsOverride: Filesystem? = nil, + restartPolicy: RestartPolicy? = nil + ) { self.autoRemove = autoRemove self.rootFsOverride = rootFsOverride + self.restartPolicy = restartPolicy } public static let `default` = ContainerCreateOptions(autoRemove: false) diff --git a/Sources/ContainerResource/Container/RestartPolicy.swift b/Sources/ContainerResource/Container/RestartPolicy.swift new file mode 100644 index 000000000..5b67ab038 --- /dev/null +++ b/Sources/ContainerResource/Container/RestartPolicy.swift @@ -0,0 +1,44 @@ +//===----------------------------------------------------------------------===// +// Copyright © 2026 Apple Inc. and the container project authors. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// https://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +//===----------------------------------------------------------------------===// + +import Foundation + +/// Declarative restart policy that the daemon stores on a created container. +/// +/// At present this is a data-shape only contract: the policy is recorded in +/// ``ContainerCreateOptions/restartPolicy`` and surfaced back via the +/// container snapshot, but the daemon does not yet observe container exits +/// and re-launch per policy. Wiring an actual restart manager is a follow-up. +public struct RestartPolicy: Codable, Sendable, Equatable { + public enum Mode: String, Codable, Sendable, Equatable { + case no + case always + case onFailure = "on-failure" + case unlessStopped = "unless-stopped" + } + + public let mode: Mode + /// Maximum number of restart attempts when ``mode`` is ``Mode/onFailure``. + /// Ignored for other modes. `0` means "unbounded retries". + public let maxRetries: Int + + public static let none = RestartPolicy(mode: .no, maxRetries: 0) + + public init(mode: Mode, maxRetries: Int = 0) { + self.mode = mode + self.maxRetries = maxRetries + } +} diff --git a/Sources/Services/ContainerAPIService/Client/Flags.swift b/Sources/Services/ContainerAPIService/Client/Flags.swift index f8361ef68..8d0c9fd01 100644 --- a/Sources/Services/ContainerAPIService/Client/Flags.swift +++ b/Sources/Services/ContainerAPIService/Client/Flags.swift @@ -345,6 +345,12 @@ public struct Flags { @Option(name: [.customLong("volume"), .short], help: "Bind mount a volume into the container") public var volumes: [String] = [] + @Option( + name: .customLong("restart"), + help: "Restart policy (no, always, on-failure[:max-retries], unless-stopped)" + ) + public var restart: String? + public func validate() throws { if dnsDisabled { let hasDNSConfig = From 43ba70f439e2eeda72936a7d8cb122b57546831f Mon Sep 17 00:00:00 2001 From: Chris George Date: Fri, 15 May 2026 11:27:24 -0700 Subject: [PATCH 2/2] refactor(api): align RestartPolicy with upstream apple/container#1258 While this PR was sitting in draft, apple/container#1258 (by JaewonHur, reviewed by saehejkang) appeared with a substantial in-flight restart manager implementation. To avoid a future merge conflict on every field of this contract, the fork's SDK shape is reshaped to match #1258 verbatim before any reviewer sees this PR. Changes ------- RestartPolicy: struct-with-mode-and-maxRetries -> bare String-backed enum { no, onFailure, always }. Matches #1258 line-for-line. Drops the 'unless-stopped' mode and the 'maxRetries' field; both are intentionally deferred so this PR mirrors upstream's deliberately conservative initial scope. Follow-ups will land them after #1258 merges (CHAOS-1321c). ContainerCreateOptions: restartPolicy goes from optional to non-optional, defaulting to .no. Custom 'init(from:)' uses 'decodeIfPresent ?? .no' so older 'options.json' blobs (no field) still decode cleanly. This is the forward-compat guarantee the original PR description promised but did not actually implement. Flags.swift: '--restart' goes from 'String?' + a hand-rolled 'parseRestartPolicy' helper to '@Option var restart: RestartPolicy = .no'. ArgumentParser handles validation natively via a new 'extension RestartPolicy: ExpressibleByArgument {}'; the parser is deleted entirely. As a side benefit, invalid input now produces ArgumentParser's standard 'Invalid value for --restart' error instead of being silently dropped. ContainerRun.swift / ContainerCreate.swift: thread 'managementFlags .restart' through directly. ContainerCreate was missing the wiring entirely in the original PR (the flag declared but never read on the create path) -- fixed here. Tests/ContainerResourceTests/RestartPolicyTests.swift (new): 8 tests covering bare-string encode/decode, every-case round-trip, unknown- string rejection, and the legacy 'options.json' forward-compat invariant (legacy blob without restartPolicy decodes with restartPolicy == .no). Verification ------------ swift build -> Build complete! (21.99s), exit 0. swift test --filter RestartPolicyTests -> 8 tests passed in 0.001s. LSP diagnostics -> clean across all 6 files. Upstream alignment matrix (this PR vs apple/container#1258) ----------------------------------------------------------- RestartPolicy shape bare enum match unless-stopped mode absent match on-failure:N retries absent match restartPolicy optionality non-optional match decodeIfPresent default .no match Flag binding ExpressibleByArg match RuntimeStatus -> ContainerStatus rename NOT in scope defer The RuntimeStatus rename and the .restarting/.bootstrapped state additions stay deferred -- they belong to the daemon-side enforcement work, which is what #1258 actually does. This PR remains data-shape only on the fork side; enforcement arrives when #1258 lands upstream and the fork bumps its sync. Refs CHAOS-1321, CHAOS-1385. Sponsors apple/container#1258, apple/container#286. --- .../Container/ContainerCreate.swift | 5 +- .../Container/ContainerRun.swift | 20 +--- .../Container/ContainerCreateOptions.swift | 24 +++- .../Container/RestartPolicy.swift | 40 +++---- .../ContainerAPIService/Client/Flags.swift | 7 +- .../RestartPolicyTests.swift | 103 ++++++++++++++++++ 6 files changed, 148 insertions(+), 51 deletions(-) create mode 100644 Tests/ContainerResourceTests/RestartPolicyTests.swift diff --git a/Sources/ContainerCommands/Container/ContainerCreate.swift b/Sources/ContainerCommands/Container/ContainerCreate.swift index 2b846e2c4..b597b97d5 100644 --- a/Sources/ContainerCommands/Container/ContainerCreate.swift +++ b/Sources/ContainerCommands/Container/ContainerCreate.swift @@ -86,7 +86,10 @@ extension Application { log: log ) - let options = ContainerCreateOptions(autoRemove: managementFlags.remove) + let options = ContainerCreateOptions( + autoRemove: managementFlags.remove, + restartPolicy: managementFlags.restart + ) let client = ContainerClient() try await client.create(configuration: ck.0, options: options, kernel: ck.1, initImage: ck.2) diff --git a/Sources/ContainerCommands/Container/ContainerRun.swift b/Sources/ContainerCommands/Container/ContainerRun.swift index dc137656a..5cc31ffff 100644 --- a/Sources/ContainerCommands/Container/ContainerRun.swift +++ b/Sources/ContainerCommands/Container/ContainerRun.swift @@ -110,7 +110,7 @@ extension Application { let options = ContainerCreateOptions( autoRemove: managementFlags.remove, - restartPolicy: Self.parseRestartPolicy(managementFlags.restart) + restartPolicy: managementFlags.restart ) try await client.create( configuration: ck.0, @@ -180,23 +180,5 @@ extension Application { throw ArgumentParser.ExitCode(exitCode) } - static func parseRestartPolicy(_ raw: String?) -> RestartPolicy? { - guard let raw, !raw.isEmpty else { return nil } - switch raw { - case "no": - return .none - case "always": - return RestartPolicy(mode: .always) - case "unless-stopped": - return RestartPolicy(mode: .unlessStopped) - default: - if raw.hasPrefix("on-failure") { - let parts = raw.split(separator: ":", maxSplits: 1) - let retries = parts.count > 1 ? Int(parts[1]) ?? 0 : 0 - return RestartPolicy(mode: .onFailure, maxRetries: retries) - } - return nil - } - } } } diff --git a/Sources/ContainerResource/Container/ContainerCreateOptions.swift b/Sources/ContainerResource/Container/ContainerCreateOptions.swift index e3424fa8b..ecdb65394 100644 --- a/Sources/ContainerResource/Container/ContainerCreateOptions.swift +++ b/Sources/ContainerResource/Container/ContainerCreateOptions.swift @@ -22,14 +22,18 @@ public struct ContainerCreateOptions: Codable, Sendable { /// Declarative restart policy recorded at creation time. /// /// Today this is data-shape only — the daemon stores the policy but does - /// not observe exits and re-launch automatically. A restart-manager - /// follow-up will honor this field at runtime. - public let restartPolicy: RestartPolicy? + /// not observe exits and re-launch automatically. Enforcement is tracked + /// by upstream [apple/container#1258](https://github.com/apple/container/pull/1258). + /// + /// Defaults to ``RestartPolicy/no``. Decoded with `decodeIfPresent` so + /// older `options.json` blobs written before the field existed continue to + /// load (forward-compatible additive change). + public let restartPolicy: RestartPolicy public init( autoRemove: Bool, rootFsOverride: Filesystem? = nil, - restartPolicy: RestartPolicy? = nil + restartPolicy: RestartPolicy = .no ) { self.autoRemove = autoRemove self.rootFsOverride = rootFsOverride @@ -38,4 +42,16 @@ public struct ContainerCreateOptions: Codable, Sendable { public static let `default` = ContainerCreateOptions(autoRemove: false) + enum CodingKeys: String, CodingKey { + case autoRemove + case rootFsOverride + case restartPolicy + } + + public init(from decoder: Decoder) throws { + let container = try decoder.container(keyedBy: CodingKeys.self) + self.autoRemove = try container.decode(Bool.self, forKey: .autoRemove) + self.rootFsOverride = try container.decodeIfPresent(Filesystem.self, forKey: .rootFsOverride) + self.restartPolicy = try container.decodeIfPresent(RestartPolicy.self, forKey: .restartPolicy) ?? .no + } } diff --git a/Sources/ContainerResource/Container/RestartPolicy.swift b/Sources/ContainerResource/Container/RestartPolicy.swift index 5b67ab038..43f7d7a0a 100644 --- a/Sources/ContainerResource/Container/RestartPolicy.swift +++ b/Sources/ContainerResource/Container/RestartPolicy.swift @@ -14,31 +14,21 @@ // limitations under the License. //===----------------------------------------------------------------------===// -import Foundation - /// Declarative restart policy that the daemon stores on a created container. /// -/// At present this is a data-shape only contract: the policy is recorded in -/// ``ContainerCreateOptions/restartPolicy`` and surfaced back via the -/// container snapshot, but the daemon does not yet observe container exits -/// and re-launch per policy. Wiring an actual restart manager is a follow-up. -public struct RestartPolicy: Codable, Sendable, Equatable { - public enum Mode: String, Codable, Sendable, Equatable { - case no - case always - case onFailure = "on-failure" - case unlessStopped = "unless-stopped" - } - - public let mode: Mode - /// Maximum number of restart attempts when ``mode`` is ``Mode/onFailure``. - /// Ignored for other modes. `0` means "unbounded retries". - public let maxRetries: Int - - public static let none = RestartPolicy(mode: .no, maxRetries: 0) - - public init(mode: Mode, maxRetries: Int = 0) { - self.mode = mode - self.maxRetries = maxRetries - } +/// The shape mirrors the in-flight upstream proposal in +/// [apple/container#1258](https://github.com/apple/container/pull/1258): +/// a bare `String`-backed enum with the conservative initial set +/// (`no`, `onFailure`, `always`). Bounded `on-failure:N` retries and +/// `unless-stopped` are intentionally deferred — they ship as separate +/// follow-ups once #1258 lands. +/// +/// At present this is a data-shape only contract on the fork: the policy is +/// recorded in ``ContainerCreateOptions/restartPolicy`` but the daemon does +/// not yet observe container exits and re-launch per policy. Enforcement +/// will arrive via the upstream restart manager (#1258). +public enum RestartPolicy: String, Sendable, Codable, Equatable, CaseIterable { + case no + case onFailure + case always } diff --git a/Sources/Services/ContainerAPIService/Client/Flags.swift b/Sources/Services/ContainerAPIService/Client/Flags.swift index 8d0c9fd01..e8457712b 100644 --- a/Sources/Services/ContainerAPIService/Client/Flags.swift +++ b/Sources/Services/ContainerAPIService/Client/Flags.swift @@ -15,9 +15,12 @@ //===----------------------------------------------------------------------===// import ArgumentParser +import ContainerResource import ContainerizationError import Foundation +extension RestartPolicy: ExpressibleByArgument {} + public struct Flags { public struct Logging: ParsableArguments { public init() {} @@ -347,9 +350,9 @@ public struct Flags { @Option( name: .customLong("restart"), - help: "Restart policy (no, always, on-failure[:max-retries], unless-stopped)" + help: "Restart policy when the container exits (no, onFailure, always)" ) - public var restart: String? + public var restart: RestartPolicy = .no public func validate() throws { if dnsDisabled { diff --git a/Tests/ContainerResourceTests/RestartPolicyTests.swift b/Tests/ContainerResourceTests/RestartPolicyTests.swift new file mode 100644 index 000000000..5a866230a --- /dev/null +++ b/Tests/ContainerResourceTests/RestartPolicyTests.swift @@ -0,0 +1,103 @@ +//===----------------------------------------------------------------------===// +// Copyright © 2026 Apple Inc. and the container project authors. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// https://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +//===----------------------------------------------------------------------===// + +import Foundation +import Testing + +@testable import ContainerResource + +/// Coverage for the SDK shape that PR #13 introduces. Wire compatibility is +/// the contract under test: the daemon does not enforce restart policy yet, +/// so behavior tests live with the future restart manager (upstream +/// apple/container#1258), not here. +struct RestartPolicyTests { + // MARK: - RestartPolicy round-trip + + @Test + func testEncodesAsBareString() throws { + let encoder = JSONEncoder() + let data = try encoder.encode(RestartPolicy.always) + let s = String(decoding: data, as: UTF8.self) + #expect(s == "\"always\"") + } + + @Test + func testDecodesEveryCase() throws { + let decoder = JSONDecoder() + for policy in RestartPolicy.allCases { + let data = try JSONEncoder().encode(policy) + let decoded = try decoder.decode(RestartPolicy.self, from: data) + #expect(decoded == policy) + } + } + + @Test + func testRejectsUnknownString() { + let bogus = Data("\"unless-stopped\"".utf8) + #expect(throws: DecodingError.self) { + try JSONDecoder().decode(RestartPolicy.self, from: bogus) + } + } + + // MARK: - ContainerCreateOptions forward-compat + + /// JSON written by a daemon version that predates `restartPolicy` MUST + /// still decode — defaulting to `.no`. This is the wire-compatibility + /// invariant the PR description promises. + @Test + func testDecodesLegacyOptionsWithoutRestartPolicy() throws { + let legacy = Data(#"{"autoRemove":false}"#.utf8) + let options = try JSONDecoder().decode(ContainerCreateOptions.self, from: legacy) + #expect(options.autoRemove == false) + #expect(options.restartPolicy == .no) + } + + @Test + func testDecodesLegacyOptionsWithAutoRemoveAndRootFsOverride() throws { + // Older clients may emit only autoRemove + rootFsOverride. rootFsOverride + // is itself optional and may not appear; we test the canonical legacy + // shape (just autoRemove). + let legacy = Data(#"{"autoRemove":true}"#.utf8) + let options = try JSONDecoder().decode(ContainerCreateOptions.self, from: legacy) + #expect(options.autoRemove == true) + #expect(options.rootFsOverride == nil) + #expect(options.restartPolicy == .no) + } + + @Test + func testRoundTripPreservesRestartPolicy() throws { + let original = ContainerCreateOptions(autoRemove: false, restartPolicy: .onFailure) + let data = try JSONEncoder().encode(original) + let decoded = try JSONDecoder().decode(ContainerCreateOptions.self, from: data) + #expect(decoded.autoRemove == original.autoRemove) + #expect(decoded.restartPolicy == original.restartPolicy) + } + + @Test + func testEncodedJSONIncludesRestartPolicyField() throws { + let options = ContainerCreateOptions(autoRemove: false, restartPolicy: .always) + let data = try JSONEncoder().encode(options) + let json = String(decoding: data, as: UTF8.self) + // Field present and lowercase per CodingKeys + raw value. + #expect(json.contains("\"restartPolicy\":\"always\"")) + } + + @Test + func testDefaultStaticHasNoRestart() { + #expect(ContainerCreateOptions.default.restartPolicy == .no) + #expect(ContainerCreateOptions.default.autoRemove == false) + } +}