Skip to content

Conversation

@mxpv
Copy link
Member

@mxpv mxpv commented Apr 19, 2021

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:

  • Keep existing task service compatible with sandboxed tasks. e.g. have sandboxes that are managed independently by clients, but keep existing task service compatible for both cases and use same API/services (the only difference is that container would need client.WithSandbox specified to assign container to a specific sandbox).
  • Keep shim’s changes minimal. There are a few existing microVMs runtimes that I used as reference for the shim design. Existing task interface is pretty much sufficient. So the goal here was to extend, but don't change existing things (actually lots of them were contributed separately Decouple shim start from task creation #5336 Runtime cleanup #5368).

Summary of changes:

  • Overall Sandbox API adds two new entities to containerd - 1) Sandbox (kin of Container), it describes sandbox state that can be saved to database. 2) Sandbox instance aka shim aka SandboxID (kin of Task, 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.
  • metadata package. Adds sandboxStore to save/load Sandbox record from boltdb.
  • runtime adds sandbox manager. It wraps existing task manager to manage shims. When Container has SandboxID field 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.
  • shim optionally may implement SandboxService. If so, it'll register an additional TTRPC endpoint that containerd can talk to to manage sandbox instance.
  • client adds Sandbox interface similar to Container client interface.

Client side example:

sandbox, err := client.NewSandbox(ctx, "sandbox-id-1",
	WithSandboxRuntime("runtime", shimOpts), // Select shim runtime and provide extra options via StartOpts.Options
	WithSandboxSpec(spec), // Sandbox spec to be written to bundle dir (currently allow arbitrary interface{})
	WithSandboxLabels(labels), // Labels to be added to store
)

// Start new sandbox instance
err = sandbox.Start(ctx)

// Now schedule tasks to this instance via
container, err := sandbox.NewContainer(ctx, "container-1", WithXYZ...)
// OR
client.NewContainer(ctx, "container-2", WithSandbox(sandbox.ID()), WithXYZ...)

// Same task service flow as usual
// ...

// Sandbox specific calls
err = sandbox.Status(ctx, out)
err = sandbox.Ping()
err = sandbox.Pause(ctx)
err = sandbox.Resume(ctx)

// Kill everything and shutdown sandbox instance
err = sandbox.Shutdown(ctx)

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.

@mxpv mxpv added this to the 1.6 milestone Apr 19, 2021
@cpuguy83 cpuguy83 requested a review from kevpar April 19, 2021 22:54
@mxpv mxpv requested review from dmcgowan and kzys April 19, 2021 23:05
@containerd containerd deleted a comment from theopenlab-ci bot Apr 20, 2021
Copy link
Member

@kzys kzys left a 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.

@mxpv mxpv force-pushed the s branch 5 times, most recently from f8d6c52 to 860b777 Compare April 20, 2021 19:00
@containerd containerd deleted a comment from theopenlab-ci bot Apr 20, 2021
@containerd containerd deleted a comment from theopenlab-ci bot Apr 20, 2021
@containerd containerd deleted a comment from theopenlab-ci bot Apr 20, 2021
Copy link
Member

@fuweid fuweid left a 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!

@abel-von
Copy link
Contributor

abel-von commented May 8, 2021

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.
I think that would be a better design, because there is no need of a shim on the host for each pod. the shimv2 server can run in the sandbox(for the vm based sandbox, it runs in the vm, and provide service through virtio-vsock or serial port, replacing the agent in the vm, or the agent serves shimv2 api directly).

if len(fieldpaths) == 0 {
fieldpaths = []string{"labels", "extensions", "spec", "runtime"}

if updated.Runtime.Name != sandbox.Runtime.Name {
Copy link
Contributor

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.

Copy link
Member

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


// SandboxRuntime is an optional extension for PlatformRuntime to manage sandboxes
type SandboxRuntime interface {
PlatformRuntime
Copy link
Contributor

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.

Copy link
Member

@mikebrow mikebrow Jul 14, 2021

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..

@containerd containerd deleted a comment from k8s-ci-robot Feb 10, 2022
@containerd containerd deleted a comment from k8s-ci-robot Feb 10, 2022
@containerd containerd deleted a comment from k8s-ci-robot Feb 10, 2022
@containerd containerd deleted a comment from k8s-ci-robot Feb 10, 2022
@containerd containerd deleted a comment from k8s-ci-robot Feb 10, 2022
@containerd containerd deleted a comment from k8s-ci-robot Feb 10, 2022
@containerd containerd deleted a comment from k8s-ci-robot Feb 10, 2022
@mxpv mxpv force-pushed the s branch 2 times, most recently from 9c9f17a to fcc2c40 Compare February 11, 2022 22:11
@containerd containerd deleted a comment from k8s-ci-robot Feb 11, 2022
@mxpv mxpv marked this pull request as ready for review February 13, 2022 22:41
@containerd containerd deleted a comment from theopenlab-ci bot Feb 13, 2022
@mxpv mxpv self-assigned this Mar 15, 2022
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
Copy link
Member

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.

Copy link
Member Author

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)

@mxpv mxpv force-pushed the s branch 4 times, most recently from ccc8e51 to 50a647f Compare March 18, 2022 18:47
@containerd containerd deleted a comment from theopenlab-ci bot Mar 18, 2022
@containerd containerd deleted a comment from theopenlab-ci bot Mar 18, 2022
@theopenlab-ci
Copy link

theopenlab-ci bot commented Mar 18, 2022

Build succeeded.

@mxpv
Copy link
Member Author

mxpv commented Mar 20, 2022

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

@theopenlab-ci
Copy link

theopenlab-ci bot commented Mar 20, 2022

Build succeeded.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.