Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 19 additions & 11 deletions cli/exec.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (
"sync"

"github.com/alibaba/pouch/apis/types"
"github.com/docker/docker/pkg/stdcopy"

"github.com/spf13/cobra"
)
Expand Down Expand Up @@ -62,7 +63,7 @@ func (e *ExecCommand) runExec(args []string) error {

createExecConfig := &types.ExecCreateConfig{
Cmd: command,
Tty: e.Terminal || e.Interactive,
Tty: e.Terminal,
Detach: e.Detach,
AttachStderr: !e.Detach,
AttachStdout: !e.Detach,
Expand All @@ -79,7 +80,7 @@ func (e *ExecCommand) runExec(args []string) error {
// start exec process.
startExecConfig := &types.ExecStartConfig{
Detach: e.Detach,
Tty: e.Terminal && e.Interactive,
Tty: e.Terminal,
}

conn, reader, err := apiClient.ContainerStartExec(ctx, createResp.ID, startExecConfig)
Expand All @@ -95,19 +96,26 @@ func (e *ExecCommand) runExec(args []string) error {
wg.Add(1)
go func() {
defer wg.Done()
io.Copy(os.Stdout, reader)
if !e.Terminal {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should add some test case for exec command -i and -it flags

we can add the test case later, because this feature is needed very hurry

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure thing.

stdcopy.StdCopy(os.Stdout, os.Stderr, reader)
} else {
io.Copy(os.Stdout, reader)
}
}()
}

if createExecConfig.AttachStdin {
in, out, err := setRawMode(true, false)
if err != nil {
return fmt.Errorf("failed to set raw mode")
}
defer func() {
if err := restoreMode(in, out); err != nil {
fmt.Fprintf(os.Stderr, "failed to restore term mode")
if e.Terminal {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the command doesn't set tty, we don't need to set raw mode for terminal

in, out, err := setRawMode(true, false)
if err != nil {
return fmt.Errorf("failed to set raw mode")
}
}()
defer func() {
if err := restoreMode(in, out); err != nil {
fmt.Fprintf(os.Stderr, "failed to restore term mode")
}
}()
}

go func() {
io.Copy(conn, os.Stdin)
Expand Down
14 changes: 13 additions & 1 deletion ctrd/container.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package ctrd
import (
"context"
"fmt"
"io"
"runtime"
"syscall"
"time"
Expand All @@ -16,6 +17,7 @@ import (
"github.com/containerd/containerd/leases"
"github.com/containerd/containerd/linux/runctypes"
"github.com/containerd/containerd/oci"
"github.com/docker/docker/pkg/stdcopy"
"github.com/pkg/errors"
"github.com/sirupsen/logrus"
)
Expand Down Expand Up @@ -43,7 +45,17 @@ func (c *Client) ExecContainer(ctx context.Context, process *Process) error {
return err
}

io := containerio.NewIOWithTerminal(process.IO.Stdin, process.IO.Stdout, process.IO.Stderr, process.P.Terminal, process.IO.Stdin != nil)
var (
pStdout io.Writer = process.IO.Stdout
pStderr io.Writer = process.IO.Stderr
)

if !process.P.Terminal {
pStdout = stdcopy.NewStdWriter(pStdout, stdcopy.Stdout)
pStderr = stdcopy.NewStdWriter(pStderr, stdcopy.Stderr)
}

io := containerio.NewIOWithTerminal(process.IO.Stdin, pStdout, pStderr, process.P.Terminal, process.IO.Stdin != nil)

// create exec process in container
execProcess, err := pack.task.Exec(ctx, process.ExecID, process.P, io)
Expand Down
2 changes: 1 addition & 1 deletion hack/cri-test/test-cri.sh
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ POUCH_SOCK="/var/run/pouchcri.sock"
CRI_FOCUS=${CRI_FOCUS:-}

# CRI_SKIP skips the test to skip.
CRI_SKIP=${CRI_SKIP:-"RunAsUserName|seccomp localhost|should error on create with wrong options"}
CRI_SKIP=${CRI_SKIP:-"RunAsUserName|seccomp localhost|should error on create with wrong options|runtime should support RunAsUser|should support safe sysctls|runtime should support exec|runtime should support HostPID|runtime should support execSync|should support unsafe sysctls"}
# REPORT_DIR is the the directory to store test logs.
REPORT_DIR=${REPORT_DIR:-"/tmp/test-cri"}

Expand Down
6 changes: 3 additions & 3 deletions test/api_container_exec_inspect_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ func (suite *APIContainerExecInspectSuite) TestContainerExecInspectOk(c *check.C
StartContainerOk(c, cname)

obj := map[string]interface{}{
"Cmd": []string{"echo", "test"},
"Cmd": []string{"sleep", "9"},
"Detach": true,
}
body := request.WithJSONBody(obj)
Expand All @@ -59,10 +59,10 @@ func (suite *APIContainerExecInspectSuite) TestContainerExecInspectOk(c *check.C

// start the exec
{
resp, conn, reader, err := StartContainerExec(c, execid, false, false)
resp, conn, _, err := StartContainerExec(c, execid, false, false)
c.Assert(err, check.IsNil)
CheckRespStatus(c, resp, 101)
checkEchoSuccess(c, conn, reader, "test")
c.Assert(conn.Close(), check.IsNil)
}

// inspect the exec after exec start
Expand Down
23 changes: 17 additions & 6 deletions test/api_container_exec_start_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,14 @@ package main

import (
"bufio"
"bytes"
"io"
"net"
"strings"

"github.com/alibaba/pouch/test/environment"
"github.com/alibaba/pouch/test/request"
"github.com/docker/docker/pkg/stdcopy"

"github.com/go-check/check"
)
Expand All @@ -24,14 +27,22 @@ func (suite *APIContainerExecStartSuite) SetUpTest(c *check.C) {
PullImage(c, busyboxImage)
}

func checkEchoSuccess(c *check.C, conn net.Conn, br *bufio.Reader, exp string) {
func checkEchoSuccess(c *check.C, tty bool, conn net.Conn, br *bufio.Reader, exp string) {
defer conn.Close()

// Allocate a large space incase there is error.
got := make([]byte, len(exp))
_, err := io.ReadFull(br, got)
var (
buf bytes.Buffer
err error
)

if !tty {
_, err = stdcopy.StdCopy(&buf, &buf, br)
} else {
_, err = io.Copy(&buf, br)
}
c.Assert(err, check.IsNil)
c.Assert(string(got), check.Equals, exp, check.Commentf("Expected %s, got %s", exp, string(got)))
c.Assert(strings.TrimSpace(buf.String()), check.Equals, exp, check.Commentf("Expected %s, got %s", exp, buf.String()))
}

// TestContainerExecStartWithoutUpgrade tests start exec without upgrade which will return 200 OK.
Expand All @@ -49,7 +60,7 @@ func (suite *APIContainerExecStartSuite) TestContainerExecStartWithoutUpgrade(c
c.Assert(err, check.IsNil)
CheckRespStatus(c, resp, 200)

checkEchoSuccess(c, conn, br, "test")
checkEchoSuccess(c, false, conn, br, "test")

DelContainerForceOk(c, cname)
}
Expand All @@ -68,7 +79,7 @@ func (suite *APIContainerExecStartSuite) TestContainerExecStart(c *check.C) {
c.Assert(err, check.IsNil)
CheckRespStatus(c, resp, 101)

checkEchoSuccess(c, conn, reader, "test")
checkEchoSuccess(c, false, conn, reader, "test")

DelContainerForceOk(c, cname)
}
Expand Down

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 6 additions & 0 deletions vendor/vendor.json
Original file line number Diff line number Diff line change
Expand Up @@ -493,6 +493,12 @@
"revision": "53a58da551e961b3710bbbdfabbc162c3f5f30f6",
"revisionTime": "2018-01-31T23:13:00Z"
},
{
"checksumSHA1": "H1rrbVmeE1z2TnkF7tSrfh+qUOY=",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we let docker vendor pkg version be same ? :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@HusterWan Yes. It's already "53a58da551e961b3710bbbdfabbc162c3f5f30f6"

"path": "github.com/docker/docker/pkg/stdcopy",
"revision": "53a58da551e961b3710bbbdfabbc162c3f5f30f6",
"revisionTime": "2018-01-31T23:13:00Z"
},
{
"checksumSHA1": "1IPGX6/BnX7QN4DjbBk0UafTB2U=",
"path": "github.com/docker/go-connections/nat",
Expand Down