Skip to content

Commit ace9702

Browse files
dcantahdcantah
committed
CLI: stop and kill should error on not found containers
We should print a clear error message to stderr on if a container can't be found, as well as exiting with a 1 exit code.
1 parent 827b46c commit ace9702

File tree

6 files changed

+252
-40
lines changed

6 files changed

+252
-40
lines changed

‎Makefile‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -174,6 +174,7 @@ integration: init-block
174174
$(SWIFT) test -c $(BUILD_CONFIGURATION) $(SWIFT_CONFIGURATION) --filter TestCLINetwork || exit_code=1 ; \
175175
$(SWIFT) test -c $(BUILD_CONFIGURATION) $(SWIFT_CONFIGURATION) --filter TestCLIRunLifecycle || exit_code=1 ; \
176176
$(SWIFT) test -c $(BUILD_CONFIGURATION) $(SWIFT_CONFIGURATION) --filter TestCLIExecCommand || exit_code=1 ; \
177+
$(SWIFT) test -c $(BUILD_CONFIGURATION) $(SWIFT_CONFIGURATION) --filter TestCLIStopKillErrors || exit_code=1 ; \
177178
$(SWIFT) test -c $(BUILD_CONFIGURATION) $(SWIFT_CONFIGURATION) --filter TestCLICreateCommand || exit_code=1 ; \
178179
$(SWIFT) test -c $(BUILD_CONFIGURATION) $(SWIFT_CONFIGURATION) --filter TestCLIRunCommand || exit_code=1 ; \
179180
$(SWIFT) test -c $(BUILD_CONFIGURATION) $(SWIFT_CONFIGURATION) --filter TestCLIImagesCommand || exit_code=1 ; \

‎Sources/ContainerCommands/Container/ContainerKill.swift‎

