Run postgres datastore tests with pgbouncer

[bradengroom]

Fixes #1217

[bradengroom]

I'll take a look at the test failures as I have time

[bradengroom]

These are the two pgbouncer settings that need to be documented.

[bradengroom]

The spanner tests failed, but that feels like an unrelated flaky test

[vroldanbet]

@bradengroom this is cool! I think we would want to retain the tests talking directly to Postgres too, would you mind redesigning this so it's an option? then we can run this as another datastore GitHub Action in parallel with the non-pgbouncer enabled test suite.

[bradengroom]

@bradengroom this is cool! I think we would want to retain the tests talking directly to Postgres too, would you mind redesigning this so it's an option? then we can run this as another datastore GitHub Action in parallel with the non-pgbouncer enabled test suite.

Sure thing. Makes sense!

[bradengroom]

The tests were unable to access the -enablePgbouncer flag if it came before the path for some reason. After looking at this, it didn't feel like there was any reason that the path needed to be treated as a unique argument. Now I treat them similarly and place path at the front.

[bradengroom]

I didn't want to structure this like all of the other datastore tests by creating a new folder for pgbouncer tests. I wanted to run all existing postgres tests through the pgbouncer setup. So I enabled this by adding an -enablePgbouncer flag that may be passed to the postgres tests.

I'm open to other ideas in supporting this.

[bradengroom]

Rebased. I think the currently failing test is possibly flaky. (unrelated? )

[vroldanbet]

@bradengroom 🤔 The way this is tested seems to introduce a new pattern into the codebase instead of using the ones in place. The way I imagined we would implement is by adding a new parameter to testdatastore.RunPostgresForTesting:

spicedb/internal/datastore/postgres/postgres_test.go

Line 75 in 2094b82

b := testdatastore.RunPostgresForTesting(t, "", config.targetMigration, config.pgVersion)

You can see that TestPostgresDatastore method fans out in different combinations of test environments that are run in parallel, like for example various postgres versions:

spicedb/internal/datastore/postgres/postgres_test.go

