sandbox: podsandbox controller init its own client by abel-von · Pull Request #9275 · containerd/containerd

abel-von · 2023-10-19T14:48:26Z

To break the cyclic dependency of cri plugin and podsandbox plugin, we define a new plugin type of SandboxesServicePlugin and when cri init it's own client, it will add the all the controllers by get them from the SandboxesServicePlugin.
when podsandbox controller init it's own client, it do not includes the SandboxesServicePlugin as in memory service.

k8s-ci-robot · 2023-10-19T14:48:36Z

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.

Details

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.

dmcgowan · 2023-10-19T21:34:14Z

services/sandbox/controller_service.go

 		ID:   "sandbox-controllers",
 		Requires: []plugin.Type{
-			plugins.ServicePlugin,
+			plugins.SandboxesServicePlugin,


This one could just depend on SandboxControllerPlugin directly. The reason we had the services indirection before was because we either had services which did not have an underlying non-grpc interface or they were wrapped in the metadata store. It seems like the sandbox controller plugins can just be used directly and doesn't need the service plugin layer at all.

abel-von · 2023-10-20T08:44:38Z

@dmcgowan I add a sandboxService into cri to fix the unit test regression issue in #8268, please take a look.

fuweid · 2023-10-20T09:03:47Z

services.go

 	}
 }
+
+func WithSandboxers(ic *plugin.InitContext) ClientOpt {


Maybe the name WithSandboxControllers makes more sense.

changed to WithInMemorySandboxControllers

pkg/cri/server/service_test.go

fuweid · 2023-10-20T09:19:45Z

pkg/cri/config/config.go

 	"github.com/containerd/log"
+	runtime "k8s.io/cri-api/pkg/apis/runtime/v1"
+
+	"github.com/containerd/containerd/pkg/cri/annotations"


We don't have goimport lint yet. But I think you should group github.com/containerd/* packages together.

And it's weird to put the cri-api deps in the config. What about holding the helper GetSandboxRuntime in the pkg/cri/utls?

We don't have goimport lint yet. But I think you should group github.com/containerd/* packages together.

It seems the rule of goimports organize imports in 3 groups, 1. packages in runtime, like "io", "fmt", 2. package of other projects, 3. packages in the same project. the "github.com/containerd/log" seems to be "package in other project", because it is an independent github project.

And it's weird to put the cri-api deps in the config. What about holding the helper GetSandboxRuntime in the pkg/cri/utls?

I tried but it will make a cyclic dependency if we define GetSandboxRuntime in github.com/containerd/containerd/pkg/cri/util

fuweid · 2023-10-20T09:31:33Z

pkg/cri/server/sandbox_service.go

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


REF: https://github.com/containerd/containerd/pull/8268/files#r1219996770

I think you should add the SandboxMode back and mark it as deprecated and then update RELEASE.md.
We should delete the SandboxMode in v2.1

Or could this be a migration now

I think it is a good idea. @dmcgowan I have defined a ConfigMigration in cri plugin in my local codes, I can add a migration from SandboxMode to Sandboxer in that commit, and submit it later. Is it ok? @fuweid

abel-von · 2023-10-25T02:19:01Z

@dmcgowan @fuweid please take a look again, sorry to bother you because I have many subsequent PRs to submit, and I am willing to merge them before 2.0.

fuweid

LGTM

abel-von · 2023-11-14T01:28:47Z

@dmcgowan could you please a look again?

To break the cyclic dependency of cri plugin and podsandbox plugin, we define a new plugin type of SandboxesServicePlugin and when cri init it's own client, it will add the all the controllers by get them from the SandboxesServicePlugin. when podsandbox controller init it's client, it will not Require the SandboxesServicePlugin. Signed-off-by: Abel Feng <[email protected]>

Signed-off-by: Abel Feng <[email protected]>

so that we can add a fakeSandboxService to the criService in tests. Signed-off-by: Abel Feng <[email protected]>

k8s-ci-robot added the needs-ok-to-test label Oct 19, 2023

dmcgowan reviewed Oct 19, 2023

View reviewed changes

abel-von force-pushed the sandbox-plugin-1019 branch from 062353a to b2879c9 Compare October 20, 2023 03:44

abel-von mentioned this pull request Oct 20, 2023

Add sandboxer configuration and move sandbox controllers to plugins #8268

Merged

abel-von force-pushed the sandbox-plugin-1019 branch 3 times, most recently from 6a71662 to e74f97e Compare October 20, 2023 08:42

abel-von force-pushed the sandbox-plugin-1019 branch from e74f97e to 29b393c Compare October 20, 2023 08:50

fuweid reviewed Oct 20, 2023

View reviewed changes

abel-von force-pushed the sandbox-plugin-1019 branch 3 times, most recently from 3cd0b26 to 80e7687 Compare October 23, 2023 08:28

abel-von force-pushed the sandbox-plugin-1019 branch from 80e7687 to d8acc17 Compare November 6, 2023 03:10

fuweid approved these changes Nov 9, 2023

View reviewed changes

abel-von added 3 commits November 15, 2023 09:22

sandbox: remove SandboxersServicePlugin

25a4c3d

Signed-off-by: Abel Feng <[email protected]>

sandbox: add a sandboxService interface to criService

32bf805

so that we can add a fakeSandboxService to the criService in tests. Signed-off-by: Abel Feng <[email protected]>

abel-von force-pushed the sandbox-plugin-1019 branch from d8acc17 to 32bf805 Compare November 15, 2023 01:32

dmcgowan approved these changes Nov 15, 2023

View reviewed changes

dmcgowan added this pull request to the merge queue Nov 15, 2023

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 15, 2023

fuweid added this pull request to the merge queue Nov 16, 2023

Merged via the queue into containerd:main with commit 09723a6 Nov 16, 2023

abel-von mentioned this pull request Nov 28, 2023

Sandbox API work continued #9431

Open

19 tasks

Conversation

abel-von commented Oct 19, 2023

Uh oh!

k8s-ci-robot commented Oct 19, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

abel-von commented Oct 20, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

abel-von Oct 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

abel-von Oct 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

abel-von commented Oct 25, 2023

Uh oh!

fuweid left a comment

Choose a reason for hiding this comment

Uh oh!

abel-von commented Nov 14, 2023

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

abel-von Oct 20, 2023 •

edited

Loading

abel-von Oct 20, 2023 •

edited

Loading