configure dispatch for tests #438

ecordell · 2022-03-01T21:15:33Z

This adds a test that can fire a large set of requests at the same time to see how spicedb handles the load (likely should make it a benchmark)

This sets up spicedb with a custom resolver that will return bufconn connections to the dispatch api, meaning that we can easily test full cluster behavior in "unit" tests.

This also addresses a long-standing comment in the consistency tests (to run with dispatch) and runs the dispatch tests in parallel.

ecordell · 2022-03-04T17:48:53Z

internal/datastore/common/revisions.go

@@ -20,6 +21,8 @@ type RemoteNowFunction func(context.Context) (datastore.Revision, error)
 // RemoteClockRevisions handles revision calculation for datastores that provide
 // their own clocks.
 type RemoteClockRevisions struct {
+	sync.RWMutex


added a mutex here to protect lastQuantizedRevision access from multiple goroutines

Could we use a CAS?

I pushed an implementation that uses atomic Load/Store, reasoning that we don't really care what the value was before the new one, just that the new one is set (so we don't need the C in CAS). We're singlelfight fetching the new value so there will only ever be one goroutine updating the value at a time already.

I could see an argument for a full CAS from a sanity checking / debug logging perspective, but lmk what you think.

ecordell · 2022-03-04T17:49:31Z

internal/dispatch/combined/combined.go

