From a7fc3341afbb73b08cc24f6608b1f12590bb83ba Mon Sep 17 00:00:00 2001 From: Ryan Hall Date: Thu, 4 Mar 2021 18:45:00 -0800 Subject: [PATCH 1/4] Enable PANIC_ON_INVARIANT_VIOLATED for tests Helpful for catching issues before they are released. --- .ci | 2 +- scripts/docker-integration-tests/m3aggregator.Dockerfile | 2 ++ scripts/docker-integration-tests/m3coordinator.Dockerfile | 2 ++ scripts/docker-integration-tests/m3dbnode.Dockerfile | 2 ++ scripts/dtest/docker-compose.yml | 4 ++++ 5 files changed, 11 insertions(+), 1 deletion(-) diff --git a/.ci b/.ci index 15209040a6..b045911f33 160000 --- a/.ci +++ b/.ci @@ -1 +1 @@ -Subproject commit 15209040a6432a3280c1e2ed2d55ebd520ebe723 +Subproject commit b045911f33a3396918b4120a59bd83c54e2d9034 diff --git a/scripts/docker-integration-tests/m3aggregator.Dockerfile b/scripts/docker-integration-tests/m3aggregator.Dockerfile index d0dce08d1a..e64339e041 100644 --- a/scripts/docker-integration-tests/m3aggregator.Dockerfile +++ b/scripts/docker-integration-tests/m3aggregator.Dockerfile @@ -8,5 +8,7 @@ ADD ./m3aggregator.yml /etc/m3aggregator/m3aggregator.yml EXPOSE 6000-6001/tcp +ENV PANIC_ON_INVARIANT_VIOLATED=true + ENTRYPOINT [ "/bin/m3aggregator" ] CMD [ "-f", "/etc/m3aggregator/m3aggregator.yml" ] diff --git a/scripts/docker-integration-tests/m3coordinator.Dockerfile b/scripts/docker-integration-tests/m3coordinator.Dockerfile index afaae86723..0319613977 100644 --- a/scripts/docker-integration-tests/m3coordinator.Dockerfile +++ b/scripts/docker-integration-tests/m3coordinator.Dockerfile @@ -8,5 +8,7 @@ ADD ./m3coordinator-local-etcd.yml /etc/m3coordinator/m3coordinator.yml EXPOSE 7201/tcp 7203/tcp +ENV PANIC_ON_INVARIANT_VIOLATED=true + ENTRYPOINT [ "/bin/m3coordinator" ] CMD [ "-f", "/etc/m3coordinator/m3coordinator.yml" ] diff --git a/scripts/docker-integration-tests/m3dbnode.Dockerfile b/scripts/docker-integration-tests/m3dbnode.Dockerfile index d928221b94..a352ad4bf5 100644 --- a/scripts/docker-integration-tests/m3dbnode.Dockerfile +++ b/scripts/docker-integration-tests/m3dbnode.Dockerfile @@ -8,5 +8,7 @@ ADD ./m3dbnode-local-etcd.yml /etc/m3dbnode/m3dbnode.yml EXPOSE 2379/tcp 2380/tcp 7201/tcp 7203/tcp 9000-9004/tcp +ENV PANIC_ON_INVARIANT_VIOLATED=true + ENTRYPOINT [ "/bin/m3dbnode" ] CMD [ "-f", "/etc/m3dbnode/m3dbnode.yml" ] diff --git a/scripts/dtest/docker-compose.yml b/scripts/dtest/docker-compose.yml index ba8502c42e..921055276d 100755 --- a/scripts/dtest/docker-compose.yml +++ b/scripts/dtest/docker-compose.yml @@ -10,6 +10,8 @@ services: - "0.0.0.0:9000:9000" volumes: - "./m3dbnode.yml:/etc/m3dbnode/m3dbnode.yml" + env: + - PANIC_ON_INVARIANT_VIOLATED=true coord01: networks: - dtest @@ -20,5 +22,7 @@ services: - "0.0.0.0:7204:7204" volumes: - "./m3coordinator.yml:/etc/m3coordinator/m3coordinator.yml" + env: + - PANIC_ON_INVARIANT_VIOLATED=true networks: dtest: From 623dcddf5be7052edf1ae19a0f25058552793102 Mon Sep 17 00:00:00 2001 From: Ryan Hall Date: Thu, 4 Mar 2021 20:00:41 -0800 Subject: [PATCH 2/4] fix tests --- src/dbnode/client/session_aggregate_test.go | 11 ++++++---- .../client/session_fetch_tagged_test.go | 14 ++++++++++++- src/dbnode/persist/fs/files_test.go | 11 +++++----- src/dbnode/storage/fs_test.go | 6 +++--- src/query/storage/m3/storage_test.go | 4 ++-- src/x/instrument/invariant.go | 11 ++++++++++ src/x/instrument/invariant_test.go | 21 +++++++------------ 7 files changed, 49 insertions(+), 29 deletions(-) diff --git a/src/dbnode/client/session_aggregate_test.go b/src/dbnode/client/session_aggregate_test.go index 2b1e5b67a9..987964c9b6 100644 --- a/src/dbnode/client/session_aggregate_test.go +++ b/src/dbnode/client/session_aggregate_test.go @@ -22,6 +22,7 @@ package client import ( "fmt" + "github.com/m3db/m3/src/x/instrument" "sync/atomic" "testing" "time" @@ -282,10 +283,12 @@ func TestSessionAggregateIDsEnqueueErr(t *testing.T) { assert.NoError(t, session.Open()) - _, _, err = session.Aggregate(testContext(), ident.StringID("namespace"), - testSessionAggregateQuery, testSessionAggregateQueryOpts(start, end)) - assert.Error(t, err) - assert.NoError(t, session.Close()) + defer instrument.SetShouldPanicEnvironmentVariable(true)() + require.Panics(t, func() { + _, _, _ = session.Aggregate(testContext(), ident.StringID("namespace"), + testSessionAggregateQuery, testSessionAggregateQueryOpts(start, end)) + }) + } func TestSessionAggregateMergeTest(t *testing.T) { diff --git a/src/dbnode/client/session_fetch_tagged_test.go b/src/dbnode/client/session_fetch_tagged_test.go index 3505f19514..20645ca13c 100644 --- a/src/dbnode/client/session_fetch_tagged_test.go +++ b/src/dbnode/client/session_fetch_tagged_test.go @@ -588,6 +588,15 @@ func mockExtendedHostQueues( require.Fail(t, "unable to find host idx: %v", host.ID()) return -1 } + expectClose := true + for _, hq := range opsByHost { + for _, e := range hq.enqueues { + if e.enqueueErr != nil { + expectClose = false + break + } + } + } s.newHostQueueFn = func( host topology.Host, opts hostQueueOpts, @@ -600,6 +609,7 @@ func mockExtendedHostQueues( hostQueue.EXPECT().Open() hostQueue.EXPECT().Host().Return(host).AnyTimes() hostQueue.EXPECT().ConnectionCount().Return(opts.opts.MinConnectionCount()).AnyTimes() + var expectNextEnqueueFn func(fns []testEnqueue) expectNextEnqueueFn = func(fns []testEnqueue) { fn := fns[0] @@ -619,7 +629,9 @@ func mockExtendedHostQueues( if len(hostEnqueues.enqueues) > 0 { expectNextEnqueueFn(hostEnqueues.enqueues) } - hostQueue.EXPECT().Close() + if expectClose { + hostQueue.EXPECT().Close() + } return hostQueue, nil } } diff --git a/src/dbnode/persist/fs/files_test.go b/src/dbnode/persist/fs/files_test.go index 556bf9efb2..153be9c101 100644 --- a/src/dbnode/persist/fs/files_test.go +++ b/src/dbnode/persist/fs/files_test.go @@ -415,8 +415,10 @@ func TestFileExists(t *testing.T) { require.NoError(t, err) require.True(t, exists) - _, err = FileExists(checkpointFilePath) - require.Error(t, err) + defer instrument.SetShouldPanicEnvironmentVariable(true)() + require.Panics(t, func() { + _, _ = FileExists(checkpointFilePath) + }) os.Remove(infoFilePath) require.False(t, mustFileExists(t, infoFilePath)) @@ -447,9 +449,8 @@ func TestCompleteCheckpointFileExists(t *testing.T) { require.NoError(t, err) require.True(t, exists) - exists, err = CompleteCheckpointFileExists("some-arbitrary-file") - require.Contains(t, err.Error(), instrument.InvariantViolatedMetricName) - require.False(t, exists) + defer instrument.SetShouldPanicEnvironmentVariable(true)() + require.Panics(t, func() { _, _ = CompleteCheckpointFileExists("some-arbitrary-file") }) } func TestShardDirPath(t *testing.T) { diff --git a/src/dbnode/storage/fs_test.go b/src/dbnode/storage/fs_test.go index 17bff01cc6..730b3acbd7 100644 --- a/src/dbnode/storage/fs_test.go +++ b/src/dbnode/storage/fs_test.go @@ -22,6 +22,7 @@ package storage import ( "errors" + "github.com/m3db/m3/src/x/instrument" "testing" "time" @@ -86,9 +87,8 @@ func TestFileSystemManagerRun(t *testing.T) { ts := time.Now() gomock.InOrder( cm.EXPECT().WarmFlushCleanup(ts, true).Return(errors.New("foo")), - fm.EXPECT().Flush(ts).Return(errors.New("bar")), ) - mgr.Run(ts, syncRun, noForce) - require.Equal(t, fileOpNotStarted, mgr.status) + defer instrument.SetShouldPanicEnvironmentVariable(true)() + require.Panics(t, func() { mgr.Run(ts, syncRun, noForce) }) } diff --git a/src/query/storage/m3/storage_test.go b/src/query/storage/m3/storage_test.go index b7c2af81d9..ac930ed1fc 100644 --- a/src/query/storage/m3/storage_test.go +++ b/src/query/storage/m3/storage_test.go @@ -851,6 +851,6 @@ func TestInvalidBlockTypes(t *testing.T) { query := &storage.FetchQuery{} fetchOpts := &storage.FetchOptions{BlockType: models.TypeMultiBlock} - _, err = s.FetchBlocks(context.TODO(), query, fetchOpts) - assert.Error(t, err) + defer instrument.SetShouldPanicEnvironmentVariable(true)() + require.Panics(t, func() { _, _ = s.FetchBlocks(context.TODO(), query, fetchOpts) }) } diff --git a/src/x/instrument/invariant.go b/src/x/instrument/invariant.go index baae4be76a..399f2dafc2 100644 --- a/src/x/instrument/invariant.go +++ b/src/x/instrument/invariant.go @@ -23,6 +23,7 @@ package instrument import ( "fmt" "os" + "strconv" "strings" "go.uber.org/zap" @@ -88,6 +89,16 @@ func InvariantErrorf(format string, a ...interface{}) error { return err } +// SetShouldPanicEnvironmentVariable sets the env variable and returns a func to reset to the previous value. +// Useful for tests to use a defer statement when they need to test a specific value. +func SetShouldPanicEnvironmentVariable(value bool) func() { + restoreValue := os.Getenv(ShouldPanicEnvironmentVariableName) + os.Setenv(ShouldPanicEnvironmentVariableName, strconv.FormatBool(value)) + return func() { + os.Setenv(ShouldPanicEnvironmentVariableName, restoreValue) + } +} + func panicIfEnvSet() { panicIfEnvSetWithMessage("") } diff --git a/src/x/instrument/invariant_test.go b/src/x/instrument/invariant_test.go index 6fef670d21..e2c51180e4 100644 --- a/src/x/instrument/invariant_test.go +++ b/src/x/instrument/invariant_test.go @@ -22,7 +22,6 @@ package instrument_test import ( "fmt" - "os" "testing" "github.com/m3db/m3/src/x/instrument" @@ -33,6 +32,7 @@ import ( ) func ExampleInvariantViolatedMetricInvocation() { + defer instrument.SetShouldPanicEnvironmentVariable(false)() testScope := tally.NewTestScope("", nil) opts := instrument.NewOptions().SetMetricsScope(testScope) @@ -44,6 +44,7 @@ func ExampleInvariantViolatedMetricInvocation() { } func TestEmitInvariantViolationDoesNotPanicIfEnvNotSet(t *testing.T) { + defer instrument.SetShouldPanicEnvironmentVariable(false)() opts := instrument.NewOptions() require.NotPanics(t, func() { instrument.EmitInvariantViolation(opts) @@ -51,6 +52,7 @@ func TestEmitInvariantViolationDoesNotPanicIfEnvNotSet(t *testing.T) { } func TestEmitAndLogInvariantViolationDoesNotPanicIfEnvNotSet(t *testing.T) { + defer instrument.SetShouldPanicEnvironmentVariable(false)() opts := instrument.NewOptions() require.NotPanics(t, func() { instrument.EmitAndLogInvariantViolation(opts, func(l *zap.Logger) { @@ -60,6 +62,7 @@ func TestEmitAndLogInvariantViolationDoesNotPanicIfEnvNotSet(t *testing.T) { } func TestErrorfDoesNotPanicIfEnvNotSet(t *testing.T) { + defer instrument.SetShouldPanicEnvironmentVariable(false)() var ( format = "some error format: %s" err_msg = "error message" @@ -72,7 +75,7 @@ func TestErrorfDoesNotPanicIfEnvNotSet(t *testing.T) { } func TestEmitInvariantViolationPanicsIfEnvSet(t *testing.T) { - defer setShouldPanicEnvironmentVariable()() + defer instrument.SetShouldPanicEnvironmentVariable(true)() opts := instrument.NewOptions() require.Panics(t, func() { @@ -81,7 +84,7 @@ func TestEmitInvariantViolationPanicsIfEnvSet(t *testing.T) { } func TestEmitAndLogInvariantViolationPanicsIfEnvSet(t *testing.T) { - defer setShouldPanicEnvironmentVariable()() + defer instrument.SetShouldPanicEnvironmentVariable(true)() opts := instrument.NewOptions() require.Panics(t, func() { @@ -92,18 +95,8 @@ func TestEmitAndLogInvariantViolationPanicsIfEnvSet(t *testing.T) { } func TestErrorfPanicsIfEnvSet(t *testing.T) { - defer setShouldPanicEnvironmentVariable()() + defer instrument.SetShouldPanicEnvironmentVariable(true)() require.Panics(t, func() { instrument.InvariantErrorf("some_error") }) } - -type cleanupFn func() - -func setShouldPanicEnvironmentVariable() cleanupFn { - restoreValue := os.Getenv(instrument.ShouldPanicEnvironmentVariableName) - os.Setenv(instrument.ShouldPanicEnvironmentVariableName, "true") - return cleanupFn(func() { - os.Setenv(instrument.ShouldPanicEnvironmentVariableName, restoreValue) - }) -} From 69d0ebefb4dd40a4ceeac66b28c9ac004ddd79a7 Mon Sep 17 00:00:00 2001 From: Ryan Hall Date: Thu, 4 Mar 2021 20:30:13 -0800 Subject: [PATCH 3/4] fix more tests --- src/dbnode/client/session_aggregate_test.go | 3 +-- src/dbnode/client/session_fetch_tagged_test.go | 10 ++++++---- src/dbnode/persist/fs/index_claims_manager_test.go | 6 +++--- src/dbnode/storage/fs_test.go | 3 ++- src/x/instrument/invariant.go | 4 ++-- 5 files changed, 14 insertions(+), 12 deletions(-) diff --git a/src/dbnode/client/session_aggregate_test.go b/src/dbnode/client/session_aggregate_test.go index 987964c9b6..aff8de1bc2 100644 --- a/src/dbnode/client/session_aggregate_test.go +++ b/src/dbnode/client/session_aggregate_test.go @@ -22,7 +22,6 @@ package client import ( "fmt" - "github.com/m3db/m3/src/x/instrument" "sync/atomic" "testing" "time" @@ -33,6 +32,7 @@ import ( "github.com/m3db/m3/src/m3ninx/idx" xerrors "github.com/m3db/m3/src/x/errors" "github.com/m3db/m3/src/x/ident" + "github.com/m3db/m3/src/x/instrument" xretry "github.com/m3db/m3/src/x/retry" xtest "github.com/m3db/m3/src/x/test" @@ -288,7 +288,6 @@ func TestSessionAggregateIDsEnqueueErr(t *testing.T) { _, _, _ = session.Aggregate(testContext(), ident.StringID("namespace"), testSessionAggregateQuery, testSessionAggregateQueryOpts(start, end)) }) - } func TestSessionAggregateMergeTest(t *testing.T) { diff --git a/src/dbnode/client/session_fetch_tagged_test.go b/src/dbnode/client/session_fetch_tagged_test.go index 20645ca13c..5c8e607baa 100644 --- a/src/dbnode/client/session_fetch_tagged_test.go +++ b/src/dbnode/client/session_fetch_tagged_test.go @@ -34,6 +34,7 @@ import ( "github.com/m3db/m3/src/m3ninx/idx" xerrors "github.com/m3db/m3/src/x/errors" "github.com/m3db/m3/src/x/ident" + "github.com/m3db/m3/src/x/instrument" xretry "github.com/m3db/m3/src/x/retry" xtest "github.com/m3db/m3/src/x/test" @@ -288,10 +289,11 @@ func TestSessionFetchTaggedIDsEnqueueErr(t *testing.T) { assert.NoError(t, session.Open()) - _, _, err = session.FetchTaggedIDs(testContext(), ident.StringID("namespace"), - testSessionFetchTaggedQuery, testSessionFetchTaggedQueryOpts(start, end)) - assert.Error(t, err) - assert.NoError(t, session.Close()) + defer instrument.SetShouldPanicEnvironmentVariable(true)() + require.Panics(t, func() { + _, _, _ = session.FetchTaggedIDs(testContext(), ident.StringID("namespace"), + testSessionFetchTaggedQuery, testSessionFetchTaggedQueryOpts(start, end)) + }) } func TestSessionFetchTaggedMergeTest(t *testing.T) { diff --git a/src/dbnode/persist/fs/index_claims_manager_test.go b/src/dbnode/persist/fs/index_claims_manager_test.go index e32d3dc70a..951761c2de 100644 --- a/src/dbnode/persist/fs/index_claims_manager_test.go +++ b/src/dbnode/persist/fs/index_claims_manager_test.go @@ -29,6 +29,7 @@ import ( "github.com/m3db/m3/src/dbnode/namespace" "github.com/m3db/m3/src/x/ident" + "github.com/m3db/m3/src/x/instrument" xtime "github.com/m3db/m3/src/x/time" ) @@ -49,9 +50,8 @@ func TestIndexClaimsManagerSingleGlobalManager(t *testing.T) { require.NoError(t, err) // Second should cause an error. - _, err = NewIndexClaimsManager(testDefaultOpts) - require.Error(t, err) - require.Equal(t, errMustUseSingleClaimsManager, err) + defer instrument.SetShouldPanicEnvironmentVariable(true)() + require.Panics(t, func() { _, _ = NewIndexClaimsManager(testDefaultOpts) }) } func TestIndexClaimsManagerConcurrentClaims(t *testing.T) { diff --git a/src/dbnode/storage/fs_test.go b/src/dbnode/storage/fs_test.go index 730b3acbd7..9ae349bf1b 100644 --- a/src/dbnode/storage/fs_test.go +++ b/src/dbnode/storage/fs_test.go @@ -22,12 +22,13 @@ package storage import ( "errors" - "github.com/m3db/m3/src/x/instrument" "testing" "time" "github.com/golang/mock/gomock" "github.com/stretchr/testify/require" + + "github.com/m3db/m3/src/x/instrument" ) func TestFileSystemManagerShouldRunDuringBootstrap(t *testing.T) { diff --git a/src/x/instrument/invariant.go b/src/x/instrument/invariant.go index 399f2dafc2..fde5a8a18a 100644 --- a/src/x/instrument/invariant.go +++ b/src/x/instrument/invariant.go @@ -93,9 +93,9 @@ func InvariantErrorf(format string, a ...interface{}) error { // Useful for tests to use a defer statement when they need to test a specific value. func SetShouldPanicEnvironmentVariable(value bool) func() { restoreValue := os.Getenv(ShouldPanicEnvironmentVariableName) - os.Setenv(ShouldPanicEnvironmentVariableName, strconv.FormatBool(value)) + _ = os.Setenv(ShouldPanicEnvironmentVariableName, strconv.FormatBool(value)) return func() { - os.Setenv(ShouldPanicEnvironmentVariableName, restoreValue) + _ = os.Setenv(ShouldPanicEnvironmentVariableName, restoreValue) } } From 306c6ce5defb542a8031a9992e18ee519c3c9ecc Mon Sep 17 00:00:00 2001 From: Ryan Hall Date: Sat, 6 Mar 2021 14:16:45 -0800 Subject: [PATCH 4/4] more tests --- src/dbnode/persist/fs/persist_manager_test.go | 8 ++++---- src/dbnode/storage/index_block_test.go | 11 +++++++---- src/dbnode/storage/namespace_test.go | 2 ++ 3 files changed, 13 insertions(+), 8 deletions(-) diff --git a/src/dbnode/persist/fs/persist_manager_test.go b/src/dbnode/persist/fs/persist_manager_test.go index 362db4b5eb..49c6c73e2b 100644 --- a/src/dbnode/persist/fs/persist_manager_test.go +++ b/src/dbnode/persist/fs/persist_manager_test.go @@ -36,6 +36,7 @@ import ( m3ninxpersist "github.com/m3db/m3/src/m3ninx/persist" "github.com/m3db/m3/src/x/checked" "github.com/m3db/m3/src/x/ident" + "github.com/m3db/m3/src/x/instrument" m3test "github.com/m3db/m3/src/x/test" xtest "github.com/m3db/m3/src/x/test" @@ -72,10 +73,9 @@ func TestPersistenceManagerPrepareDataFileExistsNoDelete(t *testing.T) { Shard: shard, BlockStart: blockStart, } - prepared, err := flush.PrepareData(prepareOpts) - require.Equal(t, errPersistManagerFileSetAlreadyExists, err) - require.Nil(t, prepared.Persist) - require.Nil(t, prepared.Close) + + defer instrument.SetShouldPanicEnvironmentVariable(true)() + require.Panics(t, func() { _, _ = flush.PrepareData(prepareOpts) }) } func TestPersistenceManagerPrepareDataFileExistsWithDelete(t *testing.T) { diff --git a/src/dbnode/storage/index_block_test.go b/src/dbnode/storage/index_block_test.go index 71f5fb8284..1298558338 100644 --- a/src/dbnode/storage/index_block_test.go +++ b/src/dbnode/storage/index_block_test.go @@ -40,6 +40,7 @@ import ( "github.com/m3db/m3/src/x/context" xerrors "github.com/m3db/m3/src/x/errors" "github.com/m3db/m3/src/x/ident" + "github.com/m3db/m3/src/x/instrument" xtest "github.com/m3db/m3/src/x/test" xtime "github.com/m3db/m3/src/x/time" @@ -196,11 +197,13 @@ func TestNamespaceIndexNewBlockFnRandomErr(t *testing.T) { ) (index.Block, error) { return nil, fmt.Errorf("randomerr") } + defer instrument.SetShouldPanicEnvironmentVariable(true)() md := testNamespaceMetadata(blockSize, 4*time.Hour) - _, err := newNamespaceIndexWithNewBlockFn(md, - namespace.NewRuntimeOptionsManager(md.ID().String()), - testShardSet, newBlockFn, opts) - require.Error(t, err) + require.Panics(t, func() { + _, _ = newNamespaceIndexWithNewBlockFn(md, + namespace.NewRuntimeOptionsManager(md.ID().String()), + testShardSet, newBlockFn, opts) + }) } func TestNamespaceIndexWrite(t *testing.T) { diff --git a/src/dbnode/storage/namespace_test.go b/src/dbnode/storage/namespace_test.go index da99357e0d..a2ba1e95b5 100644 --- a/src/dbnode/storage/namespace_test.go +++ b/src/dbnode/storage/namespace_test.go @@ -490,6 +490,8 @@ func TestNamespaceBootstrapOnlyNonBootstrappedShards(t *testing.T) { ctx := context.NewBackground() defer ctx.Close() + // do not panic for invariant violation to test some shards are still bootstrapped. + defer instrument.SetShouldPanicEnvironmentVariable(false)() require.Error(t, ns.Bootstrap(ctx, nsResult)) require.Equal(t, BootstrapNotStarted, ns.bootstrapState) }