test: add missing integration test for PouchContainer

apis/server/exec_bridge.go

@@ -9,6 +9,7 @@ import (
 	"strconv"
 
 	"github.com/alibaba/pouch/apis/types"
+	"github.com/alibaba/pouch/pkg/errtypes"
 	"github.com/alibaba/pouch/pkg/httputils"
 	"github.com/alibaba/pouch/pkg/streams"
 
@@ -75,6 +76,7 @@ func (s *Server) startContainerExec(ctx context.Context, rw http.ResponseWriter,
 	)
 
 	// TODO(huamin.thm): support detach exec process through http post method
+	// Do we need to merge the input config.Detach and ContainerExecConfig.ExecCreateConfig.Detach?
 	if !config.Detach {
 		stdin, stdout, closeFn, err = openHijackConnection(rw)
 		if err != nil {
@@ -102,6 +104,12 @@ func (s *Server) startContainerExec(ctx context.Context, rw http.ResponseWriter,
 	}
 
 	if err := s.ContainerMgr.StartExec(ctx, name, attach); err != nil {
+		if errtypes.IsConflict(err) {
+			return err
+		}
+		if errtypes.IsNotfound(err) {
+			return err
+		}
 		if config.Detach {
 			return err
 		}

apis/server/router.go

@@ -242,7 +242,7 @@ func HandleErrorResponse(w http.ResponseWriter, err error) {
 		code = http.StatusNotFound
 	} else if errtypes.IsInvalidParam(err) {
 		code = http.StatusBadRequest
-	} else if errtypes.IsAlreadyExisted(err) {
+	} else if errtypes.IsAlreadyExisted(err) || errtypes.IsConflict(err) {
 		code = http.StatusConflict
 	} else if errtypes.IsNotModified(err) {
 		code = http.StatusNotModified

apis/swagger.yml

@@ -3269,14 +3269,14 @@ definitions:
     properties:
       User:
         type: "string"
-        description: "User that will run the command"
+        description: "User with whom the exec command will run inside container."
         x-nullable: false
       Privileged:
         type: "boolean"
-        description: "Is the container in privileged mode"
+        description: "Config whether to run the exec command in container with all privilleges."
       Tty:
         type: "boolean"
-        description: "Attach standard streams to a tty"
+        description: "Config the created exec whether to allocate a pseudo-TTY."
       AttachStdin:
         type: "boolean"
         description: "Attach the standard input, makes possible user interaction"
@@ -3288,7 +3288,11 @@ definitions:
         description: "Attach the standard output"
       Detach:
         type: "boolean"
-        description: "Execute in detach mode"
+        description: |
+          Config the exec process inside the container detached from the client.
+          For example, if set to be true, the CLI command line will be detached from
+          the exec processes's stdin/stdout/stderr stream, and command will terminated
+          at once while the exec process will be running background in container.
       DetachKeys:
         type: "string"
         description: "Escape keys for detach"
@@ -3333,10 +3337,14 @@ definitions:
     description: ExecStartConfig is a temp struct used by execStart.
     properties:
       Detach:
-        description: ExecStart will first check if it's detached
+        description: |
+          Config the started exec process inside the container detached from the client.
+          For example, if set to be true, the CLI command line will be detached from
+          the exec processes's stdin/stdout/stderr stream, and command will terminated
+          at once while the exec process will be running background in container.
         type: "boolean"
       Tty:
-        description: Check if there's a tty
+        description: Config the exec start whether to allocate a pseudo-TTY.
         type: "boolean"
     example:
       Detach: false

daemon/mgr/container.go

@@ -1678,7 +1678,7 @@ func (mgr *ContainerManager) attachCRILog(c *Container, logPath string) error {
 
 func (mgr *ContainerManager) initExecIO(id string, withStdin bool) (*containerio.IO, error) {
 	if io := mgr.IOs.Get(id); io != nil {
-		return nil, errors.Wrap(errtypes.ErrConflict, "failed to create containerIO")
+		return nil, errors.Wrap(errtypes.ErrConflict, "failed to create execIO")
 	}
 
 	cntrio := containerio.NewIO(id, withStdin)

daemon/mgr/container_exec.go

@@ -70,6 +70,10 @@ func (mgr *ContainerManager) StartExec(ctx context.Context, execid string, cfg *
 		return err
 	}
 
+	if !c.IsRunning() {
+		return errors.Wrap(errtypes.ErrConflict, "cannot start an exec in not running containers")
+	}
+
 	// set exec process user, user decided by exec config
 	if execConfig.User == "" {
 		execConfig.User = c.Config.User
@@ -162,7 +166,7 @@ func (mgr *ContainerManager) StartExec(ctx context.Context, execid string, cfg *
 		IO:          eio,
 		P:           process,
 	}); err != nil {
-		return err
+		return errors.Wrapf(err, "failed to exec process in container %s", execConfig.ContainerID)
 	}
 	return <-attachErrCh
 }

pkg/errtypes/errors.go

@@ -102,6 +102,11 @@ func IsPreCheckFailed(err error) bool {
 	return checkError(err, codePreCheckFailed)
 }
 
+// IsConflict checks if the error is conflict.
+func IsConflict(err error) bool {
+	return checkError(err, codeConflict)
+}
+
 func checkError(err error, code int) bool {
 	err = causeError(err)
 

test/api_container_create_test.go

@@ -215,13 +215,29 @@ func testCreateContainerWithBadParam(c *check.C, cname string, obj map[string]in
 }
 
 // TestCreateWithBadStopTimeout using bad stopTimeout to create container.
-func (suite *APIContainerCreateSuite) TestCreateWithBadStopTimeout(c *check.C) {
+func (suite *APIContainerCreateSuite) testCreateWithBadParams(c *check.C) {
 	testCreateContainerWithBadParam(c,
 		"TestCreateWithBadStopTimeout",
 		map[string]interface{}{
 			"Image":       busyboxImage,
 			"StopTimeout": -1,
 		})
+
+	// too long length
+	testCreateContainerWithBadParam(c,
+		"1234567890-1234567890-1234567890-1234567890123456789",
+		map[string]interface{}{
+			"Image": busyboxImage,
+		})
+
+	// invalid characters
+	testCreateContainerWithBadParam(c,
+		"??????><!@#$%^&*",
+		map[string]interface{}{
+			"Image": busyboxImage,
+		})
+
+	// TODO: add more container creation option check
 }
 
 func (suite *APIContainerCreateSuite) TestCreateNvidiaConfig(c *check.C) {

test/api_container_exec_inspect_test.go

@@ -1,9 +1,12 @@
 package main
 
 import (
-	"time"
+	"fmt"
+	"os/exec"
+	"strconv"
+	"strings"
+	"syscall"
 
-	"github.com/alibaba/pouch/apis/types"
 	"github.com/alibaba/pouch/test/environment"
 	"github.com/alibaba/pouch/test/request"
 
@@ -26,70 +29,60 @@ func (suite *APIContainerExecInspectSuite) SetUpTest(c *check.C) {
 
 // TestContainerCreateExecOk tests execing containers is OK.
 func (suite *APIContainerExecInspectSuite) TestContainerExecInspectOk(c *check.C) {
-	c.Skip("skip flaky test due to issue#1372")
 	cname := "TestContainerExecInspectOk"
 
 	CreateBusyboxContainerOk(c, cname)
+	defer DelContainerForceMultyTime(c, cname)
 
 	StartContainerOk(c, cname)
 
-	obj := map[string]interface{}{
-		"Cmd":    []string{"sleep", "9"},
-		"Detach": true,
-	}
-	body := request.WithJSONBody(obj)
-	resp, err := request.Post("/containers/"+cname+"/exec", body)
-	c.Assert(err, check.IsNil)
-	CheckRespStatus(c, resp, 201)
-
-	var execCreateResp types.ExecCreateResp
-	err = request.DecodeBody(&execCreateResp, resp.Body)
-	c.Assert(err, check.IsNil)
-
-	execid := execCreateResp.ID
-
-	// inspect the exec before exec start
-	resp, err = request.Get("/exec/" + execid + "/json")
-	c.Assert(err, check.IsNil)
-	CheckRespStatus(c, resp, 200)
-
-	var execInspect01 types.ContainerExecInspect
-	request.DecodeBody(&execInspect01, resp.Body)
+	// create an exec and get the execID
+	// and inspect the exec before exec start
 
-	c.Assert(execInspect01.Running, check.Equals, false)
-	c.Assert(execInspect01.ExitCode, check.Equals, int64(0))
+	execid := CreateExecCmdOk(c, cname, "sleep", "12345678")
+	{
+		execInspectResp := InspectExecOk(c, execid)
+		c.Assert(execInspectResp.Running, check.Equals, false)
+		c.Assert(execInspectResp.ExitCode, check.Equals, int64(0))
+	}
 
-	// start the exec
+	// set the detach to be true
+	// and start the exec
 	{
-		resp, conn, _, err := StartContainerExec(c, execid, false, false)
+		obj := map[string]interface{}{
+			"Detach": true,
+		}
+		body := request.WithJSONBody(obj)
+		resp, err := request.Post(fmt.Sprintf("/exec/%s/start", execid), body)
 		c.Assert(err, check.IsNil)
-		CheckRespStatus(c, resp, 101)
-		c.Assert(conn.Close(), check.IsNil)
+		CheckRespStatus(c, resp, 200)
 	}
 
 	// inspect the exec after exec start
-	resp, err = request.Get("/exec/" + execid + "/json")
-	c.Assert(err, check.IsNil)
-	CheckRespStatus(c, resp, 200)
-
-	var execInspect02 types.ContainerExecInspect
-	request.DecodeBody(&execInspect02, resp.Body)
-
-	c.Assert(execInspect02.Running, check.Equals, true)
-	c.Assert(execInspect02.ExitCode, check.Equals, int64(0))
-
-	// sleep 10s to wait the process exit
-	time.Sleep(10 * time.Second)
+	{
+		execInspectResp := InspectExecOk(c, execid)
+		c.Assert(execInspectResp.Running, check.Equals, true)
+		c.Assert(execInspectResp.ExitCode, check.Equals, int64(0))
+	}
 
-	resp, err = request.Get("/exec/" + execid + "/json")
-	c.Assert(err, check.IsNil)
-	CheckRespStatus(c, resp, 200)
+	// find the exec command and terminate it.
+	{
+		cmd := exec.Command("bash", "-c", "ps aux | grep 'sleep 12345678' | awk '{print $2}'")
+		output, err := cmd.Output()
+		c.Assert(err, check.IsNil)
 
-	var execInspect03 types.ContainerExecInspect
-	request.DecodeBody(&execInspect03, resp.Body)
+		outputStr := strings.TrimSpace(string(output))
+		pids := strings.SplitN(outputStr, "\n", -1)
+		c.Assert(len(pids), check.Equals, 1)
 
-	c.Assert(execInspect03.Running, check.Equals, false)
-	c.Assert(execInspect03.ExitCode, check.Equals, int64(0))
+		// kill the exec process by sending terminal signal
+		pid, err := strconv.Atoi(pids[0])
+		c.Assert(err, check.IsNil)
+		err = syscall.Kill(pid, syscall.SIGTERM)
+		c.Assert(err, check.IsNil)
 
-	DelContainerForceMultyTime(c, cname)
+		execInspectResp := InspectExecOk(c, execid)
+		c.Assert(execInspectResp.Running, check.Equals, false)
+		c.Assert(execInspectResp.ExitCode, check.Equals, int64(0))
+	}
 }