@@ -125,9 +125,12 @@ func NewDispatcher(nsm namespace.Manager, options ...Option) (dispatch.Dispatche
 // NewClusterDispatcher takes a caching redispatcher (such as one created by
 // NewDispatcher) and returns a cluster dispatcher suitable for use as the
 // dispatcher for the dispatch grpc server.
-func NewClusterDispatcher(cachingRedispatch dispatch.Dispatcher, nsm namespace.Manager, config *ristretto.Config) (dispatch.Dispatcher, error) {
+func NewClusterDispatcher(cachingRedispatch dispatch.Dispatcher, nsm namespace.Manager, prometheusSubsystem string, config *ristretto.Config) (dispatch.Dispatcher, error) {


made the subsystem configurable - these are registered process-globally by prometheus, so if we spin up a bunch of servers in parallel for tests they need to have unique subsystems

Maybe we should change to a ClusterDispatcher config struct now?

ecordell · 2022-03-04T17:51:23Z

internal/services/dispatch/v1/acl.go

 	dispatchv1 "github.com/authzed/spicedb/pkg/proto/dispatch/v1"
 )

 type dispatchServer struct {
 	dispatchv1.UnimplementedDispatchServiceServer
+	shared.WithServiceSpecificInterceptors


We missed dispatch when switching to the service-specific middleware

ecordell · 2022-03-04T17:52:28Z

internal/testserver/cluster.go

+	balancer.Register(hashbalancer.NewConsistentHashringBuilder(xxhash.Sum64, 20, 1))
+
+	// register a manual resolver that we can feed addresses during tests
+	resolver.Register(testResolverbuilder)


resolver registration is not thread safe, so we register one resolver that handles many prefixes instead of many resolvers that each handles one

Add a comment?

there's lots of comments on the object that implements it?

added more detail here though

josephschorr · 2022-03-04T21:06:28Z

internal/datastore/common/revisions.go


 	lastQuantizedRevision, err, _ := rcr.updateGroup.Do("", func() (interface{}, error) {
+		rcr.Lock()
+		defer rcr.Unlock()
 		log.Debug().Time("now", localNow).Time("valid", rcr.revisionValidThrough).Msg("computing new revision")

 		nowHLC, err := rcr.NowFunc(ctx)


Does this mean the DB access is under the lock? If so, we can't have that occur.

refactored so that this is not an issue anymore

josephschorr · 2022-03-04T21:06:35Z

internal/datastore/common/revisions.go

@@ -20,6 +21,8 @@ type RemoteNowFunction func(context.Context) (datastore.Revision, error)
 // RemoteClockRevisions handles revision calculation for datastores that provide
 // their own clocks.
 type RemoteClockRevisions struct {
+	sync.RWMutex


Could we use a CAS?

josephschorr · 2022-03-04T21:06:58Z

internal/dispatch/combined/combined.go

@@ -125,9 +125,12 @@ func NewDispatcher(nsm namespace.Manager, options ...Option) (dispatch.Dispatche
 // NewClusterDispatcher takes a caching redispatcher (such as one created by
 // NewDispatcher) and returns a cluster dispatcher suitable for use as the
 // dispatcher for the dispatch grpc server.
-func NewClusterDispatcher(cachingRedispatch dispatch.Dispatcher, nsm namespace.Manager, config *ristretto.Config) (dispatch.Dispatcher, error) {
+func NewClusterDispatcher(cachingRedispatch dispatch.Dispatcher, nsm namespace.Manager, prometheusSubsystem string, config *ristretto.Config) (dispatch.Dispatcher, error) {


Maybe we should change to a ClusterDispatcher config struct now?

josephschorr · 2022-03-04T21:07:14Z

internal/logging/interceptor.go

+// Copyright (c) The go-grpc-middleware Authors.
+// Licensed under the Apache License 2.0.
+
+// Package logging is a copy of https://github.com/grpc-ecosystem/go-grpc-middleware/tree/v2/providers/zerolog


Did you file an issue with them as well?

grpc-ecosystem/go-grpc-middleware#487

Also removed the mutex; made copies of the logger instead

josephschorr · 2022-03-04T21:08:36Z

internal/testserver/cluster.go

+	balancer.Register(hashbalancer.NewConsistentHashringBuilder(xxhash.Sum64, 20, 1))
+
+	// register a manual resolver that we can feed addresses during tests
+	resolver.Register(testResolverbuilder)


Add a comment?

josephschorr · 2022-03-04T21:09:04Z

internal/testserver/cluster.go

+
+// TestClusterWithDispatch creates a cluster with `size` nodes
+// The cluster has a real dispatch stack that uses bufconn grpc connections
+func TestClusterWithDispatch(t testing.TB, size int, ds datastore.Datastore) ([]*grpc.ClientConn, func()) {


Use a uint for size

josephschorr · 2022-03-04T21:09:15Z

internal/testserver/cluster.go

+	addresses := make([]resolver.Address, 0, size)
+	for i := 0; i < size; i++ {
+		addresses = append(addresses, resolver.Address{
+			Addr:       prefix + "_" + strconv.Itoa(i),


fmt.Stringf

josephschorr · 2022-03-04T21:10:13Z

pkg/cmd/server/server.go

@@ -193,6 +203,7 @@ func (c *Config) Complete() (RunnableServer, error) {
 				v1SchemaServiceOption,
 			)
 		},
+		// grpc.NumStreamWorkers(uint32(runtime.NumCPU())),


josephschorr · 2022-03-07T17:03:04Z

internal/datastore/spanner/spanner_test.go

@@ -1,3 +1,4 @@
+// go:build ci


How is this different from the line below it?

they're the same, but 1.17 go fmt enforces both lines during the transition from +build to go:build

https://go.googlesource.com/proposal/+/master/design/draft-gobuild.md

josephschorr · 2022-03-07T17:03:31Z

internal/datastore/common/revisions.go

@@ -2,6 +2,7 @@ package common



Can we add a test for this module that is run with the race detector in CI?

josephschorr · 2022-03-07T17:03:42Z

internal/dispatch/combined/combined.go

@@ -125,9 +125,12 @@ func NewDispatcher(nsm namespace.Manager, options ...Option) (dispatch.Dispatche
 // NewClusterDispatcher takes a caching redispatcher (such as one created by
 // NewDispatcher) and returns a cluster dispatcher suitable for use as the
 // dispatcher for the dispatch grpc server.
-func NewClusterDispatcher(cachingRedispatch dispatch.Dispatcher, nsm namespace.Manager, config *ristretto.Config) (dispatch.Dispatcher, error) {
+func NewClusterDispatcher(cachingRedispatch dispatch.Dispatcher, nsm namespace.Manager, prometheusSubsystem string, config *ristretto.Config) (dispatch.Dispatcher, error) {


josephschorr · 2022-03-11T18:37:05Z

internal/services/perf_test.go

+
+func TestBurst(t *testing.T) {
+	blacklist := []string{
+		"spanner", // spanner emulator doesn't support parallel transactions


Can we have the ID be pulled from the spanner package or would that create a weird dependency?

josephschorr · 2022-03-11T18:37:24Z

internal/testserver/cluster.go

+}
+
+// SafeManualResolverBuilder is a resolver builder
+type SafeManualResolverBuilder struct {


what makes it safe and manual?

it's similar to the manual resolver builder: https://pkg.go.dev/google.golang.org/grpc@v1.45.0/resolver/manual

but is thread safe

Add that to the comment?

josephschorr · 2022-03-11T18:38:05Z

internal/testserver/datastore/datastore.go

+func NewTestDatastoreBuilder(t testing.TB, engine string) TestDatastoreBuilder {
+	switch engine {
+	case "memory":
+		return TestDatastoreBuilderFunc(func(t testing.TB, initFunc InitFunc) datastore.Datastore {


why is memory special?

there's no external resource (docker image) that the "builder" needs to track, unlike with all of the other datastores.

josephschorr · 2022-03-14T16:22:34Z

internal/dispatch/cluster/cluster.go

+// NewClusterDispatcher takes a caching redispatcher (such as one created by
+// combined.NewDispatcher) and returns a cluster dispatcher suitable for use as
+// the dispatcher for the dispatch grpc server.
+func NewClusterDispatcher(cachingRedispatch dispatch.Dispatcher, nsm namespace.Manager, options ...Option) (dispatch.Dispatcher, error) {


Should the internal dispatcher be called cachingDispatcher? It doesn't have to be cached, right?

josephschorr · 2022-03-14T16:24:26Z

internal/testserver/cluster.go

+}
+
+// SafeManualResolverBuilder is a resolver builder
+type SafeManualResolverBuilder struct {


Add that to the comment?

josephschorr · 2022-03-14T16:24:50Z

internal/testserver/cluster.go

+}
+
+func (b *SafeManualResolverBuilder) Build(target resolver.Target, cc resolver.ClientConn, opts resolver.BuildOptions) (resolver.Resolver, error) {
+	if target.URL.Scheme != "test" {


Put into a const?

this adds a test that can fire a large set of requests at the same time to see how spicedb handles the load it also adds a way to start up a spicedb cluster configured with dispatch over bufconn

this adds an abstraction that makes it easy to spin up a container for a backing datastore and generate test datastores for it

josephschorr · 2022-03-14T19:08:49Z

internal/dispatch/cluster/cluster.go

+	}
+}
+
+// NewClusterDispatcher takes a caching redispatcher (such as one created by


Fix comment

josephschorr

LGTM

github-actions bot added area/CLI Affects the command line area/tooling Affects the dev or user toolchain (e.g. tests, ci, build tools) labels Mar 1, 2022

ecordell force-pushed the perftest branch from 44458b9 to cf78e81 Compare March 3, 2022 15:35

github-actions bot added the area/dispatch Affects dispatching of requests label Mar 3, 2022

ecordell force-pushed the perftest branch from cf78e81 to 8fd016a Compare March 3, 2022 15:44

ecordell marked this pull request as ready for review March 3, 2022 15:48

ecordell force-pushed the perftest branch from 8fd016a to 29aefc3 Compare March 4, 2022 15:56

github-actions bot added the area/datastore Affects the storage system label Mar 4, 2022

ecordell force-pushed the perftest branch 9 times, most recently from decc06f to 7b44cfa Compare March 4, 2022 20:53

ecordell commented Mar 4, 2022

View reviewed changes

ecordell requested a review from josephschorr March 4, 2022 21:04

josephschorr reviewed Mar 4, 2022

View reviewed changes

ecordell force-pushed the perftest branch 8 times, most recently from c23bf55 to 1d322c7 Compare March 7, 2022 15:15

ecordell requested a review from josephschorr March 7, 2022 15:32

josephschorr reviewed Mar 7, 2022

View reviewed changes

ecordell force-pushed the perftest branch 12 times, most recently from 729b15c to c3272b4 Compare March 11, 2022 16:52

josephschorr reviewed Mar 11, 2022

View reviewed changes

ecordell force-pushed the perftest branch 3 times, most recently from de4b12e to e50dd92 Compare March 11, 2022 20:55

ecordell enabled auto-merge March 14, 2022 13:04

ecordell requested a review from josephschorr March 14, 2022 13:16

josephschorr reviewed Mar 14, 2022

View reviewed changes

ecordell added 4 commits March 14, 2022 14:27

test: test bursty traffic with dispatch

a0db724

this adds a test that can fire a large set of requests at the same time to see how spicedb handles the load it also adds a way to start up a spicedb cluster configured with dispatch over bufconn

consistency tests: use dispatch api and run in parallel

c2a338a

protect lastQuantizedRevision with atomics

8393bac

datastore: refactor and run performance tests against all impls

0134a62

this adds an abstraction that makes it easy to spin up a container for a backing datastore and generate test datastores for it

ecordell force-pushed the perftest branch from e50dd92 to 6c9bd06 Compare March 14, 2022 18:30

josephschorr reviewed Mar 14, 2022

View reviewed changes

dispatch: use option pattern for cluster dispatcher to match others

79622b7

ecordell force-pushed the perftest branch from 6c9bd06 to 79622b7 Compare March 14, 2022 19:10

josephschorr approved these changes Mar 14, 2022

View reviewed changes

ecordell merged commit 2cb5e07 into authzed:main Mar 14, 2022

github-actions bot locked and limited conversation to collaborators Mar 14, 2022

configure dispatch for tests #438

configure dispatch for tests #438

Conversation

ecordell commented Mar 1, 2022 • edited

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

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

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

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

josephschorr left a comment

Choose a reason for hiding this comment

ecordell commented Mar 1, 2022 •

edited