Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions pkg/app/api/grpcapi/piped_api.go
Original file line number Diff line number Diff line change
Expand Up @@ -753,6 +753,22 @@ func (a *PipedAPI) ListEvents(ctx context.Context, req *pipedservice.ListEventsR
},
},
}
switch req.Order {
case pipedservice.ListEventsOrder_FROM_OLDEST:
opts.Orders = []datastore.Order{
{
Field: "CreatedAt",
Direction: datastore.Asc,
},
}
case pipedservice.ListEventsOrder_FROM_NEWEST:
opts.Orders = []datastore.Order{
{
Field: "CreatedAt",
Direction: datastore.Desc,
},
}
}

events, err := a.eventStore.ListEvents(ctx, opts)
if err != nil {
Expand Down
7 changes: 7 additions & 0 deletions pkg/app/api/service/pipedservice/service.proto
Original file line number Diff line number Diff line change
Expand Up @@ -362,9 +362,16 @@ message GetLatestEventResponse {
pipe.model.Event event = 1 [(validate.rules).message.required = true];
}

enum ListEventsOrder {
NONE = 0;
FROM_OLDEST = 1;
FROM_NEWEST = 2;
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

How about ListOrder enum that contains NONE, DESC, ASC?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

That sounds good, that's more versatile. I initially thought about making it but I wasn't sure which should be general, CreatedAt or UpdatedAt, that's why I made a special enum for now. but what do you think?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think it is fine to make ListOrder. It can be used by other RPCs too.
Let's do it.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Fine, I'll do so!


message ListEventsRequest {
int64 from = 1 [(validate.rules).int64.gt = 0];
int64 to = 2 [(validate.rules).int64.gt = 0];
ListEventsOrder order = 3 [(validate.rules).enum.defined_only = true];
}

message ListEventsResponse {
Expand Down
21 changes: 21 additions & 0 deletions pkg/app/piped/apistore/eventstore/BUILD.bazel
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_test")

go_library(
name = "go_default_library",
srcs = ["store.go"],
importpath = "github.com/pipe-cd/pipe/pkg/app/piped/apistore/eventstore",
visibility = ["//visibility:public"],
deps = [
"//pkg/app/api/service/pipedservice:go_default_library",
"//pkg/model:go_default_library",
"@org_golang_google_grpc//:go_default_library",
"@org_uber_go_zap//:go_default_library",
],
)

go_test(
name = "go_default_test",
size = "small",
srcs = ["store_test.go"],
embed = [":go_default_library"],
)
154 changes: 154 additions & 0 deletions pkg/app/piped/apistore/eventstore/store.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,154 @@
// Copyright 2021 The PipeCD Authors.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

package eventstore

import (
"context"
"sync/atomic"
"time"

"go.uber.org/zap"
"google.golang.org/grpc"

"github.com/pipe-cd/pipe/pkg/app/api/service/pipedservice"
"github.com/pipe-cd/pipe/pkg/model"
)

// Getter helps get an event. All objects returned here must be treated as read-only.
type Getter interface {
// GetLatest returns the latest event that meets the given conditions.
GetLatest(ctx context.Context, name string, labels map[string]string) (*model.Event, bool)
}

type Store interface {
// Run starts syncing the event list with the control-plane.
Run(ctx context.Context) error
// Getter returns a getter for retrieving an event.
Getter() Getter
}

type apiClient interface {
GetLatestEvent(ctx context.Context, req *pipedservice.GetLatestEventRequest, opts ...grpc.CallOption) (*pipedservice.GetLatestEventResponse, error)
ListEvents(ctx context.Context, req *pipedservice.ListEventsRequest, opts ...grpc.CallOption) (*pipedservice.ListEventsResponse, error)
}

type store struct {
apiClient apiClient
syncInterval time.Duration
gracePeriod time.Duration
logger *zap.Logger

// Mark that it has handled all events that was created before this value.
milestone int64
// The key is supposed to be event-definition ID, a string consists of name and labels.
// And the value is the address to the latest Event.
latestEventMap atomic.Value
}

const (
defaultSyncInterval = time.Minute
)

// NewStore creates a new event store instance.
// This syncs with the control plane to keep the list of events for this runner up-to-date.
func NewStore(apiClient apiClient, gracePeriod time.Duration, logger *zap.Logger) Store {
s := &store{
apiClient: apiClient,
syncInterval: defaultSyncInterval,
gracePeriod: gracePeriod,
logger: logger.Named("event-store"),
}
s.latestEventMap.Store(make(map[string]*model.Event))
return s
}

// Run starts runner that periodically makes the Events in the cache up-to-date
// by fetching from the control-plane.
func (s *store) Run(ctx context.Context) error {
s.logger.Info("start running event store")

syncTicker := time.NewTicker(s.syncInterval)
defer syncTicker.Stop()

// Do first sync without waiting the first ticker.
s.milestone = time.Now().Add(-time.Hour).Unix()
s.sync(ctx)

for {
select {
case <-syncTicker.C:
s.sync(ctx)

case <-ctx.Done():
s.logger.Info("event store has been stopped")
return nil
}
}
}

// sync fetches a list of events inside the range between own milestone and the current local time.
// Only this function takes responsibility for updating the cache.
func (s *store) sync(ctx context.Context) error {
// TODO: Use UTC and let control-plane convert it into the control-plane's Local time
// Unexpected behavior can be happened if Piped uses different timezone from the control-plane.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I will look into this concern when working on another PR.

@nghialv nghialv Jan 14, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think the timezone difference is not a problem, because we are using Unix time (the number of seconds elapsed since January 1, 1970 UTC.).
But the clock drift may cause the problem. But that is rare.

Btw, maybe we can leave the to parameter as zero to use time.Now of the server.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I think the timezone difference is not a problem, because we are using Unix time (the number of seconds elapsed since January 1, 1970 UTC.).

I put off thinking about it but after thinking, it's a basic thing and no problem as you mentiond.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Btw, maybe we can leave the to parameter as zero to use time.Now of the server.

Cool, I'll try it.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Btw, maybe we can leave the to parameter as zero to use time.Now of the server.

After thinking, it makes derivating the next's from tougher. As you mentioned, clock drifts is reraly happened. I guess it's enough to be as-is.

@nakabonne nakabonne Jan 14, 2021

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Not tougher, I came up with a better solution just now. I'm going to make From and To optional.

to := time.Now().Unix()
resp, err := s.apiClient.ListEvents(ctx, &pipedservice.ListEventsRequest{
From: s.milestone,
To: to,
Order: pipedservice.ListEventsOrder_FROM_OLDEST,
})
if err != nil {
s.logger.Error("failed to list events", zap.Error(err))
return err
}
// Set this time's "to" as the next time's "from".
s.milestone = to

// Make the cache up-to-date by traversing events sorted by oldest first.
eventMap := s.latestEventMap.Load().(map[string]*model.Event)
for _, event := range resp.Events {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

resp.Events may contain multiple events for the same key/definition-id, so I think we should build their key/definition-id and filter duplicates before locking the mutex to update the list.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Good point. I initially attempted to do so, but I didn't. That's necessary to keep the time to lock mutex as short as possible. But don't we really have to care about that performance? I guess we should prioritize to keep simplicity than performance at this time, that's why I settled on just sorting by oldest first and traversing all events.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

But don't we really have to care about that performance?

Actually, I didn't want to say that we don't really care about the performance.
What I wanted to say before is that the complexity of the concurrency model is not worth the performance it brings.

But calculating the ID and filtering the duplicates before locking can be done easily. So it's worth it.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

You're quite right. Adding filtering is not complicated enough to worry about complexity.

id := model.EventDefinitionID(event.Name, event.Labels)
eventMap[id] = event
}
s.latestEventMap.Store(eventMap)

return nil
}

func (s *store) Getter() Getter {
return s
}

func (s *store) GetLatest(ctx context.Context, name string, labels map[string]string) (*model.Event, bool) {
eventMap := s.latestEventMap.Load().(map[string]*model.Event)
id := model.EventDefinitionID(name, labels)
event, ok := eventMap[id]

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

One thing I want to ask about our event matching, in case we have an event

name = foo
labels: { label-1: value-1, label-2: value-2}

That event will match the following case or not?

piVersion: pipecd.dev/v1beta1
kind: EventWatcher
spec:
  events:
    - name: foo
      labels:
           label-1: value-1
      replacements:
        - file: dev/app1/deployment.yaml
          yamlField: $.spec.template.spec.containers[0].image

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Or all labels must be specified to be matched.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I totally assume exact matches are needed, but on second thought, seems like that case should be treated as a matching case. Let me think, just a moment please...

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@nghialv I'm still debating but which one do you prefer?

@nghialv nghialv Jan 14, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If possible, the event should be matched when the name is equal AND the event's labels contain the specified ones.
(Because they are labels (not ID) and users will have more options.)

So I think we can change the map to a new data structure like map[event-name]ListOfEventsHaveSameName.

But one thing we have to consider is what happens when multiple events match the specified label list.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

But one thing we have to consider is what happens when multiple events match the specified label list.

So in order to avoid this complexity and avoid adding the dynamic label by users (e.g. version number),
I think we should not allow subset matching.

Name and all labels must be matched.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

My feeling is exactly the same as yours. Some people can be helped by subset matching, but too complicated to handle for us. Thank you for a great opportunity to re-think it and letting me know your cool thought!

if ok {
return event, true
}

// If not found in the cache, fetch from the control-plane.
resp, err := s.apiClient.GetLatestEvent(ctx, &pipedservice.GetLatestEventRequest{
Name: name,
Labels: labels,
})
if err != nil {
s.logger.Error("failed to get the latest event", zap.Error(err))
return nil, false
}

// NOTE: Don't update the cache to prevent it from being overwritten by slightly older ones.

@nghialv nghialv Jan 14, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why we don't update the in-memory map?
The event has CreatedAt field, so we can use that to check whether the event should be updated or not.

And I think in the microservices list of our users, there are always some applications that have not been actively developed compared to others. Their events will be very sparse. So without saving into the list, piped will always make the call to control-plane to get the same one.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The sync function periodically updates the cache, and it works in another goroutine. What I'm most worried about is, while processing GetLatest, sync can update the cache. I mean:

	// NOTE: Don't update the cache to prevent it from being overwritten by slightly older ones.
	eventMap = s.latestEventMap.Load().(map[string]*model.Event)
	eventMap[id] = resp.Event        // I'm most worried about the "sync()" function updates the cache at this point.
	s.latestEventMap.Store(eventMap) // Then the latest cache is overwritten by the old one here.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

If Piped was run on the multi-core processor, I think it can happen, but let me know if I'm wrong.

@nghialv nghialv Jan 14, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The data race can be resolved in some ways:

  • change from using atomic to using a mutex. (fetch the list from control-plane, lock, and update). I think it is fine enough for our case: 1 call/m and EventWatcher.
  • use 2 separate maps, one for LIST API, one for GET API

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Ah, not fancy 😄 The first one looks enough! Only two goroutines can access this and it rarely happens to wait for unlocking. Thank you for telling me!

return resp.Event, true
}
15 changes: 15 additions & 0 deletions pkg/app/piped/apistore/eventstore/store_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
// Copyright 2021 The PipeCD Authors.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

package eventstore
2 changes: 2 additions & 0 deletions pkg/model/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ go_library(
"deployment.go",
"docs.go",
"environment.go",
"event.go",
"filestore.go",
"image_name.go",
"imageprovider.go",
Expand All @@ -79,6 +80,7 @@ go_test(
srcs = [
"apikey_test.go",
"common_test.go",
"event_test.go",
"image_name_test.go",
"model_test.go",
"piped_test.go",
Expand Down
43 changes: 43 additions & 0 deletions pkg/model/event.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
// Copyright 2021 The PipeCD Authors.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

package model

import (
"fmt"
"sort"
"strings"
)

// EventDefinitionID builds a unique identifier based on the given name and labels.
// It returns the exact same string as long as both are the same.
func EventDefinitionID(name string, labels map[string]string) string {

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I've been thinking about what we should call something whose uniqueness is consists of its Name and Labels. I just settled on calling it Event Definition.

An Event is like an Instance In Object-oriented world, and an event definition is like a Class. Each Event has its own ID and at the same time, each Event definition has its own ID. The event definition ID must be the exact same string as long as Name and Labels are the same.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I just figured out it would be nice if we build this when saving and keep it as a field of Event.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This will be used only by EventStore right. So I think it should be moved to that package as an internal function.
So because of an internal function, we will not have to care about its name.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Actually it will be used by #1406. It could be increased the need to build a unique identity made of Name and Labels, that's why I decided to make a helper function.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I see. Now we are having our cache/store for the event. So maybe we don't need to merge them for simplicity.

The user can merge by changing their configuration if needed.
We implicitly merge them and combine their changes into a single commit that may cause the behavior they were not expected.
(I am not sure that I understood the merging correctly.)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

(I am not sure that I understood the merging correctly.)

Your understanding is correct. I attempted to merge it to combine their changes into a single commit.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Okay, I agree with all of them. So let's this function private and think it if needed in the future!

if len(labels) == 0 {
return name
}

var b strings.Builder
b.WriteString(name)

// Guarantee uniqueness by sorting by keys.
keys := make([]string, 0, len(labels))
for key := range labels {
keys = append(keys, key)
}
sort.Strings(keys)
for _, key := range keys {
b.WriteString(fmt.Sprintf("/%s:%s", key, labels[key]))
}
return b.String()
}
63 changes: 63 additions & 0 deletions pkg/model/event_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
// Copyright 2021 The PipeCD Authors.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

package model

import (
"testing"

"github.com/stretchr/testify/assert"
)

func TestEventDefinitionID(t *testing.T) {
testcases := []struct {
testname string
name string
labels map[string]string
want string
}{
{
testname: "no name and labels given",
want: "",
},
{
testname: "no labels given",
name: "name1",
want: "name1",
},
{
testname: "no name given",
labels: map[string]string{
"key1": "value1",
},
want: "/key1:value1",
},
{
testname: "labels given",
name: "name1",
labels: map[string]string{
"key1": "value",
"key2": "value",
"key3": "value",
},
want: "name1/key1:value/key2:value/key3:value",
},
}
for _, tc := range testcases {
t.Run(tc.testname, func(t *testing.T) {
got := EventDefinitionID(tc.name, tc.labels)
assert.Equal(t, tc.want, got)
})
}
}