feature: support event service #2053

HusterWan · 2018-08-05T09:47:03Z

Signed-off-by: Michael Wan zirenwan@gmail.com

Ⅰ. Describe what this PR did

This PR is to finish the core logic of event service

Event Service has two function:

Publish: send event msg to Event Service
Subscribe: the client can subscribe all events of the pouchd

I also support the filter to filter the event messages that client does not care about

Ⅱ. Does this pull request fix one issue?

fixes part of #2052

Ⅲ. Describe how you did it

Ⅳ. Describe how to verify it

Ⅴ. Special notes for reviews

codecov-io · 2018-08-05T12:12:26Z

Codecov Report

Merging #2053 into master will increase coverage by 0.12%.
The diff coverage is 71.73%.

@@            Coverage Diff             @@
##           master    #2053      +/-   ##
==========================================
+ Coverage    63.8%   63.92%   +0.12%     
==========================================
  Files         202      205       +3     
  Lines       15621    15759     +138     
==========================================
+ Hits         9967    10074     +107     
- Misses       4413     4435      +22     
- Partials     1241     1250       +9

Flag	Coverage Δ
#criv1alpha1test	`33.23% <0%> (-0.31%)`	⬇️
#criv1alpha2test	`33.87% <0%> (-0.34%)`	⬇️
#integrationtest	`37.94% <0%> (-0.33%)`	⬇️
#unittest	`23.6% <71.73%> (+0.49%)`	⬆️

Impacted Files	Coverage Δ
daemon/events/filter.go	`60% <60%> (ø)`
daemon/events/events.go	`72.05% <72.05%> (ø)`
apis/filters/parse.go	`72.3% <72.3%> (ø)`
cri/v1alpha2/cri.go	`66.02% <0%> (-0.18%)`	⬇️
daemon/mgr/container.go	`53.89% <0%> (+0.15%)`	⬆️
cri/v1alpha1/cri.go	`65.36% <0%> (+0.17%)`	⬆️
daemon/logger/jsonfile/utils.go	`73.17% <0%> (+1.62%)`	⬆️
... and 2 more

allencloud · 2018-08-06T03:35:21Z

apis/swagger.yml

@@ -3597,6 +3597,49 @@ definitions:
          items:
            type: "string"

+  EventsMessage:


Please also add the route definition. like:

/containers/{id}/stop:

i have split this whole feature into 4 parts, the api will be added at API pr

@allencloud more information can refer: #2052

allencloud · 2018-08-06T03:36:02Z

apis/swagger.yml

+    description: "represents the information an event contains"
+    type: "object"
+    properties:
+      status:


Please more details about the description of all these field. It would helps a lot for others to understand these.

allencloud · 2018-08-06T03:36:57Z

apis/swagger.yml

+  EventType:
+    description: the type of event. For example, "container" or "image"
+    type: "string"
+    enum: ["container", "daemon", "image", "network", "plugin", "volume"]


We have already implemented all these object's events? Just double check.
Otherwise, I encourage that we add more information about which ones are TODO.

allencloud · 2018-08-06T03:37:32Z

daemon/events/events.go

