Sandbox API: implement Controller.Status for SandboxAPI #7470
Conversation
|
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 Once the patch is verified, the new status will be reflected by the 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. |
|
/test pull-containerd-sandboxed-node-e2e |
d34a46c to
deafdab
Compare
|
/ok-to-test |
|
/test pull-containerd-sandboxed-node-e2e |
3a735a0 to
fb2f985
Compare
| @@ -149,7 +165,7 @@ func (c *Controller) waitSandboxExit(ctx context.Context, id string, exitCh <-ch | |||
| } | |||
|
|
|||
| // 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 { | |||
There was a problem hiding this comment.
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
-
in
RunPodSandbox()containerd/pkg/cri/sbserver/sandbox_run.go
Line 215 in 2716289
About this problem, I think it can be solved in two steps:
- In the short term,
TaskEventis still used, consistent with other places - In the long run, we can add a
SandboxEventdesigned forsandbox, and then make a unified replacement.
There was a problem hiding this comment.
@fuweid @lengrongfu would it be ok to leave a TODO for this and get this PR in?
There was a problem hiding this comment.
Add TODO issue #7548
fb2f985 to
4c04181
Compare
|
Can you rebase against |
9d804c0 to
d6a5f63
Compare
Signed-off-by: rongfu.leng <rongfu.leng@daocloud.io>
d6a5f63 to
0f54c47
Compare
|
/test pull-containerd-sandboxed-node-e2e |
|
@fuweid LGTY? |
| @@ -123,7 +139,7 @@ func (c *Controller) waitSandboxExit(ctx context.Context, id string, exitCh <-ch | |||
|
|
|||
| 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 { | |||
There was a problem hiding this comment.
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
This pr is Sansbox API work in #7312. Implemented
Controller.Status