Skip to content

Sandbox API: implement Controller.Wait and Controller.Stop#7401

Merged
estesp merged 2 commits intocontainerd:mainfrom
wllenyj:sandbox_stop
Sep 22, 2022
Merged

Sandbox API: implement Controller.Wait and Controller.Stop#7401
estesp merged 2 commits intocontainerd:mainfrom
wllenyj:sandbox_stop

Conversation

@wllenyj
Copy link
Copy Markdown
Contributor

@wllenyj wllenyj commented Sep 19, 2022 •
edited
Loading

This pr is Sansbox API work in #7312. Implemented Controller.Wait and Controller.Stop.

TODO list:

  • don't use CRI.sandboxStore in Controller
  • The Controller maintains its own Status instead of CRI's sandboxStore.
  • implement TaskExit event backoff in Controller.Stop

Signed-off-by: wanglei01 [email protected]

@k8s-ci-robot
Copy link
Copy Markdown

k8s-ci-robot commented Sep 19, 2022

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

@wllenyj wllenyj force-pushed the sandbox_stop branch 2 times, most recently from b1eb0cf to f8b57dc Compare September 19, 2022 08:09
@wllenyj wllenyj changed the title WIP: Controller.Wait and Controller.Stop Sandbox API: Controller.Wait and Controller.Stop Sep 20, 2022
@wllenyj wllenyj changed the title Sandbox API: Controller.Wait and Controller.Stop Sandbox API: implement Controller.Wait Sep 20, 2022
@mxpv
Copy link
Copy Markdown
Member

mxpv commented Sep 21, 2022

This looks good to me. I think it's fine to follow up on TODOs in separate PRs (if possible) to keep PR sizes sane. Just need to get CI tests green.

@samuelkarp
Copy link
Copy Markdown
Member

samuelkarp commented Sep 21, 2022

Would you mind rebasing on main? We just merged a fix (in #7413) to CI that had caused the sbserver CRI implementation to not be tested properly.

@mxpv mxpv added this to the 1.7 milestone Sep 21, 2022
@wllenyj wllenyj marked this pull request as ready for review September 21, 2022 14:33
@wllenyj wllenyj changed the title Sandbox API: implement Controller.Wait Sandbox API: implement Controller.Wait and Controller.Stop Sep 21, 2022
@wllenyj
Copy link
Copy Markdown
Contributor Author

wllenyj commented Sep 21, 2022

Thanks. @mxpv @samuelkarp
I've rebase it.

@estesp
Copy link
Copy Markdown
Member

estesp commented Sep 21, 2022

looks like all the sandbox runs have actual failures that need investigation

@wllenyj
Copy link
Copy Markdown
Contributor Author

wllenyj commented Sep 21, 2022 •
edited
Loading

looks like all the sandbox runs have actual failures that need investigation

Thanks, I will check it right away

@wllenyj
Copy link
Copy Markdown
Contributor Author

wllenyj commented Sep 21, 2022

@estesp Thanks for your reminder.
A wrong return type is used. It has been fixed.

@wllenyj wllenyj force-pushed the sandbox_stop branch 2 times, most recently from f8d29eb to 8e82872 Compare September 21, 2022 17:46
@wllenyj
CRI: implement Controller.Wait for SandboxAPI
9279069 
Rework sandbox monitoring, we should rely on Controller.Wait instead of
CRIService.StartSandboxExitMonitor

Signed-off-by: WangLei <[email protected]>
@wllenyj
CRI: implement Controller.Stop for SandboxAPI
82890dd 
Signed-off-by: WangLei <[email protected]>
estesp
estesp approved these changes Sep 22, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@estesp estesp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@estesp estesp merged commit 8f95bac into containerd:main Sep 22, 2022
@mikebrow mikebrow mentioned this pull request Jan 11, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mxpv mxpv mxpv approved these changes

@estesp estesp estesp approved these changes

Assignees

No one assigned

Labels

needs-ok-to-test

Projects

None yet

Milestone

1.7

Development

Successfully merging this pull request may close these issues.

6 participants

@wllenyj @k8s-ci-robot @mxpv @samuelkarp @estesp @mradyd918