Implement a Cache For Monitor Updates

[dave-tucker]

(See Original Comments on eBay#35)

This makes a few significant API changes in order to provide a cache for monitor updates.
Not only does it improve the API, but it also makes it possible to implement a Get (from cache) in the ORM API proposed in #78

The API changes are as follows:

1. Restrict the client to have only one DatabaseSchema. This mirrors how ovsdb-server is run in the wild (even though techincally a server can support multiple schemas). This has the advantage of removing the database argument from the Transact, Monitor... APIs
2. Replace Disconnected(*OvsClient) with Disconnected().The client needs to know about a handler to register it, but the handler shouldn't need to know about the client. Even if it did, that could be achieved through channels. Either way, the Go compiler didn't seem to like passing that client around once I'd added the cache feature.

The feature itself can be seen in operation in the play_with_ovs example.

In short, the OvsClient exposes a Cache and associated handler to ensure the cache is updated.
As such, a user can call Monitor without a handler defined and the initial state and all updates will populate in to the cache.

The cache API is very simple:

# Get a list of all tables
client.Cache.Tables()

# Get a table
client.Cache.Table("Open_vSwitch")

# Get a list of all row uuids in a table
client.Cache.Tables("Open_vSwitch").Rows()

# Get a row in a table by UUID
client.Cache.Tables("Open_vSwitch").Row("uuid")

In addition, a user may choose to register a calback so they can perform an action when a cache item is updated:

ovs.Cache.AddEventHandler(&libovsdb.EventHandlerFuncs{
AddFunc: func(table string, row libovsdb.Row) {
	if table == bridgeTable {
		update <- row
	}
},
})

Misc changes included in this PR to make sure CI is green:

1. Fix linting/formatting of encoding_test.go
2. Fix play_with_ovs which was a case of s/"ovsTable"/ovsTable/g

/cc @amorenoz @hzhou8

Closes: #85

[coveralls]

Coverage increased (+1.06%) to 77.99% when pulling e554ba1 on dave-tucker:cache into 177adf8 on socketplane:main.

[dave-tucker]

@vtolstov to your comment in #78

populate is getting called for every update notification which can happen a lot. we also claim a write lock on the cache when we are doing populate. in summary, we need that code to execute as fast as possible to avoid blocking reads, but the eventhandlers are "unknown" - could be fast running code, could be slow running code.

calling the handlers synchronously doesn't seem like a good idea because if a library user writes a handler that is slow, then all cache reads will be blocked until all handlers are done processing events.

calling the handler async gets past the locking issues, but as you mentioned, having unbounded numbers of goroutines isn't great either.

one other thing i can think of is to create buffered channels for Add/Update/Delete operations and have a goroutine that reads those channels and dispatches events synchronously. worst case, the channel buffer fills and we block on sending updates to the channel until there is room. to avoid blocking in that case, we could use a ring buffer for the channels, and then we'd drop events when the channels are full.

i'm curious as to what behaviour would be preferred?

[dave-tucker]

I pushed an update that changes the locks to RWMutex as suggested by @vtolstov in #78

[vtolstov]

@vtolstov to your comment in #78

populate is getting called for every update notification which can happen a lot. we also claim a write lock on the cache when we are doing populate. in summary, we need that code to execute as fast as possible to avoid blocking reads, but the eventhandlers are "unknown" - could be fast running code, could be slow running code.

calling the handlers synchronously doesn't seem like a good idea because if a library user writes a handler that is slow, then all cache reads will be blocked until all handlers are done processing events.

calling the handler async gets past the locking issues, but as you mentioned, having unbounded numbers of goroutines isn't great either.

one other thing i can think of is to create buffered channels for Add/Update/Delete operations and have a goroutine that reads those channels and dispatches events synchronously. worst case, the channel buffer fills and we block on sending updates to the channel until there is room. to avoid blocking in that case, we could use a ring buffer for the channels, and then we'd drop events when the channels are full.

i'm curious as to what behaviour would be preferred?

I'm prefer something like worker pool of goroutines. So user of library can decide how much resources can be used and how fast updates processed. Another thing that may be we can have some deadline for event processing and if consumer is slow - log some warning and drop some events.
I'm keep in my mind something like github.com/panjf2000/ants so we can wait for free the goroutine in pool or get error that all goroutines in pool are busy and event will be dropped

[dave-tucker]

@vtolstov

I'm prefer something like worker pool of goroutines. So user of library can decide how much resources can be used and how fast updates processed.

Ack. ants looks really cool. thanks for sharing!

Kubernetes has a similar problem in the cache in client-go, and I think perhaps we could follow the same pattern.

1. We state emphatically in the documentation that the cache is not designed to service slow handlers
2. If you really need to do something slow in a handler function, then instead write a small handler to push events in to a queue in your code, and service that queue with as many goroutines as you want

This way, we can keep the libovsdb footprint small - one additional goroutine to dispatch events to listeners - and give the user complete control of the thread pool for event handling.

Now I just need to cobble together a quick ring buffer and we should be good to go 😉

[vtolstov]

@vtolstov

I'm prefer something like worker pool of goroutines. So user of library can decide how much resources can be used and how fast updates processed.

Ack. ants looks really cool. thanks for sharing!

Kubernetes has a similar problem in the cache in client-go, and I think perhaps we could follow the same pattern.

1. We state emphatically in the documentation that the cache is not designed to service slow handlers
2. If you really need to do something slow in a handler function, then instead write a small handler to push events in to a queue in your code, and service that queue with as many goroutines as you want

This way, we can keep the libovsdb footprint small - one additional goroutine to dispatch events to listeners - and give the user complete control of the thread pool for event handling.

Now I just need to cobble together a quick ring buffer and we should be good to go

I think that this will be good.

[dave-tucker]

@vtolstov done. the ring buffer isn't very well optimised, but works well enough for the single writer, multiple reader we have currently:

go test -bench=. -run=XXX ./internal/buffer                                                                                                                              ✔  
goos: linux
goarch: amd64
pkg: github.com/socketplane/libovsdb/internal/buffer
cpu: Intel(R) Core(TM) i7-8650U CPU @ 1.90GHz
BenchmarkWrite-8        96289977                12.37 ns/op
BenchmarkRead-8         551214096                1.920 ns/op
PASS
ok      github.com/socketplane/libovsdb/internal/buffer 3.472s

I've used RWMutex on the cache as suggested, but based on our usage pattern we might actually be better off using sync.Map - it's something worth experimenting with.

@hzhou8 @amorenoz I think this is pretty ready for review now if you have a spare minute.

[coveralls]

Pull Request Test Coverage Report for Build 792357558

• 151 of 182 (82.97%) changed or added relevant lines in 2 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+1.0%) to 77.635%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
client.go	32	39	82.05%
cache.go	119	143	83.22%

