WIP: Cached KV-based store #384

levb · 2022-11-15T14:39:31Z

Summary

A cluster aware, plugin-KV based list store for things like App's and Subscription's.

added an initial implementation, based on pluginapi.cluster.Mutex + local tests
moved the AppStore to use it

Ticket Link

TODO - several tickets

TODO

Add Clean() to the CachedStore API to use in debug clean
Document the design
Test command that can be executed on bigger clusters
Implement data migration
Restore MessageHasBeenPosted, separate PR?
Alternate, single-writer implementation based OnClusterLeaderChanged + benchmark (ideally in a cluster)
Manually test with cluster
Move the subscription store

hanzei · 2022-11-15T15:01:16Z

@levb Before I jump into the code: Is there a rough overview of the design of the cached store?

levb · 2022-11-15T15:14:23Z

@hanzei not yet, still WIP

in brief - for a given CachedStore (e.g. apps, subscriptions) a single index key in the KV holds a list of (key, data-SHA) and the individual items are stored as keyed, in the prefixed namespace. The in-memory index/cache is a sync.Map. KV writes are from whatever host received the request, protected for exclusivity by cluster.Mutex. Plugin cluster messages are sent to all hosts to notify them of new cache entries (PUT, DELETE).

The upcoming single-writer version will send the same cluster messages, but the writes will be serialized on the master, with no need for clusterMutex.Lock()

codecov-commenter · 2022-11-15T15:14:59Z

Codecov Report

Base: 21.06% // Head: 23.73% // Increases project coverage by +2.66% 🎉

Coverage data is based on head (b285fd3) compared to base (d6253ea).
Patch coverage: 33.23% of modified lines in pull request are covered.

Additional details and impacted files

@@                         Coverage Diff                          @@
##           lev-MM-48793-bot-subs-simplified     #384      +/-   ##
====================================================================
+ Coverage                             21.06%   23.73%   +2.66%     
====================================================================
  Files                                    81       85       +4     
  Lines                                  5473     5579     +106     
====================================================================
+ Hits                                   1153     1324     +171     
+ Misses                                 4181     4091      -90     
- Partials                                139      164      +25

Impacted Files	Coverage Δ
apps/app.go	`0.00% <0.00%> (ø)`
apps/manifest.go	`91.17% <0.00%> (-2.77%)`	⬇️
server/config/service.go	`0.00% <0.00%> (ø)`
server/proxy/bindings.go	`33.96% <0.00%> (ø)`
server/proxy/enable.go	`4.41% <0.00%> (ø)`
server/proxy/install.go	`0.00% <0.00%> (ø)`
server/proxy/invoke_oauth2.go	`0.00% <0.00%> (ø)`
server/proxy/notify.go	`0.00% <0.00%> (ø)`
server/proxy/service.go	`6.89% <0.00%> (-0.09%)`	⬇️
server/proxy/synchronize.go	`0.00% <0.00%> (ø)`
... and 20 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

hanzei · 2022-11-15T15:30:35Z

The upcoming single-writer version will send the same cluster messages, but the writes will be serialized on the master, with no need for clusterMutex.Lock()

What are the advantages of that approach vs the current usterMutex.Lock() one?

levb · 2022-11-15T15:39:05Z

What are the advantages of that approach vs the current usterMutex.Lock() one?

Hypothetically:

Fewer DB queries
Better reliability (TL;DR - cluster leader is "server-native", trustworthy, and cheap. cluster.Mutex - homegrown. I specifically do not understand how cluster.Mutex is reset if the locking plugin terminates). The argument about distributing the load - I do not 100% agree with. Specifically, 1/5 consolidating DB writes to a single host may actually achieve better performance; definitely not sub-par unless other overhead (CPU, memory, IO) requires distributed writers.

1/5 I'd much prefer to submit both implementations, with tests to demonstrate the differences.

hanzei

A few comments. I will post my high-level ones in Mattermost for a broader discussion.

server/plugin.go

hanzei · 2022-11-15T14:56:03Z

server/store/cached_store.go

+	Data []IndexEntry[T]
+}
+
+type Index[T any] map[string]*IndexEntry[T]


Does Index need to be part of the API?

Well, there's no real external API since these are wrapped in AppStore and the likes, but I am making some types public with the intent of go-documenting them.

hanzei · 2022-11-15T14:56:59Z

server/store/cached_store.go

+
+func MakeCachedStore[T any](name string, api plugin.API, mmapi *pluginapi.Client, logger utils.Logger) (*CachedStore[T], error) {
+	s := &CachedStore[T]{
+		name:   name,


Can namebe an empty string?

👍 will resolve. In theory, it could - in practice it should not be.

hanzei · 2022-11-15T15:23:49Z

server/store/cached_store.go

+type cachedStoreEvent[T any] struct {
+	Key       string    `json:"key"`
+	Method    string    `json:"method"`
+	SentAt    time.Time `json:"sent_at"`


What is SentAt used for?

Nothing ATM, debugging and metrics later.

hanzei · 2022-11-15T15:42:33Z

server/store/cached_store.go

+	prevIndex, err := s.syncFromKV(true)
+	if err != nil {
+		return err
+	}


Why is the sync with the KV store needed here?

It just verifies that the in-memory cache is indeed the same as the KV state, based on the index hash; will not reload unless the hashes differ (which they never should)

Co-authored-by: Ben Schumacher <[email protected]>

… into lev-hashstore

…in-apps into lev-hashstore

… into lev-hashstore

javaguirre

I have some small details I want to discuss, but I have to spend a bit more time reviewing the code.

server/config/service.go

javaguirre · 2022-11-29T09:42:29Z

server/config/service.go

+		conf.AllowHTTPApps = true
+	}
+
+	conf.AWSAccessKey = os.Getenv(upaws.AccessEnvVar)


Why not separating code that has different responsibilities? I understand it may be because It's not used anywhere else. It could help for legibility (AWS Cloud config (142-171?), Proxy setup (103-122).

javaguirre · 2022-11-29T10:29:22Z

server/store/cached_store.go

+	if cachedIndex.hash() != prevPersistedIndex.hash() {
+		add, change, remove := cachedIndex.compareTo(prevPersistedIndex)
+		if logWarnings {
+			s.logger.Warnf("stale cache for %s, rebuilding. extra keys: %v, missing keys:%v, different values for keys:%v", s.name, remove, add, change)


Can't we log the warning and control from the outside if we want to see that level of logging and remove logWarnings?

Co-authored-by: Ben Schumacher <[email protected]>

… into lev-hashstore

…fied' into lev-hashstore

levb · 2023-02-01T14:33:46Z

server/config/service.go

@@ -52,7 +52,7 @@ type service struct {
 	mattermostConfig *model.Config
 }

-func MakeService(mm *pluginapi.Client, pliginManifest model.Manifest, botUserID string, telemetry *telemetry.Telemetry, i18nBundle *i18n.Bundle, log utils.Logger) (Service, error) {


@hanzei I took out your logging fix because I think the logging changes in this PR already handle the invalid config use case. I tested with a missing SiteURL, and get the initial logging (ensureBot), followed by the expected activate error (invalid config) logged at INFO, then exit.

…ost/mattermost-plugin-apps into lev-hashstore

… into lev-better-log

…gin-apps into lev-hashstore

levb · 2023-02-19T14:19:54Z

This PR will be closed, re-submitted as a series of PRs: #457, #458, #459

Lev Brouk added 4 commits November 14, 2022 11:58

wip: added cachedstore stats to /apps debug kv info

b4f50d7

wip more

d520d1b

Added cluster event PUT/DELETE handling

4f54a69

adjusted the uninstall test

08c2b06

levb added 2: Dev Review Requires review by a core committer Do Not Merge Should not be merged until this label is removed Work In Progress Not yet ready for review labels Nov 15, 2022

levb added this to the v1.3.0 milestone Nov 15, 2022

levb requested review from javaguirre and hanzei November 15, 2022 14:39

go.mod + lint

af72ce3

Merge branch 'master' into lev-hashstore

9cbae6f

hanzei requested changes Nov 15, 2022

View reviewed changes

levb and others added 10 commits November 15, 2022 08:35

Update server/plugin.go

1cbbbd3

Co-authored-by: Ben Schumacher <[email protected]>

wrapped errors

6c171cd

Merge branch 'master' of github.com:mattermost/mattermost-plugin-apps…

79fe858

… into lev-hashstore

Merge branch 'lev-hashstore' of github.com:mattermost/mattermost-plug…

748cd10

…in-apps into lev-hashstore

Merge branch 'master' of github.com:mattermost/mattermost-plugin-apps…

ff1a101

… into lev-hashstore

works with existing channel

ffa14cb

Fixed expand for /apps debug logs command

cfaccc4

i18n

00bbc48

Merge branch 'master' into lev-cc-logger

96ae842

Merge branch 'lev-cc-logger' into lev-hashstore

e9fa4bb

javaguirre suggested changes Nov 29, 2022

View reviewed changes

Update server/proxy/synchronize.go

09c6ed9

Co-authored-by: Ben Schumacher <[email protected]>

Lev Brouk added 4 commits January 28, 2023 14:07

lint and updated server to 7.7.1

e0345ff

go 1.18

3db7a12

un-skip flaky tests

d0f5296

Merge branch 'lev-MM-48793-bot-subs-simplified' into lev-hashstore

b87cf56

levb changed the base branch from master to lev-MM-48793-bot-subs-simplified January 31, 2023 01:45

Lev Brouk added 4 commits January 30, 2023 17:46

style

6c32672

style

c830a17

Merge branch 'master' of github.com:mattermost/mattermost-plugin-apps…

5be02ef

… into lev-hashstore

Merge remote-tracking branch 'mattermost/lev-MM-48793-bot-subs-simpli…

67eb268

…fied' into lev-hashstore

levb commented Feb 1, 2023

View reviewed changes

Lev Brouk added 14 commits February 1, 2023 06:40

unskipped previously flaky tests

b4bbb58

refactored mutex vs single-writer

b285fd3

Merge branch 'lev-MM-48793-bot-subs-simplified' of github.com:matterm…

b48eac1

…ost/mattermost-plugin-apps into lev-hashstore

rename

f41b92a

Merge branch 'lev-MM-48793-bot-subs-simplified' of github.com:matterm…

0d0ff62

…ost/mattermost-plugin-apps into lev-hashstore

updated to the custom server branch, onClusterLeaderChange

85ffec3

Added initial implementation of SingleWriterCachedStore

35cc55a

manually tested on a miniHA

10f7786

Merge branch 'lev-MM-48793-bot-subs-simplified' of github.com:matterm…

96ec68b

…ost/mattermost-plugin-apps into lev-hashstore

Better logging of bindings and calls

8e1004c

Merge branch 'master' of github.com:mattermost/mattermost-plugin-apps…

b76b401

… into lev-better-log

removed a merge redundancy

3173ba5

Merge branch 'lev-better-log' of github.com:mattermost/mattermost-plu…

9e9b271

…gin-apps into lev-hashstore

fix Put to properly differentiate the leader

76a2d2d

levb closed this Feb 19, 2023

mattermost-build removed Work In Progress Not yet ready for review Do Not Merge Should not be merged until this label is removed labels Feb 19, 2023

hanzei added Invalid This doesn't seem right and removed 2: Dev Review Requires review by a core committer labels Feb 23, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: Cached KV-based store #384

WIP: Cached KV-based store #384

levb commented Nov 15, 2022 •

edited

Loading

hanzei commented Nov 15, 2022

levb commented Nov 15, 2022

codecov-commenter commented Nov 15, 2022 •

edited

Loading

hanzei commented Nov 15, 2022

levb commented Nov 15, 2022

hanzei left a comment

hanzei Nov 15, 2022

levb Nov 15, 2022

hanzei Nov 15, 2022

levb Nov 15, 2022

hanzei Nov 15, 2022

levb Nov 15, 2022

hanzei Nov 15, 2022

levb Nov 15, 2022

javaguirre left a comment

javaguirre Nov 29, 2022

javaguirre Nov 29, 2022

levb Feb 1, 2023

levb Feb 1, 2023

levb commented Feb 19, 2023

WIP: Cached KV-based store #384

WIP: Cached KV-based store #384

Conversation

levb commented Nov 15, 2022 • edited Loading

Summary

Ticket Link

TODO

hanzei commented Nov 15, 2022

levb commented Nov 15, 2022

codecov-commenter commented Nov 15, 2022 • edited Loading

Codecov Report

hanzei commented Nov 15, 2022

levb commented Nov 15, 2022

hanzei left a 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

javaguirre left a 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

levb commented Feb 19, 2023

levb commented Nov 15, 2022 •

edited

Loading

codecov-commenter commented Nov 15, 2022 •

edited

Loading