Some cleanup after switching to new client. #137
Conversation
| defer cancel() | ||
|
|
||
| resCh := make(chan execResult, 1) | ||
| go func() { |
There was a problem hiding this comment.
Will this go routine be stuck if the process.Start(ctx) errors out ? Because there will be no listener on the channel.
There was a problem hiding this comment.
actually it won't since its buffered channel
There was a problem hiding this comment.
If Start fails, we'll go to defer to delete the container, and then Wait will return. :)
However, this is not that obvious... I think we should make it more clear, let me see.
There was a problem hiding this comment.
You are correct, I even found a containerd bug when trying to address this containerd/containerd#1376.
|
LGTM |
|
LGTM (FWIW) |
Random-Liu
force-pushed
the
cleanup-with-new-client
branch
2 times, most recently
from
bbf796a
to
9a4aaea
Compare
|
@abhinandanpb Addressed comments. PTAL. |
|
The new commit also added the |
| @@ -120,6 +127,8 @@ func (c *criContainerdService) execInContainer(ctx context.Context, id string, o | |||
| return nil, fmt.Errorf("failed to create exec %q: %v", execID, err) | |||
| } | |||
| defer func() { | |||
| // TODO(random-liu): There is a containerd bug here containerd#1376, revisit this | |||
| // after that is fixed. | |||
| if _, err := process.Delete(ctx); err != nil { | |||
There was a problem hiding this comment.
wondering if we should just defer on process.Delete(ctx, containerd.WithProcessKill) ?
There was a problem hiding this comment.
As is mentioned in the containerd issue containerd/containerd#1376, it doesn't work.
| exitCode, err := c.waitContainerExec(eventstream, id, execID) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to wait for exec in container %q to finish: %v", id, err) | ||
| var timeoutCh <-chan time.Time |
There was a problem hiding this comment.
And then use context.WithDeadline if the timeout is set and just return
select{
<-ctx.Done() : return err
....
<-resCh : ....
}
There was a problem hiding this comment.
Just a thought. This works great too. WDYT ?
There was a problem hiding this comment.
I've tried that. The problem is that once context is cancelled, we could not use it to call Delete or Kill. Of course, we could use context.Background() for those operations, but I'm not sure whether we should do that.
That is also why I filed #142.
|
LGTM |
|
@abhinandanpb Thanks for reviewing! :) |
Signed-off-by: Lantao Liu <lantaol@google.com>
Random-Liu
force-pushed
the
cleanup-with-new-client
branch
from
9a4aaea
to
ed640d3
Compare
|
Squashed commits. Will merge after test passes. |
…ient Some cleanup after switching to new client.
Some cleanup after switching to new containerd client.
Signed-off-by: Lantao Liu lantaol@google.com