Fix CI integration test coldstart issues. #1230
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -58,7 +58,7 @@ extension Application { | |
| do { | ||
| var args = ["log"] | ||
| args.append(self.follow ? "stream" : "show") | ||
| args.append(contentsOf: ["--info", logOptions.debug ? "--debug" : nil].compactMap { $0 }) | ||
|
Contributor
Author
There was a problem hiding this comment. Now we only pass debug into |
||
| if !self.follow { | ||
| args.append(contentsOf: ["--last", last]) | ||
| } | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18,6 +18,7 @@ import ArgumentParser | |
| import ContainerAPIClient | ||
| import ContainerPersistence | ||
| import ContainerPlugin | ||
| import ContainerXPC | ||
| import ContainerizationError | ||
| import Foundation | ||
| import TerminalProgress | ||
|
|
@@ -48,9 +49,15 @@ extension Application { | |
| var kernelInstall: Bool? | ||
|
|
||
| @Option( | ||
| help: "Number of seconds to wait for API service to become responsive", | ||
| transform: { | ||
| guard let timeoutSeconds = Double($0) else { | ||
| throw ValidationError("Invalid timeout value: \($0)") | ||
| } | ||
| return .seconds(timeoutSeconds) | ||
| } | ||
| ) | ||
| var timeout: Duration = XPCClient.xpcRegistrationTimeout | ||
|
Contributor
Author
There was a problem hiding this comment. Key point here is that we use the common timeout value now. |
||
|
|
||
| @OptionGroup | ||
| public var logOptions: Flags.Logging | ||
|
|
@@ -98,7 +105,7 @@ extension Application { | |
| // Now ping our friendly daemon. Fail if we don't get a response. | ||
| do { | ||
| print("Verifying apiserver is running...") | ||
| _ = try await ClientHealthCheck.ping(timeout: timeout) | ||
| } catch { | ||
| throw ContainerizationError( | ||
| .internalError, | ||
|
|
@@ -124,7 +131,7 @@ extension Application { | |
| do { | ||
| try await pullCommand.run() | ||
| } catch { | ||
| log.error("failed to install base container filesystem", metadata: ["error": "\(error)"]) | ||
| } | ||
| } | ||
|
|
||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -57,7 +57,12 @@ extension Application.VolumeCommand { | |
| try await ClientVolume.delete(name: volume.name) | ||
| prunedVolumes.append(volume.name) | ||
| } catch { | ||
| log.error( | ||
| "failed to prune volume", | ||
| metadata: [ | ||
| "id": "\(volume.name)", | ||
| "error": "\(error)", | ||
| ]) | ||
| } | ||
| } | ||
|
|
||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -206,7 +206,8 @@ extension PluginLoader { | |
| plugin: Plugin, | ||
| pluginStateRoot: URL? = nil, | ||
| args: [String]? = nil, | ||
| instanceId: String? = nil, | ||
| debug: Bool = false, | ||
| ) throws { | ||
| // We only care about loading plugins that have a service | ||
| // to expose; otherwise, they may just be CLI commands. | ||
|
|
@@ -223,9 +224,10 @@ extension PluginLoader { | |
| env[ApplicationRoot.environmentName] = appRoot.path(percentEncoded: false) | ||
| env[InstallRoot.environmentName] = installRoot.path(percentEncoded: false) | ||
|
|
||
| let processedArgs = (args ?? ["start"]) + (debug ? ["--debug"] : []) | ||
|
Contributor
Author
There was a problem hiding this comment. This is the main change, adds The implication is that all plugins need to implement a |
||
| let plist = LaunchPlist( | ||
| label: id, | ||
| arguments: [plugin.binaryURL.path] + processedArgs + serviceConfig.defaultArguments, | ||
| environment: env, | ||
| limitLoadToSessionType: [.Aqua, .Background, .System], | ||
| runAtLoad: serviceConfig.runAtLoad, | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,6 +19,13 @@ import ContainerizationError | |
| import Foundation | ||
|
|
||
| public final class XPCClient: Sendable { | ||
| /// The maximum amount of time to wait for a request to a recently | ||
| /// registered XPC service. Once a service has launched, XPC | ||
| /// requests only have milliseconds of overhead, but in some instances, | ||
| /// macOS can take 5 seconds (or considerably longer) to launch a | ||
| /// service after it has been registered. | ||
| public static let xpcRegistrationTimeout: Duration = .seconds(60) | ||
|
Contributor
Author
There was a problem hiding this comment. Our common timeout value, chosen to work with our slow service startup in CI. |
||
|
|
||
| private nonisolated(unsafe) let connection: xpc_connection_t | ||
| private let q: DispatchQueue? | ||
| private let service: String | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -117,7 +117,11 @@ public struct XPCServer: Sendable { | |
| // a final XPC_ERROR_CONNECTION_INVALID message. | ||
| // We can ignore this if we know we have already handled | ||
| // the request. | ||
| self.log.error( | ||
| "xpc client handler connection error", | ||
| metadata: [ | ||
| "error": "\(object.errorDescription ?? "no description")" | ||
| ]) | ||
| } | ||
| default: | ||
| fatalError("unhandled xpc object type: \(xpc_get_type(object))") | ||
|
|
@@ -195,14 +199,14 @@ public struct XPCServer: Sendable { | |
| let response = try await handler(message) | ||
| xpc_connection_send_message(connection, response.underlying) | ||
| } catch let error as ContainerizationError { | ||
| log.error("route handler threw an error", metadata: ["route": "\(route)", "error": "\(error)"]) | ||
| Self.replyWithError( | ||
| connection: connection, | ||
| object: object, | ||
| err: error | ||
| ) | ||
| } catch { | ||
| log.error("route handler threw an error", metadata: ["route": "\(route)", "error": "\(error)"]) | ||
| let message = XPCMessage(object: object) | ||
| let reply = message.reply() | ||
|
|
||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -46,22 +46,22 @@ extension APIServer { | |
| func run() async throws { | ||
| let commandName = Self.configuration.commandName ?? "container-apiserver" | ||
| let log = APIServer.setupLogger(debug: debug) | ||
| log.info("starting helper", metadata: ["name": "\(commandName)"]) | ||
|
Contributor
Author
There was a problem hiding this comment. This pattern is used for startup/shutdown for all services. |
||
| defer { | ||
| log.info("stopping helper", metadata: ["name": "\(commandName)"]) | ||
| } | ||
|
|
||
| do { | ||
| log.info("configuring XPC server") | ||
| var routes = [XPCRoute: XPCServer.RouteHandler]() | ||
| let pluginLoader = try initializePluginLoader(log: log) | ||
| try await initializePlugins(pluginLoader: pluginLoader, log: log, routes: &routes) | ||
| let containersService = try initializeContainersService( | ||
|
Contributor
Author
There was a problem hiding this comment. Just some name nitpickery for "containers service" and "networks service". |
||
| pluginLoader: pluginLoader, | ||
| log: log, | ||
| routes: &routes | ||
| ) | ||
| let networkService = try await initializeNetworksService( | ||
| pluginLoader: pluginLoader, | ||
| containersService: containersService, | ||
| log: log, | ||
|
|
@@ -131,7 +131,7 @@ extension APIServer { | |
| */ | ||
| } | ||
| } catch { | ||
| log.error("helper failed", metadata: ["name": "\(commandName)", "error": "\(error)"]) | ||
| APIServer.exit(withError: error) | ||
| } | ||
| } | ||
|
|
@@ -222,13 +222,14 @@ extension APIServer { | |
| routes[XPCRoute.getDefaultKernel] = harness.getDefaultKernel | ||
| } | ||
|
|
||
| private func initializeContainersService(pluginLoader: PluginLoader, log: Logger, routes: inout [XPCRoute: XPCServer.RouteHandler]) throws -> ContainersService { | ||
| log.info("initializing containers service") | ||
|
|
||
| let service = try ContainersService( | ||
| appRoot: appRoot, | ||
| pluginLoader: pluginLoader, | ||
| log: log, | ||
| debugHelpers: debug | ||
|
Contributor
Author
There was a problem hiding this comment. Propagate debug settings from API server to any runtime helpers we create. |
||
| ) | ||
| let harness = ContainersHarness(service: service, log: log) | ||
|
|
||
|
|
@@ -250,20 +251,21 @@ extension APIServer { | |
| return service | ||
| } | ||
|
|
||
| private func initializeNetworksService( | ||
| pluginLoader: PluginLoader, | ||
| containersService: ContainersService, | ||
| log: Logger, | ||
| routes: inout [XPCRoute: XPCServer.RouteHandler] | ||
| ) async throws -> NetworksService { | ||
| log.info("initializing networks service") | ||
|
|
||
| let resourceRoot = appRoot.appendingPathComponent("networks") | ||
| let service = try await NetworksService( | ||
| pluginLoader: pluginLoader, | ||
| resourceRoot: resourceRoot, | ||
| containersService: containersService, | ||
| log: log, | ||
| debugHelpers: debug | ||
|
Contributor
Author
There was a problem hiding this comment. Propagate debug settings from API server to any network helpers we create. |
||
| ) | ||
|
|
||
| let defaultNetwork = try await service.list() | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -44,7 +44,7 @@ public class DirectoryWatcher { | |
| throw ContainerizationError(.invalidState, message: "failed to start watching on \(directoryURL.path)") | ||
| } | ||
|
|
||
| log.info("starting directory watcher", metadata: ["path": "\(directoryURL.path)"]) | ||
|
|
||
| let descriptor = open(directoryURL.path, O_EVTONLY) | ||
|
|
||
|
|
||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't do this anymore since we'll want to add other options (--log-root) soon. Make it so we can accumulate startup options.