In-Memory Server #116

dave-tucker · 2021-05-17T13:43:35Z

Closes: #130

coveralls · 2021-05-17T13:49:17Z

Pull Request Test Coverage Report for Build 921917853

889 of 1184 (75.08%) changed or added relevant lines in 12 files are covered.
2 unchanged lines in 2 files lost coverage.
Overall coverage increased (+2.1%) to 72.373%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
ovsdb/mutation.go	5	7	71.43%
ovsdb/notation.go	57	63	90.48%
server/monitor.go	68	74	91.89%
ovsdb/condition.go	83	111	74.77%
server/server.go	171	219	78.08%
cache/cache.go	181	234	77.35%
server/database.go	300	452	66.37%

Files with Coverage Reduction	New Missed Lines	%
cache/cache.go	1	69.98%
ovsdb/condition.go	1	73.24%

Totals
Change from base Build 918907846:	2.1%
Covered Lines:	2727
Relevant Lines:	3768

💛 - Coveralls

dave-tucker · 2021-05-27T15:55:31Z

This Implementation

$time go run ./cmd/stress -verbose -ninserts 3000 -ovsdb tcp:localhost:56640
rootUUID is 305b9683-9b5f-4dae-9469-0cf32af1ef91
.......................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................


Summary:
        Insertions: 2999
        Deletions: 0
go run ./cmd/stress -verbose -ninserts 3000 -ovsdb tcp:localhost:56640  80.47s user 2.61s system 57% cpu 2:24.46 total

The Real McCoy

$time go run ./cmd/stress -verbose -ninserts 3000 -ovsdb tcp:localhost:49153
rootUUID is 7ae7e1c7-c127-4c94-a2e7-c63073a11ec6
........................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................


Summary:
        Insertions: 3000
        Deletions: 1
go run ./cmd/stress -verbose -ninserts 3000 -ovsdb tcp:localhost:49153  70.19s user 2.02s system 70% cpu 1:42.49 total

I'll take being ~42seconds slower than the original on an unoptimised implementation 🎉

dave-tucker · 2021-06-01T07:23:58Z

I was upset at the performance so I added the indexes to the cache and now it flies!
But I've also managed to break something fundamental....
When you run ./stress again, it attempts to delete only one bridge, yet there are 3000 in the cache.
So either inserts aren't working correctly, or select, or deletes or propagating them back to the cache.
Either way, I've pushed what I have for now, and either the issue will be found in code review... or I'll find it in the interim

dave-tucker · 2021-06-01T08:01:06Z

or I'll find it in the interim

not found yet, but I think when I changed Populate to use the new Create/Update/Delete functions I broke the locking.
I'm going to try having private funcs that are called with lock held, and public ones that grab the necessary lock for you.

dave-tucker · 2021-06-01T22:37:31Z

The cache locking was (probably) a red herring 🐟 however I still have my doubts. Either way:

real ovsdb
go run ./cmd/stress -verbose -inserts 3000 -ovsdb tcp::49160 74.05s user 1.72s system 67% cpu 1:52.76 total

this impl
go run ./cmd/stress -verbose -inserts 3000 -ovsdb tcp::56640 70.99s user 2.05s system 96% cpu 1:15.73 total

amorenoz · 2021-06-02T13:45:33Z

ovsdb/notation.go

+
+// interfaceToSetMapOrUUIDInterface takes a reflect.Value and converts it to
+// the correct OVSDB Notation (Set, Map, UUID) using reflection
+func interfaceToOVSDBNotationInterface(v reflect.Value) (interface{}, error) {


I wonder if we could somehow pick up the ColumnSchema the Mutation / Condition applies to and use bindings.go::OvsToNative et.al. I think it would be more reliable.

I initially thought about this but:

I couldn't figure out how to get the ColumnSchema to be available when marshalling to a struct from JSON

The alternative to ☝️ was to indirect access to the value using Value(schema *ColumnSchema) which would convert it, but I disregarded that but because you could use the wrong ColumnSchema AND you had to use NewCondition/Mutation directly and couldn't quickly create instances of the struct.

The other big issue with OvsToNative apart from needing the ColumnSchema was that it assumes sensible types, and doesn't support the "interesting" types we get from the back from the JSON Parser. E.g []interface{}{"set", []interface{}{[]interface{}{"named-uuid", "foo"}, []interface{}{"named-uuid", "bar"}}}

Ultimately we'd need to fold this code in to OvsToNative anyway so it's probably ok where it is for now.

I agree OvsToNative is cannot deal with the different kind of types per column but using the *ColumnSchema will save many problems. If there is any type mismatch it will be picked early on.

cache/cache.go

mapper/mapper.go

cache/cache.go

server/database.go

amorenoz · 2021-06-02T15:56:30Z

Only reviewed the first half, but posting some comments early on

cache/cache.go

server/database.go

This allows for a Condition to be evaluated based on two supplied values. Signed-off-by: Dave Tucker <[email protected]>

The unmarshal methods didn't use pointer receivers Go case statements don't fallthrough While scalar values are easily type asserted when marshalled to an interface{} the same is not true for map, set and uuid types. We need to use reflect to determine the OVSDB type before storing as an interface{} value. Signed-off-by: Dave Tucker <[email protected]>

This was incorrectly disallowing mutations where a single string would be inserted in to a []string... or int, real etc... Signed-off-by: Dave Tucker <[email protected]>

Signed-off-by: Dave Tucker <[email protected]>

And rename Set to Create. This allows for a caller to Add/Update/Delete cached rows with the appropriate locking. Signed-off-by: Dave Tucker <[email protected]>

To make this work, we add NamedUUID detection. A named UUID is considered a default value as it's yet to have been processed by the server. Signed-off-by: Dave Tucker <[email protected]>

This adds an initial in-memory server implementation. It implements enough of the spec to be used for testing. Signed-off-by: Dave Tucker <[email protected]>

This commit make a few changes to the cache. 1. Deprecates the TableCache.Set API, instead only allowing data to be provided in a call to NewTableCache 2. Disallow RowCache creation via API and instead init the cache based on the contents of the provided schema. This removes having calling code check for table existence 3. Rename Row.Set to Row.Create and provide an additionally check the type of the provided Model matches the schema 4. Cache row indexes for increased lookup speed Signed-off-by: Dave Tucker <[email protected]>

Since the TableCache already does this, we don't need to implement checks. Just check for errors on cache operations Signed-off-by: Dave Tucker <[email protected]>

Signed-off-by: Dave Tucker <[email protected]>

1. Rename ninserts to inserts 2. Adds support for multiple clients in series/parallel 3. Ensures that we wait for the event to be received in cache Signed-off-by: Dave Tucker <[email protected]>

This ensures minimal delay between sending a transact response and the update notification being sent via monitor Signed-off-by: Dave Tucker <[email protected]>

This removes the lock on TableCache and instead achieves the same result by locking all of the RowCaches. This is possible because the map in the TableCache doesn't change after creation, only the contents of the RowCache. Signed-off-by: Dave Tucker <[email protected]>

Signed-off-by: Dave Tucker <[email protected]>

dave-tucker · 2021-06-08T10:52:22Z

Ok I didn't break it, GitHub did: https://www.githubstatus.com/
Issues rendering static assets....

Signed-off-by: Dave Tucker <[email protected]>

dave-tucker · 2021-06-09T14:50:44Z

@amorenoz I think that's most of the comments addressed. I've opened 2 issues to track the other comments so they don't get lost. This is getting pretty big so I'd like to get it merged then we can start fixing issues in the main branch.

amorenoz · 2021-06-09T14:51:54Z

@amorenoz I think that's most of the comments addressed. I've opened 2 issues to track the other comments so they don't get lost. This is getting pretty big so I'd like to get it merged then we can start fixing issues in the main branch.

Agree. LGTM

dave-tucker changed the title ~~Server~~ [WIP] In-Memory Server May 17, 2021

dave-tucker force-pushed the server branch 2 times, most recently from 7ab7452 to d522b8f Compare May 21, 2021 08:18

dave-tucker mentioned this pull request May 24, 2021

Rename ORM to Mapper and move to dedicated package #118

Merged

dave-tucker force-pushed the server branch 7 times, most recently from beee0ed to 08498f8 Compare May 27, 2021 15:43

dave-tucker changed the title ~~[WIP] In-Memory Server~~ In-Memory Server May 27, 2021

dave-tucker marked this pull request as ready for review May 27, 2021 15:43

dave-tucker requested a review from amorenoz May 27, 2021 16:00

dave-tucker mentioned this pull request May 27, 2021

Add generated code using libovsdb ovn-kubernetes/ovn-kubernetes#2217

Merged

dave-tucker force-pushed the server branch from 9654e34 to 6901397 Compare June 1, 2021 07:59

dave-tucker force-pushed the server branch from 6901397 to dd88da9 Compare June 1, 2021 22:29

amorenoz reviewed Jun 2, 2021

View reviewed changes

amorenoz reviewed Jun 4, 2021

View reviewed changes

dave-tucker added 5 commits June 7, 2021 17:59

ovsdb: Add Evaluate function to Conditions

871e9cf

This allows for a Condition to be evaluated based on two supplied values. Signed-off-by: Dave Tucker <[email protected]>

ovsdb: Fix ValidateMutation

2b8dadd

This was incorrectly disallowing mutations where a single string would be inserted in to a []string... or int, real etc... Signed-off-by: Dave Tucker <[email protected]>

ovsdb: Fix typo in MutationSubtract

2e56979

Signed-off-by: Dave Tucker <[email protected]>

ovsdb: Add String function for Condition

068eb2b

Signed-off-by: Dave Tucker <[email protected]>

dave-tucker added 8 commits June 7, 2021 17:59

cache: Add Row Create, Update, Delete operations

bafcd54

And rename Set to Create. This allows for a caller to Add/Update/Delete cached rows with the appropriate locking. Signed-off-by: Dave Tucker <[email protected]>

mapper: Add _uuid when calling NewRow

bbcbf5a

To make this work, we add NamedUUID detection. A named UUID is considered a default value as it's yet to have been processed by the server. Signed-off-by: Dave Tucker <[email protected]>

server: Initial Commit

c51ffcb

This adds an initial in-memory server implementation. It implements enough of the spec to be used for testing. Signed-off-by: Dave Tucker <[email protected]>

server: Let cache handle index checking

678beab

Since the TableCache already does this, we don't need to implement checks. Just check for errors on cache operations Signed-off-by: Dave Tucker <[email protected]>

server: Use index-based lookups for conditions

e3aaeb1

Signed-off-by: Dave Tucker <[email protected]>

stress: Add support for multiple clients

ef78bf2

1. Rename ninserts to inserts 2. Adds support for multiple clients in series/parallel 3. Ensures that we wait for the event to be received in cache Signed-off-by: Dave Tucker <[email protected]>

server: Don't use buffered channels

927c073

This ensures minimal delay between sending a transact response and the update notification being sent via monitor Signed-off-by: Dave Tucker <[email protected]>

dave-tucker force-pushed the server branch from aa08686 to 927c073 Compare June 8, 2021 08:44

dave-tucker added this to the 0.4.0 milestone Jun 8, 2021

dave-tucker added 2 commits June 8, 2021 11:07

server: Lock cache for transaction processing

c8481d3

Signed-off-by: Dave Tucker <[email protected]>

dave-tucker force-pushed the server branch from 2afc57f to 3d9c141 Compare June 9, 2021 13:44

This was referenced Jun 9, 2021

Remove interfaceToOVSDBNotationInterface #149

Closed

Optimise index checking in API.Get() #150

Closed

server: Ensure mutate operations write back to db

0d7102e

Signed-off-by: Dave Tucker <[email protected]>

dave-tucker force-pushed the server branch from 3d9c141 to 0d7102e Compare June 9, 2021 13:49

dave-tucker merged commit 93508a6 into ovn-org:main Jun 9, 2021

dave-tucker mentioned this pull request Jun 10, 2021

stress: Support multiple clients #134

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

In-Memory Server #116

In-Memory Server #116

dave-tucker commented May 17, 2021 •

edited

Loading

coveralls commented May 17, 2021 •

edited

Loading

dave-tucker commented May 27, 2021

dave-tucker commented Jun 1, 2021

dave-tucker commented Jun 1, 2021 •

edited

Loading

dave-tucker commented Jun 1, 2021

amorenoz Jun 2, 2021

dave-tucker Jun 7, 2021

amorenoz Jun 8, 2021

amorenoz commented Jun 2, 2021

dave-tucker commented Jun 8, 2021

dave-tucker commented Jun 9, 2021

amorenoz commented Jun 9, 2021

In-Memory Server #116

In-Memory Server #116

Conversation

dave-tucker commented May 17, 2021 • edited Loading

coveralls commented May 17, 2021 • edited Loading

Pull Request Test Coverage Report for Build 921917853

💛 - Coveralls

dave-tucker commented May 27, 2021

dave-tucker commented Jun 1, 2021

dave-tucker commented Jun 1, 2021 • edited Loading

dave-tucker commented Jun 1, 2021

amorenoz Jun 2, 2021

Choose a reason for hiding this comment

dave-tucker Jun 7, 2021

Choose a reason for hiding this comment

amorenoz Jun 8, 2021

Choose a reason for hiding this comment

amorenoz commented Jun 2, 2021

dave-tucker commented Jun 8, 2021

dave-tucker commented Jun 9, 2021

amorenoz commented Jun 9, 2021

dave-tucker commented May 17, 2021 •

edited

Loading

coveralls commented May 17, 2021 •

edited

Loading

dave-tucker commented Jun 1, 2021 •

edited

Loading