sandbox: Move CRI Image Service and CRI Base seperate plugins #9391

abel-von · 2023-11-17T07:56:27Z

This PR cherry-picked the #9119 and split the CRI plugin into plugins below:

the io.containerd.internal.v1.cri plugin is for loading cri configuration and validation
the io.containerd.image.v1.cri-image-service plugin is a image service to create/pull/remove/query images.
the io.containerd.sandbox.controller.v1.podsandbox the a sandbox controller implement that create a pause container to mimic a sandbox, it is a legacy support.
the io.containerd.grpc.v1.cri-grpc-service is the CRI grpc service plugin, to serve grpc API calls of CRI interfaces.

k8s-ci-robot · 2023-11-17T07:56:37Z

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.

abel-von · 2023-11-17T08:24:40Z

PTAL @mxpv @fuweid @dmcgowan

fuweid · 2023-11-17T15:11:06Z

All the integration test cases fail. I think there is regression caused by 09723a6

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

abel-von · 2023-11-20T14:10:37Z

I think it is not caused by 09723a6. It is actually because we changed the cri plugin type from io.containerd.grpc.v1 to io.containerd.internal.v1 so that the RootDir and StateDir in Config changed.
Recently we init the Config in the code below

	c := criconfig.Config{
		PluginConfig:       *pluginConfig,
		ContainerdRootDir:  filepath.Dir(ic.Properties[plugins.PropertyRootDir]),
		ContainerdEndpoint: ic.Properties[plugins.PropertyGRPCAddress],
		RootDir:            ic.Properties[plugins.PropertyRootDir],
		StateDir:           ic.Properties[plugins.PropertyStateDir],
	}

I think we may have to keep the RootDir and StateDir io.containerd.grpc.v1.cri, or we migrate cri root dirs and state dirs to the new path, but we can not simply rename them to the new path because some paths such as io fifos are stored in the db.
@fuweid

abel-von · 2023-11-21T01:29:00Z

I think the CRI integration test passed, one of them failed because an occasional failure of the container registry. could you please rerun it to make sure all CI steps green? @fuweid

fuweid · 2023-11-22T09:13:26Z

pkg/cri/cri.go

-		ID:     "cri",
-		Config: &config,
+		Type: plugins.GRPCPlugin,
+		ID:   "cri-grpc-service",


could you just keep it as cri? It's unlikely to do live-migration because the running containers are using the volume in this plugin folder.

We probably need to add some release upgrade integration cases for this before we accept this.

REF:

containerd/integration/release_upgrade_linux_test.go

Line 52 in 6ed8f01

// Add exec/stats/stop-existing-running-pods/...

could you just keep it as cri? It's unlikely to do live-migration because the running containers are using the volume in this plugin folder.

That is actually why the intergration test failed before. So that I add a commit 9a18e8e to keep the plugin folder path "io.containerd.grpc.v1.cri" as before.

I think it is no use to change the name here because we want to add a plugin of type io.containerd.internal.v1 to carry the cri configs and other plugins will be depend on this plugin to get the cri configs.

We probably need to add some release upgrade integration cases for this before we accept this.

added a test case to manipulate the containers in the pod before upgrade. 0597be4849

The added integration test case reveals an existing compatibility issues, and I fixed in e18e86905b3071, all these are included in this PR, could you please take a look at the new commits ?

or, as Max has approved, shall I put the two new commits in a new PR? @mxpv @fuweid

I think we don't need to rename it cri-grp-service. Just use cri because it's grpc plugin. It won't conflict with internal one.

mikebrow

looking good .. yes, suggest splitting up the test and fix (last two commits) into another PR .. tag that PR for cherry pick..

abel-von · 2023-11-29T02:50:14Z

the integration test codes is splited into the new PR #9434

please take a look @mikebrow @fuweid

fuweid

Basically, it looks good to me

fuweid · 2023-11-30T02:59:35Z

pkg/cri/cri.go

-		ID:     "cri",
-		Config: &config,
+		Type: plugins.GRPCPlugin,
+		ID:   "cri-grpc-service",


I think we don't need to rename it cri-grp-service. Just use cri because it's grpc plugin. It won't conflict with internal one.

fuweid · 2023-11-30T03:15:22Z

