Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bugfix: refactor io to avoid output leak in execsync of cri #1388

Closed

Conversation

YaoZengzeng
Copy link
Contributor

@YaoZengzeng YaoZengzeng commented May 23, 2018

Signed-off-by: YaoZengzeng [email protected]

Ⅰ. Describe what this PR did

Now pouch will close and remove all the IO when exec finished.

Actually it will lead to a tricky bug.

Because there are so many levels of buffer in the path of exec. So when exec finished, some output of the exec may still be in one of the middle buffer. And if IO is closed now, we won't be able to get them.

This is why some of the CRI CI test case failed sometimes.

So we should let IO close itself other than triggered by the signal that exec finished.

Ⅱ. Does this pull request fix one issue?

Ⅲ. Describe how you did it

The IO path as follows:

fifo -> ring buffer -> backend

Before this PR, when exec finished, it will close IO including close ring and backend, but there may be still some data left in the fifo or ring.

So what this PR do is : the close of ring buffer is triggered by the data copying between fifo and ring.

And when the exec finished , it will close IO which include waiting for the ring to flush the data to the backend and finally close the backend.

Ⅳ. Describe how to verify it

Ⅴ. Special notes for reviews

@pouchrobot pouchrobot added kind/bug This is bug report for project kind/refactor size/L labels May 23, 2018
@YaoZengzeng YaoZengzeng force-pushed the io-race branch 8 times, most recently from 06b45e8 to 044b026 Compare May 23, 2018 12:49
@fuweid fuweid self-assigned this May 23, 2018
@fuweid fuweid self-requested a review May 23, 2018 15:14
@YaoZengzeng YaoZengzeng force-pushed the io-race branch 7 times, most recently from b323f20 to 7ac67c3 Compare May 24, 2018 07:52
@YaoZengzeng YaoZengzeng force-pushed the io-race branch 2 times, most recently from 3e67690 to fcb11a8 Compare May 24, 2018 09:11
@pouchrobot pouchrobot added size/XL and removed size/L labels May 24, 2018
@pouchrobot
Copy link
Collaborator

ping @YaoZengzeng

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/383092229
build duration: 583s

@pouchrobot
Copy link
Collaborator

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

@pouchrobot
Copy link
Collaborator

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

@YaoZengzeng
Copy link
Contributor Author

The message of conflict is annoying, close it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants