Sandbox API #6703

mxpv · 2022-03-20T22:02:13Z

Last rebase didn't go well and Github won't let me to reopen the old PR (nor recognizes new commits). Context and discussion here: #5396

dmcgowan · 2022-03-22T04:50:31Z

services/sandbox/controller_local.go

+
+func init() {
+	plugin.Register(&plugin.Registration{
+		Type: plugin.ServicePlugin,


I think this is cleanup we need to or should do across the service packages today, but in the case where there is a defined interface for service, it is better to export an implementation of that interface rather than that of the generated API client.

sandbox/controller.go

dmcgowan · 2022-03-22T04:55:53Z

sandbox_store.go

+var _ sb.Store = (*remoteSandboxStore)(nil)
+
+// NewRemoteSandboxStore create client for sandbox store
+func NewRemoteSandboxStore(client api.StoreClient) sb.Store {


Note for followup: This can be moved to a subpackage such as sandbox/proxy

dmcgowan · 2022-03-22T04:57:57Z

services.go

@@ -155,3 +159,17 @@ func WithIntrospectionService(in introspection.Service) ServicesOpt {
 		s.introspectionService = in
 	}
 }
+
+// WithSandboxStore sets the sandbox store.
+func WithSandboxStore(client sandboxsapi.StoreClient) ServicesOpt {


Note for follow up: We are trying to prevent directly using these generated interfaces in the client interface. The alternative would be to explicitly get an instance of the interface using the API client and passing in that instance.

dmcgowan · 2022-03-22T05:00:19Z

Mostly just a few questions for me. I think we can handle some of the changes in follow-ups since they are similar to ongoing refactoring around the API and protobufs. If we can agree on the API we should just get this in.

theopenlab-ci · 2022-03-29T21:39:11Z

Build succeeded.

containerd-build-arm64 : SUCCESS in 4m 25s (non-voting)
containerd-test-arm64 : SUCCESS in 5m 59s (non-voting)
containerd-integration-test-arm64 : SUCCESS in 25m 10s (non-voting)

dmcgowan · 2022-03-30T18:57:02Z

/test pull-containerd-node-e2e

cpuguy83

Is there some way to test this with ctr?

I see I can create a sandbox but then what?
I see there is a running v2 shim after.

Apologies, I'm a bit confused as to what's supposed to happen for these sandboxes.

When I create a sandbox using defaults I get a runc-v2 shim with no tasks.
How do I attach a task to it?

How would cri use this to implement the pod sandbox?

cpuguy83 · 2022-03-30T20:54:54Z

sandbox/controller.go

+// When running the traditional containerd shim, the workflow looks as follows:
+// For each new task we're about to run:
+//  1. Invoke `shim_binary --start` to obtain `TaskService` address (printed in stdout)
+//  2. Call TaskService.RunContainer(id=1)


I don't think these RunContainer calls are a real API? Is the doc out of date with the implementation?

Yep, removed outdated docs

cpuguy83 · 2022-03-30T21:43:08Z

cmd/ctr/commands/sandboxes/sandboxes.go

+	Name:      "run",
+	Aliases:   []string{"create", "c", "r"},
+	Usage:     "run a new sandbox",
+	ArgsUsage: "[flags] <pod-config.json> <sandbox-id>",


No pod-config.json, I think?

This can be any spec recognized to a shim implementation.
These includes the container spec used when launching pause container:

containerd/pkg/cri/server/sandbox_run.go

Line 177 in 887615c

spec, err := c.sandboxContainerSpec(id, config, &image.ImageSpec.Config, sandbox.NetNSPath, ociRuntime.PodAnnotations)

cpuguy83 · 2022-03-30T21:45:56Z

cmd/ctr/commands/sandboxes/sandboxes.go

+		}
+
+		for _, sandbox := range list {
+			_, err := fmt.Fprintf(writer, "%s\t%s\t%s\t\n", sandbox.ID, sandbox.CreatedAt, sandbox.Runtime.Name)


for follow up maybe, but this output is not easy to read.

ID CREATED RUNTIME test 2022-03-30 21:39:50.043075106 +0000 UTC io.containerd.runc.v2

cpuguy83 · 2022-03-30T21:50:35Z

api/services/sandbox/v1/sandbox.proto

+
+message ControllerStartRequest {
+	string sandbox_id = 1;
+	repeated containerd.types.Mount rootfs = 2;


Where does this rootfs come from? Who determines what the rootfs is?

Client is expected to specify this.
Same way as CRI does today via WithNewSnapshot call when creating a sandbox.

For VMs (incl. Firecracker), would we assume they boot Linux from the rootfs? How much compatibility between non-VM sandboxes and VM sandboxes is expected?

I've been working on a prototype with "pause" containers and this field was needed to pass a mount, so the shim could launch pause containers from the runtime via start sandbox request. For Firecracker this can be an image to boot from. Here containerd's client is required to provide proper values that will be recognized by shim implementations.

Where does this rootfs come from? Who determines what the rootfs is?

Client is expected to specify this.
Same way as CRI does today via WithNewSnapshot call when creating a sandbox.

I've been working on a prototype with "pause" containers and this field was needed to pass a mount, so the shim could launch pause containers from the runtime via start sandbox request. For Firecracker this can be an image to boot from. Here containerd's client is required to provide proper values that will be recognized by shim implementations.

Client is expected to specify this.
Same way as CRI does today via WithNewSnapshot call when creating a sandbox.

mxpv · 2022-03-31T21:50:19Z

I see I can create a sandbox but then what?
I see there is a running v2 shim after.
Apologies, I'm a bit confused as to what's supposed to happen for these sandboxes.
When I create a sandbox using defaults I get a runc-v2 shim with no tasks.
How do I attach a task to it?

This is daemon side API to manage sandboxes.
To test it end to end, we require shim and CRI (or invoke it from ctr) to support this new APIs.
This will be done in subsequent PRs as this one already huge enough to review.

Is there some way to test this with ctr?

Yes, there is ctr subcommands that allow you to invoke the API. See sandboxes.go

How would cri use this to implement the pod sandbox?

This requires changes on CRI side. That's ongoing work.

theopenlab-ci · 2022-03-31T22:29:50Z

Build succeeded.

containerd-build-arm64 : SUCCESS in 5m 17s (non-voting)
containerd-test-arm64 : SUCCESS in 6m 01s (non-voting)
containerd-integration-test-arm64 : FAILURE in 29m 09s (non-voting)

kzys · 2022-04-01T18:26:14Z

api/services/sandbox/v1/sandbox.proto

+
+import "google/protobuf/any.proto";
+import "google/protobuf/timestamp.proto";
+import weak "gogoproto/gogo.proto";


We should remove weak after merge.

kzys · 2022-04-01T18:27:45Z

api/services/sandbox/v1/sandbox.proto

+
+message ControllerStartRequest {
+	string sandbox_id = 1;
+	repeated containerd.types.Mount rootfs = 2;


For VMs (incl. Firecracker), would we assume they boot Linux from the rootfs? How much compatibility between non-VM sandboxes and VM sandboxes is expected?

kzys · 2022-04-01T18:31:40Z

api/services/sandbox/v1/sandbox.proto

+
+message ControllerShutdownRequest {
+	string sandbox_id = 1;
+	uint32 timeout_secs = 2;


Would we need the timeout on ControllerStartRequest?

Should we have this timeout in addition to the underlying RPC framework's timeout? For example, gRPC itself could have its own timeout. https://github.com/grpc/grpc/blob/master/doc/PROTOCOL-HTTP2.md

Sandbox implementations are expected to shutdown both running containers (if any) and the sandbox itself on shutdown request. So timeout would be useful to try to shutdown containers properly first and in case if that take too long - kill them. There is similar logic in CRI:

containerd/pkg/cri/server/sandbox_stop.go

Line 51 in 1ba613e

func (c *criService) stopPodSandbox(ctx context.Context, sandbox sandboxstore.Sandbox) error {

Start request should just launch a new sandbox, so there is no need to have a dedicated timeout parameter.

This seems a little high-level to me. Shouldn't the client drive this?

Signed-off-by: Maksym Pavlenko <pavlenko.maksym@gmail.com>

theopenlab-ci · 2022-04-08T21:03:59Z

Build succeeded.

containerd-build-arm64 : SUCCESS in 4m 30s (non-voting)
containerd-test-arm64 : SUCCESS in 5m 48s (non-voting)
containerd-integration-test-arm64 : FAILURE in 26m 55s (non-voting)

runtime/v2/runc/pause/sandbox.go

cpuguy83

LGTM

PR containerd#6366 implemented a tree-wide change to replace github.com/pkg/errors to errors. The new sandbox API PR containerd#6703 had few errors.Wrap*() leftovers and pulled github.com/pkg/errors back. This commit replaces those leftovers by following the pattern in containerd#6366. Signed-off-by: Mikko Ylinen <mikko.ylinen@intel.com>

mxpv force-pushed the s branch from da8492c to 74a4e6b Compare March 20, 2022 22:03

containerd deleted a comment from k8s-ci-robot Mar 20, 2022

mxpv mentioned this pull request Mar 20, 2022

Sandbox API #5396

Closed

mxpv force-pushed the s branch from 74a4e6b to 9ece170 Compare March 21, 2022 18:43

containerd deleted a comment from theopenlab-ci bot Mar 21, 2022

dmcgowan added this to the 1.7 milestone Mar 21, 2022

dmcgowan added this to New in Code Review via automation Mar 21, 2022

dmcgowan reviewed Mar 22, 2022

View reviewed changes

sandbox/controller.go Outdated Show resolved Hide resolved

dmcgowan reviewed Mar 22, 2022

View reviewed changes

mxpv force-pushed the s branch from cf5c88e to 1125871 Compare March 22, 2022 23:02

containerd deleted a comment from theopenlab-ci bot Mar 23, 2022

dmcgowan moved this from New to Ready For Review in Code Review Mar 25, 2022

mxpv force-pushed the s branch from 1125871 to d4dc823 Compare March 29, 2022 21:12

dmcgowan added the impact/changelog label Mar 29, 2022

containerd deleted a comment from theopenlab-ci bot Mar 30, 2022

containerd deleted a comment from k8s-ci-robot Mar 30, 2022

dmcgowan approved these changes Mar 30, 2022

View reviewed changes

cpuguy83 requested changes Mar 30, 2022

View reviewed changes

cpuguy83 reviewed Mar 30, 2022

View reviewed changes

kzys reviewed Apr 1, 2022

View reviewed changes

mxpv added 3 commits April 8, 2022 13:33

[sandbox] Add protobuf definitions

4445d0a

Signed-off-by: Maksym Pavlenko <pavlenko.maksym@gmail.com>

[sandbox] Add basic sandbox structures and interfaces

87d4c89

Signed-off-by: Maksym Pavlenko <pavlenko.maksym@gmail.com>

[sandbox] Implement metadata store

cab7d5b

Signed-off-by: Maksym Pavlenko <pavlenko.maksym@gmail.com>

mxpv added 6 commits April 8, 2022 13:33

Restore sandboxes on daemon restart

0d165e6

Signed-off-by: Maksym Pavlenko <pavlenko.maksym@gmail.com>

[Sandbox] Add Wait and PID

b7a3695

Signed-off-by: Maksym Pavlenko <pavlenko.maksym@gmail.com>

[sandbox] Fix CI

de49745

Signed-off-by: Maksym Pavlenko <pavlenko.maksym@gmail.com>

[sandbox] Cleanup interfaces

85a49e4

Signed-off-by: Maksym Pavlenko <pavlenko.maksym@gmail.com>

[sandbox] Migrate from gogo to Any

d0b32c0

Signed-off-by: Maksym Pavlenko <pavlenko.maksym@gmail.com>

[Sandbox] Remove outdated documentation

b446c76

Signed-off-by: Maksym Pavlenko <pavlenko.maksym@gmail.com>

mxpv force-pushed the s branch from e1b4248 to b446c76 Compare April 8, 2022 20:35

dmcgowan approved these changes Apr 11, 2022

View reviewed changes

cpuguy83 reviewed Apr 13, 2022

View reviewed changes

runtime/v2/runc/pause/sandbox.go Show resolved Hide resolved

cpuguy83 approved these changes Apr 18, 2022

View reviewed changes

kzys approved these changes Apr 18, 2022

View reviewed changes

dmcgowan merged commit be60973 into containerd:main Apr 19, 2022

Code Review automation moved this from Ready For Review to Done Apr 19, 2022

mythi mentioned this pull request May 12, 2022

sandbox: replace github.com/pkg/errors with native errors #6937

Merged

mxpv deleted the s branch June 16, 2022 19:50

mxpv mentioned this pull request Aug 19, 2022

Sandbox API work #7312

Closed

17 tasks

dmcgowan mentioned this pull request Dec 10, 2022

Prepare release notes for v1.7.0-beta.1 #7793

Merged

dmcgowan mentioned this pull request Jan 3, 2023

Prepare release notes for v1.7.0-beta.2 #7903

Merged

dmcgowan mentioned this pull request Jan 28, 2023

Prepare release notes for v1.7.0-beta.3 #8014

Merged

dmcgowan mentioned this pull request Feb 15, 2023

Prepare release notes for v1.7.0 beta.4 #8110

Merged

This was referenced Feb 25, 2023

Prepare release notes for v1.7.0-rc.0 #8162

Merged

Prepare release notes for v1.7.0-rc.1 #8192

Merged

This was referenced Mar 7, 2023

Add release notes for v1.7.0-rc.2 #8216

Merged

Prepare release notes for v1.7.0-rc.3 #8233

Merged

Prepare release notes for v1.7.0 #8242

Merged

zhaojizhuang mentioned this pull request May 21, 2023

[sandbox] Optimize metadata Schema notes to correctly match metadata structures #8547

Merged

Joseph0207 approved these changes Sep 11, 2023

View reviewed changes

cpuguy83 mentioned this pull request Nov 3, 2023

Support V3 containerd task service and Sandboxers containerd/runwasi#385

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sandbox API #6703

Sandbox API #6703

mxpv commented Mar 20, 2022 •

edited

Loading

dmcgowan Mar 22, 2022

dmcgowan Mar 22, 2022

dmcgowan Mar 22, 2022

dmcgowan commented Mar 22, 2022

theopenlab-ci bot commented Mar 29, 2022

dmcgowan commented Mar 30, 2022

cpuguy83 left a comment

cpuguy83 Mar 30, 2022

mxpv Mar 31, 2022

cpuguy83 Mar 30, 2022

mxpv Mar 31, 2022

cpuguy83 Mar 30, 2022

cpuguy83 Mar 30, 2022

mxpv Mar 31, 2022

kzys Apr 1, 2022

mxpv Apr 1, 2022

SteveBombardier Dec 15, 2023

SteveBombardier Dec 15, 2023

SteveBombardier Dec 15, 2023

SteveBombardier Dec 15, 2023

SteveBombardier Dec 15, 2023

SteveBombardier Dec 15, 2023

mxpv commented Mar 31, 2022 •

edited

Loading

theopenlab-ci bot commented Mar 31, 2022

kzys Apr 1, 2022

SteveBombardier Dec 15, 2023

kzys Apr 1, 2022

kzys Apr 1, 2022

mxpv Apr 1, 2022 •

edited

Loading

cpuguy83 Apr 13, 2022

theopenlab-ci bot commented Apr 8, 2022

cpuguy83 left a comment

Sandbox API #6703

Sandbox API #6703

Conversation

mxpv commented Mar 20, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmcgowan commented Mar 22, 2022

theopenlab-ci bot commented Mar 29, 2022

dmcgowan commented Mar 30, 2022

cpuguy83 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mxpv commented Mar 31, 2022 • edited Loading

theopenlab-ci bot commented Mar 31, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mxpv Apr 1, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theopenlab-ci bot commented Apr 8, 2022

cpuguy83 left a comment

Choose a reason for hiding this comment

mxpv commented Mar 20, 2022 •

edited

Loading

mxpv commented Mar 31, 2022 •

edited

Loading

mxpv Apr 1, 2022 •

edited

Loading