Events Service #956

ehazlett · 2017-06-05T19:57:43Z

This adds support for events across containerd services.

Example:

$> ctr events
2017-06-13 15:43:35.229454419 +0000 UTC   /namespaces/create   name=dev labels=map[]
2017-06-13 15:43:39.832087236 +0000 UTC   /snapshot/prepare   key=redis parent=sha256:0a3d2e9bb4363183dd62f7172e1722d253fd0e5e752184c275363c0a4fe7dfe6
2017-06-13 15:43:39.842880341 +0000 UTC   /containers/create   id=redis image=docker.io/library/redis:alpine runtime=linux
2017-06-13 15:43:39.881559185 +0000 UTC   /runtime/task-create   id=redis type=CREATE pid=2560 status=0 exited=0001-01-01 00:00:00 +0000 UTC
2017-06-13 15:43:39.886259089 +0000 UTC   /runtime/create   id=redis bundle=/run/containerd/linux/default/redis rootfs=type=overlay:src=overlay checkpoint=
2017-06-13 15:43:39.889641438 +0000 UTC   /tasks/create   id=redis
2017-06-13 15:43:39.896877495 +0000 UTC   /runtime/task-start   id=redis type=START pid=2560 status=0 exited=0001-01-01 00:00:00 +0000 UTC
2017-06-13 15:43:39.897113105 +0000 UTC   /tasks/start   id=redis
2017-06-13 15:43:42.63134793 +0000 UTC   /runtime/task-exit   id=redis type=EXIT pid=2560 status=0 exited=2017-06-13 15:43:42.630947926 +0000 UTC
2017-06-13 15:43:42.663976636 +0000 UTC   /runtime/delete   id=redis runtime=linux status=0 exited=2017-06-13 15:43:42.630947926 +0000 UTC
2017-06-13 15:43:42.664148833 +0000 UTC   /tasks/delete   id=redis
2017-06-13 15:43:42.675727546 +0000 UTC   /snapshot/remove   key=redis
2017-06-13 15:43:42.69592711 +0000 UTC   /containers/delete   id=redis

This closes #785

stevvooe · 2017-06-05T20:45:40Z

cmd/ctr/events.go

Let's try to follow the format of the type urls prescribed in google protobuf. I'll try to hunt down a link, but let's see if you find the right one! ;)

The only thing I could find was https://github.com/containerd/containerd/blob/master/services/containers/helpers.go#L30. But that just specifies the TypeUrl as the version which is then the same for everything. Am I missing something?

I've filed #960 to hunt this down. It includes a link to the correct destination, although, google provides pretty poor tooling for getting this right.

stevvooe · 2017-06-05T20:47:45Z

events/poster.go

The poster shouldn't really have access to the envelope type. The envelope needs to be created by the poster.

ah ok makes sense

mlaventure · 2017-06-05T21:46:21Z

cmd/ctr/events.go

could still have TIMESTAMP\tTOPIC\tEVENT or something similar :)

mlaventure · 2017-06-05T21:47:17Z

events/emitter.go

nit: can be done directly when assigning to listeners below

mlaventure · 2017-06-07T19:21:05Z

cmd/ctr/events.go

the result of that Unmarshalling, if successful, is never used, is something missing?

mlaventure · 2017-06-07T19:21:23Z

api/types/event/event.proto

nit: misaligned

gah i need to grab someone's vim config as these always misalign for me! ;)

i think this is aligned now...

codecov-io · 2017-06-08T16:05:32Z

Codecov Report

Merging #956 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #956   +/-   ##
=======================================
  Coverage   59.28%   59.28%           
=======================================
  Files           5        5           
  Lines         781      781           
=======================================
  Hits          463      463           
  Misses        204      204           
  Partials      114      114

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ff2ceec...ab41816. Read the comment docs.

ehazlett · 2017-06-08T18:27:58Z

This has events for the following services:

containers
content
execution
images
namespaces
snapshot

As well as the runtime. I think this is complete for this PR. We can add to others in follow ups if desired.

ehazlett · 2017-06-08T18:29:05Z

I think the last item for discussion is whether we want to include the namespace as part of the topic path or not: /<namespace>/containers/create vs. /containers/create.

@stevvooe @mlaventure @crosbymichael
wdyt?

mlaventure · 2017-06-08T18:41:55Z

@ehazlett if we go including the namespace as part of the topic path, are operations affecting them to be shown as / (e.g. /create)?

