From aed58d3ab88d80e47a6a21071eb337d165bd6f48 Mon Sep 17 00:00:00 2001 From: Joseph Schorr Date: Thu, 12 Dec 2024 14:01:59 -0500 Subject: [PATCH 1/2] Change GC test to always call GC directly This should hopefully remove the flaky nature of the test --- internal/datastore/postgres/postgres_shared_test.go | 3 ++- pkg/datastore/test/revisions.go | 13 +++++++++---- 2 files changed, 11 insertions(+), 5 deletions(-) diff --git a/internal/datastore/postgres/postgres_shared_test.go b/internal/datastore/postgres/postgres_shared_test.go index aaadbb52d4..340d1ece39 100644 --- a/internal/datastore/postgres/postgres_shared_test.go +++ b/internal/datastore/postgres/postgres_shared_test.go @@ -798,9 +798,10 @@ func QuantizedRevisionTest(t *testing.T, b testdatastore.RunningEngineForTest) { ds := b.NewDatastore(t, func(engine, uri string) datastore.Datastore { var err error conn, err = pgx.Connect(ctx, uri) - RegisterTypes(conn.TypeMap()) require.NoError(err) + RegisterTypes(conn.TypeMap()) + ds, err := newPostgresDatastore( ctx, uri, diff --git a/pkg/datastore/test/revisions.go b/pkg/datastore/test/revisions.go index ed21c944ab..1795b6beeb 100644 --- a/pkg/datastore/test/revisions.go +++ b/pkg/datastore/test/revisions.go @@ -96,8 +96,10 @@ func RevisionSerializationTest(t *testing.T, tester DatastoreTester) { // are invalid, and revisions inside the GC window are valid. func RevisionGCTest(t *testing.T, tester DatastoreTester) { require := require.New(t) + gcWindow := 300 * time.Millisecond - ds, err := tester.New(0, 10*time.Millisecond, 300*time.Millisecond, 1) + // NOTE: we disable the background GC process here and instead manually run it below. + ds, err := tester.New(0, veryLargeGCInterval, gcWindow, 1) require.NoError(err) ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) @@ -125,13 +127,16 @@ func RevisionGCTest(t *testing.T, tester DatastoreTester) { require.NoError(err) require.NoError(ds.CheckRevision(ctx, head), "expected head revision to be valid in GC Window") - // Make sure GC kicks in after the window. - time.Sleep(300 * time.Millisecond) + // Sleep to ensure we're past the GC window. + time.Sleep(gcWindow) gcable, ok := ds.(common.GarbageCollector) if ok { + // Run garbage collection. gcable.ResetGCCompleted() - require.Eventually(func() bool { return gcable.HasGCRun() }, 10*time.Second, 150*time.Millisecond, "GC was never run as expected") + err := common.RunGarbageCollection(gcable, gcWindow, 10*time.Second) + require.NoError(err) + require.True(gcable.HasGCRun(), "GC was never run as expected") } // FIXME currently the various datastores behave differently when a revision was requested and GC Window elapses. From 6c2324700f7b0c8be421960ad808ee6a685ae02c Mon Sep 17 00:00:00 2001 From: Joseph Schorr Date: Fri, 13 Dec 2024 14:32:39 -0500 Subject: [PATCH 2/2] Add GC run test --- pkg/datastore/test/datastore.go | 1 + pkg/datastore/test/revisions.go | 44 +++++++++++++++++++++++++++++++++ 2 files changed, 45 insertions(+) diff --git a/pkg/datastore/test/datastore.go b/pkg/datastore/test/datastore.go index fc3802bb86..da4543b630 100644 --- a/pkg/datastore/test/datastore.go +++ b/pkg/datastore/test/datastore.go @@ -208,6 +208,7 @@ func OnlyGCTests(t *testing.T, tester DatastoreTester, concurrent bool) { t.Run("TestRevisionGC", runner(tester, RevisionGCTest)) t.Run("TestInvalidReads", runner(tester, InvalidReadsTest)) + t.Run("TestGCProcessRuns", runner(tester, GCProcessRunTest)) } // All runs all generic datastore tests on a DatastoreTester. diff --git a/pkg/datastore/test/revisions.go b/pkg/datastore/test/revisions.go index 1795b6beeb..d3a80acf64 100644 --- a/pkg/datastore/test/revisions.go +++ b/pkg/datastore/test/revisions.go @@ -92,6 +92,50 @@ func RevisionSerializationTest(t *testing.T, tester DatastoreTester) { require.NoError(meta.Validate()) } +// GCProcessRunTest tests whether the custom GC process runs for the datastore. +// For datastores that do not have custom GC processes, will no-op. +func GCProcessRunTest(t *testing.T, tester DatastoreTester) { + require := require.New(t) + gcWindow := 300 * time.Millisecond + gcInterval := 500 * time.Millisecond + + ds, err := tester.New(0, gcInterval, gcWindow, 1) + require.NoError(err) + + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + + testCaveat := createCoreCaveat(t) + _, err = ds.ReadWriteTx(ctx, func(ctx context.Context, rwt datastore.ReadWriteTransaction) error { + if err := rwt.WriteNamespaces(ctx, ns.Namespace("foo/createdtxgc")); err != nil { + return err + } + return rwt.WriteCaveats(ctx, []*core.CaveatDefinition{ + testCaveat, + }) + }) + require.NoError(err) + + _, err = ds.ReadWriteTx(ctx, func(ctx context.Context, rwt datastore.ReadWriteTransaction) error { + return rwt.WriteNamespaces(ctx, testNamespace) + }) + require.NoError(err) + + gcable, ok := ds.(common.GarbageCollector) + if !ok { + return + } + + // Reset that GC was run. + gcable.ResetGCCompleted() + + // Wait the GC interval + a bit more time. + time.Sleep(gcInterval + 100*time.Millisecond) + + // Ensure GC was run. + require.True(gcable.HasGCRun(), "GC was never run as expected") +} + // RevisionGCTest makes sure revision GC takes place, revisions out-side of the GC window // are invalid, and revisions inside the GC window are valid. func RevisionGCTest(t *testing.T, tester DatastoreTester) {