Some cleanup after switching to new client.

Signed-off-by: Lantao Liu <lantaol@google.com>

pkg/server/container_execsync.go

@@ -23,10 +23,10 @@ import (
 	"time"
 
 	"github.com/containerd/containerd"
-	"github.com/containerd/containerd/api/services/events/v1"
-	"github.com/containerd/containerd/typeurl"
+	"github.com/containerd/containerd/errdefs"
 	"github.com/golang/glog"
 	"golang.org/x/net/context"
+	"golang.org/x/sys/unix"
 	"k8s.io/client-go/tools/remotecommand"
 	"k8s.io/kubernetes/pkg/kubelet/apis/cri/v1alpha1/runtime"
 )
@@ -72,10 +72,21 @@ type execOptions struct {
 	timeout time.Duration
 }
 
+// execResult is the result returned by exec.
+type execResult struct {
+	exitCode uint32
+	err      error
+}
+
 // execInContainer executes a command inside the container synchronously, and
 // redirects stdio stream properly.
-// TODO(random-liu): Support timeout.
 func (c *criContainerdService) execInContainer(ctx context.Context, id string, opts execOptions) (*uint32, error) {
+	// Cancel the context before returning to ensure goroutines are stopped.
+	// This is important, because if `Start` returns error, `Wait` will hang
+	// forever unless we cancel the context.
+	ctx, cancel := context.WithCancel(ctx)
+	defer cancel()
+
 	// Get container from our container store.
 	cntr, err := c.containerStore.Get(id)
 	if err != nil {
@@ -88,11 +99,7 @@ func (c *criContainerdService) execInContainer(ctx context.Context, id string, o
 		return nil, fmt.Errorf("container is in %s state", criContainerStateToString(state))
 	}
 
-	// TODO(random-liu): Store container client in container store.
-	container, err := c.client.LoadContainer(ctx, id)
-	if err != nil {
-		return nil, fmt.Errorf("failed to load container: %v", err)
-	}
+	container := cntr.Container
 	spec, err := container.Spec()
 	if err != nil {
 		return nil, fmt.Errorf("failed to get container spec: %v", err)
@@ -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 {
 			glog.Errorf("Failed to delete exec process %q for container %q: %v", execID, id, err)
 		}
@@ -131,50 +140,40 @@ func (c *criContainerdService) execInContainer(ctx context.Context, id string, o
 		}
 	})
 
-	// Get containerd event client first, so that we won't miss any events.
-	// TODO(random-liu): Add filter to only subscribe events of the exec process.
-	// TODO(random-liu): Use `Wait` after is fixed. (containerd#1279, containerd#1287)
-	cancellable, cancel := context.WithCancel(ctx)
-	eventstream, err := c.eventService.Subscribe(cancellable, &events.SubscribeRequest{})
-	if err != nil {
-		return nil, fmt.Errorf("failed to subscribe event stream: %v", err)
-	}
-	defer cancel()
-
+	resCh := make(chan execResult, 1)
+	go func() {
+		// Wait will return if context is cancelled.
+		exitCode, err := process.Wait(ctx)
+		resCh <- execResult{
+			exitCode: exitCode,
+			err:      err,
+		}
+		glog.V(2).Infof("Exec process %q exits with exit code %d and error %v", execID, exitCode, err)
+	}()
 	if err := process.Start(ctx); err != nil {
 		return nil, fmt.Errorf("failed to start exec %q: %v", execID, err)
 	}
 
-	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
+	if opts.timeout == 0 {
+		// Do not set timeout if it's 0.
+		timeoutCh = make(chan time.Time)
+	} else {
+		timeoutCh = time.After(opts.timeout)
 	}
-
-	// Wait for the io to be drained.
-	process.IO().Wait()
-
-	return exitCode, nil
-}
-
-// waitContainerExec waits for container exec to finish and returns the exit code.
-func (c *criContainerdService) waitContainerExec(eventstream events.Events_SubscribeClient, id string,
-	execID string) (*uint32, error) {
-	for {
-		evt, err := eventstream.Recv()
-		if err != nil {
-			return nil, err
+	select {
+	case <-timeoutCh:
+		// Ignore the not found error because the process may exit itself before killing.
+		if err := process.Kill(ctx, unix.SIGKILL); err != nil && !errdefs.IsNotFound(err) {
+			return nil, fmt.Errorf("failed to kill exec %q: %v", execID, err)
 		}
-		// Continue until the event received is of type task exit.
-		if !typeurl.Is(evt.Event, &events.TaskExit{}) {
-			continue
-		}
-		any, err := typeurl.UnmarshalAny(evt.Event)
-		if err != nil {
-			return nil, err
-		}
-		e := any.(*events.TaskExit)
-		if e.ContainerID == id && e.ID == execID {
-			return &e.ExitStatus, nil
+		// Wait for the process to be killed.
+		<-resCh
+		return nil, fmt.Errorf("timeout %v exceeded", opts.timeout)
+	case res := <-resCh:
+		if res.err != nil {
+			return nil, fmt.Errorf("failed to wait for exec %q: %v", execID, res.err)
 		}
+		return &res.exitCode, nil
 	}
 }