-
Notifications
You must be signed in to change notification settings - Fork 487
Swap to APIServer for all communications #628
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
4838bcf
to
e7e4dcb
Compare
1a56c99
to
6a497e6
Compare
6a497e6
to
5d5e622
Compare
Sources/Services/ContainerAPIService/Containers/ContainersService.swift
Outdated
Show resolved
Hide resolved
Sources/Services/ContainerAPIService/Containers/ContainersService.swift
Outdated
Show resolved
Hide resolved
Sources/Services/ContainerAPIService/Containers/ContainersService.swift
Outdated
Show resolved
Hide resolved
Sources/Services/ContainerAPIService/Containers/ContainersService.swift
Outdated
Show resolved
Hide resolved
return container | ||
} | ||
|
||
private func gracefulStopContainer(_ lc: LinuxContainer, stopOpts: ContainerStopOptions) async throws { |
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.
Note about code not modified in this PR:
private nonisolated func configureProcessConfig()
This should be able to become:
private static func configureProcessConfig()
Same for closeHandle()
.
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.
Another lowkey obsessive nit in the non-modified code above - move getDefaultNameserver
out from between the configure...()
funcs. All the private methods could probably stand to be reordered sensibly. In a swift file that's pushing 1200 lines it will help new developers.
560d05a
to
07707d5
Compare
Converting to draft because there's a small cz change that I want in that will allow us to "fix" some behavior in stop() |
Today, we have a sort of odd model all things considered. We talk to the APIServer to do the initial container creation, which mostly all of the work is just registering the runtime helper with launchd. After this registration, almost all communication from a client is talking directly to that runtime helper. This has a couple rather annoying issues in that we now need a sort of event/notification channel that the helper will establish with the APIServer for errors/start/exited cases. If instead of talking to the runtime helper directly, we instead took a detour through the APIServer, the APIServer is clued into exactly what order of operations is occurring. This makes "did starting the container fail? Okay we should clean up" scenarios much simpler, and it also simplifies the clients quite a bit as they don't need this split brained client model, everyone just talks to the APIServer. This change is in pursuit of that. I have reworked our clients, the ContainerService and some of our XPC types to accomplish it. The biggest "contract" changes are in the SandboxService. The first is today we have on the flag when we register any runtime helper that makes any xpc messages wake up the registered process. This isn't great in scenarios where the process may have crashed, or it exited normally and we're just trying to invoke an RPC on it. Today the helper would spawn again and try and answer our request. It'd be much nicer if we have a connection object that will become invalid if the process that vended it to us is gone. To accomplish this, now the runtime helpers will listen on an anonymous xpc connection and vend endpoints from this via only one handler exposed by the SandboxService (createEndpoint). From that point onwards all communication will be through the endpoint the service vended a client. The second change is the runtime helper will not exit on its own when the container exits, and the event mechanism has been removed. Now the APIServer simply calls wait() to listen for container exit in the background, and once we get an exit we will explicitly tell the helper to shutdown. The rationale is if shutdown is driven by the APIServer now, we can be certain we received everything we need from the helpers before they power down.
07707d5
to
66fef47
Compare
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.
👍
if let h { | ||
request.set(key: key, value: h) | ||
} | ||
} |
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.
This code repeats 4 times. Consider extracting it into helper methods in XPCMessage
:
static func stdioKey(for index: Int) throws -> XPCKeys {
switch index {
case 0: return .stdin
case 1: return .stdout
case 2: return .stderr
default:
throw ContainerizationError(.invalidArgument, message: "invalid fd \(index)")
}
}
static func setStdioHandles(on request: XPCMessage, stdio:
[FileHandle?]) throws {
for (index, handle) in stdio.enumerated() {
if let handle {
request.set(key: stdioKey(for: index), value: handle)
}
}
}
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.
John had commented the same, I'm going to do that (and some other cleanups) in a followup
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.
Changes look good!
Today, we have a sort of odd model all things considered. We talk to the APIServer to do the initial container creation, which mostly all of the work is just registering the runtime helper with launchd. After this registration, almost all communication from a client is talking directly to that runtime helper. This has a couple rather annoying issues in that we now need a sort of event/notification channel that the helper will establish with the APIServer for errors/start/exited cases.
If instead of talking to the runtime helper directly, we instead took a detour through the APIServer, the APIServer is clued into exactly what order of operations is occurring. This makes "did starting the container fail? Okay we should clean up" scenarios much simpler, and it also simplifies the clients quite a bit as they don't need this split brained client model, everyone just talks to the APIServer. This change is in pursuit of that. I have reworked our clients, the ContainerService and some of our XPC types to accomplish it.
The biggest "contract" change is in the SandboxService. Today we have on the flag when we register any runtime helper that makes any xpc messages wake up the registered process. This isn't great in scenarios where the process have may crashed, or it exited normally and we're just trying to invoke an RPC on it. Today the helper would spawn again and try and answer our request. It'd be much nicer if we have a connection object that will become invalid if the process that vended it to us is gone. To accomplish this, now the runtime helpers will listen on an anonymous xpc connection and vend endpoints from this via only one handler exposed by the SandboxService (createEndpoint). From that point onwards all communication will be through the endpoint the service vended a client.