Add Piped API to give back the latest Event #1410

nakabonne · 2021-01-13T01:29:56Z

What this PR does / why we need it:

Which issue(s) this PR fixes:

Does this PR introduce a user-facing change?:

NONE

pipecd-bot

Some issues were detected while linting go source files in your changes.

pkg/app/api/service/pipedservice/pipedclientfake/fakeclient.go

pipecd-bot · 2021-01-13T01:36:28Z

Code coverage for golang is 33.90%. This pull request decreases coverage by -0.16%.

File	Function	Base	Head	Diff
pkg/app/api/grpcapi/piped_api.go	PipedAPI.GetLatestEvent	`--`	`0.00%`	`+0.00%`
pkg/app/piped/cloudprovider/kubernetes/state.go	determineRoleHealth	`--`	`0.00%`	`+0.00%`
pkg/app/piped/cloudprovider/kubernetes/state.go	determineRoleBindingHealth	`--`	`0.00%`	`+0.00%`
pkg/app/piped/cloudprovider/kubernetes/state.go	determineClusterRoleHealth	`--`	`0.00%`	`+0.00%`
pkg/app/piped/cloudprovider/kubernetes/state.go	determineClusterRoleBindingHealth	`--`	`0.00%`	`+0.00%`
pkg/datastore/eventstore.go	NewEventStore	`--`	`100.00%`	`+100.00%`
pkg/datastore/eventstore.go	eventStore.AddEvent	`--`	`100.00%`	`+100.00%`
pkg/datastore/eventstore.go	eventStore.ListEvents	`--`	`0.00%`	`+0.00%`
pkg/app/piped/cloudprovider/kubernetes/state.go	determineResourceHealth	`0.00%`	`0.00%`	`+0.00%`
pkg/datastore/event.go	NewEventStore	`100.00%`	`--`	`-100.00%`
pkg/datastore/event.go	eventStore.AddEvent	`100.00%`	`--`	`-100.00%`

nakabonne · 2021-01-13T02:18:36Z

pkg/app/api/grpcapi/piped_api.go

 }