Totals
Change from base Build 790941841:	1.0%
Covered Lines:	788
Relevant Lines:	1015

---

💛 - Coveralls

[dave-tucker]

@amorenoz comments addressed. Please take another look.

Thanks to your comments I made some improvements to the ring buffer impl and added some additional docs.
Read performance increased 🎉 and so did Write performance, until it was spoiled by Go's race detector.
I've instead had to use atomic.Value for the backing array 😢 performance is not as good for writes now, but it's good enough,

[dave-tucker]

@amorenoz suggested that we use channels instead.
I've implemented it and (as we guessed) it simplifies the implementation and performs a little better.

The only drawback is that when the buffer is full, we drop events vs in the ring buffer we would overwrite older events.

In both cases you're losing events, and it's up to the library user to make sure that doesn't happen.
However with the ring buffer you lose old events, whereas with the channels you lose new events.

As neither has a clear advantage over the other I suggest we stick with the simple implementation for now, and we can re-visit the buffer again if we need it in future.

[dave-tucker]

# CHANNELS
$ time go run ./example/stress/stress.go -ovsdb tcp::49154 -ninserts 100                                                                                                 Summary:
        Insertions: 202
        Deletions: 100
go run ./example/stress/stress.go -ovsdb tcp::49154 -ninserts 100  0.68s user 0.15s system 129% cpu 0.645 total
# BUFFER
$ time go run ./example/stress/stress.go -ovsdb tcp::49154 -ninserts 100                                                                                                  Summary:
        Insertions: 188
        Deletions: 90
go run ./example/stress/stress.go -ovsdb tcp::49154 -ninserts 100  1.27s user 0.18s system 177% cpu 0.822 total

[amorenoz]

LGTM