Sandbox API: Add a new mode config for sandbox controller impls by Burning1020 · Pull Request #7590 · containerd/containerd

Burning1020 · 2022-10-27T13:34:33Z

Works for #7312:

Add a new config as sandbox controller mod, which can be either
"podsandbox" or "shim". If empty, set it to default "podsandbox"
when CRI plugin inits.

Signed-off-by: Zhang Tianyang [email protected]

mxpv · 2022-10-27T19:04:39Z

metadata/sandbox_test.go

 		},
-		Runtime: api.RuntimeOpts{Name: "test"},
+		Runtime:    api.RuntimeOpts{Name: "test"},
+		Controller: "remote",


Sandboxes are implemented by shims (side by side with task service), so we can reuse Runtime field to find right shim for tasks/sandboxing. There is no need in dedicated Controller field.

Reasonable. Thanks!

pkg/cri/sbserver/sandbox_run.go

Burning1020 · 2022-11-01T15:19:01Z

CC @mxpv I've pushed again. Use sandbox_mode="shim|podsandbox" to determinate which controller.

mxpv · 2022-11-02T00:24:27Z

pkg/cri/config/config.go

 	// while using default snapshotters for operational simplicity.
 	// See https://github.com/containerd/containerd/issues/6657 for details.
 	Snapshotter string `toml:"snapshotter" json:"snapshotter"`
+	// SandboxMode setting sandbox controller mode of sandbox at runtime level like Snapshotter


Suggested change

// SandboxMode setting sandbox controller mode of sandbox at runtime level like Snapshotter

// SandboxMode defines which sandbox runtime to use when scheduling pods

// This features requires experimental CRI server to be enabled (use ENABLE_CRI_SANDBOXES=1)

Burning1020 · 2022-11-02T02:19:46Z

CC @fuweid @dmcgowan

fuweid · 2022-11-02T15:04:20Z

sandbox/controller.go

 	"github.com/containerd/containerd/api/services/sandbox/v1"
 )

+const DefaultSandboxController = "podsandbox"


If the mode is type, I think we should use const type and add validation for this.

type Mode string const ( ModeDefault Mode = "default" ModeRemote = "remote" ) func ValidateMode() error { ... }

Something like that.

I want make "SandboxMode" similar to "Snapshotter", they're all string type. If we change it to Mode type, there wolud be some conversion. Is it necessary?

fuweid · 2022-11-02T15:05:39Z

pkg/cri/sbserver/sandbox_run.go

+	if err != nil {
+		return nil, fmt.Errorf("failed to get sandbox runtime: %w", err)
+	}
+	// Absent case


we can verify the Runtime.Config when setup CRI plugin. And update it to default mode if it is empty.

k8s-ci-robot · 2022-11-04T12:57:53Z

@Burning1020: The /test command needs one or more targets.
The following commands are available to trigger required jobs:

/test pull-containerd-build
/test pull-containerd-node-e2e
/test pull-containerd-sandboxed-node-e2e

Use /test all to run the following jobs that were automatically triggered:

pull-containerd-build
pull-containerd-node-e2e

Details

In response to this:

/test

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.

mxpv · 2022-11-08T02:40:53Z

sandbox/controller.go

+type Mode string
+
+const (
+	// ModePodSandbox means use Controller implementation from sbserver podsandbox package.


I feel like this is CRI specific and should be maintained in cri package.

fuweid · 2022-11-09T04:08:56Z

pkg/cri/config/config.go

 	"github.com/containerd/containerd/plugin"
 )

+type Mode string


What about SandboxMode?

updated to SandboxControllerMode

Add a new config as sandbox controller mod, which can be either "podsandbox" or "shim". If empty, set it to default "podsandbox" when CRI plugin inits. Signed-off-by: Zhang Tianyang <[email protected]>

fuweid

LGTM on green

mxpv · 2022-11-09T23:09:50Z

/test pull-containerd-sandboxed-node-e2e

Burning1020 · 2022-11-10T02:06:48Z

/retest

Burning1020 force-pushed the sc-switch branch from a15d40b to 764932c Compare October 27, 2022 14:01

mxpv reviewed Oct 27, 2022

View reviewed changes

Burning1020 force-pushed the sc-switch branch 2 times, most recently from eb14f1d to c8b6ea5 Compare November 1, 2022 07:18

Burning1020 closed this Nov 1, 2022

Burning1020 reopened this Nov 1, 2022

mxpv approved these changes Nov 2, 2022

View reviewed changes

mxpv added this to the 1.7 milestone Nov 2, 2022

fuweid reviewed Nov 2, 2022

View reviewed changes

Burning1020 force-pushed the sc-switch branch 2 times, most recently from 09d0dcf to 8ca5390 Compare November 3, 2022 13:21

Burning1020 changed the title ~~Add a switch between sandbox controller impls~~ Sandbox API: Add a new mode config for sandbox controller impls Nov 4, 2022

Burning1020 force-pushed the sc-switch branch 3 times, most recently from 6727116 to 5ccbc87 Compare November 4, 2022 07:27

Burning1020 closed this Nov 4, 2022

Burning1020 reopened this Nov 4, 2022

Burning1020 closed this Nov 4, 2022

Burning1020 reopened this Nov 4, 2022

Burning1020 requested a review from fuweid November 7, 2022 09:17

mxpv reviewed Nov 8, 2022

View reviewed changes

Burning1020 force-pushed the sc-switch branch 2 times, most recently from 572a591 to 233399c Compare November 8, 2022 13:07

Burning1020 requested review from mxpv and removed request for fuweid November 9, 2022 03:49

fuweid reviewed Nov 9, 2022

View reviewed changes

Sandbox API: Add a new mode config for sandbox controller impls

c953eec

Add a new config as sandbox controller mod, which can be either "podsandbox" or "shim". If empty, set it to default "podsandbox" when CRI plugin inits. Signed-off-by: Zhang Tianyang <[email protected]>

Burning1020 force-pushed the sc-switch branch from 233399c to c953eec Compare November 9, 2022 04:13

fuweid approved these changes Nov 9, 2022

View reviewed changes

mxpv approved these changes Nov 9, 2022

View reviewed changes

mxpv merged commit 7eeaed8 into containerd:main Nov 10, 2022

mxpv mentioned this pull request Nov 10, 2022

Sandbox API work #7312

Closed

17 tasks

Burning1020 deleted the sc-switch branch November 11, 2022 01:23

pacoxu mentioned this pull request Apr 10, 2023

feature: runc supports shim sandbox mode #8369

Closed

	// SandboxMode setting sandbox controller mode of sandbox at runtime level like Snapshotter
	// SandboxMode defines which sandbox runtime to use when scheduling pods
	// This features requires experimental CRI server to be enabled (use ENABLE_CRI_SANDBOXES=1)

Conversation

Burning1020 commented Oct 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Burning1020 commented Nov 1, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Burning1020 commented Nov 2, 2022

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!

k8s-ci-robot commented Nov 4, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Burning1020 Nov 8, 2022 • 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!

fuweid left a comment

Choose a reason for hiding this comment

Uh oh!

mxpv commented Nov 9, 2022

Uh oh!

Burning1020 commented Nov 10, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Burning1020 commented Oct 27, 2022 •

edited

Loading

Burning1020 Nov 8, 2022 •

edited

Loading