feature: add timeout handler for execSync in cri part #1280

ZouRui89 · 2018-05-08T07:16:17Z

Signed-off-by: Zou Rui [email protected]

Ⅰ. Describe what this PR did

This pr adds timeout handler for execSync in cri part.
I/O pipe mechanism is used to enhance the degree of simplicity, based on which the data type of memBuffer is changed to allow I/O operations.
Thus, instead of checking if the execution is finished every 100 millisecond, the error io.copy returns is used to control the data flow.

Ⅱ. Does this pull request fix one issue?

fixes #1274

Ⅲ. Describe how you did it

Ⅳ. Describe how to verify it

the cri-test has passed.

Ⅴ. Special notes for reviews

fuweid · 2018-05-08T07:49:24Z

daemon/mgr/cri.go

@@ -23,6 +24,7 @@ import (
 	// NOTE: "golang.org/x/net/context" is compatible with standard "context" in golang1.7+.


Please remove the useless line :)

fuweid · 2018-05-08T08:02:13Z

daemon/mgr/cri.go

 	var output bytes.Buffer
-	startConfig := &apitypes.ExecStartConfig{}
+	_, err = writer.Write(output.Bytes())


The pipe doesn't work here.

fuweid · 2018-05-11T07:08:54Z

daemon/mgr/cri.go

