bugfix: fix exec stuck when exec get error #1605

Ace-Tang · 2018-06-28T04:58:16Z

when daemon deal with exec process get error, process will get stuck,
since http connection has been hijacked, it won't finish and return
with error message. Fix it by write error message into hijacked
connection, close io when process get error.

fixes: #1151

Signed-off-by: Ace-Tang aceapril@126.com

Ⅰ. Describe what this PR did

Ⅱ. Does this pull request fix one issue?

fixes: #1151

Ⅲ. Describe how you did it

Ⅳ. Describe how to verify it

Ⅴ. Special notes for reviews

Ace-Tang · 2018-06-28T05:57:21Z

Hi, @Letty5411 , I want you to help look at a test

func (suite *APIContainerExecStartSuite) TestContainerExecStartNotFound(c *check.C) {
    obj := map[string]interface{}{
        "Detach": false,
        "Tty":    false,
    }   
    body := request.WithJSONBody(obj)
    resp, err := request.Post("/exec/TestContainerExecStartNotFound/start", body)
    c.Assert(err, check.IsNil)
    CheckRespStatus(c, resp, 404)
}

As for fix exec hang, I ignore the error https://github.com/alibaba/pouch/pull/1605/files#diff-b7f6767e7c90c31b2aec9ef134e49e54L75, since error message can not be return by a hijacked connect. Does the test fails because I ignore the error?

do not worry about this, I have fix this.

codecov-io · 2018-06-28T09:28:26Z

Codecov Report

Merging #1605 into master will decrease coverage by 1.19%.
The diff coverage is 16.88%.

@@            Coverage Diff            @@
##           master    #1605     +/-   ##
=========================================
- Coverage   41.97%   40.78%   -1.2%     
=========================================
  Files         270      270             
  Lines       17609    18100    +491     
=========================================
- Hits         7392     7382     -10     
- Misses       9307     9806    +499     
- Partials      910      912      +2

Impacted Files	Coverage Δ
daemon/mgr/container.go	`38.52% <ø> (-11.68%)`	⬇️
cri/v1alpha2/cri_stream.go	`0% <0%> (ø)`	⬆️
cli/exec.go	`0% <0%> (ø)`	⬆️
cri/v1alpha1/cri_stream.go	`0% <0%> (ø)`	⬆️
cri/v1alpha1/cri.go	`0% <0%> (ø)`	⬆️
cri/v1alpha2/cri.go	`0% <0%> (ø)`	⬆️
apis/server/router.go	`91.36% <100%> (+0.06%)`	⬆️
daemon/mgr/container_exec.go	`67.74% <39.13%> (-2.38%)`	⬇️
apis/server/exec_bridge.go	`68.18% <60%> (-1.82%)`	⬇️
daemon/mgr/image.go	`54.09% <0%> (-14.38%)`	⬇️
... and 7 more

fuweid · 2018-06-28T09:29:47Z

daemon/mgr/container_exec.go


 	"github.com/alibaba/pouch/apis/types"
 	"github.com/alibaba/pouch/ctrd"
 	"github.com/alibaba/pouch/pkg/errtypes"
 	"github.com/alibaba/pouch/pkg/randomid"
+	"github.com/docker/docker/pkg/stdcopy"


move the package into next group

fuweid · 2018-06-28T09:37:09Z

cli/exec.go

+	case err := <-stdoutDone:
+		if err != nil {
+			logrus.Debugf("receive stdout error: %s", err)
+			return err


If the code goes into this part, the following code will go into the end?

go func() { if stdin { io.Copy(conn, os.Stdin) } close(stdinDone) }()

It is nit comment. If there exists goroutine leak, the cli will die to make the goroutine exit....

Maybe depend on when golang schedule the goroutine if stdin is flase, if stdin if true, it must became a leak.

Yes. But the cli will return and exit so that the leak problem will be solved. This part is LGTM right now.

Ace-Tang · 2018-06-28T11:11:00Z

@YaoZengzeng , Could you help to see if the patch will infect CRI part.

Ace-Tang · 2018-06-28T12:02:01Z

daemon/mgr/container_exec.go

-			exitCode := 126
-			execConfig.ExitCode = int64(exitCode)
-		}
-		mgr.ExecProcesses.Put(execid, execConfig)


I think mgr.ExecProcesses.Put(execid, execConfig) is unnecessary， since it has added in CreateExec, WDYT, @fuweid

@allencloud , @HusterWan , does mgr.ExecProcesses.Put(execid, execConfig) can be removed here?

@Ace-Tang I do not think so, we will use exec/EXEC_ID/json to judge the exec result, so we should update the execConfig when exec done

Ace-Tang · 2018-06-28T14:00:40Z

daemon/mgr/container_exec.go