Lines changed: 55 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,11 @@ extension Application {
4040
@Argument(help: "Container IDs")
4141
var containerIds: [String] = []
4242

43+
struct KillError: Error {
44+
let succeeded: [String]
45+
let failed: [(String, Error)]
46+
}
47+
4348
public func validate() throws {
4449
if containerIds.count == 0 && !all {
4550
throw ContainerizationError(.invalidArgument, message: "no containers specified and --all not supplied")
@@ -50,31 +55,67 @@ extension Application {
5055
}
5156

5257
public mutating func run() async throws {
53-
let set = Set<String>(containerIds)
58+
var containers = [ClientContainer]()
59+
var allErrors: [String] = []
5460

55-
var containers = try await ClientContainer.list().filter { c in
56-
c.status == .running
57-
}
58-
if !self.all {
59-
containers = containers.filter { c in
60-
set.contains(c.id)
61+
let allContainers = try await ClientContainer.list()
62+
63+
if self.all {
64+
containers = allContainers.filter { c in
65+
c.status == .running
66+
}
67+
} else {
68+
let containerMap = Dictionary(uniqueKeysWithValues: allContainers.map { ($0.id, $0) })
69+
70+
for id in containerIds {
71+
if let container = containerMap[id] {
72+
containers.append(container)
73+
} else {
74+
allErrors.append("Error: No such container: \(id)")
75+
}
6176
}
6277
}
6378

6479
let signalNumber = try Signals.parseSignal(signal)
6580

66-
var failed: [String] = []
81+
do {
82+
try await Self.killContainers(containers: containers, signal: signalNumber)
83+
for container in containers {
84+
print(container.id)
85+
}
86+
} catch let error as KillError {
87+
for id in error.succeeded {
88+
print(id)
89+
}
90+
91+
for (_, err) in error.failed {
92+
allErrors.append("Error from APIServer: \(err.localizedDescription)")
93+
}
94+
}
95+
if !allErrors.isEmpty {
96+
var stderr = StandardError()
97+
for error in allErrors {
98+
print(error, to: &stderr)
99+
}
100+
throw ExitCode(1)
101+
}
102+
}
103+
104+
static func killContainers(containers: [ClientContainer], signal: Int32) async throws {
105+
var succeeded: [String] = []
106+
var failed: [(String, Error)] = []
107+
67108
for container in containers {
68109
do {
69-
try await container.kill(signalNumber)
70-
print(container.id)
110+
try await container.kill(signal)
111+
succeeded.append(container.id)
71112
} catch {
72-
log.error("failed to kill container \(container.id): \(error)")
73-
failed.append(container.id)
113+
failed.append((container.id, error))
74114
}
75115
}
76-
if failed.count > 0 {
77-
throw ContainerizationError(.internalError, message: "kill failed for one or more containers \(failed.joined(separator: ","))")
116+
117+
if !failed.isEmpty {
118+
throw KillError(succeeded: succeeded, failed: failed)
78119
}
79120
}
80121
}

‎Sources/ContainerCommands/Container/ContainerStop.swift‎

Lines changed: 59 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -43,65 +43,101 @@ extension Application {
4343
@Argument(help: "Container IDs")
4444
var containerIds: [String] = []
4545

46+
package struct StopError: Error {
47+
let succeeded: [String]
48+
let failed: [(String, Error)]
49+
}
50+
4651
public func validate() throws {
4752
if containerIds.count == 0 && !all {
48-
throw ContainerizationError(.invalidArgument, message: "no containers specified and --all not supplied")
53+
throw ContainerizationError(
54+
.invalidArgument,
55+
message: "no containers specified and --all not supplied"
56+
)
4957
}
5058
if containerIds.count > 0 && all {
5159
throw ContainerizationError(
52-
.invalidArgument, message: "explicitly supplied container IDs conflict with the --all flag")
60+
.invalidArgument,
61+
message: "explicitly supplied container IDs conflict with the --all flag"
62+
)
5363
}
5464
}
5565

5666
public mutating func run() async throws {
57-
let set = Set<String>(containerIds)
5867
var containers = [ClientContainer]()
68+
var allErrors: [String] = []
69+
5970
if self.all {
6071
containers = try await ClientContainer.list()
6172
} else {
62-
containers = try await ClientContainer.list().filter { c in
63-
set.contains(c.id)
73+
let allContainers = try await ClientContainer.list()
74+
let containerMap = Dictionary(uniqueKeysWithValues: allContainers.map { ($0.id, $0) })
75+
76+
for id in containerIds {
77+
if let container = containerMap[id] {
78+
containers.append(container)
79+
} else {
80+
allErrors.append("Error: No such container: \(id)")
81+
}
6482
}
6583
}
6684

6785
let opts = ContainerStopOptions(
6886
timeoutInSeconds: self.time,
6987
signal: try Signals.parseSignal(self.signal)
7088
)
71-
let failed = try await Self.stopContainers(containers: containers, stopOptions: opts)
72-
if failed.count > 0 {
73-
throw ContainerizationError(
74-
.internalError,
75-
message: "stop failed for one or more containers \(failed.joined(separator: ","))"
76-
)
89+
90+
do {
91+
try await Self.stopContainers(containers: containers, stopOptions: opts)
92+
for container in containers {
93+
print(container.id)
94+
}
95+
} catch let error as ContainerStop.StopError {
96+
for id in error.succeeded {
97+
print(id)
98+
}
99+
100+
for (_, err) in error.failed {
101+
allErrors.append("Error from APIServer: \(err)")
102+
}
103+
}
104+
105+
if !allErrors.isEmpty {
106+
var stderr = StandardError()
107+
for err in allErrors {
108+
print(err, to: &stderr)
109+
}
110+
throw ExitCode(1)
77111
}
78112
}
79113

80-
static func stopContainers(containers: [ClientContainer], stopOptions: ContainerStopOptions) async throws -> [String] {
81-
var failed: [String] = []
82-
try await withThrowingTaskGroup(of: ClientContainer?.self) { group in
114+
static func stopContainers(containers: [ClientContainer], stopOptions: ContainerStopOptions) async throws {
115+
var succeeded: [String] = []
116+
var failed: [(String, Error)] = []
117+
try await withThrowingTaskGroup(of: (String, Error?).self) { group in
83118
for container in containers {
84119
group.addTask {
85120
do {
86121
try await container.stop(opts: stopOptions)
87-
print(container.id)
88-
return nil
122+
return (container.id, nil)
89123
} catch {
90-
log.error("failed to stop container \(container.id): \(error)")
91-
return container
124+
return (container.id, error)
92125
}
93126
}
94127
}
95128

96-
for try await ctr in group {
97-
guard let ctr else {
98-
continue
129+
for try await (id, error) in group {
130+
if let error = error {
131+
failed.append((id, error))
132+
} else {
133+
succeeded.append(id)
99134
}
100-
failed.append(ctr.id)
101135
}
102136
}
103137

104-
return failed
138+
if !failed.isEmpty {
139+
throw StopError(succeeded: succeeded, failed: failed)
140+
}
105141
}
106142
}
107143
}

‎Sources/ContainerCommands/Container/ProcessUtils.swift‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,14 @@ import ContainerizationError
2020
import ContainerizationOS
2121
import Foundation
2222

23+
struct StandardError: TextOutputStream, Sendable {
24+
private static let handle = FileHandle.standardError
25+
26+
public func write(_ string: String) {
27+
Self.handle.write(Data(string.utf8))
28+
}
29+
}
30+
2331
extension Application {
2432
static func ensureRunning(container: ClientContainer) throws {
2533
if container.status != .running {

‎Sources/ContainerCommands/System/SystemStop.swift‎

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -65,9 +65,13 @@ extension Application {
6565
let containers = try await ClientContainer.list()
6666
let signal = try Signals.parseSignal("SIGTERM")
6767
let opts = ContainerStopOptions(timeoutInSeconds: Self.stopTimeoutSeconds, signal: signal)
68-
let failed = try await ContainerStop.stopContainers(containers: containers, stopOptions: opts)
69-
if !failed.isEmpty {
70-
log.warning("some containers could not be stopped gracefully", metadata: ["ids": "\(failed)"])
68+
69+
do {
70+
try await ContainerStop.stopContainers(containers: containers, stopOptions: opts)
71+
} catch let error as ContainerStop.StopError {
72+
for (id, error) in error.failed {
73+
log.warning("container \(id) failed to stop: \(error)")
74+
}
7175
}
7276
} catch {
7377
log.warning("failed to stop all containers", metadata: ["error": "\(error)"])

‎Tests/CLITests/Subcommands/Containers/TestCLIStopKillErrors.swift‎

Lines changed: 122 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,122 @@
1+
//===----------------------------------------------------------------------===//
2+
// Copyright © 2025 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+
class TestCLIStopKillErrors: CLITest {
21+
@Test func testStopNonExistentContainer() throws {
22+
let nonExistentName = "nonexistent-container-\(UUID().uuidString)"
23+
24+
let (_, error, status) = try run(arguments: [
25+
"stop",
26+
nonExistentName,
27+
])
28+
29+
#expect(status == 1)
30+
#expect(error.contains(nonExistentName))
31+
}
32+
33+
@Test func testStopMultipleNonExistentContainers() throws {
34+
let nonExistent1 = "nonexistent-1-\(UUID().uuidString)"
35+
let nonExistent2 = "nonexistent-2-\(UUID().uuidString)"
36+
37+
let (_, error, status) = try run(arguments: [
38+
"stop",
39+
nonExistent1,
40+
nonExistent2,
41+
])
42+
43+
#expect(status == 1)
44+
#expect(error.contains(nonExistent1))
45+
#expect(error.contains(nonExistent2))
46+
}
47+
48+
@Test func testKillNonExistentContainer() throws {
49+
let nonExistentName = "nonexistent-container-\(UUID().uuidString)"
50+
51+
let (_, error, status) = try run(arguments: [
52+
"kill",
53+
nonExistentName,
54+
])
55+
56+
#expect(status == 1)
57+
#expect(error.contains(nonExistentName))
58+
}
59+
60+
@Test func testKillMultipleNonExistentContainers() throws {
61+
let nonExistent1 = "nonexistent-1-\(UUID().uuidString)"
62+
let nonExistent2 = "nonexistent-2-\(UUID().uuidString)"
63+
64+
let (_, error, status) = try run(arguments: [
65+
"kill",
66+
nonExistent1,
67+
nonExistent2,
68+
])
69+
70+
#expect(status == 1)
71+
#expect(error.contains(nonExistent1))
72+
#expect(error.contains(nonExistent2))
73+
}
74+
75+
@Test func testStopMixedExistentAndNonExistent() throws {
76+
let existentName = "test-stop-mixed-\(UUID().uuidString)"
77+
let nonExistentName = "nonexistent-\(UUID().uuidString)"
78+
79+
try doCreate(name: existentName)
80+
try doStart(name: existentName)
81+
try waitForContainerRunning(existentName)
82+
83+
defer {
84+
try? doStop(name: existentName)
85+
try? doRemove(name: existentName)
86+
}
87+
88+
let (output, error, status) = try run(arguments: [
89+
"stop",
90+
existentName,
91+
nonExistentName,
92+
])
93+
94+
#expect(status == 1)
95+
#expect(output.contains(existentName))
96+
#expect(error.contains(nonExistentName))
97+
}
98+
99+
@Test func testKillMixedExistentAndNonExistent() throws {
100+
let existentName = "test-kill-mixed-\(UUID().uuidString)"
101+
let nonExistentName = "nonexistent-\(UUID().uuidString)"
102+
103+
try doCreate(name: existentName)
104+
try doStart(name: existentName)
105+
try waitForContainerRunning(existentName)
106+
107+
defer {
108+
try? doStop(name: existentName)
109+
try? doRemove(name: existentName)
110+
}
111+
112+
let (output, error, status) = try run(arguments: [
113+
"kill",
114+
existentName,
115+
nonExistentName,
116+
])
117+
118+
#expect(status == 1)
119+
#expect(output.contains(existentName))
120+
#expect(error.contains(nonExistentName))
121+
}
122+
}

0 commit comments

Comments
 (0)