Sandbox API: implement Controller.Status for SandboxAPI by lengrongfu · Pull Request #7470 · containerd/containerd

lengrongfu · 2022-10-04T02:17:07Z

This pr is Sansbox API work in #7312. Implemented Controller.Status

k8s-ci-robot · 2022-10-04T02:17:16Z

Hi @lengrongfu. 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.

mxpv · 2022-10-04T16:20:26Z

/test pull-containerd-sandboxed-node-e2e

pkg/cri/sbserver/podsandbox/sandbox_stop.go

pkg/cri/sbserver/podsandbox/controller.go

samuelkarp · 2022-10-07T01:11:35Z

/ok-to-test

samuelkarp · 2022-10-07T01:12:24Z

/test pull-containerd-sandboxed-node-e2e

pkg/cri/sbserver/podsandbox/controller.go

fuweid · 2022-10-12T14:27:22Z

pkg/cri/sbserver/podsandbox/controller.go


 // handleSandboxExit handles TaskExit event for sandbox.
-func handleSandboxExit(ctx context.Context, sb sandboxstore.Sandbox) error {
+func handleSandboxExit(ctx context.Context, e *eventtypes.TaskExit, sb sandboxstore.Sandbox) error {


suggest to use handleSandboxExit(context.Context, sandboxstore.Sandbox, TaskExit)

and since the TaskEvent is designed for shim.Task, do you need to extend the TaskExit with sandboxID here?
cc @mxpv @mikebrow

Good question, in order to keep the responsibility single, TaskEvent should not be used here in theory, but TaskEvent is currently used elsewhere, e.g.a

in startSandboxExitMonitor()

containerd/pkg/cri/server/events.go

Line 122 in 2716289

e := &eventtypes.TaskExit{

in RunPodSandbox()

containerd/pkg/cri/sbserver/sandbox_run.go

Line 215 in 2716289

e := &eventtypes.TaskExit{

About this problem, I think it can be solved in two steps:

In the short term, TaskEvent is still used, consistent with other places

In the long run, we can add a SandboxEvent designed for sandbox, and then make a unified replacement.

@fuweid @lengrongfu would it be ok to leave a TODO for this and get this PR in?

Add TODO issue #7548

mxpv · 2022-11-08T02:46:23Z

Can you rebase against main branch? It should include a fix for the Vagrant CI job.

Signed-off-by: rongfu.leng <[email protected]>

mxpv · 2022-11-15T21:09:05Z

/test pull-containerd-sandboxed-node-e2e

mxpv · 2022-11-15T21:36:48Z

@fuweid LGTY?

mikebrow

see comment..

mikebrow · 2022-11-18T19:56:29Z

pkg/cri/sbserver/podsandbox/controller.go

 			sb, err := c.sandboxStore.Get(id)
 			if err == nil {
-				if err := handleSandboxExit(dctx, sb); err != nil {
+				if err := handleSandboxExit(dctx, sb, &eventtypes.TaskExit{ExitStatus: exitStatus, ExitedAt: protobuf.ToTimestamp(exitedAt)}); err != nil {


nod to @fuweid 's suggestion to add new api params to the end vs insert new/additional fields in between.. I see you already did that!

Additionally it feels(reads) wrong to create a partial task exit event for a sandbox then only use the new fields.. and discard the task exit event.. maybe just pass in the new fields as params? ExitedAt time.Time and ExitStatus uint32

fuweid

Lgtm

Let's handle the suggestion in followups

k8s-ci-robot added the needs-ok-to-test label Oct 4, 2022

mxpv approved these changes Oct 4, 2022

View reviewed changes

mxpv added this to the 1.7 milestone Oct 4, 2022

fuweid reviewed Oct 5, 2022

View reviewed changes

pkg/cri/sbserver/podsandbox/sandbox_stop.go Outdated Show resolved Hide resolved

fuweid reviewed Oct 5, 2022

View reviewed changes

pkg/cri/sbserver/podsandbox/sandbox_stop.go Show resolved Hide resolved

lengrongfu force-pushed the feat/sandbox_api_status branch from d34a46c to deafdab Compare October 6, 2022 02:00

fuweid reviewed Oct 6, 2022

View reviewed changes

pkg/cri/sbserver/podsandbox/sandbox_stop.go Outdated Show resolved Hide resolved

samuelkarp reviewed Oct 7, 2022

View reviewed changes

pkg/cri/sbserver/podsandbox/controller.go Outdated Show resolved Hide resolved

samuelkarp added the area/cri Container Runtime Interface (CRI) label Oct 7, 2022

k8s-ci-robot added ok-to-test and removed needs-ok-to-test labels Oct 7, 2022

lengrongfu force-pushed the feat/sandbox_api_status branch 2 times, most recently from 3a735a0 to fb2f985 Compare October 8, 2022 01:56

mxpv mentioned this pull request Oct 10, 2022

Sandbox API work #7312

Closed

17 tasks

mxpv requested a review from fuweid October 10, 2022 23:00

sparr reviewed Oct 12, 2022

View reviewed changes

pkg/cri/sbserver/podsandbox/controller.go Show resolved Hide resolved

fuweid reviewed Oct 12, 2022

View reviewed changes

lengrongfu mentioned this pull request Oct 18, 2022

Resolve in the sandbox use TaskEvent #7548

Closed

lengrongfu force-pushed the feat/sandbox_api_status branch from fb2f985 to 4c04181 Compare October 18, 2022 06:58

lengrongfu force-pushed the feat/sandbox_api_status branch from 9d804c0 to d6a5f63 Compare November 9, 2022 05:02

feat add sandbox api status func

0f54c47

Signed-off-by: rongfu.leng <[email protected]>

lengrongfu force-pushed the feat/sandbox_api_status branch from d6a5f63 to 0f54c47 Compare November 9, 2022 06:36

fuweid self-requested a review November 16, 2022 02:02

mxpv linked an issue Nov 17, 2022 that may be closed by this pull request

Sandbox API work #7312

Closed

17 tasks

mxpv removed a link to an issue Nov 17, 2022

Sandbox API work #7312

Closed

17 tasks

mikebrow reviewed Nov 18, 2022

View reviewed changes

fuweid approved these changes Nov 23, 2022

View reviewed changes

mxpv merged commit 6d830d3 into containerd:main Nov 23, 2022

Conversation

lengrongfu commented Oct 4, 2022

Uh oh!

k8s-ci-robot commented Oct 4, 2022

Uh oh!

mxpv commented Oct 4, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

samuelkarp commented Oct 7, 2022

Uh oh!

samuelkarp commented Oct 7, 2022

Uh oh!

Uh oh!

fuweid Oct 12, 2022

Choose a reason for hiding this comment

Uh oh!

lengrongfu Oct 13, 2022

Choose a reason for hiding this comment

Uh oh!

mxpv Oct 18, 2022

Choose a reason for hiding this comment

Uh oh!

lengrongfu Oct 18, 2022

Choose a reason for hiding this comment

Uh oh!

mxpv commented Nov 8, 2022

Uh oh!

mxpv commented Nov 15, 2022

Uh oh!

mxpv commented Nov 15, 2022

Uh oh!

mikebrow left a comment

Choose a reason for hiding this comment

Uh oh!

mikebrow Nov 18, 2022

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!

7 participants