Use channel to pass the stop info instead of polling for container stop#570
Use channel to pass the stop info instead of polling for container stop#570Random-Liu merged 1 commit intocontainerd:masterfrom
Conversation
|
/assign @Random-Liu |
miaoyq
force-pushed
the
fixes-569
branch
3 times, most recently
from
c31b3f2 to
11b1ddb
Compare
|
We need to handle cri-containerd restart. We should change |
| } | ||
|
|
||
| // Stop close stopCh of the container. | ||
| func (c *Container) Stop() { |
There was a problem hiding this comment.
nit: Instead of adding a function to Container, can we make a type of stopCh and add function to it instead?
In this way, it can be reused in Sandbox after my change is in.
| case <-ticker.C: | ||
| continue | ||
| // Poll once before waiting for stopCheckPollInterval. | ||
| container, err := c.containerStore.Get(id) |
There was a problem hiding this comment.
Can we pass container containerstore.Container to the waitContainerStop function to avoid this?
There was a problem hiding this comment.
Yean, it's better, will do.
| return | ||
| } | ||
|
|
||
| cntr.Stop() |
There was a problem hiding this comment.
It should be better to do this after cntr.Status.UpdateSync.
We can ensure that the whole container is over for the user who watch the channel.
There was a problem hiding this comment.
We should do this also with "case *eventtypes.TaskOOM"
There was a problem hiding this comment.
We shouldn't do for TaskOOM. :)
There was a problem hiding this comment.
It should be better to do this after cntr.Status.UpdateSync.
We can ensure that the whole container is over for the user who watch the channel.
Agree.
There was a problem hiding this comment.
Have changed it yesterday, :-)
| @@ -138,33 +134,25 @@ func (c *criContainerdService) stopContainer(ctx context.Context, container cont | |||
|
|
|||
| // waitContainerStop polls container state until timeout exceeds or container is stopped. | |||
miaoyq
force-pushed
the
fixes-569
branch
2 times, most recently
from
bee9025 to
e1e84d7
Compare
|
@Random-Liu @yanxuean Have addressed all comments, PTAL. |
|
Ooh, missed the test cases, will fix it. |
Fixed. |
| return c.Status.Delete() | ||
| } | ||
|
|
||
| // StopCh is used to pass the stop information of a container. |
There was a problem hiding this comment.
s/pass/propagate/g
And please also change the other places.
|
|
||
| cio "github.com/containerd/cri-containerd/pkg/server/io" | ||
| "github.com/containerd/cri-containerd/pkg/store" | ||
| "k8s.io/kubernetes/pkg/kubelet/apis/cri/v1alpha1/runtime" |
There was a problem hiding this comment.
Move this to the group above.
|
LGTM with 2 nits. |
Signed-off-by: Yanqiang Miao <[email protected]>
|
/lgtm |
5 participants
Fixes #569
/cc @Random-Liu @yanxuean
Signed-off-by: Yanqiang Miao [email protected]