From 14fac8c0f0a8b81a20ffaf6655abf6864fa98092 Mon Sep 17 00:00:00 2001
From: zhangyue <zy675793960@yeah.net>
Date: Thu, 11 Jul 2019 14:47:42 +0800
Subject: [PATCH] feature: add exec timeout support

Signed-off-by: zhangyue <zy675793960@yeah.net>
---
 cri/v1alpha2/cri.go                        | 19 ++++---
 ctrd/container.go                          | 64 ++++++++++++++--------
 hack/testing/run_daemon_cri_integration.sh |  1 -
 3 files changed, 50 insertions(+), 34 deletions(-)

diff --git a/cri/v1alpha2/cri.go b/cri/v1alpha2/cri.go
index 71e20f17af..051a0567b1 100644
--- a/cri/v1alpha2/cri.go
+++ b/cri/v1alpha2/cri.go
@@ -1146,15 +1146,6 @@ func (c *CriManager) ReopenContainerLog(ctx context.Context, r *runtime.ReopenCo
 func (c *CriManager) ExecSync(ctx context.Context, r *runtime.ExecSyncRequest) (*runtime.ExecSyncResponse, error) {
 	id := r.GetContainerId()
 
-	timeout := time.Duration(r.GetTimeout()) * time.Second
-	var cancel context.CancelFunc
-	if timeout == 0 {
-		ctx, cancel = context.WithCancel(ctx)
-	} else {
-		ctx, cancel = context.WithTimeout(ctx, timeout)
-	}
-	defer cancel()
-
 	createConfig := &apitypes.ExecCreateConfig{
 		Cmd: r.GetCmd(),
 	}
@@ -1170,6 +1161,16 @@ func (c *CriManager) ExecSync(ctx context.Context, r *runtime.ExecSyncRequest) (
 		UseStderr: true,
 		Stderr:    stderrBuf,
 	}
+
+	timeout := time.Duration(r.GetTimeout()) * time.Second
+	var cancel context.CancelFunc
+	if timeout == 0 {
+		ctx, cancel = context.WithCancel(ctx)
+	} else {
+		ctx, cancel = context.WithTimeout(ctx, timeout)
+	}
+	defer cancel()
+
 	if err := c.ContainerMgr.StartExec(ctx, execid, attachCfg); err != nil {
 		return nil, fmt.Errorf("failed to start exec for container %q: %v", id, err)
 	}
diff --git a/ctrd/container.go b/ctrd/container.go
index 8b5285c40a..2c972bb16f 100644
--- a/ctrd/container.go
+++ b/ctrd/container.go
@@ -140,6 +140,12 @@ func (c *Client) execContainer(ctx context.Context, process *Process) error {
 	errCh := make(chan error, 1)
 	defer close(errCh)
 
+	// start the exec process
+	if err := execProcess.Start(ctx); err != nil {
+		errCh <- err
+		return errors.Wrapf(err, "failed to start exec, exec id %s", execID)
+	}
+
 	go func() {
 		var msg *Message
 
@@ -149,38 +155,48 @@ func (c *Client) execContainer(ctx context.Context, process *Process) error {
 				exitCode: 126,
 				exitTime: time.Now().UTC(),
 			}
-		}
-
-		// success to start which means the cmd is valid and wait
-		// for process.
-		if msg == nil {
-			status := <-exitStatus
-			msg = &Message{
-				err:      status.Error(),
-				exitCode: status.ExitCode(),
-				exitTime: status.ExitTime(),
+		} else {
+			select {
+			case status := <-exitStatus:
+				msg = &Message{
+					err:      status.Error(),
+					exitCode: status.ExitCode(),
+					exitTime: status.ExitTime(),
+				}
+			case <-ctx.Done():
+				// Ignore the not found error because the process may exit itself before kill
+				if err := execProcess.Kill(ctx, syscall.SIGKILL); err != nil && !errdefs.IsNotFound(err) {
+					//try to force kill the exec process
+					if err := execProcess.Kill(ctx, syscall.SIGTERM); err != nil && !errdefs.IsNotFound(err) {
+						logrus.Errorf("failed to kill the exec process, %v", err)
+					}
+				}
+				// Wait for process to be killed
+				status := <-exitStatus
+				msg = &Message{
+					err:      errors.Wrapf(status.Error(), "failed to exec process %s, timeout", execID),
+					exitCode: status.ExitCode(),
+					exitTime: status.ExitTime(),
+				}
 			}
 		}
 
-		// XXX: if exec process get run, io should be closed in this function,
-		for _, hook := range c.hooks {
-			if err := hook(process.ExecID, msg); err != nil {
-				logrus.Errorf("failed to execute the exec exit hooks: %v", err)
-				break
+		if msg != nil {
+			// XXX: if exec process get run, io should be closed in this function,
+			for _, hook := range c.hooks {
+				if err := hook(process.ExecID, msg); err != nil {
+					logrus.Errorf("failed to execute the exec exit hooks: %v", err)
+					break
+				}
 			}
-		}
 
-		// delete the finished exec process in containerd
-		if _, err := execProcess.Delete(context.TODO()); err != nil {
-			logrus.Warnf("failed to delete exec process %s: %s", process.ExecID, err)
+			// delete the finished exec process in containerd
+			if _, err := execProcess.Delete(context.TODO()); err != nil {
+				logrus.Warnf("failed to delete exec process %s: %s", process.ExecID, err)
+			}
 		}
 	}()
 
-	// start the exec process
-	if err := execProcess.Start(ctx); err != nil {
-		errCh <- err
-		return err
-	}
 	return nil
 }
 
diff --git a/hack/testing/run_daemon_cri_integration.sh b/hack/testing/run_daemon_cri_integration.sh
index 5afcc9e016..02596c78b7 100755
--- a/hack/testing/run_daemon_cri_integration.sh
+++ b/hack/testing/run_daemon_cri_integration.sh
@@ -19,7 +19,6 @@ export PATH="${GOPATH}/bin:${PATH}"
 
 # CRI_SKIP skips the test to skip.
 DEFAULT_CRI_SKIP="should error on create with wrong options"
-DEFAULT_CRI_SKIP+="|runtime should support execSync with timeout"
 CRI_SKIP="${CRI_SKIP:-"${DEFAULT_CRI_SKIP}"}"
 
 # CRI_FOCUS focuses the test to run.