+// GetLatestEvent gives back the latest event that meets the given conditions.
+func (a *PipedAPI) GetLatestEvent(ctx context.Context, req *pipedservice.GetLatestEventRequest) (*pipedservice.GetLatestEventResponse, error) {


I'm still debating letting it give back a list like ListNotCompletedDeployments. Honestly not sure it's able to perform IN query against map[string]string for all Datastore type well. I attempted to put off it but seems like I'd better carefully look into it at this point.

nghialv · 2021-01-13T04:48:01Z

I am thinking that maybe we should have a simple local store for the latest Events inside piped. (ref: command store)
And EventWatcher will retrieve the needed events from that local store instead of the control-plane.

What does the local store

that local store will periodically fetch the events of a project from its milestone/marked-created-at via something like ListEvents(milestone) then store them inside the memory
if the requested event from EventWatcher was not found inside the memory. The store sends a request to fetch that single event via GetLatestEvent(name, labels).

(This local store acts as a memory cache)

nakabonne · 2021-01-13T05:06:06Z

@nghialv Acutually I was also looking into having the local store inside Piped, but was not sure how to have the only latest Events. Your suggestion that cache the Event structure as-is looks great!

But honestly, I don't get what you said exactly especially the part of milestone/marked-created-at. It means we periodically fetch all Events newly created after marked-created-at (or updated-at)?
(Also, isn't it wrong that piped saves the marked-created-at timestamp inside itself?)

nakabonne · 2021-01-13T05:17:48Z

You often use milestone and i can imagine it's a useful term. I now roughly understand that mean but not get it exactly. What did you refer to when you started using it? Please let me know if you remember.

nakabonne · 2021-01-13T05:20:44Z

I just want to know its accurate meaning.

nghialv · 2021-01-13T05:26:14Z

Sorry, I am having a MTG now.

It means we periodically fetch all Events newly created after marked-created-at (or updated-at)?

Yes, fetch all newly created events after the milestone/marked-created-at value.

piped saves the marked-created-at timestamp inside itself?

Yes, that value is just stored in memory and can be reset when piped restarted.

I am thinking about the flow as following:

when the piped is started, the milestone/marked-created-at value is set by time.Now() - time.Hour or something like this
each 1m, piped fetches all newly created events after the current milestone value, and then update that value to the biggest createdAt value from the response

nghialv · 2021-01-13T05:28:49Z

While saving the event into the local store, the one that has the same ID (list of labels(name = event-name, repo-id= foo...)) will be overridden.

nakabonne · 2021-01-13T05:31:37Z

Sorry to interrupt your MTG. You can answer after you finished that.

The flow you suggested looks great. I'll look into if we really can do it with no problem 👍

nghialv · 2021-01-13T05:32:32Z

👍

pipecd-bot

Some issues were detected while linting go source files in your changes.

pkg/app/api/service/pipedservice/pipedclientfake/fakeclient.go

nakabonne · 2021-01-13T06:33:34Z

Seems like no problem with this solution. I'm still not sure if this usage of Milestone is correct, let me know if it's wrong.
And let me implement the apistore for Event at #1411.

nakabonne · 2021-01-13T06:34:08Z

Fixed done!

pipecd-bot · 2021-01-13T06:35:28Z

Code coverage for golang is 33.86%. This pull request decreases coverage by -0.20%.

File	Function	Base	Head	Diff
pkg/app/api/grpcapi/piped_api.go	PipedAPI.GetLatestEvent	`--`	`0.00%`	`+0.00%`
pkg/app/api/grpcapi/piped_api.go	PipedAPI.ListEvents	`--`	`0.00%`	`+0.00%`
pkg/app/piped/cloudprovider/kubernetes/state.go	determineRoleHealth	`--`	`0.00%`	`+0.00%`
pkg/app/piped/cloudprovider/kubernetes/state.go	determineRoleBindingHealth	`--`	`0.00%`	`+0.00%`
pkg/app/piped/cloudprovider/kubernetes/state.go	determineClusterRoleHealth	`--`	`0.00%`	`+0.00%`
pkg/app/piped/cloudprovider/kubernetes/state.go	determineClusterRoleBindingHealth	`--`	`0.00%`	`+0.00%`
pkg/datastore/eventstore.go	NewEventStore	`--`	`100.00%`	`+100.00%`
pkg/datastore/eventstore.go	eventStore.AddEvent	`--`	`100.00%`	`+100.00%`
pkg/datastore/eventstore.go	eventStore.ListEvents	`--`	`0.00%`	`+0.00%`
pkg/app/piped/cloudprovider/kubernetes/state.go	determineResourceHealth	`0.00%`	`0.00%`	`+0.00%`
pkg/datastore/event.go	NewEventStore	`100.00%`	`--`	`-100.00%`
pkg/datastore/event.go	eventStore.AddEvent	`100.00%`	`--`	`-100.00%`

nghialv · 2021-01-13T06:46:06Z

pkg/app/api/service/pipedservice/service.proto

+
+message ListEventsRequest {
+    // Mark that the client has handled all events created before this value.
+    int64 milestone = 1 [(validate.rules).int64.gt = 0];


nit: This is API definition so I think it could be from (if the event for this value should be included too) or after.
(Milestone or marked-something is just a name used inside piped code.)

Thank you. Acutually I've cared about this part. from looks good

Exactly we don't include the given value itself, so after is more appropriate.

nakabonne · 2021-01-13T06:47:10Z

pkg/app/api/grpcapi/piped_api.go

+	if len(events) == 0 {
+		a.logger.Error("no events found",
+			zap.String("project-id", projectID),
+			zap.Int64("milestone", req.Milestone),
+		)
+		return nil, status.Error(codes.NotFound, "no events found")
+	}


Other List~ API doesn't treat the not found case as an error, but I'd say it should be handled.

I am not sure why we need to add this NotFound error while the empty list already means nothing was found.

Come to think of it, right, it represents Not Found well. I'll fix it.

nghialv · 2021-01-13T06:47:20Z

pkg/app/api/service/pipedservice/service.proto

    // By that way we can control the traffic to the datastore in a better way.
    rpc ReportApplicationLiveStateEvents(ReportApplicationLiveStateEventsRequest) returns (ReportApplicationLiveStateEventsResponse) {}
+
+    // GetLatestEvent gives back the latest event that meets the given conditions.


nit: gives back -> returns
Just to be unified.

nghialv · 2021-01-13T06:50:42Z

pkg/app/api/grpcapi/piped_api.go

+		return nil, status.Error(codes.Internal, "failed to list event")
+	}
+	if len(events) == 0 {
+		a.logger.Error("no events found",


I think this shouldn't be an Error log.

Right, I started thinking the log itself is no longer needed.

nakabonne · 2021-01-13T07:06:34Z

@nghialv Thanks for taking a look. Fixed all of them.

pipecd-bot · 2021-01-13T07:11:58Z

Code coverage for golang is 33.87%. This pull request decreases coverage by -0.19%.

File	Function	Base	Head	Diff
pkg/app/api/grpcapi/piped_api.go	PipedAPI.GetLatestEvent	`--`	`0.00%`	`+0.00%`
pkg/app/api/grpcapi/piped_api.go	PipedAPI.ListEvents	`--`	`0.00%`	`+0.00%`
pkg/app/piped/cloudprovider/kubernetes/state.go	determineRoleHealth	`--`	`0.00%`	`+0.00%`
pkg/app/piped/cloudprovider/kubernetes/state.go	determineRoleBindingHealth	`--`	`0.00%`	`+0.00%`
pkg/app/piped/cloudprovider/kubernetes/state.go	determineClusterRoleHealth	`--`	`0.00%`	`+0.00%`
pkg/app/piped/cloudprovider/kubernetes/state.go	determineClusterRoleBindingHealth	`--`	`0.00%`	`+0.00%`
pkg/datastore/eventstore.go	NewEventStore	`--`	`100.00%`	`+100.00%`
pkg/datastore/eventstore.go	eventStore.AddEvent	`--`	`100.00%`	`+100.00%`
pkg/datastore/eventstore.go	eventStore.ListEvents	`--`	`0.00%`	`+0.00%`
pkg/app/piped/cloudprovider/kubernetes/state.go	determineResourceHealth	`0.00%`	`0.00%`	`+0.00%`
pkg/datastore/event.go	NewEventStore	`100.00%`	`--`	`-100.00%`
pkg/datastore/event.go	eventStore.AddEvent	`100.00%`	`--`	`-100.00%`

nghialv · 2021-01-13T07:34:13Z

pkg/app/api/service/pipedservice/service.proto

+
+message ListEventsRequest {
+    // returns all events created after this value.
+    int64 after = 1 [(validate.rules).int64.gt = 0];


How about making this to be more familiar, by using from and to.
The response will contain all events in this range [from, to).
(from <= && < to)
(And the mark at the piped should be to)

Looks good. Then PIped uses to+1 value for the next time, right?

That can be confusable, I mean for the next time Piped uses the maximum created_at in the previous result.

I think piped uses to, because that the previous call, to was not included.

And by this way we don't need to find the maximum created_at value from the previous response, Just use time.Now()

I see, maybe I don't fully understand the standard about queries.

Ok, let me confirm this is correct or not: from <= response < to

first call: [time.Now - time.Hour, time.Now)

from the second call: [previousCall.To, time.Now)

Ok, let me confirm this is correct or not: from <= response < to

Yes, that is correct.

Fine. Definitely that is more familiar for most developers.

pipecd-bot · 2021-01-13T07:49:16Z

The following ISSUES will be created once got merged. If you want me to skip creating the issue, you can use /todo skip command.

Details

1. Enable to use labels to filter Events

https://github.com/pipe-cd/pipe/blob/a3dbc03c7f887d45d9d46b193ce51bc81588cd60/pkg/app/api/grpcapi/piped_api.go#L705-L708

This was created by todo plugin since "TODO:" was found in `a3dbc03` when #1410 was merged. cc: @nakabonne.

nakabonne · 2021-01-13T07:51:03Z

Changed to use from and to instead of after.

nghialv · 2021-01-13T07:53:16Z

Nice. Thank you.
/lgtm

pipecd-bot · 2021-01-13T07:55:29Z

Code coverage for golang is 33.87%. This pull request decreases coverage by -0.19%.

File	Function	Base	Head	Diff
pkg/app/api/grpcapi/piped_api.go	PipedAPI.GetLatestEvent	`--`	`0.00%`	`+0.00%`
pkg/app/api/grpcapi/piped_api.go	PipedAPI.ListEvents	`--`	`0.00%`	`+0.00%`
pkg/app/piped/cloudprovider/kubernetes/state.go	determineRoleHealth	`--`	`0.00%`	`+0.00%`
pkg/app/piped/cloudprovider/kubernetes/state.go	determineRoleBindingHealth	`--`	`0.00%`	`+0.00%`
pkg/app/piped/cloudprovider/kubernetes/state.go	determineClusterRoleHealth	`--`	`0.00%`	`+0.00%`
pkg/app/piped/cloudprovider/kubernetes/state.go	determineClusterRoleBindingHealth	`--`	`0.00%`	`+0.00%`
pkg/datastore/eventstore.go	NewEventStore	`--`	`100.00%`	`+100.00%`
pkg/datastore/eventstore.go	eventStore.AddEvent	`--`	`100.00%`	`+100.00%`
pkg/datastore/eventstore.go	eventStore.ListEvents	`--`	`0.00%`	`+0.00%`
pkg/app/piped/cloudprovider/kubernetes/state.go	determineResourceHealth	`0.00%`	`0.00%`	`+0.00%`
pkg/datastore/event.go	NewEventStore	`100.00%`	`--`	`-100.00%`
pkg/datastore/event.go	eventStore.AddEvent	`100.00%`	`--`	`-100.00%`

khanhtc1202 · 2021-01-13T08:33:52Z

/approve

pipecd-bot · 2021-01-13T08:33:55Z

This pull request is APPROVED by khanhtc1202.

Approvers can cancel the approval by writing /approve cancel in a comment. Any additional commits also will change this pull request to be not-approved.

@nghialv

**What this PR does / why we need it**: This PR adds a new `apistore` for Events according to @nghialv 's [suggestion](#1410 (comment)). It allows us to reduce communication number by caching the latest events. This will be used by #1411 at first. **Which issue(s) this PR fixes**: Ref #1381 **Does this PR introduce a user-facing change?**:  ```release-note NONE ``` This PR was merged by Kapetanios.

Add Piped API to give back the latest Event

b25c48e

pipecd-bot added release-note-none area/api size/L labels Jan 13, 2021

pipecd-bot requested review from khanhtc1202 and nghialv January 13, 2021 01:29

pipecd-bot reviewed Jan 13, 2021

View reviewed changes

pkg/app/api/service/pipedservice/pipedclientfake/fakeclient.go Show resolved Hide resolved

pkg/app/api/service/pipedservice/pipedclientfake/fakeclient.go Show resolved Hide resolved

nakabonne commented Jan 13, 2021

View reviewed changes

Add ListEvents

f5c6a50

pipecd-bot reviewed Jan 13, 2021

View reviewed changes

pkg/app/api/service/pipedservice/pipedclientfake/fakeclient.go Show resolved Hide resolved

pkg/app/api/service/pipedservice/pipedclientfake/fakeclient.go Show resolved Hide resolved

nghialv reviewed Jan 13, 2021

View reviewed changes

nakabonne commented Jan 13, 2021

View reviewed changes

nghialv reviewed Jan 13, 2021

View reviewed changes

Fix

7d7eef6

nghialv reviewed Jan 13, 2021

View reviewed changes

Use from and to instead of after

a3dbc03

pipecd-bot added the lgtm label Jan 13, 2021

pipecd-bot added the approved label Jan 13, 2021

pipecd-bot merged commit 86009e2 into master Jan 13, 2021

pipecd-bot deleted the event-piped-api branch January 13, 2021 08:40

pipecd-bot mentioned this pull request Jan 13, 2021

Enable to use labels to filter Events #1421

Closed

nakabonne mentioned this pull request Jan 14, 2021

Add apistore for caching the latest events #1429

Merged

pipecd-bot mentioned this pull request Jan 25, 2021

Update feature status #1474

Merged

Add Piped API to give back the latest Event #1410

Add Piped API to give back the latest Event #1410

Uh oh!

Conversation

nakabonne commented Jan 13, 2021

Uh oh!

pipecd-bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

pipecd-bot commented Jan 13, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nghialv commented Jan 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nakabonne commented Jan 13, 2021

Uh oh!

nakabonne commented Jan 13, 2021

Uh oh!

nakabonne commented Jan 13, 2021

Uh oh!

nghialv commented Jan 13, 2021

Uh oh!

nghialv commented Jan 13, 2021

Uh oh!

nakabonne commented Jan 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nghialv commented Jan 13, 2021

Uh oh!

pipecd-bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

nakabonne commented Jan 13, 2021

Uh oh!

nakabonne commented Jan 13, 2021

Uh oh!

pipecd-bot commented Jan 13, 2021

Uh oh!

nghialv Jan 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

nakabonne commented Jan 13, 2021

Uh oh!

pipecd-bot commented Jan 13, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nakabonne Jan 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

nghialv commented Jan 13, 2021 •

edited

Loading

nakabonne commented Jan 13, 2021 •

edited

Loading

nghialv Jan 13, 2021 •

edited

Loading

nakabonne Jan 13, 2021 •

edited

Loading

nghialv Jan 13, 2021 •

edited

Loading

This was created by todo plugin since "TODO:" was found in `a3dbc03` when #1410 was merged. cc: @nakabonne.