[RFC] refactor: redesign container io in pouch #2375

fuweid · 2018-10-28T10:08:26Z

Ⅰ. Describe what this PR did

Redesign container io in pouch. but Why?

The original design always uses ringbuffer to cache the data which causes streams lost data sometime. It makes the regression test unstable.

It also makes caller hard to touch data from containerd-shim. That is why we still miss the ReopenLog in CRI. I think we should fix that.

design overview

Each container/process will hold streams struct to handle the io stream. The struct is like:

// Stream is used to handle container IO.
type Stream struct {
        sync.WaitGroup
        stdin          io.ReadCloser
        stdinPipe      io.WriteCloser
        stdout, stderr *multiWriter
}

In the stream struct, both the stdout and stderr are the multiple writer's hub. It means that the data from stdout/stderr of process will have more copies. For example, when user use run to start container, the container's output data will have two copies. One is for the terminal side, the other one is for container's log, such as json-file by default.

But for the stdin, we don't need the hub like stdout/stderr, because the input of process is only one. Any attach requests can send the data to the process in container. So we exports the stdinPipe to allow different attach requests to touch the same input channel.

Based on this, the stream acts like bridge to connect attach request and stream of process in container. If the user wants to attach running container, just adds hijack/stream into the output hub and attach it to the stdinPipe. The stream can provides more flexible than before.

Before this change, the CRI struggles to integrating with Container Mgr. Why? because CRI component doesn't know the stream has been finished. Always use dead loop to check the Exec status. It's not good patten.

why add logdriver/crilog into the writer hub?

Both logdriver and crilog don't act like io.Writer. It always add meta data for the raw data. Adding the partial label is one of examples for the acting way of logdriver. In order to avoid to add duplicate code for each driver, we use LogCopier to handle this.

There is a little bit different between crilog and logdriver. The crilog has ReopenLog request. It means that it will add new crilog and close previous one.

In current design, we might have duplicate data in the crilog. Because we close previous, the writer-hub might write something into that. I think it's ok right now.

What is the CloseIO and `StdinOnce`?

The contained-shim will open fifo twice for reading and writing. For the writing mode, the shim doesn't close stdin fifo until the client calls CloseIO. In some case, the pouch daemon might be crash before finishing the input. If shim doesn't hold writing mode fifo, the process in container will consider that it is EOF signal and exit.

Based on this case, if the client sends EOF signal to input channel, the pouchd should send the CloseIO to shim to let the process exit.

StdinOnce in container's config is used by attach request. If the StdinOnce is true, when one attach request finishes stream copy, the pouchd will closes the input of process. So other attach requests to the same container will be stopped.

If the user wants StdinOnce, it should set it to true during creating container.

NOTE: doc link is here.

Ⅱ. Does this pull request fix one issue?

NONE

Ⅲ. Why don't you add test cases (unit test/integration test)? (你真的觉得不需要加测试吗？)

Added it.

Ⅳ. Describe how to verify it

Wait for CI.

Ⅴ. Special notes for reviews

I remove the non-blocking feature in this change and will add it in next patch.

codecov · 2018-10-28T10:08:28Z

Codecov Report

Merging #2375 into master will increase coverage by 0.26%.
The diff coverage is 76.55%.

@@            Coverage Diff             @@
##           master    #2375      +/-   ##
==========================================
+ Coverage   68.31%   68.58%   +0.26%     
==========================================
  Files         275      272       -3     
  Lines       18331    18206     -125     
==========================================
- Hits        12523    12486      -37     
+ Misses       4372     4302      -70     
+ Partials     1436     1418      -18

Flag	Coverage Δ
#criv1alpha1test	`31.39% <61.97%> (-0.25%)`	⬇️
#criv1alpha2test	`35.46% <66.28%> (-0.16%)`	⬇️
#integrationtest	`40.01% <59.56%> (+0.19%)`	⬆️
#nodee2etest	`32.87% <51.71%> (-0.15%)`	⬇️
#unittest	`26.44% <16.94%> (+0.98%)`	⬆️

