bugfix: distinct init process exit and exec process exit #2347
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2347 +/- ##
==========================================
+ Coverage 66.57% 68.09% +1.51%
==========================================
Files 265 265
Lines 18209 18213 +4
==========================================
+ Hits 12123 12402 +279
+ Misses 4682 4389 -293
- Partials 1404 1422 +18
|
allencloud
added
the
need-test
|
Could you add some integration test to cover this kind of distinguish? @HusterWan |
| containerID = exitEvent.ContainerID | ||
| attributes["exitcode"] = strconv.Itoa(int(exitEvent.ExitStatus)) | ||
| attributes["exitCode"] = strconv.Itoa(int(exitEvent.ExitStatus)) |
There was a problem hiding this comment.
Why to make this exitcode uppercase? Will this bring some compatible issues for some early adopters? @HusterWan
There was a problem hiding this comment.
I have confirmed with other adopters that used the events policy of pouchd, they had done compability with the exitCode.
So in order to stay with moby, i change the exitcode to exitCode
There was a problem hiding this comment.
I will add the event document later
HusterWan
force-pushed
the
zr/fix-exit-event
branch
from
37835c0 to
9926f54
Compare
@allencloud test cases added |
allencloud
removed
the
need-test
| if exitEvent.ID == exitEvent.ContainerID { | ||
| action = "die" | ||
| } else { | ||
| action = "exec_die" |
There was a problem hiding this comment.
In swagger.yml, we defined the /events API like the following:
/events:
get:
summary: "Subscribe pouchd events to users"
description: |
Stream real-time events from the server.
Report various object events of pouchd when something happens to them.
Containers report these events: create`, `destroy`, `die`, `oom`, `pause`, `rename`, `resize`, `restart`, `start`, `stop`, `top`, `unpause`, and `update`
Images report these events: `pull`, `untag`
Volumes report these events: `create`, `destroy`
Networks report these events: `create`, `connect`, `disconnect`, `destroy`
But here, I a afraid that we are adding another event type for container : exec's exec_die, right?
While this has not been collected in to API illustration. If we need to add this in swagger.yml, we should fix that. @HusterWan
And maybe we could have a very thorough thinking for the all workflow of the change: API, daemon, containerd, doc, test and some others.
There was a problem hiding this comment.
great advise, i will update this or soon
|
LGTM until the API swagger.yml has updated if necessary. |
pouchrobot
added
the
LGTM
Signed-off-by: Michael Wan <zirenwan@gmail.com>
HusterWan
force-pushed
the
zr/fix-exit-event
branch
from
9926f54 to
f87c632
Compare
|
I will merge this soon, while I still have some confusion. |
@allencloud I will support all import event types later that will help us know what |
Signed-off-by: Michael Wan zirenwan@gmail.com
Ⅰ. Describe what this PR did
Fix the process
exitevents not distinct theexecprocess exit and the container exit.Now when a container exit we got events below:
when we execute an exec command, also got a container
dieevent:Ⅱ. Does this pull request fix one issue?
none
Ⅲ. Why don't you add test cases (unit test/integration test)? (你真的觉得不需要加测试吗？)
add
TestExecDieEventWorks,TestDieEventWorkscasesⅣ. Describe how to verify it
Ⅴ. Special notes for reviews
none