kata-agent RemoveContainer timeout buggy?
It looks like the goroutine will be left running when kata-agent RemoveContainer() returns after a timeout. In addition, the sandbox lock is no longer held so any actions taken by the goroutine could cause problems.
Am I understanding the code correctly?
func (a *agentGRPC) RemoveContainer(ctx context.Context, req *pb.RemoveContainerRequest) (*gpb.Empty, error) { ...
timeout := int(req.Timeout)
a.sandbox.Lock() defer a.sandbox.Unlock()
if timeout == 0 { ... } else { done := make(chan error) go func() { if err := ctr.removeContainer(); err != nil { done <- err close(done) return }
//Find the sandbox storage used by this container for _, path := range ctr.mounts { if _, ok := a.sandbox.storages[path]; ok { if err := a.sandbox.unsetAndRemoveSandboxStorage(path); err != nil { done <- err close(done) return } } } close(done) }()
select { case err := <-done: if err != nil { return emptyResp, err } case <-time.After(time.Duration(req.Timeout) * time.Second): return emptyResp, grpcStatus.Errorf(codes.DeadlineExceeded, "Timeout reached after %ds", timeout)
Stefan
On Fri, Nov 9, 2018 at 6:46 PM Stefan Hajnoczi stefanha@redhat.com wrote:
It looks like the goroutine will be left running when kata-agent RemoveContainer() returns after a timeout. In addition, the sandbox lock is no longer held so any actions taken by the goroutine could cause problems.
Am I understanding the code correctly?
Yes, you are right! The main issue with the approach is that `ctr.removeContainer` doesn't honor timeout and we cannot force quit it in the middle.
Any suggestions?
Cheers, Tao
func (a *agentGRPC) RemoveContainer(ctx context.Context, req *pb.RemoveContainerRequest) (*gpb.Empty, error) { ...
timeout := int(req.Timeout) a.sandbox.Lock() defer a.sandbox.Unlock() if timeout == 0 { ... } else { done := make(chan error) go func() { if err := ctr.removeContainer(); err != nil { done <- err close(done) return } //Find the sandbox storage used by this container for _, path := range ctr.mounts { if _, ok := a.sandbox.storages[path]; ok { if err := a.sandbox.unsetAndRemoveSandboxStorage(path); err != nil { done <- err close(done) return } } } close(done) }() select { case err := <-done: if err != nil { return emptyResp, err } case <-time.After(time.Duration(req.Timeout) * time.Second): return emptyResp, grpcStatus.Errorf(codes.DeadlineExceeded, "Timeout reached after %ds", timeout)
Stefan _______________________________________________ kata-dev mailing list kata-dev@lists.katacontainers.io http://lists.katacontainers.io/cgi-bin/mailman/listinfo/kata-dev
On Fri, Nov 09, 2018 at 07:10:02PM +0800, Tao Peng wrote:
On Fri, Nov 9, 2018 at 6:46 PM Stefan Hajnoczi stefanha@redhat.com wrote:
It looks like the goroutine will be left running when kata-agent RemoveContainer() returns after a timeout. In addition, the sandbox lock is no longer held so any actions taken by the goroutine could cause problems.
Am I understanding the code correctly?
Yes, you are right! The main issue with the approach is that `ctr.removeContainer` doesn't honor timeout and we cannot force quit it in the middle.
Any suggestions?
If it's not possible to implement a timeout correctly then maybe the timeout parameter should be deprecated for now?
Stefan
participants (2)
-
Stefan Hajnoczi
-
Tao Peng