Impacted Files	Coverage Δ
cri/stream/remotecommand/httpstream.go	`46.63% <ø> (ø)`	⬆️
cri/v1alpha2/cri_utils.go	`91.21% <ø> (+0.69%)`	⬆️
cri/stream/remotecommand/attach.go	`64.7% <ø> (-1.97%)`	⬇️
cri/v1alpha1/cri_utils.go	`83.53% <ø> (+0.31%)`	⬆️
daemon/logger/syslog/syslog.go	`75.82% <100%> (+1.11%)`	⬆️
pkg/streams/multi.go	`100% <100%> (ø)`
ctrd/watch.go	`83.33% <50%> (+4.54%)`	⬆️
daemon/mgr/container.go	`59.22% <62.68%> (-0.85%)`	⬇️
ctrd/container.go	`59.28% <63.06%> (+0.95%)`	⬆️
pkg/ioutils/writer.go	`66.66% <66.66%> (ø)`
... and 35 more

fuweid · 2018-10-29T07:10:58Z

ping @starnop and @YaoZengzeng to help me review the cri part. Thanks

ZYecho · 2018-10-29T09:00:24Z

cli/run.go

@@ -71,6 +72,7 @@ func (rc *RunCommand) runRun(args []string) error {
 	}
 	containerName := rc.name
 	config.ContainerConfig.OpenStdin = rc.stdin
+	config.ContainerConfig.StdinOnce = rc.stdin // close stdin after attach connection closed


if pouch run with -i -d flag enable and when the terminal detach, this container can't be attached again, right?

good catch.

Maybe add a another flag for end-users to control is a good choice?

it has been blocked by

if (rc.attach || rc.stdin) && rc.detach { return fmt.Errorf("Conflicting options: -a (or -i) and -d") }

Maybe add a another flag for end-users to control is a good choice?

basically, I don't think so. The cli is just to help user to use pouch easier. This configuration is too complicated. I don't see any requirement for this.

ZYecho · 2018-10-29T09:07:16Z

daemon/containerio/io.go

+	"github.com/alibaba/pouch/daemon/logger"
+	"github.com/alibaba/pouch/daemon/logger/crilog"
+	"github.com/alibaba/pouch/pkg/streams"
+	"github.com/sirupsen/logrus"


add blank line before this line?

test/cli_run_interactive_test.go

daemon/logger/copier.go

ZYecho · 2018-10-29T09:23:16Z

In this pr desc, the stdinPipe of the Stream struct was shared if there's more than one attacher? @fuweid I did't know the difference between stdinPipe and stdin in Stream struct.

fuweid · 2018-10-29T09:27:15Z

In this pr desc, the stdinPipe of the Stream struct was shared if there's more than one attacher? @fuweid I did't know the difference between stdinPipe and stdin in Stream struct.

Yes. it can be shared by more than one attacher. I was struggled from naming this. stdin is reader of pipe and stdinPipe is writer of pipe.

cli/exec.go

starnop · 2018-10-31T05:08:43Z

I have tested the function ReopenContainerLog. Congratulations! Everything is ok, after this pull request merged, I will complete it. And then, LGTM. :)

Ace-Tang · 2018-10-31T08:21:21Z

daemon/containerio/cio.go