+
+	defer func() {
+		if err != nil {
+			logrus.Errorf("failed to publishing event {action: %s, type: %s, id: %s}: %v", msg.Action, msg.Type, msg.ID, err)


s/publishing/publish

fuweid · 2018-08-06T04:05:37Z

apis/filters/parse.go

+// ErrBadFormat is an error returned when a filter is not in the form key=value
+//
+// Deprecated: this error will be removed in a future version
+var ErrBadFormat = errors.New("bad format of filter (expected name=value)")


could we remove this part? 😄

fuweid · 2018-08-06T04:05:53Z

apis/filters/parse_test.go

+)
+
+func TestParseArgs(t *testing.T) {
+	// equivalent of `docker ps -f 'created=today' -f 'image.name=ubuntu*' -f 'image.name=*untu'`


s/docker/pouch/g

fuweid · 2018-08-08T06:14:31Z

apis/filters/parse_test.go

+)
+
+func TestParseArgs(t *testing.T) {
+	// equivalent of `pouch ps -f 'created=today' -f 'image.name=ubuntu*' -f 'image.name=*untu'`


ping @Ace-Tang This function is duplicate with your implementation about ps filter?

fuweid · 2018-08-08T06:21:26Z

daemon/events/events.go

+		msg.Status = action
+	}
+
+	defer func() {


we don't defer here and can log the info if err = nil. defer is not straight forward

fuweid · 2018-08-08T06:23:43Z

daemon/events/events.go

+	)
+
+	closeAll := func() {
+		defer close(errq)


why we use defer here? it is not necessary.

fuweid · 2018-08-08T06:25:18Z

daemon/events/events.go

+			case ev := <-channel.C:
+				env, ok := ev.(*types.EventsMessage)
+				if !ok {
+					err = errors.Errorf("invalid message encountered %#v; please file a bug", ev)


please file a bug. funny. If we meet it, could we panic?

no need panic, return err is ok, WDYT?

fuweid · 2018-08-08T06:28:31Z

daemon/events/events.go

+		defer channel.Close()
+	}
+
+	ch = evch


I don't need named return var here. The two statements are unless here.

fuweid · 2018-08-08T06:33:37Z

daemon/events/events.go

+// Publish sends an event. The caller will be considered the initial
+// publisher of the event. This means the timestamp will be calculated
+// at this point and this method may read from the calling context.
+// TODO: do we need lock when use broadcaster???


we don't need to lock the broadcaster, because broadcaster uses channel to receive the event. please remove this comment.

you are right, i just record this to reminder me to check the lock.

fuweid · 2018-08-08T06:35:46Z

daemon/events/events_test.go

+	eventsService := NewEvents()
+
+	t.Log("subscribe")
+	var cancel1, cancel2 func()


since the following statements use :=, it seems this line is useless here. how do you think?

Signed-off-by: Michael Wan <zirenwan@gmail.com>

HusterWan · 2018-08-08T07:55:08Z

Updated @fuweid

dismiss change request, since you separate the whole functionality

fuweid

LGTM

HusterWan force-pushed the zr/support-event branch from 8a0ab24 to 341dd69 Compare August 5, 2018 09:47

pouchrobot added kind/feature size/XXL labels Aug 5, 2018

HusterWan force-pushed the zr/support-event branch 3 times, most recently from 2c97668 to 46cf65f Compare August 5, 2018 11:39

HusterWan force-pushed the zr/support-event branch from 46cf65f to 9b007dd Compare August 5, 2018 13:12

allencloud previously requested changes Aug 6, 2018

View reviewed changes

fuweid reviewed Aug 6, 2018

View reviewed changes

HusterWan force-pushed the zr/support-event branch 2 times, most recently from 695bcab to 904a45f Compare August 6, 2018 05:13

fuweid reviewed Aug 8, 2018

View reviewed changes

feature: support event service

c72af5e

Signed-off-by: Michael Wan <zirenwan@gmail.com>

HusterWan force-pushed the zr/support-event branch from 904a45f to c72af5e Compare August 8, 2018 07:53

fuweid approved these changes Aug 9, 2018

View reviewed changes

fuweid merged commit a3ce826 into AliyunContainerService:master Aug 9, 2018

This was referenced Aug 9, 2018

Weekly Report of PouchContainer from 2018-08-02 to 2018-08-09 #2067

Closed

Weekly Report of PouchContainer from 2018-08-04 to 2018-08-11 #2076

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature: support event service #2053

feature: support event service #2053

HusterWan commented Aug 5, 2018

codecov-io commented Aug 5, 2018 •

edited

Loading

allencloud Aug 6, 2018

HusterWan Aug 6, 2018

HusterWan Aug 6, 2018

allencloud Aug 6, 2018

allencloud Aug 6, 2018

allencloud Aug 6, 2018

HusterWan Aug 6, 2018

fuweid Aug 6, 2018

fuweid Aug 6, 2018

fuweid Aug 8, 2018

fuweid Aug 8, 2018

fuweid Aug 8, 2018

fuweid Aug 8, 2018

HusterWan Aug 8, 2018

fuweid Aug 8, 2018

fuweid Aug 8, 2018

fuweid Aug 8, 2018

HusterWan Aug 8, 2018

fuweid Aug 8, 2018

HusterWan commented Aug 8, 2018

fuweid left a comment

feature: support event service #2053

feature: support event service #2053

Conversation

HusterWan commented Aug 5, 2018

Ⅰ. Describe what this PR did

Ⅱ. Does this pull request fix one issue?

Ⅲ. Describe how you did it

Ⅳ. Describe how to verify it

Ⅴ. Special notes for reviews

codecov-io commented Aug 5, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HusterWan commented Aug 8, 2018

fuweid left a comment

Choose a reason for hiding this comment

codecov-io commented Aug 5, 2018 •

edited

Loading