pkg/cri/server/base/cri_base.go

+		}
+	}
+
+	// For backward compatibility, we have to keep the rootDir and stateDir the same as before.


I think it's not a long-term solution. We should introduce some toggle to switch to use cri.base root or state dir in the future.

I think it's not a long-term solution. We should introduce some toggle to switch to use cri.base root or state dir in the future.

we may have to record the root and state dir in the sandbox object in cri cache, when recovering from restart, we need to detect if it is "io.containerd.internal.v1.cri" or "io.containerd.grpc.v1.cri", but this is a big change, can we leave it in another PR?

I think we don't need to rename it cri-grp-service. Just use cri because it's grpc plugin. It won't conflict with internal one.

changed back to "cri"

we may have to record the root and state dir in the sandbox object in cri cache, when recovering from restart, we need to detect if it is "io.containerd.internal.v1.cri" or "io.containerd.grpc.v1.cri", but this is a big change, can we leave it in another PR?

Yes. We can handle it in the follow-up. I am not against to introduce fallback logic to handle existing running pods. 😂

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

fuweid

LGTM

k8s-ci-robot added the needs-ok-to-test label Nov 17, 2023

abel-von force-pushed the sandbox-plugin-1117 branch 2 times, most recently from 4c29c26 to 0efc665 Compare November 17, 2023 08:19

abel-von changed the title ~~sandbox: Move CRI Image Service a seperate plugin~~ sandbox: Move CRI Image Service and CRI Base a seperate plugin Nov 17, 2023

abel-von changed the title ~~sandbox: Move CRI Image Service and CRI Base a seperate plugin~~ sandbox: Move CRI Image Service and CRI Base seperate plugins Nov 17, 2023

Move CRI image service into a separate plugin

e15c246

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

abel-von force-pushed the sandbox-plugin-1117 branch from 0efc665 to ebb1655 Compare November 20, 2023 07:32

fuweid reviewed Nov 22, 2023

View reviewed changes

mxpv approved these changes Nov 26, 2023

View reviewed changes

abel-von force-pushed the sandbox-plugin-1117 branch 2 times, most recently from 75820ea to 0597be4 Compare November 28, 2023 08:35

abel-von mentioned this pull request Nov 28, 2023

Sandbox API work continued #9431

Open

19 tasks

mikebrow reviewed Nov 28, 2023

View reviewed changes

abel-von force-pushed the sandbox-plugin-1117 branch from e18e869 to 9a18e8e Compare November 29, 2023 02:19

k8s-ci-robot added the size/XL label Nov 29, 2023

abel-von requested review from fuweid and mikebrow November 30, 2023 01:10

fuweid reviewed Nov 30, 2023

View reviewed changes

abel-von added 3 commits November 30, 2023 23:06

sandbox: make a clear dependency of cri plugins

c8012b6

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

sandbox: migrate sandbox_mode to sandboxer

b0fef67

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

sandbox: keep rootDir and stateDir compatible

e1b4958

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

abel-von force-pushed the sandbox-plugin-1117 branch from 9a18e8e to e1b4958 Compare November 30, 2023 15:09

fuweid approved these changes Dec 1, 2023

View reviewed changes

fuweid removed the needs-ok-to-test label Dec 1, 2023

fuweid added the ok-to-test label Dec 1, 2023

mxpv added this pull request to the merge queue Dec 1, 2023

Merged via the queue into containerd:main with commit 47163c3 Dec 1, 2023

sandbox: Move CRI Image Service and CRI Base seperate plugins #9391

sandbox: Move CRI Image Service and CRI Base seperate plugins #9391

Uh oh!

Conversation

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

Uh oh!

Uh oh!

k8s-ci-robot commented Nov 17, 2023

Uh oh!

abel-von commented Nov 17, 2023

Uh oh!

fuweid commented Nov 17, 2023

Uh oh!

abel-von commented Nov 20, 2023

Uh oh!

abel-von commented Nov 21, 2023

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mikebrow left a comment

Choose a reason for hiding this comment

Uh oh!

abel-von commented Nov 29, 2023

Uh oh!

fuweid left a comment

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fuweid left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

abel-von commented Nov 17, 2023 •

edited

Loading