Skip to content

Commit 43ba70f

Browse files
committed
refactor(api): align RestartPolicy with upstream apple#1258
While this PR was sitting in draft, apple#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 apple#1258 verbatim before any reviewer sees this PR. Changes ------- RestartPolicy: struct-with-mode-and-maxRetries -> bare String-backed enum { no, onFailure, always }. Matches apple#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 apple#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#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 apple#1258 actually does. This PR remains data-shape only on the fork side; enforcement arrives when apple#1258 lands upstream and the fork bumps its sync. Refs CHAOS-1321, CHAOS-1385. Sponsors apple#1258, apple#286.
1 parent d9aaa44 commit 43ba70f

6 files changed

Lines changed: 148 additions & 51 deletions

File tree

Sources/ContainerCommands/Container/ContainerCreate.swift

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -86,7 +86,10 @@ extension Application {
8686
log: log
8787
)
8888

89-
let options = ContainerCreateOptions(autoRemove: managementFlags.remove)
89+
let options = ContainerCreateOptions(
90+
autoRemove: managementFlags.remove,
91+
restartPolicy: managementFlags.restart
92+
)
9093
let client = ContainerClient()
9194
try await client.create(configuration: ck.0, options: options, kernel: ck.1, initImage: ck.2)
9295

Sources/ContainerCommands/Container/ContainerRun.swift

Lines changed: 1 addition & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -110,7 +110,7 @@ extension Application {
110110

111111
let options = ContainerCreateOptions(
112112
autoRemove: managementFlags.remove,
113-
restartPolicy: Self.parseRestartPolicy(managementFlags.restart)
113+
restartPolicy: managementFlags.restart
114114
)
115115
try await client.create(
116116
configuration: ck.0,
@@ -180,23 +180,5 @@ extension Application {
180180
throw ArgumentParser.ExitCode(exitCode)
181181
}
182182

183-
static func parseRestartPolicy(_ raw: String?) -> RestartPolicy? {
184-
guard let raw, !raw.isEmpty else { return nil }
185-
switch raw {
186-
case "no":
187-
return .none
188-
case "always":
189-
return RestartPolicy(mode: .always)
190-
case "unless-stopped":
191-
return RestartPolicy(mode: .unlessStopped)
192-
default:
193-
if raw.hasPrefix("on-failure") {
194-
let parts = raw.split(separator: ":", maxSplits: 1)
195-
let retries = parts.count > 1 ? Int(parts[1]) ?? 0 : 0
196-
return RestartPolicy(mode: .onFailure, maxRetries: retries)
197-
}
198-
return nil
199-
}
200-
}
201183
}
202184
}

Sources/ContainerResource/Container/ContainerCreateOptions.swift