+	defer func() {
+		if err != nil {
+			var stdout io.Writer = eio.Stdout
+			if !execConfig.Tty && !eio.MuxDisabled {


update the judgement, @fuweid

fuweid

The ExecProcesses is useful and please add it back?

fuweid · 2018-06-28T12:17:14Z

cli/exec.go

 	"os"
-	"sync"

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


Could you mind to split it into two parts？

I will try...

But I think this part is not fixed this time.

Ace-Tang · 2018-07-02T09:44:58Z

Updated as the comment, @HusterWan @fuweid

allencloud · 2018-07-05T03:00:42Z

apis/server/exec_bridge.go

@@ -55,8 +56,13 @@ func (s *Server) startContainerExec(ctx context.Context, rw http.ResponseWriter,
 	name := mux.Vars(req)["name"]
 	_, upgrade := req.Header["Upgrade"]

+	if err := s.ContainerMgr.CheckExecExist(ctx, name); err != nil {


I am wondering if we should keep consistent that object existence checking should be located in the manager layer rather than bridge layer?

So maybe we should encapsulate this checking into the function StartExec.

if check in StartExec, it too late. StartExec will not return error, since hikacked connection not take error back, we only write error message into connect, then api will get the error message, if check is in StartExec, we will never get error.

allencloud · 2018-07-05T03:02:51Z

apis/server/router.go

@@ -186,6 +186,7 @@ func filter(handler handler, s *Server) http.HandlerFunc {
 			return
 		}
 		// Handle error if request handling fails.
+		logrus.Errorf("Handler for %s %s, client %s returen error: %s", req.Method, req.URL.RequestURI(), clientInfo, err)


s/returen/returned

pouchrobot · 2018-07-05T06:24:13Z

ping @Ace-Tang
We found that this PR is 10 commits, which is more than 10 commits, behind master.
Please rebase the branch against master and push it back again. Thanks a lot.

when daemon deal with exec process get error, process will get stuck, since http connection has been hijacked, it won't finish and return with error message. Fix it by write error message into hijacked connection, close io when process get error. fixes: #1151 Signed-off-by: Ace-Tang <aceapril@126.com>

fuweid · 2018-07-05T11:32:00Z

LGTM

Ace-Tang requested a review from fuweid June 28, 2018 08:33

fuweid reviewed Jun 28, 2018

View reviewed changes

Ace-Tang commented Jun 28, 2018

View reviewed changes

fuweid reviewed Jul 2, 2018

View reviewed changes

pouchrobot added conflict/needs-rebase size/XL labels Jul 4, 2018

Ace-Tang removed the conflict/needs-rebase label Jul 4, 2018

allencloud reviewed Jul 5, 2018

View reviewed changes

pouchrobot added the gap/needs-rebase label Jul 5, 2018

fuweid removed the gap/needs-rebase label Jul 5, 2018

fuweid merged commit 540f51b into AliyunContainerService:master Jul 5, 2018

pouchrobot mentioned this pull request Jul 12, 2018

Weekly Report of PouchContainer from 2018-07-05 to 2018-07-12 #1722

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bugfix: fix exec stuck when exec get error #1605

bugfix: fix exec stuck when exec get error #1605

Ace-Tang commented Jun 28, 2018 •

edited

Loading

Ace-Tang commented Jun 28, 2018 •

edited

Loading

codecov-io commented Jun 28, 2018 •

edited

Loading

fuweid Jun 28, 2018

Ace-Tang Jun 28, 2018

fuweid Jun 28, 2018

fuweid Jun 28, 2018

Ace-Tang Jun 28, 2018

fuweid Jun 28, 2018 •

edited

Loading

Ace-Tang commented Jun 28, 2018

Ace-Tang Jun 28, 2018

Ace-Tang Jul 2, 2018

HusterWan Jul 2, 2018

Ace-Tang Jun 28, 2018

fuweid left a comment

fuweid Jun 28, 2018

Ace-Tang Jul 2, 2018

Ace-Tang Jul 2, 2018 •

edited

Loading

Ace-Tang commented Jul 2, 2018

allencloud Jul 5, 2018

Ace-Tang Jul 5, 2018

allencloud Jul 5, 2018

pouchrobot commented Jul 5, 2018

fuweid commented Jul 5, 2018

bugfix: fix exec stuck when exec get error #1605

bugfix: fix exec stuck when exec get error #1605

Conversation

Ace-Tang commented Jun 28, 2018 • edited Loading

Ⅰ. Describe what this PR did

Ⅱ. Does this pull request fix one issue?

Ⅲ. Describe how you did it

Ⅳ. Describe how to verify it

Ⅴ. Special notes for reviews

Ace-Tang commented Jun 28, 2018 • edited Loading

codecov-io commented Jun 28, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fuweid Jun 28, 2018 • edited Loading

Choose a reason for hiding this comment

Ace-Tang commented Jun 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fuweid left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ace-Tang Jul 2, 2018 • edited Loading

Choose a reason for hiding this comment

Ace-Tang commented Jul 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pouchrobot commented Jul 5, 2018

fuweid commented Jul 5, 2018

Ace-Tang commented Jun 28, 2018 •

edited

Loading

Ace-Tang commented Jun 28, 2018 •

edited

Loading

codecov-io commented Jun 28, 2018 •

edited

Loading

fuweid Jun 28, 2018 •

edited

Loading

Ace-Tang Jul 2, 2018 •

edited

Loading