-
Notifications
You must be signed in to change notification settings - Fork 3.8k
Sandbox API #5396
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
Sandbox API #5396
Conversation
kzys
left a comment
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.
Looks good to me! Mostly questions.
f8d6c52 to
860b777
Compare
fuweid
left a comment
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.
Leave some comments on this.
I was thinking the sandbox API is built on existing task API but it looks like mixing two APIs. Needs more time to review this. Thanks for sharing!
|
I have some questions about that shimv2 api and sandbox api. If we create a sandbox with sandbox api, then the task can run in the sandbox. so if a shimv2 server is started after sanbox started, and serves an endpoint that containerd can connect, then containerd can start task directly through the shimv2 service in the sandbox. |
metadata/sandbox.go
Outdated
| if len(fieldpaths) == 0 { | ||
| fieldpaths = []string{"labels", "extensions", "spec", "runtime"} | ||
|
|
||
| if updated.Runtime.Name != sandbox.Runtime.Name { |
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.
Is there any reason that we validate "updated.Runtime.Name != sandbox.Runtime.Name" when "len(fieldpaths) == 0"?
Seem we should validate Runtime.Name for all update operations.
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.
agree I think it should validate here for all update operations.. to make sure there is no issue with the ns bucket, esp. in case there might be some reuse issue? and then again below in the switch on field update we should check for immutable
runtime/runtime.go
Outdated
|
|
||
| // SandboxRuntime is an optional extension for PlatformRuntime to manage sandboxes | ||
| type SandboxRuntime interface { | ||
| PlatformRuntime |
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.
With the new SandboxRuntime interface, it's natural to rename PlatformRuntime as TaskRuntime. And we may add more service runtime to SandboxRuntime, such as pod specific image service for confidential computing.
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.
was renamed from Runtime to PlatformRuntime 3years back..
"This renames the runtime interface to PlatformRuntime to denote the
layer at which the runtime is being abstracted. This should be used to
abstract different platforms that vary greatly and do not have full
compat with OCI based binary runtimes."
I understand there is a push to consider changing how we abstract runtimes and image services in the container runtimes..but not sure that should hold up/or be a part of this pr.. i see sandbox as a lower level containerd service than cri pod..
9c9f17a to
fcc2c40
Compare
sandbox/store.go
Outdated
| // Runtime shim to use for this sandbox | ||
| Runtime RuntimeOpts | ||
| // Spec carries the runtime specification used to implement the sandbox | ||
| Spec *types.Any |
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.
In the exported interface I wonder if we can just stick to interface{} and when we need a *types.Any we can type assert for that or marshal to it if something else. I think it is already going to be a challenge using types from gogo/protobuf so it is better we find a better solution now.
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 is relatively straightforward to do (mostly adds typeurl.MarshalAny/typeurl.UnmarshalAny back and forth). However one particular limitation of this is that containerd needs to know the type to be passed in advance (via typeurl.Register), so custom types will not be allowed without forking contained. This is fine as we currently use container spec and its already registered, but might be a limitation if someone decides to pass custom spec to the runtime.
Possibly migration to protobuf's native Any type would be a better option? (1 2)
ccc8e51 to
50a647f
Compare
|
Build succeeded.
|
|
Last rebase didn't go well and Github won't let me to reopen this PR (nor it sees no new commits any more). Had to open another one: #6703 |
|
Build succeeded.
|
I think I'm pretty happy with how it looks now and want start getting feedback.
This PR implements sandbox proposal described in #4131
Implementation goals:
client.WithSandboxspecified to assign container to a specific sandbox).Summary of changes:
Sandbox(kin ofContainer), it describes sandbox state that can be saved to database. 2) Sandbox instance aka shim akaSandboxID(kin ofTask, not exactly represented as entity, but rather as a runtime ID) - runtime representation of sandbox. Internally its backed by the runtime shim that implements sandbox extensions.metadatapackage. Adds sandboxStore to save/load Sandbox record from boltdb.runtimeadds sandbox manager. It wraps existing task manager to manage shims. WhenContainerhasSandboxIDfield specified, sandbox manager will use this ID to look up for an appropriate shim instance to route request to. If it's empty, sandbox manager will route request to existing task manager, so existing behavior is preserved.shimoptionally may implementSandboxService. If so, it'll register an additional TTRPC endpoint that containerd can talk to to manage sandbox instance.clientaddsSandboxinterface similar toContainerclient interface.Client side example:
Shim side: b564c33
There are a few remaining bits left - client examples, publish proper events, more test coverage, integrations tests, etc. However I'd like to gather feedback on the overall design/implementation.