Add sandboxer configuration and move sandbox controllers to plugins #8268

abel-von · 2023-03-14T15:03:47Z

The sandbox controller is hard coded in the cri plugin yet, in this PR I reconstruct the codes to make the "shim" and "podsandbox" controllers a sandbox controller plugin in containerd.

This is the first PR of the proposal #7739

k8s-ci-robot · 2023-03-14T15:03:57Z

Hi @abel-von. Thanks for your PR.

I'm waiting for a containerd member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Burning1020 · 2023-03-15T09:34:30Z

PTAL @dmcgowan @mxpv @fuweid

dmcgowan · 2023-06-06T16:51:12Z

api/services/sandbox/v1/sandbox.proto

-	repeated containerd.types.Mount rootfs = 2;
-	google.protobuf.Any options = 3;
-	string netns_path = 4;
+	string sandboxer = 1;


Since this API has already been included in a release, we don't want to make numeric changes like this. We are not planning on changing the API versions (from v1 to v2) for the 2.0 release. The sandboxer could be set to a consistent number such as 10 at the end though, the order isn't as important as the compatibility.

dmcgowan · 2023-06-06T16:53:16Z

pkg/cri/config/config.go

 	// This features requires experimental CRI server to be enabled (use ENABLE_CRI_SANDBOXES=1)
 	// shim - means use whatever Controller implementation provided by shim (e.g. use RemoteController).
 	// podsandbox - means use Controller implementation from sbserver podsandbox package.
-	SandboxMode string `toml:"sandbox_mode" json:"sandboxMode"`
+	Sandboxer string `toml:"sandboxer" json:"sandboxer"`


Maintain the previous key as well, but set it as deprecated. We will deal with the removal of deprecated keys in a consistent way.

I don't quite get what should we do exactly, These are the options I can think of.

keep the tag key as "sandbox_mode"

Sandboxer string `toml:"sandbox_mode" json:"sandboxMode"`

add another field SandboxMode

// Deprecated SandboxMode string `toml:"sandbox_mode" json:"sandboxMode"` Sandboxer string `toml:"sandboxer" json:"sandboxer"`

add multiple keys for the same field? it seems not supported for toml or json marshal framework

Sandboxer string `toml:"sandboxer" json:"sandboxer" toml:"sandboxe_mode"`

@dmcgowan How do we deal with this config key? is it option 2 above?

dmcgowan · 2023-06-06T16:57:19Z

pkg/cri/sbserver/sandbox_run.go

@@ -212,6 +213,50 @@ func (c *criService) RunPodSandbox(ctx context.Context, r *runtime.RunPodSandbox
 		return nil, fmt.Errorf("unable to save sandbox %q to store: %w", id, err)
 	}

+	// Create sandbox container root directories.


Can you explain a little more why this functionality was moved out of the podsandbox controller back to here? This logic should be implementation specific rather than done by by the CRI server.

It seems the shim sandbox controller also need to implement a same logic to create these files, otherwise the hostname, hosts and DNS configs are lost in containers. I didn't start a container with runc and sandbox_mode of shim successfully, But I think if we can succeed start it, these files should be created.

If we move it into podsandbox controller, I think we should copy these codes into the shim controller too. So I think maybe we should reserve it as a common logic in the CRI server.

Yes I think it is a implementation specific logic because some sandbox like wasm may not need these files, but I think most of the sandbox need these files.

I can remove this commit if it is neccesary, and implement it in our sandbox controller.

It could make sense to break this functionality into a library which different implementations can use. We are trying to minimize the amount of host setup needed when using the sandbox interface. The sandbox implementations should own this rather than it being a common operation for any sandbox.

OK，the commit is removed

mxpv · 2023-06-06T16:52:04Z

metadata/sandbox.go