-	if !stdin {
-		fifos.In = ""
+	if !withTerminal {
+		cfg.Stderr = filepath.Join(fifoDir, processID+"-stderr")


I do not think stderr fifo has related with terminal is open

Yes. I checked that containerd-shim. The shim will open socket as pty-slave. There is only one output from pty-slave so that we don't need stderr fifo if the withTerminal is true

Ace-Tang · 2018-10-31T08:23:36Z

daemon/containerio/cio.go

 	}
-	if g.dir != "" {
-		return os.RemoveAll(g.dir)


Before, we remove fifo dir here, now it moved to closeFn, does it matter

I don't think so. Before we need the dir to cleanup. I move it the closeFn, like [email protected]. All the cio.go's changes are for upgrading containerd.

Ace-Tang · 2018-10-31T08:24:34Z

daemon/containerio/cio.go

-func (g *wgCloser) Cancel() {
-	g.cancel()
+// TODO(fuweid): pipes will be removed when update vendor to [email protected].
+type pipes struct {


Do we use pipe, direct IO before ? I will do more check

Ace-Tang · 2018-10-31T08:28:26Z

daemon/containerio/cio.go

 }

-func copyIO(fifos *containerdio.FIFOSet, ioset *ioSet, tty bool) (_ *wgCloser, err error) {
+func openPipes(ctx context.Context, fifos *CioFIFOSet) (_ pipes, err0 error) {


I know, you implement pipe for here use.

rudyfly · 2018-11-01T06:52:24Z

Remove blank line https://github.com/alibaba/pouch/pull/2375/files#diff-d9e51acfdfc53f5e42e18981e1d9f2b1R712

fuweid · 2018-11-02T03:36:34Z

any update @Ace-Tang @YaoZengzeng

YaoZengzeng · 2018-11-02T10:19:13Z

ctrd/container.go

+
+				// Both the caller and container/exec process holds write side pipe
+				// for the stdin. When the caller closes the write pipe, the process doesn't
+				// exit until the caller.


Maybe we missed some comment here? 😆

yes. Wait() to make sure pouchd receives all the data from shim. And Close() is used to make sure the Attach() should exit because there is no data. Thank @YaoZengzeng. In the cleanup, we should Wait() before Close(). I miss that. will update. 👍

fuweid · 2018-11-04T05:41:53Z

@YaoZengzeng I have updated that the containerio does Close action which includes Wait action. However, just in case, we wait for the Wait in 10 seconds. If not, we will go ahead.

zhuangqh · 2018-11-04T12:37:10Z

pkg/streams/multi.go

+		mw.writers = append(mw.writers[:i-n], mw.writers[i-n+1:]...)
+	}
+	mw.Unlock()
+	return len(p), nil


if all of the writer writing bytes fails here, still return len(p). Is it reasonable?

since we don't return error here, we need to return the len(p) here. The writer hub uses io.Pipe as io.WriteCloser. All data are handled in memory, not the real file or network IO. If it run into a error, there is some wrong with pouch and we should restart pouchd.

I think I can add the warning message here if it run into error during writing. How do you think?

I see. Print the warning message is enough.

zhuangqh

I am confused about the lastErr pattern. You will lose some error messages. What about using multierror or print a log.

zhuangqh · 2018-11-05T01:25:49Z

pkg/streams/multi.go

+		mw.writers = append(mw.writers[:i-n], mw.writers[i-n+1:]...)
+	}
+	mw.Unlock()
+	return len(p), nil


I see. Print the warning message is enough.

zhuangqh · 2018-11-05T01:43:06Z

pkg/streams/utils.go

+func (s *Stream) CopyPipes(p Pipes) {
+	copyfn := func(styp string, w io.WriteCloser, r io.ReadCloser) {
+		s.Add(1)
+		go func() {


Controlled by waitGroup to promise all of the data has finished copying(abnormal exit is also regarded as finish). Just comment to record this.

zhuangqh · 2018-11-05T01:45:51Z

pkg/streams/utils.go

+	}
+
+	if s.stdin != nil && p.Stdin != nil {
+		go func() {


Not controlled by waitGroup. Because it may be suddenly closed by user. Just comment to record this.

zhuangqh · 2018-11-05T03:15:03Z

apis/server/container_bridge.go

 	}

+	attach.UseStdin = req.FormValue("stdin") == "1"


using httputils.BoolValue(req, "stdin") instead?

Ace-Tang · 2018-11-05T03:26:38Z

LGTM， more test later.

fuweid · 2018-11-05T03:36:08Z

ping @zhuangqh I update with your recommendation. please take a look.

rudyfly · 2018-11-01T07:09:39Z

daemon/containerio/cio.go

-	return fifos, nil
+	closeFn := func() error {
+		if err := os.RemoveAll(fifoDir); err != nil {
+			logrus.WithError(err).Warnf("failed to remove process(id=%v) fifo dir", processID)


Why not return error?

rudyfly · 2018-11-01T07:11:41Z

daemon/containerio/cio.go

-// NewFifos returns a new set of fifos for the task
-func NewFifos(id string, stdin bool) (*containerdio.FIFOSet, error) {
+// NewCioFIFOSet prepares fifo files.
+func NewCioFIFOSet(processID string, withStdin bool, withTerminal bool) (*CioFIFOSet, error) {
 	root := "/run/containerd/fifo"


Can config for io root directory?

Yes. But I just keep it unchanged. If we have this requirement, we can file other PR to handle this.

rudyfly · 2018-11-05T06:06:35Z

cri/v1alpha1/cri.go

-
-	err = c.ContainerMgr.StartExec(ctx, execid, attachConfig)
-	if err != nil {
+	if err := c.ContainerMgr.StartExec(ctx, execid, attachCfg); err != nil {
 		return nil, fmt.Errorf("failed to start exec for container %q: %v", id, err)


change fmt.Errorf to errors.Wrapf?

Just keep it unchanged and file other PR to handle this.

rudyfly · 2018-11-05T06:08:01Z

cri/v1alpha2/cri.go

@@ -27,6 +26,7 @@ import (
 	"github.com/alibaba/pouch/pkg/errtypes"
 	"github.com/alibaba/pouch/pkg/meta"
 	"github.com/alibaba/pouch/pkg/reference"
+	pkgstreams "github.com/alibaba/pouch/pkg/streams"


why don't use steams?

because cri has a package named by streams. Just use pkgstreams to distinguish two packages

emmmmm, cri package is steam, your package is steams, he he he he

rudyfly · 2018-11-05T06:09:31Z

cri/v1alpha2/cri.go

-		}, nil
+	execConfig, err := c.ContainerMgr.GetExecConfig(ctx, execid)
+	if err != nil {
+		return nil, fmt.Errorf("failed to inspect exec for container %q: %v", id, err)


Can we change all fmt.Errorf("failed to xxxx %q: %v", id, err) to errors.Wrapf()

rudyfly · 2018-11-05T06:11:23Z

ctrd/container.go

@@ -791,3 +831,107 @@ func withCheckpointOpt(checkpoint *containerdtypes.Descriptor) containerd.NewTas
 		return nil
 	}
 }
+
+// InitStdio allows caller to handle any initialize job.


s/job/jobs?

I think job is ok 😂

rudyfly · 2018-11-05T06:13:40Z

daemon/logger/crilog/log.go

+type streamType string
+
+const (
+	// Stdout stream type.


s/streamStdout/Stdout

rudyfly · 2018-11-05T06:13:55Z

daemon/logger/crilog/log.go

+const (
+	// Stdout stream type.
+	streamStdout streamType = "stdout"
+	// Stderr stream type.


s/streamStderr/Stderr

rudyfly · 2018-11-05T06:20:51Z

daemon/mgr/container.go

-		options = append(options, containerio.WithStdin(attach.Stdin))
-	} else {
-		options = append(options, containerio.WithDiscard())
+	if cntrio = mgr.IOs.Get(c.ID); cntrio == nil {


containerIO's abbreviation is cntrio, is it ok? some is container -> ctr

container -> cntr I think it's ok 😅

YaoZengzeng · 2018-11-05T06:29:02Z

LGTM
@fuweid Your work is really brilliant 😎

Signed-off-by: Wei Fu <[email protected]>

fuweid · 2018-11-05T06:40:42Z

ping @zhuangqh @rudyfly I changes cntrio -> ctrio in containerio package.

zhuangqh

LGTM

pouchrobot added kind/design kind/refactor size/XXL labels Oct 28, 2018

fuweid changed the title ~~[WIP] refactor: redesign container io in pouch~~ [WIP|RFC] refactor: redesign container io in pouch Oct 28, 2018

allencloud mentioned this pull request Oct 28, 2018

test: refactor the cli stop test #2372

Merged

pouchrobot added conflict/needs-rebase and removed conflict/needs-rebase labels Oct 29, 2018

fuweid requested review from YaoZengzeng and Ace-Tang October 29, 2018 07:10

fuweid assigned starnop and unassigned starnop Oct 29, 2018

fuweid requested a review from starnop October 29, 2018 07:10

fuweid mentioned this pull request Oct 29, 2018

feature: add support for maximum log size and max number of log files #2262

Closed

ZYecho reviewed Oct 29, 2018

View reviewed changes

starnop reviewed Oct 29, 2018

View reviewed changes

daemon/logger/copier.go Show resolved Hide resolved

HusterWan reviewed Oct 30, 2018

View reviewed changes

cli/exec.go Show resolved Hide resolved

Ace-Tang reviewed Oct 31, 2018

View reviewed changes

YaoZengzeng reviewed Nov 2, 2018

View reviewed changes

zhuangqh reviewed Nov 4, 2018

View reviewed changes

zhuangqh reviewed Nov 5, 2018

View reviewed changes

allencloud changed the title ~~[WIP|RFC] refactor: redesign container io in pouch~~ [RFC] refactor: redesign container io in pouch Nov 5, 2018

allencloud added the priority/P1 this is high priority that all maintainers should stop to handle this issue label Nov 5, 2018

pouchrobot added the LGTM one maintainer or community participant agrees to merge the pull reuqest. label Nov 5, 2018

rudyfly reviewed Nov 5, 2018

View reviewed changes

refactor: redesign container io in pouch

a00e255

Signed-off-by: Wei Fu <[email protected]>

zhuangqh approved these changes Nov 5, 2018

View reviewed changes

allencloud merged commit b83956b into AliyunContainerService:master Nov 5, 2018

fuweid deleted the refactor_stream_io branch November 5, 2018 08:16

starnop mentioned this pull request Nov 7, 2018

feature: implement the feature of reopen log about cri #2447

Merged

pouchrobot mentioned this pull request Nov 9, 2018

WeeklyReport of PouchContainer from 2018-11-02 to 2018-11-09 #2457

Closed

[RFC] refactor: redesign container io in pouch #2375

[RFC] refactor: redesign container io in pouch #2375

Conversation

fuweid commented Oct 28, 2018 • edited Loading

Ⅰ. Describe what this PR did

design overview

why add logdriver/crilog into the writer hub?

What is the CloseIO and StdinOnce?

Ⅱ. Does this pull request fix one issue?

Ⅲ. Why don't you add test cases (unit test/integration test)? (你真的觉得不需要加测试吗？)

Ⅳ. Describe how to verify it

Ⅴ. Special notes for reviews

codecov bot commented Oct 28, 2018 • edited Loading

Codecov Report

fuweid commented Oct 29, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZYecho Oct 29, 2018 • edited Loading

Choose a reason for hiding this comment

fuweid Oct 29, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZYecho commented Oct 29, 2018

fuweid commented Oct 29, 2018 • edited Loading

starnop commented Oct 31, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rudyfly commented Nov 1, 2018

fuweid commented Nov 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fuweid commented Nov 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhuangqh left a 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

Ace-Tang commented Nov 5, 2018

fuweid commented Nov 5, 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

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

rudyfly Nov 5, 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

YaoZengzeng commented Nov 5, 2018

fuweid commented Nov 5, 2018

zhuangqh left a comment

Choose a reason for hiding this comment

fuweid commented Oct 28, 2018 •

edited

Loading

What is the CloseIO and `StdinOnce`?

codecov bot commented Oct 28, 2018 •

edited

Loading

ZYecho Oct 29, 2018 •

edited

Loading

fuweid Oct 29, 2018 •

edited

Loading

fuweid commented Oct 29, 2018 •

edited

Loading

rudyfly Nov 5, 2018 •

edited

Loading