Lines changed: 20 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -22,14 +22,18 @@ public struct ContainerCreateOptions: Codable, Sendable {
2222
/// Declarative restart policy recorded at creation time.
2323
///
2424
/// Today this is data-shape only — the daemon stores the policy but does
25-
/// not observe exits and re-launch automatically. A restart-manager
26-
/// follow-up will honor this field at runtime.
27-
public let restartPolicy: RestartPolicy?
25+
/// not observe exits and re-launch automatically. Enforcement is tracked
26+
/// by upstream [apple/container#1258](https://github.com/apple/container/pull/1258).
27+
///
28+
/// Defaults to ``RestartPolicy/no``. Decoded with `decodeIfPresent` so
29+
/// older `options.json` blobs written before the field existed continue to
30+
/// load (forward-compatible additive change).
31+
public let restartPolicy: RestartPolicy
2832

2933
public init(
3034
autoRemove: Bool,
3135
rootFsOverride: Filesystem? = nil,
32-
restartPolicy: RestartPolicy? = nil
36+
restartPolicy: RestartPolicy = .no
3337
) {
3438
self.autoRemove = autoRemove
3539
self.rootFsOverride = rootFsOverride
@@ -38,4 +42,16 @@ public struct ContainerCreateOptions: Codable, Sendable {
3842

3943
public static let `default` = ContainerCreateOptions(autoRemove: false)
4044

45+
enum CodingKeys: String, CodingKey {
46+
case autoRemove
47+
case rootFsOverride
48+
case restartPolicy
49+
}
50+
51+
public init(from decoder: Decoder) throws {
52+
let container = try decoder.container(keyedBy: CodingKeys.self)
53+
self.autoRemove = try container.decode(Bool.self, forKey: .autoRemove)
54+
self.rootFsOverride = try container.decodeIfPresent(Filesystem.self, forKey: .rootFsOverride)
55+
self.restartPolicy = try container.decodeIfPresent(RestartPolicy.self, forKey: .restartPolicy) ?? .no
56+
}
4157
}

Sources/ContainerResource/Container/RestartPolicy.swift

Lines changed: 15 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -14,31 +14,21 @@
1414
// limitations under the License.
1515
//===----------------------------------------------------------------------===//
1616

17-
import Foundation
18-
1917
/// Declarative restart policy that the daemon stores on a created container.
2018
///
21-
/// At present this is a data-shape only contract: the policy is recorded in
22-
/// ``ContainerCreateOptions/restartPolicy`` and surfaced back via the
23-
/// container snapshot, but the daemon does not yet observe container exits
24-
/// and re-launch per policy. Wiring an actual restart manager is a follow-up.
25-
public struct RestartPolicy: Codable, Sendable, Equatable {
26-
public enum Mode: String, Codable, Sendable, Equatable {
27-
case no
28-
case always
29-
case onFailure = "on-failure"
30-
case unlessStopped = "unless-stopped"
31-
}
32-
33-
public let mode: Mode
34-
/// Maximum number of restart attempts when ``mode`` is ``Mode/onFailure``.
35-
/// Ignored for other modes. `0` means "unbounded retries".
36-
public let maxRetries: Int
37-
38-
public static let none = RestartPolicy(mode: .no, maxRetries: 0)
39-
40-
public init(mode: Mode, maxRetries: Int = 0) {
41-
self.mode = mode
42-
self.maxRetries = maxRetries
43-
}
19+
/// The shape mirrors the in-flight upstream proposal in
20+
/// [apple/container#1258](https://github.com/apple/container/pull/1258):
21+
/// a bare `String`-backed enum with the conservative initial set
22+
/// (`no`, `onFailure`, `always`). Bounded `on-failure:N` retries and
23+
/// `unless-stopped` are intentionally deferred — they ship as separate
24+
/// follow-ups once #1258 lands.
25+
///
26+
/// At present this is a data-shape only contract on the fork: the policy is
27+
/// recorded in ``ContainerCreateOptions/restartPolicy`` but the daemon does
28+
/// not yet observe container exits and re-launch per policy. Enforcement
29+
/// will arrive via the upstream restart manager (#1258).
30+
public enum RestartPolicy: String, Sendable, Codable, Equatable, CaseIterable {
31+
case no
32+
case onFailure
33+
case always
4434
}

Sources/Services/ContainerAPIService/Client/Flags.swift

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -15,9 +15,12 @@
1515
//===----------------------------------------------------------------------===//
1616

1717
import ArgumentParser
18+
import ContainerResource
1819
import ContainerizationError
1920
import Foundation
2021

22+
extension RestartPolicy: ExpressibleByArgument {}
23+
2124
public struct Flags {
2225
public struct Logging: ParsableArguments {
2326
public init() {}
@@ -347,9 +350,9 @@ public struct Flags {
347350

348351
@Option(
349352
name: .customLong("restart"),
350-
help: "Restart policy (no, always, on-failure[:max-retries], unless-stopped)"
353+
help: "Restart policy when the container exits (no, onFailure, always)"
351354
)
352-
public var restart: String?
355+
public var restart: RestartPolicy = .no
353356

354357
public func validate() throws {
355358
if dnsDisabled {
Lines changed: 103 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,103 @@
1+
//===----------------------------------------------------------------------===//
2+
// Copyright © 2026 Apple Inc. and the container project authors.
3+
//
4+
// Licensed under the Apache License, Version 2.0 (the "License");
5+
// you may not use this file except in compliance with the License.
6+
// You may obtain a copy of the License at
7+
//
8+
// https://www.apache.org/licenses/LICENSE-2.0
9+
//
10+
// Unless required by applicable law or agreed to in writing, software
11+
// distributed under the License is distributed on an "AS IS" BASIS,
12+
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
// See the License for the specific language governing permissions and
14+
// limitations under the License.
15+
//===----------------------------------------------------------------------===//
16+
17+
import Foundation
18+
import Testing
19+
20+
@testable import ContainerResource
21+
22+
/// Coverage for the SDK shape that PR #13 introduces. Wire compatibility is
23+
/// the contract under test: the daemon does not enforce restart policy yet,
24+
/// so behavior tests live with the future restart manager (upstream
25+
/// apple/container#1258), not here.
26+
struct RestartPolicyTests {
27+
// MARK: - RestartPolicy round-trip
28+
29+
@Test
30+
func testEncodesAsBareString() throws {
31+
let encoder = JSONEncoder()
32+
let data = try encoder.encode(RestartPolicy.always)
33+
let s = String(decoding: data, as: UTF8.self)
34+
#expect(s == "\"always\"")
35+
}
36+
37+
@Test
38+
func testDecodesEveryCase() throws {
39+
let decoder = JSONDecoder()
40+
for policy in RestartPolicy.allCases {
41+
let data = try JSONEncoder().encode(policy)
42+
let decoded = try decoder.decode(RestartPolicy.self, from: data)
43+
#expect(decoded == policy)
44+
}
45+
}
46+
47+
@Test
48+
func testRejectsUnknownString() {
49+
let bogus = Data("\"unless-stopped\"".utf8)
50+
#expect(throws: DecodingError.self) {
51+
try JSONDecoder().decode(RestartPolicy.self, from: bogus)
52+
}
53+
}
54+
55+
// MARK: - ContainerCreateOptions forward-compat
56+
57+
/// JSON written by a daemon version that predates `restartPolicy` MUST
58+
/// still decode — defaulting to `.no`. This is the wire-compatibility
59+
/// invariant the PR description promises.
60+
@Test
61+
func testDecodesLegacyOptionsWithoutRestartPolicy() throws {
62+
let legacy = Data(#"{"autoRemove":false}"#.utf8)
63+
let options = try JSONDecoder().decode(ContainerCreateOptions.self, from: legacy)
64+
#expect(options.autoRemove == false)
65+
#expect(options.restartPolicy == .no)
66+
}
67+
68+
@Test
69+
func testDecodesLegacyOptionsWithAutoRemoveAndRootFsOverride() throws {
70+
// Older clients may emit only autoRemove + rootFsOverride. rootFsOverride
71+
// is itself optional and may not appear; we test the canonical legacy
72+
// shape (just autoRemove).
73+
let legacy = Data(#"{"autoRemove":true}"#.utf8)
74+
let options = try JSONDecoder().decode(ContainerCreateOptions.self, from: legacy)
75+
#expect(options.autoRemove == true)
76+
#expect(options.rootFsOverride == nil)
77+
#expect(options.restartPolicy == .no)
78+
}
79+
80+
@Test
81+
func testRoundTripPreservesRestartPolicy() throws {
82+
let original = ContainerCreateOptions(autoRemove: false, restartPolicy: .onFailure)
83+
let data = try JSONEncoder().encode(original)
84+
let decoded = try JSONDecoder().decode(ContainerCreateOptions.self, from: data)
85+
#expect(decoded.autoRemove == original.autoRemove)
86+
#expect(decoded.restartPolicy == original.restartPolicy)
87+
}
88+
89+
@Test
90+
func testEncodedJSONIncludesRestartPolicyField() throws {
91+
let options = ContainerCreateOptions(autoRemove: false, restartPolicy: .always)
92+
let data = try JSONEncoder().encode(options)
93+
let json = String(decoding: data, as: UTF8.self)
94+
// Field present and lowercase per CodingKeys + raw value.
95+
#expect(json.contains("\"restartPolicy\":\"always\""))
96+
}
97+
98+
@Test
99+
func testDefaultStaticHasNoRestart() {
100+
#expect(ContainerCreateOptions.default.restartPolicy == .no)
101+
#expect(ContainerCreateOptions.default.autoRemove == false)
102+
}
103+
}

0 commit comments

Comments
 (0)