-	var execConfig *ContainerExecConfig
-	for {
-		execConfig, err = c.ContainerMgr.GetExecConfig(ctx, execid)
+	var recv bytes.Buffer


I think we should create goroutine here to make the timeout work.

readWaitCh := make(chan error, 1) go func() { defer reader.Close() _ , err = io.Copy(&recv, reader) readWaitCh <- err }() .... select { case <-ctx.Done(): ... case <-readWaitCh: ... }

fuweid · 2018-05-14T01:35:39Z

daemon/mgr/cri.go

+		readWaitCh <- err
+	}()
+
+	select {


Please add the ctx.Done() selector here because the request maybe canceled without waiting the result.

timeoutCh is doing the same thing as ctx.Done( ), since I have tried ctx.Done( ) and some unknown errors will be brought about by that.
And this is the exact way that cri-containerd used to check the timeout moment, therefore I am following the way it is.

Actually, the ctx.Done() has been handled by the interceptor. It has the functionality like the timeoutCh, but not the same thing. Therefore, could you mind to provide information about what unknown error you mentioned?

fuweid · 2018-05-14T01:41:17Z

daemon/mgr/cri.go

+
+	select {
+	case <-timeoutCh:
+		return nil, fmt.Errorf("timeout %v exceeded", timeout)


If this is the timeout, should we stop the exec process? If not, the timeout doesn't make senses here.

Can you be more specific about the stop execution? I have thought about that but have no idea about how to implement it. :)

YaoZengzeng · 2018-05-14T03:12:00Z

daemon/containerio/mem_buffer.go

@@ -12,7 +11,7 @@ func init() {
 }

 type memBuffer struct {


Since we have changed the backend from memory buffer to pipe, the name memBuffer is no longer properly.

@ZouRui89 Please change to a more appropriate name.

YaoZengzeng · 2018-05-14T03:13:01Z

daemon/containerio/options.go

@@ -102,7 +102,7 @@ func WithStdinHijack() func(*Option) {
 }

 // WithMemBuffer specified the memory buffer backend.
-func WithMemBuffer(memBuffer *bytes.Buffer) func(*Option) {
+func WithMemBuffer(memBuffer *io.PipeWriter) func(*Option) {


Same as the issue mentioned above.

YaoZengzeng · 2018-05-14T03:19:20Z

daemon/mgr/cri.go

+	select {
+	case <-ctx.Done():
+		return nil, fmt.Errorf("timeout %v exceeded", timeout)
+	case <-readWaitCh:


If what we get from readWaitCh is an error other then nil that means something wrong with the exec.

So if we get an error, maybe report it directly to k8s is more properly.

If the error is not nil, I have used logrus.Infof("failed to read data from the pipe") to report the situation.

fuweid · 2018-05-14T05:03:13Z

daemon/mgr/cri.go

+	go func() {
+		defer reader.Close()
+		_, err = io.Copy(&recv, reader)
+		if err != nil {


I think we don't need to check the status here. Just put the error into the readWaitCh and check it on the receiver's side. As @YaoZengzeng said, we should return the error to the k8s. WDYT

Right. That sounds more reasonable. :)

pouchrobot · 2018-05-14T06:49:03Z

ping @ZouRui89

CI fails according integration system.
Please refer to the CI failure Details button to corresponding test, and update your PR to pass CI.

If this is flaky test, welcome to track this with profiling an issue.

build url: https://travis-ci.org/alibaba/pouch/builds/378578974
build duration: 1259s

Signed-off-by: Zou Rui <[email protected]>

pouchrobot · 2018-05-15T02:12:24Z

ping @ZouRui89
Conflict happens after merging a previous commit.
Please rebase the branch against master and push it back again. Thanks a lot.

pouchrobot added kind/feature size/M labels May 8, 2018

fuweid reviewed May 8, 2018

View reviewed changes

ZouRui89 force-pushed the cri_timeout branch 13 times, most recently from f67060d to 16ecda0 Compare May 11, 2018 03:49

fuweid reviewed May 11, 2018

View reviewed changes

ZouRui89 force-pushed the cri_timeout branch 9 times, most recently from 9f3b771 to 263b28a Compare May 11, 2018 09:35

pouchrobot added size/L and removed size/M labels May 11, 2018

ZouRui89 force-pushed the cri_timeout branch 2 times, most recently from 97312bb to 5432020 Compare May 11, 2018 11:01

ZouRui89 force-pushed the cri_timeout branch 10 times, most recently from b88a93f to b665e79 Compare May 13, 2018 10:18

fuweid reviewed May 14, 2018

View reviewed changes

ZouRui89 force-pushed the cri_timeout branch from b665e79 to a6d1b16 Compare May 14, 2018 02:15

YaoZengzeng reviewed May 14, 2018

View reviewed changes

ZouRui89 force-pushed the cri_timeout branch from a6d1b16 to c68eb7f Compare May 14, 2018 03:37

pouchrobot added size/XL and removed size/L labels May 14, 2018

ZouRui89 force-pushed the cri_timeout branch from c68eb7f to 756c8be Compare May 14, 2018 03:43

fuweid reviewed May 14, 2018

View reviewed changes

ZouRui89 force-pushed the cri_timeout branch 2 times, most recently from 774f70a to c95951b Compare May 14, 2018 06:34

ZouRui89 force-pushed the cri_timeout branch from c95951b to 6158dac Compare May 14, 2018 07:05

feature: add timeout handler for execsync in cri part

913fed4

Signed-off-by: Zou Rui <[email protected]>

ZouRui89 force-pushed the cri_timeout branch from 6158dac to 913fed4 Compare May 14, 2018 09:49

pouchrobot added the conflict/needs-rebase label May 15, 2018

ZouRui89 closed this May 15, 2018

ZouRui89 deleted the cri_timeout branch May 16, 2018 01:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature: add timeout handler for execSync in cri part #1280

feature: add timeout handler for execSync in cri part #1280

ZouRui89 commented May 8, 2018 •

edited

Loading

fuweid May 8, 2018

fuweid May 8, 2018

fuweid May 11, 2018

fuweid May 14, 2018

ZouRui89 May 14, 2018 •

edited

Loading

fuweid May 14, 2018

fuweid May 14, 2018

ZouRui89 May 14, 2018

YaoZengzeng May 14, 2018

YaoZengzeng May 14, 2018

YaoZengzeng May 14, 2018

ZouRui89 May 14, 2018

fuweid May 14, 2018 •

edited by ZouRui89

Loading

ZouRui89 May 14, 2018

pouchrobot commented May 14, 2018

pouchrobot commented May 15, 2018

		@@ -23,6 +24,7 @@ import (
		// NOTE: "golang.org/x/net/context" is compatible with standard "context" in golang1.7+.

feature: add timeout handler for execSync in cri part #1280

feature: add timeout handler for execSync in cri part #1280

Conversation

ZouRui89 commented May 8, 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

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

ZouRui89 May 14, 2018 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fuweid May 14, 2018 • edited by ZouRui89 Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pouchrobot commented May 14, 2018

pouchrobot commented May 15, 2018

ZouRui89 commented May 8, 2018 •

edited

Loading

ZouRui89 May 14, 2018 •

edited

Loading

fuweid May 14, 2018 •

edited by ZouRui89

Loading