-
Notifications
You must be signed in to change notification settings - Fork 950
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: distinct init process exit and exec process exit #2347
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -231,9 +231,14 @@ func (c *Client) collectContainerdEvents() { | |
logrus.Warnf("failed to parse %s event: %#v", TaskExitEventTopic, out) | ||
continue | ||
} | ||
action = "die" | ||
if exitEvent.ID == exitEvent.ContainerID { | ||
action = "die" | ||
} else { | ||
action = "exec_die" | ||
attributes["execID"] = exitEvent.ID | ||
} | ||
containerID = exitEvent.ContainerID | ||
attributes["exitcode"] = strconv.Itoa(int(exitEvent.ExitStatus)) | ||
attributes["exitCode"] = strconv.Itoa(int(exitEvent.ExitStatus)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why to make this exitcode uppercase? Will this bring some compatible issues for some early adopters? @HusterWan There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have confirmed with other adopters that used the events policy of pouchd, they had done compability with the So in order to stay with moby, i change the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I will add the |
||
case TaskOOMEventTopic: | ||
oomEvent, ok := out.(*eventstypes.TaskOOM) | ||
if !ok { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In swagger.yml, we defined the /events API like the following:
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great advise, i will update this or soon