Lines 62 to 91 in 2094b82

	for _, config := range []struct {
	targetMigration string
	migrationPhase string
	pgVersion string
	}{
	{"head", "", pgversion.MinimumSupportedPostgresVersion},
	{"head", "", "14"},
	{"head", "", "15"},
	{"head", "", "16"},
	} {
	config := config
	t.Run(fmt.Sprintf("postgres-%s-%s-%s", config.pgVersion, config.targetMigration, config.migrationPhase), func(t *testing.T) {
	t.Parallel()
	b := testdatastore.RunPostgresForTesting(t, "", config.targetMigration, config.pgVersion)
	test.All(t, test.DatastoreTesterFunc(func(revisionQuantization, gcInterval, gcWindow time.Duration, watchBufferLength uint16) (datastore.Datastore, error) {
	ds := b.NewDatastore(t, func(engine, uri string) datastore.Datastore {
	ds, err := newPostgresDatastore(uri,
	RevisionQuantization(revisionQuantization),
	GCWindow(gcWindow),
	GCInterval(gcInterval),
	WatchBufferLength(watchBufferLength),
	DebugAnalyzeBeforeStatistics(),
	MigrationPhase(config.migrationPhase),
	)
	require.NoError(t, err)
	return ds
	})
	return ds, nil
	}))

[bradengroom]

@vroldanbet Thanks for the review!

then we can run this as another datastore GitHub Action in parallel with the non-pgbouncer enabled test suite.

I think I over-indexed on this comment in order to get the separate CI steps. I've updated the postgres steps to include pgbouncer tests. Let me know if I've still misunderstood something. I don't mind iterating on it until the preferred patterns are met.

[bradengroom]

There is a failing CI step, but it looks like a vulnerable dependency that should be addressed in a separate PR.

[vroldanbet]

The changes look good to me! I left a few questions. I think my main concern is that CI time has increased greatly, making the postgres datastore test now the slowest of the CI jobs:

This is not surprising given we are greatly increasing the combination of parallel tasks to run, and please note we are using beefier runners here.

Sorry for the back and forth - I think the foundation is the right one. We can certainly run these tests on our local machines but CI may need more granular jobs. Another alternative would be to reduce the number of combinations, although I think being exhaustive here is a good thing. I think that for now adding a new pgbouncer datastore as you did initially may be a good compromise, but hopefully we can now avoid using specific environment variables to tweak the behaviour.

The failed trivy is yet another gRPC vulnerability (we fixed one just 2 weeks ago) that is triggered via grpc-health-check. I see they have updated to a new version but it hasn't been released yet, so we will need to build from a specific commit. I'll open a PR. EDIT: I've opened #1613

[vroldanbet]

I'm aware this is advice provided by the community (I certainly didn't verify this myself) but it's surprising given session is the default. I wonder if this is related to the zalando operator mentioned by the user in Discord not setting session as the default. Anyway, it does not hurt adding it.

[bradengroom]

Yeah it looks like the Zalando operator chooses a different default (transaction) for some reason:
https://github.com/zalando/postgres-operator/blob/409e4c78344f397d2c771129bcfb6968f84c1972/docs/reference/operator_parameters.md?plain=1#L998

By the way, we don't have to specify this setting here. I was just trying to explicitly document it in the test. I can remove this line and the tests continue to run successfully though. I'm happy to remove it if you prefer

[bradengroom]

I think that for now adding a new pgbouncer datastore as you did initially may be a good compromise, but hopefully we can now avoid using specific environment variables to tweak the behaviour.

@vroldanbet Do you have thoughts on what that'd look like? I think maybe I'm missing an alternate pattern that you're seeing.

My goal is to avoid writing new tests specifically for pgbouncer. (Every test for postgres should be a valid test for pgbouncer). I'd like to just point all of the existing tests at both options.

So I need to signal to the test suite somehow that we care about testing pgbouncer vs postgres in the current run.

Each github action points to a function in the magefile setup. Each function invokes this datastoreTest function which invokes a new go test command.

spicedb/magefiles/test.go

Line 85 in aec1c86

return goTest(fmt.Sprintf("./internal/datastore/%s/...", datastore), "-tags", "ci,docker", "-timeout", "10m")

Since we have to go through a go test command, it feels like I have to communicate my intent through the command somehow. Leaving me with few options: environment variables or cli flags. Maybe the build tags are also an option? I'm less familiar with that.

Sorry if I'm overlooking something obvious and making this more complicated than it should be 😅

[vroldanbet]

@bradengroom I've pushed 8d1dc41 with what I think is a sensible approach to have those tests run in different CI jobs. Thoughts?

[vroldanbet]

LGTM! Kudos for all the clean up and being willing to look into all suggestions! 🎉

I suspect the tests were already slow the moment I added the PR to test multiple PG versions 😢 They continue to take the same amount of time. I think we can work on that on a follow up if needed.

[vroldanbet]

I think there may be a race happening with database creation, but I think it predated this PR:

Error Trace:	/home/runner/actions-runner/_work/spicedb/spicedb/internal/testserver/datastore/postgres.go:[13](https://github.com/authzed/spicedb/actions/runs/6665578278/job/18115460184?pr=1594#step:4:14)4
            	            				/home/runner/actions-runner/_work/spicedb/spicedb/internal/datastore/postgres/postgres_shared_test.go:207
            	            				/home/runner/actions-runner/_work/spicedb/spicedb/pkg/datastore/test/datastore.go:33
            	            				/home/runner/actions-runner/_work/spicedb/spicedb/pkg/datastore/test/namespace.go:38
            	            				/home/runner/actions-runner/_work/spicedb/spicedb/pkg/datastore/test/datastore.go:72
            	Error:      	Received unexpected error:
            	            	failed to connect to `host=localhost user=postgres database=db5d7fdd59`: server error (FATAL: database "db5d7fdd59" does not exist (SQLSTATE 3D000))
            	Test:       	TestPostgresDatastoreWithPgBouncerWithoutCommitTimestamps/postgres-13.8