Conversation
kzys
left a comment
There was a problem hiding this comment.
Looks good to me! Mostly questions.
f8d6c52 to
860b777
Compare
fuweid
left a comment
There was a problem hiding this comment.
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.
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.
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.
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.
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..
ef3756d to
482c080
Compare
c0dcbef to
66fec58
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.
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.
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)
|
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.