Also thinking of filtering, if I want to request all container events across namespaces, how would I do so /*/container/create ?

With the field and no namespace as part of the topic, it'd just be /container/create (assuming an empty value would mean don't filter per that field) I guess.

ehazlett · 2017-06-08T19:32:49Z

Ya let's remove it. They will just be /containers/create with a Namespace field.

stevvooe · 2017-06-08T23:50:37Z

@ehazlett @mlaventure The namespace should be part of the request context, such that you do a new request for each namespace. This should elide the need for namespace fields.

Are there problems with this approach that I'm missing?

mlaventure · 2017-06-09T13:47:14Z

@stevvooe Do you mean when we request for event?

In that case, I guess it's fine yes, as long as I can send a request with an empty namespace and get events across all of those.

When streaming the event however, I don't think you can get a different header for each event sent, can you?

ehazlett · 2017-06-09T14:03:37Z

@stevvooe the field we're talking about here is the field that's sent to the client as part of the event data (not internal). originally i mistakenly sent the namespace with the event. i've updated to have the namespace as part of the Envelope that the emitter adds so no service sends a namespace. thx!

stevvooe · 2017-06-12T18:38:33Z

services/events/service.go

This method should only emit events from a single namespace.

I've updated to make the client stream to only receive events from the client namespace.

stevvooe · 2017-06-12T18:40:23Z

api/types/event/snapshot.proto

Should we declare these types in the package that emits the events, along with the service?

Doesn't matter to me. I kept them in the events type package to make it logical for events but I could see it in the others too. I don't have a strong feeling either way.

After looking at this, these should be declared by the services that emit them, rather than the event package. I'm not sure we need a secondary events package. The envelope can be part of the events service and the other events can be part of the services that emit them.

stevvooe · 2017-06-12T18:46:49Z

services/namespaces/service.go

Namespaces can also be created implicitly, when in use. This makes it easy to just start using the API, but we never call into the a namespace service or store to make this happen.

aaronlehmann · 2017-06-15T00:44:32Z

cmd/ctr/events.go

This is simple string concatenation, so it could be "id="+e.ContainerID.

This used to do a lot more -- I'll simpify 👍

aaronlehmann · 2017-06-15T00:52:54Z

events/emitter.go

Isn't some synchronization necessary on the various methods that use listeners? It looks like these are invoked from different goroutines.

aaronlehmann · 2017-06-15T00:55:30Z

events/emitter.go

It looks like a slow or unresponsive listener will block the other listeners from receiving events. See docker-archive/classicswarm#2718 for an example of a similar problem in classic Swarm.

Should this use go-events instead so the events are broadcasted to all listeners in a nonblocking fashion?

I wasn't aware of go-events. I'll look thx.

aaronlehmann · 2017-06-15T17:17:12Z

events/emitter.go

The broadcaster needs to be closed when you're done using it. Otherwise, it leaks a goroutine.

We use this wrapper in swarmkit, which may be helpful as an example: https://github.com/docker/swarmkit/blob/master/watch/watch.go

I added a Close for the emitter to close this.

aaronlehmann · 2017-06-15T17:18:15Z

events/emitter.go

I don't think this needs to be a pointer.

I'm unclear on what the mutex is used for. It doesn't seem to be protecting sinks, because the presence in the map is always tested before acquiring the mutex. But I don't think any locking is needed for broadcaster.Add and broadcaster.Remove.

I updated to acquire before testing and holds when creating a new broadcaster. If we don't need any locking for the broadcaster I believe that's the only thing that is mutated outside of the broadcaster so we could probably just remove it. wdyt?

aaronlehmann · 2017-06-15T17:57:31Z

events/emitter.go

This read from the map is not protected by the mutex.

ugh ok i'll step back and go over it better. sorry for the churn.

Signed-off-by: Evan Hazlett <[email protected]> events: update events package to include emitter and use envelope proto Signed-off-by: Evan Hazlett <[email protected]> events: add events service Signed-off-by: Evan Hazlett <[email protected]> events: enable events service and update ctr events to use events service Signed-off-by: Evan Hazlett <[email protected]> event listeners Signed-off-by: Evan Hazlett <[email protected]> events: helper func for emitting in services Signed-off-by: Evan Hazlett <[email protected]> events: improved cli for containers and tasks Signed-off-by: Evan Hazlett <[email protected]> create event envelope with poster Signed-off-by: Evan Hazlett <[email protected]> events: introspect event data to use for type url Signed-off-by: Evan Hazlett <[email protected]> events: use pb encoding; add event types Signed-off-by: Evan Hazlett <[email protected]> events: instrument content and snapshot services with events Signed-off-by: Evan Hazlett <[email protected]> events: instrument image service with events Signed-off-by: Evan Hazlett <[email protected]> events: instrument namespace service with events Signed-off-by: Evan Hazlett <[email protected]> events: add namespace support Signed-off-by: Evan Hazlett <[email protected]> events: only send events from namespace requested from client Signed-off-by: Evan Hazlett <[email protected]> events: switch to go-events for broadcasting Signed-off-by: Evan Hazlett <[email protected]>

ehazlett · 2017-06-20T18:17:51Z

ping @aaronlehmann @stevvooe

stevvooe · 2017-06-20T18:37:04Z

api/types/event/event.proto

@@ -0,0 +1,13 @@
+syntax = "proto3";
+
+package containerd.v1.types;


Should this be containerd.v1.types.events?

stevvooe · 2017-06-20T18:50:58Z

LGTM

I think we can handle the package organization in another pass.

aaronlehmann · 2017-06-20T18:54:59Z

cmd/ctr/events.go

+		if err := proto.Unmarshal(evt.Event.Value, e); err != nil {
+			return out, err
+		}
+		out = fmt.Sprintf("id=%s", e.ContainerID)


More Sprintf abuse :) (and other instances below)

@aaronlehmann what is Sprintf abuse?

Using it to concatenate two strings instead of the + operator.

aaronlehmann · 2017-06-20T18:56:01Z

events/emitter.go

+	e.m.Unlock()
+}
+
+func (e *Emitter) Close() error {


Where is this called?

aaronlehmann · 2017-06-20T19:02:20Z

events/sink.go

+	event Event
+}
+
+type eventSink struct {


Could this be replaced by a combination of events.Chanel and events.Filter?

events.Channel gracefully handles closes when there are writes outstanding. It looks like this implementation wouldn't. If we can provide a guarantee that Close will never cross with a Write, it doesn't matter, but I'm not sure we can.

We have to take another pass here when filters are integrated. Could we fix this at that time?

Sounds okay. I don't see this Close method being called currently. Maybe we should remove it for now to avoid a possible "write to closed channel" panic.

It's there to satisfy the sink interface anyway. I'll remove closing the channel.

Signed-off-by: Evan Hazlett <[email protected]>

crosbymichael · 2017-06-20T20:23:16Z

LGTM

ehazlett force-pushed the events-service branch from 6dd7624 to 06092a0 Compare June 5, 2017 20:35

stevvooe reviewed Jun 5, 2017

View reviewed changes

crosbymichael modified the milestone: Alpha Release 1 Jun 6, 2017

ehazlett force-pushed the events-service branch from 06092a0 to 388b5a6 Compare June 7, 2017 13:48

mlaventure reviewed Jun 7, 2017

View reviewed changes

ehazlett force-pushed the events-service branch 2 times, most recently from 62e0dfa to d785898 Compare June 8, 2017 14:29

ehazlett changed the title ~~WIP: Events Service~~ Events Service Jun 8, 2017

ehazlett force-pushed the events-service branch from 7265ea0 to bb68671 Compare June 8, 2017 15:54

ehazlett force-pushed the events-service branch from 07d3883 to acf9272 Compare June 8, 2017 18:32

ehazlett force-pushed the events-service branch from 305c324 to 9c1c406 Compare June 8, 2017 19:40

samuelkarp mentioned this pull request Jun 8, 2017

Proposal: Event and API object revisions #871

Closed

ehazlett force-pushed the events-service branch from 9c1c406 to 3130294 Compare June 9, 2017 14:02

ehazlett force-pushed the events-service branch from 3130294 to 914783d Compare June 9, 2017 18:31

stevvooe reviewed Jun 12, 2017

View reviewed changes

aaronlehmann reviewed Jun 15, 2017

View reviewed changes

wking mentioned this pull request Jun 15, 2017

runtime: Add an 'event' operation for subscribing to pushes opencontainers/runtime-spec#508

Closed

ehazlett force-pushed the events-service branch from bbbccd5 to 87b3cfd Compare June 19, 2017 20:10

ehazlett force-pushed the events-service branch from 87b3cfd to 935645b Compare June 20, 2017 14:47

stevvooe reviewed Jun 20, 2017

View reviewed changes

aaronlehmann reviewed Jun 20, 2017

View reviewed changes

events/emitter.go Outdated

e.m.Unlock()

}

func (e *Emitter) Close() error {

Copy link

aaronlehmann Jun 20, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where is this called?

aaronlehmann reviewed Jun 20, 2017

View reviewed changes

ehazlett force-pushed the events-service branch from 0e59be4 to db8c656 Compare June 20, 2017 19:10

events: be nice to Sprintf

ab41816

Signed-off-by: Evan Hazlett <[email protected]>

ehazlett force-pushed the events-service branch from db8c656 to ab41816 Compare June 20, 2017 20:17

crosbymichael merged commit f3d9aae into containerd:master Jun 20, 2017

This was referenced Jun 20, 2017

Organize Events Package #1037

Closed

Use goevents for eventSink #1038

Closed

		@@ -0,0 +1,13 @@
		syntax = "proto3";

		package containerd.v1.types;

Events Service #956

Events Service #956

Uh oh!

Conversation

ehazlett commented Jun 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-io commented Jun 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

ehazlett commented Jun 8, 2017

Uh oh!

ehazlett commented Jun 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mlaventure commented Jun 8, 2017

Uh oh!

ehazlett commented Jun 8, 2017

Uh oh!

stevvooe commented Jun 8, 2017

Uh oh!

mlaventure commented Jun 9, 2017

Uh oh!

ehazlett commented Jun 9, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ehazlett commented Jun 5, 2017 •

edited

Loading

codecov-io commented Jun 8, 2017 •

edited

Loading

ehazlett commented Jun 8, 2017 •

edited

Loading