bugfix: avoid data race in set exec exit status #2152
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
The result may error depend on which goroutine run last |
Ace-Tang
commented
Aug 23, 2018
| @@ -77,7 +77,6 @@ func (mgr *ContainerManager) StartExec(ctx context.Context, execid string, attac | |||
| exitCode := 126 | |||
| execConfig.ExitCode = int64(exitCode) | |||
| } | |||
| mgr.ExecProcesses.Put(execid, execConfig) | |||
There was a problem hiding this comment.
execConfig is a pointer, no need to use mgr.ExecProcesses.Put again.
There was a problem hiding this comment.
For this part, I want to raise proposal that we should deep copy data from manager data store to avoid the data race. But it will introduce the complicated.
Codecov Report
@@ Coverage Diff @@
## master #2152 +/- ##
==========================================
- Coverage 64.46% 39.5% -24.96%
==========================================
Files 209 209
Lines 16680 16678 -2
==========================================
- Hits 10752 6589 -4163
- Misses 4599 9053 +4454
+ Partials 1329 1036 -293
|
yyb196
reviewed
Aug 23, 2018
| logrus.Errorf("failed to execute the exec exit hooks: %v", err) | ||
| break | ||
| // run hook if not got fail here | ||
| if err := <-fail; err == nil { |
There was a problem hiding this comment.
can we get the real exit code?
There was a problem hiding this comment.
When fails here, we got a empty(meaningless) exit status, that means exit code is zero.
fuweid
reviewed
Aug 24, 2018
test/cli_exec_test.go
Outdated
|
|
||
| // test a 'executable file not found' fail should get exit code 126 | ||
| name = "TestExecFailCode" | ||
| command.PouchRun("run", "-d", "--name", name, busyboxImage, "top").Assert(c, icmd.Success) |
There was a problem hiding this comment.
could you update the case description, since there are two cases here?
there are two goroutines response for changed exec config information, if exec process got error, data race appears, exec exit hook runs in a goroutine, it modify exec exit status, StartExec also modify exec exit status in another goroutine since error happend. Fix it by only one place can modify exec exit status. Signed-off-by: Ace-Tang <aceapril@126.com>
|
fail with |
fuweid
reviewed
Aug 28, 2018
| func (suite *PouchExecSuite) TestExecFail(c *check.C) { | ||
| name := "TestExecFail" | ||
| res := command.PouchRun("run", "-d", "--name", name, "-u", name, busyboxImage, "top") | ||
| defer DelContainerForceMultyTime(c, name) | ||
| c.Assert(res.Stderr(), check.NotNil) | ||
|
|
||
| // test a 'executable file not found' fail should get exit code 126. |
There was a problem hiding this comment.
could we split it into two cases?
| @@ -77,7 +77,6 @@ func (mgr *ContainerManager) StartExec(ctx context.Context, execid string, attac | |||
| exitCode := 126 | |||
| execConfig.ExitCode = int64(exitCode) | |||
| } | |||
| mgr.ExecProcesses.Put(execid, execConfig) | |||
There was a problem hiding this comment.
For this part, I want to raise proposal that we should deep copy data from manager data store to avoid the data race. But it will introduce the complicated.
|
@fuweid, deep copy may need, but for this case, we should keep the data race from the logic, deep copy won't avoid this. |
|
LGTM |
pouchrobot
added
the
LGTM
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
there are two goroutines response for changed exec config information,
if exec process got error, data race appears, exec exit hook runs in a
goroutine, it modify exec exit status, StartExec also modify exec exit
status in another goroutine since error happend. Fix it by only one
place can modify exec exit status.
Signed-off-by: Ace-Tang aceapril@126.com
Ⅰ. Describe what this PR did
Ⅱ. 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