+	if sandboxer == nil {
+		inst.Sandboxer = ""
+	} else {
+		inst.Sandboxer = string(sandboxer)


Can we update tests for this pls?

mxpv · 2023-06-06T16:59:57Z

pkg/cri/sbserver/podsandbox/controller.go

+// As the dependency from this controller to cri plugin is hard to decouple,
+// we define a global podsandbox controller and register it to containerd plugin manager first,
+// we will initialize this controller when we initialize the cri plugin.
+var controller = &Controller{}


Can we register multiple plugins from the same CRI config instead of global variable? This would require a bit of refactoring in initCRIService.

I have tried to do that but it made a plugin dependency cycle that is hard to untie, the initialization of fields in this Contoller depends on many kinds plugins and makes cyclic dependency too easily. I tried many times in different ways and at last I gave up and get this global variable idea.

mikebrow · 2023-10-12T15:31:12Z

/ok-to-test

As we are going to support more kinds of sandboxers, we have to tell containerd which sandboxer used to manipulate a specific sandbox. Signed-off-by: Abel Feng <fshb1988@gmail.com>

Signed-off-by: Abel Feng <fshb1988@gmail.com>

make containerd extensible to support more sandbox controllers registered into containerd by config. we change the default sandbox controller plugin's name from "local" to "shim". to make sure we can get the controller by the plugin name it registered into containerd. Signed-off-by: Abel Feng <fshb1988@gmail.com>

Signed-off-by: Abel Feng <fshb1988@gmail.com>

When call sandbox controller to create sandbox, we change the param from sandbox id to total sandbox object to git all information to controller, so that sandbox controller do not rely on the sandbox store anymore, this is more decouple for the sandbox controller plugin inside containerd, and it is neccesary for remote sandbox controller plugins as it is not able to get sandbox from the sandbox store anymore. Signed-off-by: Abel Feng <fshb1988@gmail.com>

The ShimManager.Start() will call loadShim() to get the existing shim if SandboxID is specified for a container, but shimTask.PID() is called in loadShim, which will call Connect() of Task API with the ID of a task that is not created yet(containerd is getting the shim and Task API address to call Create, so the task is not created yet). In this commit we change the logic of loadShim() to get the shim without calling Connect() of the not created container ID. Signed-off-by: Abel Feng <fshb1988@gmail.com>

Signed-off-by: Abel Feng <fshb1988@gmail.com>

abel-von · 2023-10-16T14:45:08Z

@mxpv @dmcgowan @fuweid Could you please take a look again and merge this pr if possible? I am waiting for this pr to be merged and then submit the subsequent PRs about the podsandbox controller and other sandbox controller related codes.

dmcgowan · 2023-10-18T22:13:41Z

pkg/cri/server/service.go

+		return nil, fmt.Errorf("failed to get sandbox runtime: %w", err)
+	}
+
+	return c.client.SandboxController(ociRuntime.Sandboxer), nil


This is causing a unit test regression as some of the unit tests do not have the client defined. We should figure out a way to avoid calling the client here.

I'm sorry I don't quite get it, when I run make test, all tests passed, and we can see many places in cri codes to call client, why should we avoid calling client here? I think I can make it by defining a sandboxControllers in criService, but I don't get the reason why we have to do it. Could you please tell me why?

I see, it is the TestListContainerStats that need to get platfrom from sandbox controller.

Maybe I can define a sandboxService interface in cri and define the Platform() method in it, and I can define a fakeSandboxService struct in test codes and init such an object into the criService in newTestCRIService.

Also opened #9268 to fix the test. The direction we are going through is like you have it, reducing use of the client.

I made a fix in #9275, please take a look.

k8s-ci-robot added the needs-ok-to-test label Mar 14, 2023

abel-von force-pushed the sandbox-plugin branch 8 times, most recently from 72fb327 to 78ff094 Compare March 14, 2023 17:14

abel-von closed this Mar 15, 2023

abel-von reopened this Mar 15, 2023

abel-von force-pushed the sandbox-plugin branch 2 times, most recently from ea1bce3 to 9415b46 Compare March 21, 2023 08:16

abel-von closed this Mar 21, 2023

abel-von reopened this Mar 21, 2023

abel-von force-pushed the sandbox-plugin branch 6 times, most recently from 1b635f0 to 44a9799 Compare June 5, 2023 15:02

dmcgowan reviewed Jun 6, 2023

View reviewed changes

mxpv reviewed Jun 6, 2023

View reviewed changes

abel-von force-pushed the sandbox-plugin branch 2 times, most recently from ddc9e8c to ca58cd1 Compare June 7, 2023 09:58

abel-von force-pushed the sandbox-plugin branch from ca58cd1 to 7fa7a29 Compare July 13, 2023 09:08

k8s-ci-robot added ok-to-test and removed needs-ok-to-test labels Oct 12, 2023

abel-von force-pushed the sandbox-plugin branch from 4a713f4 to 04f7756 Compare October 13, 2023 02:09

abel-von added 8 commits October 16, 2023 20:49

sandbox: add sandboxer field of sandbox requests

f372b35

As we are going to support more kinds of sandboxers, we have to tell containerd which sandboxer used to manipulate a specific sandbox. Signed-off-by: Abel Feng <fshb1988@gmail.com>

sandbox: change SandboxMode to Sandboxer

69e501e

Signed-off-by: Abel Feng <fshb1988@gmail.com>

sandbox: add Sandboxer field to sandbox metadata

8b35976

Signed-off-by: Abel Feng <fshb1988@gmail.com>

sandbox: ignore not found error when remove sandbox

0707f68

Signed-off-by: Abel Feng <fshb1988@gmail.com>

sandbox: remove global variable of podsandbox controller

3ef300c

Signed-off-by: Abel Feng <fshb1988@gmail.com>

abel-von force-pushed the sandbox-plugin branch from 04f7756 to 3ef300c Compare October 16, 2023 13:23

mxpv approved these changes Oct 18, 2023

View reviewed changes

dmcgowan approved these changes Oct 18, 2023

View reviewed changes

dmcgowan merged commit 9807675 into containerd:main Oct 18, 2023
41 checks passed

dmcgowan added the impact/changelog label Oct 18, 2023

dmcgowan reviewed Oct 18, 2023

View reviewed changes

abel-von mentioned this pull request Oct 20, 2023

sandbox: podsandbox controller init its own client #9275

Merged

dmcgowan changed the title ~~Sandbox: make sandbox controller plugin~~ Add sandboxer configuration and move sandbox controllers to plugins Nov 1, 2023

dmcgowan mentioned this pull request Nov 2, 2023

Prepare release notes for 2.0.0-beta.0 #9318

Merged

cpuguy83 mentioned this pull request Nov 3, 2023

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

Open

dmcgowan added the area/runtime Runtime label Nov 8, 2023

abel-von mentioned this pull request Nov 28, 2023

Sandbox API work continued #9431

Open

19 tasks

ambarve mentioned this pull request Feb 8, 2024

Containerd doesn't issue shim delete for containers anymore. #9727

Open

dmcgowan mentioned this pull request Apr 19, 2024

Prepare release notes for v2.0.0-rc.1 #10098

Merged

dmcgowan mentioned this pull request May 3, 2024

Prepare release notes for api/v1.8.0-rc.0 #10167

Merged

dmcgowan mentioned this pull request May 17, 2024

Prepare release notes for v2.0.0-rc.2 #10240

Merged

dmcgowan mentioned this pull request Jun 13, 2024

Prepare v2.0.0-rc.3 release #10331

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add sandboxer configuration and move sandbox controllers to plugins #8268

Add sandboxer configuration and move sandbox controllers to plugins #8268

abel-von commented Mar 14, 2023

k8s-ci-robot commented Mar 14, 2023

Burning1020 commented Mar 15, 2023

dmcgowan Jun 6, 2023

abel-von Jun 7, 2023

dmcgowan Jun 6, 2023

abel-von Jun 7, 2023

abel-von Oct 13, 2023

dmcgowan Jun 6, 2023

abel-von Jun 7, 2023 •

edited

Loading

dmcgowan Sep 4, 2023

abel-von Sep 5, 2023

mxpv Jun 6, 2023

abel-von Jun 7, 2023

mxpv Jun 6, 2023

abel-von Jun 7, 2023

mikebrow commented Oct 12, 2023

abel-von commented Oct 16, 2023

dmcgowan Oct 18, 2023

abel-von Oct 19, 2023

abel-von Oct 19, 2023

abel-von Oct 19, 2023

dmcgowan Oct 19, 2023

abel-von Oct 20, 2023

Add sandboxer configuration and move sandbox controllers to plugins #8268

Add sandboxer configuration and move sandbox controllers to plugins #8268

Conversation

abel-von commented Mar 14, 2023

k8s-ci-robot commented Mar 14, 2023

Burning1020 commented Mar 15, 2023

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

abel-von Jun 7, 2023 • edited Loading

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

mikebrow commented Oct 12, 2023

abel-von commented Oct 16, 2023

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

abel-von Jun 7, 2023 •

edited

Loading