-
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
feature: events service logic code of all links #2071
feature: events service logic code of all links #2071
Conversation
10747de
to
d97bb02
Compare
Codecov Report
@@ Coverage Diff @@
## master #2071 +/- ##
==========================================
+ Coverage 58.7% 64.17% +5.46%
==========================================
Files 209 212 +3
Lines 16226 16548 +322
==========================================
+ Hits 9526 10619 +1093
+ Misses 5534 4618 -916
- Partials 1166 1311 +145
|
d97bb02
to
4d0817c
Compare
apis/swagger.yml
Outdated
description: | | ||
Stream real-time events from the server. | ||
Report various object events of pouchd when something happens to them. | ||
Containers report these events: `attach`, create`, `destroy`, `detach`, `die`, `exec_create`, `exec_start`, `exec_die`, `oom`, `pause`, `rename`, `resize`, `restart`, `start`, `stop`, `top`, `unpause`, and `update` |
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.
We have supported all these events? If not, we need to remove the not-supported ones. We just support more important ones.
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.
OK,i will remove then events we not supported
"time" | ||
|
||
"github.com/alibaba/pouch/apis/filters" | ||
"github.com/alibaba/pouch/apis/types" |
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.
add a blank line please.
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.
ok
cli/events.go
Outdated
// RFC3339NanoFixed is our own version of RFC339Nano because we want one | ||
// that pads the nano seconds part with zeros to ensure | ||
// the timestamps are aligned in the logs. | ||
RFC3339NanoFixed := "2006-01-02T15:04:05.000000000Z07:00" |
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.
please use utils.TimeLayout
to keep consistent. How about help change the name to utils.RFC3339TimeLayout
?
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.
good point!
98dfe59
to
d960b9c
Compare
d960b9c
to
efb00ab
Compare
ping @HusterWan |
1f771eb
to
a49ddd2
Compare
Signed-off-by: Michael Wan <[email protected]>
a49ddd2
to
8eb98c4
Compare
output.Flush() | ||
enc := json.NewEncoder(output) | ||
|
||
// parse the since and until parameters |
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.
could we extract the logic of parse and validate parameter into one function?
onlyPastEvents = until.Before(now) | ||
if !onlyPastEvents { | ||
dur := until.Sub(now) | ||
timeout = time.NewTimer(dur).C |
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.
do we need to stop the timer?
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.
i think it no need to stop it
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.
why? The timer should be closed if the goroutine exits. If not, it maybe impact other timers.
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.
We only call NewTimer
when just need to, and when created the timer, we will wait until the timer close.
Do i missing something?
ctrd/client.go
Outdated
|
||
// collectContainerdEvents collects events generated by containerd. | ||
func (c *Client) collectContainerdEvents() { | ||
initTypeURL() |
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.
could we put initTypeURL()
out of the collectContainerdEvents()
? I think that the init thing should be done before subscribe event.
pkg/ioutils/writeflusher.go
Outdated
// WriteFlusher wraps the Write and Flush operation ensuring that every write | ||
// is a flush. In addition, the Close method can be called to intercept | ||
// Read/Write calls if the targets lifecycle has already ended. | ||
type WriteFlusher struct { |
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.
could we use apis/server/utils.go#flusher
, since the functionality is existing?
Signed-off-by: Michael Wan <[email protected]>
8eb98c4
to
743ef50
Compare
LGTM |
Signed-off-by: Michael Wan [email protected]
Ⅰ. Describe what this PR did
This PR contains all code of events service:
the events come from two source: generated by pouchd and containerd. the volume, image, network events are all generated by pouchd, but the container die and oom and so on events are generated by containerd. so, we should also collect events from containerd
Ⅱ. Does this pull request fix one issue?
fixes #2052
Ⅲ. Describe how you did it
Ⅳ. Describe how to verify it
Give some examples that subscribe pouchd events
creating a container
remove a container
support filter when subscribe events
create a same container:
only care about volume events
Ⅴ. Special notes for reviews