This repository has been archived by the owner on May 12, 2021. It is now read-only.
cli: Don't wait for OCI delete to stop the sandbox #232
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@chavafg could you please try this patch with both |
jodh-intel
reviewed
Apr 19, 2018
cli/delete.go
Outdated
| if _, err := vci.StopSandbox(sandboxID); err != nil { | ||
| return err | ||
| func deleteSandbox(sandboxID, containerID string, forceStop bool) error { | ||
| if forceStop { |
There was a problem hiding this comment.
This seems like a fairly fundamental change so I was expecting to see some new/modified tests for this...?
There was a problem hiding this comment.
Yes but I need to make sure this is the right thing to do and it fixes all issues before I spend some time on those tests.
There was a problem hiding this comment.
@sboeuf - in which case, I wonder if @grahamwhaley's soak test could help us in this regard?
There was a problem hiding this comment.
I have in mind a more global PR. I am currently working on #213 (the unit tests are a nightmare...), but I hope I can update this one tomorrow.
There was a problem hiding this comment.
For the soak test, we should look at kata-containers/tests#195 or maybe preferably kata-containers/tests#215 to add that ability/check to kata.
|
Hi @sboeuf |
|
I see 2 kata-runtime errors when running Container ID: Pod ID: Full log: Again after this, |
|
Thanks @chavafg , I'll have to investigate a bit more on this one! |
|
@chavafg @egernst FWIW, I could properly run a But something went wrong with Resulting in error when running |
sboeuf
changed the title
sboeuf
force-pushed
the
fix_openshift_k8s
branch
from
3dd3747
to
82a0d12
Compare
|
Ready to be reviewed. |
sboeuf
force-pushed
the
fix_openshift_k8s
branch
from
82a0d12
to
26ba724
Compare
bergwolf
reviewed
Apr 24, 2018
cli/delete.go
Outdated
| // reason because that's the behavior of "--force". Also, a | ||
| // previous call to kill might have been performed earlier, | ||
| // causing this one to fail. | ||
| if err := vci.KillContainer(sandboxID, containerID, syscall.SIGKILL, true); err != nil { |
There was a problem hiding this comment.
deleteContainer(forceStop==true) will also send SIGKILL to the container. Is there any reason that we need to do it here before virtcontainers does it?
There was a problem hiding this comment.
well, deleteSandbox() also sends it. I do not see why we need it here.
There was a problem hiding this comment.
Where are we killing the containers ?
There was a problem hiding this comment.
Oh nevermind, I forgot this was handled through virtcontainers...
sboeuf
force-pushed
the
fix_openshift_k8s
branch
3 times, most recently
from
202cd2d
to
ff6c8c6
Compare
|
@WeiZhang555 @laijs @bergwolf PTAL |
bergwolf
reviewed
Apr 24, 2018
| } | ||
|
|
||
| switch containerType { | ||
| case vc.PodSandbox: |
There was a problem hiding this comment.
@sboeuf when do crio/cri-containerd/docker-shim etc. set the podsandbox annotation? It is set for the first container created in a sandbox?
There was a problem hiding this comment.
It is set for the container representing the sandbox, and yes it is usually the first one. In case of CRIO, they create what they call an infra container as being the sandbox, and then they spawn a bunch of other containers later.
bergwolf
approved these changes
Apr 25, 2018
jodh-intel
reviewed
Apr 25, 2018
| case vc.PodContainer: | ||
| _, err = vci.StopContainer(sandboxID, containerID) | ||
| default: | ||
| return fmt.Errorf("Invalid container type found") |
There was a problem hiding this comment.
Can you add containerID to this error?
|
I think the changes make, but I see them as two separate changesets, hence better as two separate commits. Also, I am finding the description of the PR (what has this got to do with "wait"-ing?) and the commit description confusing. |
|
@jodh-intel I can split them into 2 commits but this means the first one won't be working without the second, which should be fine since we only care about the status of the whole PR. |
added 2 commits
April 25, 2018 09:01
The way a delete works, it was always trying to stop the sandbox, even when the force flag was not enabled. Because we want to be able to stop the sandbox from a kill command, this means a sandbox stop might be called twice, and we don't want the second stop to fail, leading to the failure of the delete command. That's why this commit checks for the sandbox status before to try stopping the sandbox. Fixes kata-containers#246 Signed-off-by: Sebastien Boeuf <[email protected]>
The same way a caller of "kata-runtime kill 12345" expects the container 12345 to be killed, the same call to a container representing a sandbox should actually kill the sandbox, meaning it would be stopped after the container has been killed. This way, the caller knows the VM is stopped after kill returns. This is an issue raised by Openshift and Kubernetes tests. They call into delete way after the call to kill has been submitted, and in the meantime they kill all processes related to the container, meaning they do kill the VM before we could do it ourselves. In this case, the delete responsible of stopping the VM comes too late and it returns an error when trying to destroy the sandbox while trying to communicate with the agent since the VM is not here anymore. This commit addresses this issue by letting "kill" call into StopSandbox() if the command relates to a sandbox instead of a simple container. Fixes kata-containers#246 Signed-off-by: Sebastien Boeuf <[email protected]>
sboeuf
force-pushed
the
fix_openshift_k8s
branch
from
ff6c8c6
to
07af4ed
Compare
Codecov Report
@@ Coverage Diff @@
## master #232 +/- ##
=========================================
+ Coverage 65.15% 65.26% +0.1%
=========================================
Files 74 74
Lines 7863 7876 +13
=========================================
+ Hits 5123 5140 +17
+ Misses 2186 2182 -4
Partials 554 554
Continue to review full report at Codecov.
|
|
@jodh-intel I have just split into 2 different commits, providing a separate description for |
|
Thanks @sboeuf! I appreciate that that's a lot of text to describe a relatively small code change, but I do think it makes the behaviour and rationale for the change much clearer. lgtm |
|
@sboeuf - I'll let you remove the dnm label when you're ready. |
mcastelino
pushed a commit
to mcastelino/tests
that referenced
this pull request
Jan 23, 2019
This change adds openshift installation into the CI process and executes a simple test that verifies connectivity between an openshift pod and the host. Fixes: clearcontainers#220. Depends-on: github.com/kata-containers/runtime#232 Signed-off-by: Salvador Fuentes <[email protected]>
zklei
pushed a commit
to zklei/runtime
that referenced
this pull request
Jun 13, 2019
Once all vCPUs have been connected, cpuset cgroup MUST BE updated, to achieve that, each cpuset cgroup parent of the container MUST BE updated with the actual range of vCPUs. fixes kata-containers#239 fixes kata-containers#232 Signed-off-by: Julio Montes <[email protected]>
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This commit tries to address uses cases where the caller of the
runtime expects a kill to stop the container representing the pod,
meaning in our case the end of the VM.
This leads to some rework of the delete command since we want a
delete --force to always work, even if the container/pod has been
killed before.
Fixes #197
Signed-off-by: Sebastien Boeuf [email protected]