From 1eefd4188243a8942250815d7e7cc70b235dfc7b Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Thu, 8 Oct 2020 19:14:22 -0400 Subject: [PATCH 1/3] ci: go-test-race more packages --- .circleci/config.yml | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/.circleci/config.yml b/.circleci/config.yml index a7f4e22328..d09c589c88 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -243,8 +243,9 @@ jobs: --junitfile $TEST_RESULTS_DIR/gotestsum-report.xml -- \ -tags="$GOTAGS" -p 2 \ -race -gcflags=all=-d=checkptr=0 \ - ./agent/{ae,cache,checks,config,pool,proxycfg,router} \ - ./agent/consul/{authmethod/...,autopilot,fsm,state,stream} \ + ./agent/{ae,cache,cache-types,checks,config,pool,proxycfg,router}/... \ + ./agent/consul/{authmethod,autopilot,fsm,state,stream}/... \ + ./agent/{grpc,rpc,rpcclient,submatview}/... \ ./snapshot - store_test_results: From af8a6177973a7e519a0da7fa5fb6f2609d05c5f4 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Thu, 8 Oct 2020 19:43:49 -0400 Subject: [PATCH 2/3] grpc: fix data rate in stats handler test --- agent/grpc/stats_test.go | 30 ++++++++++++++++++++---------- 1 file changed, 20 insertions(+), 10 deletions(-) diff --git a/agent/grpc/stats_test.go b/agent/grpc/stats_test.go index e913181239..f98d0f3cb2 100644 --- a/agent/grpc/stats_test.go +++ b/agent/grpc/stats_test.go @@ -3,15 +3,17 @@ package grpc import ( "context" "net" + "sync" "testing" "time" "github.com/armon/go-metrics" - "github.com/hashicorp/consul/agent/grpc/internal/testservice" - "github.com/hashicorp/consul/sdk/testutil/retry" "github.com/stretchr/testify/require" "golang.org/x/sync/errgroup" "google.golang.org/grpc" + + "github.com/hashicorp/consul/agent/grpc/internal/testservice" + "github.com/hashicorp/consul/sdk/testutil/retry" ) func noopRegister(*grpc.Server) {} @@ -57,15 +59,18 @@ func TestHandler_EmitsStats(t *testing.T) { require.NoError(t, err) cancel() + conn.Close() + handler.srv.GracefulStop() // Wait for the server to stop so that active_streams is predictable. - retry.RunWith(fastRetry, t, func(r *retry.R) { - expectedGauge := []metricCall{ - {key: []string{"testing", "grpc", "server", "active_conns"}, val: 1}, - {key: []string{"testing", "grpc", "server", "active_streams"}, val: 1}, - {key: []string{"testing", "grpc", "server", "active_streams"}, val: 0}, - } - require.Equal(r, expectedGauge, sink.gaugeCalls) - }) + require.NoError(t, g.Wait()) + + expectedGauge := []metricCall{ + {key: []string{"testing", "grpc", "server", "active_conns"}, val: 1}, + {key: []string{"testing", "grpc", "server", "active_streams"}, val: 1}, + {key: []string{"testing", "grpc", "server", "active_conns"}, val: 0}, + {key: []string{"testing", "grpc", "server", "active_streams"}, val: 0}, + } + require.Equal(t, expectedGauge, sink.gaugeCalls) expectedCounter := []metricCall{ {key: []string{"testing", "grpc", "server", "request"}, val: 1}, @@ -96,17 +101,22 @@ func patchGlobalMetrics(t *testing.T) *fakeMetricsSink { } type fakeMetricsSink struct { + lock sync.Mutex metrics.BlackholeSink gaugeCalls []metricCall incrCounterCalls []metricCall } func (f *fakeMetricsSink) SetGaugeWithLabels(key []string, val float32, labels []metrics.Label) { + f.lock.Lock() f.gaugeCalls = append(f.gaugeCalls, metricCall{key: key, val: val, labels: labels}) + f.lock.Unlock() } func (f *fakeMetricsSink) IncrCounterWithLabels(key []string, val float32, labels []metrics.Label) { + f.lock.Lock() f.incrCounterCalls = append(f.incrCounterCalls, metricCall{key: key, val: val, labels: labels}) + f.lock.Unlock() } type metricCall struct { From 3ff6c5b3d3d6174e9ad69602842c24be70ea8896 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Thu, 8 Oct 2020 20:15:13 -0400 Subject: [PATCH 3/3] cache-types: skip tests with races --- agent/cache-types/connect_ca_leaf.go | 9 +++------ agent/cache-types/connect_ca_leaf_test.go | 12 +++++++++--- agent/cache-types/norace_test.go | 5 +++++ agent/cache-types/race_test.go | 5 +++++ agent/cache-types/testing.go | 6 ++++-- 5 files changed, 26 insertions(+), 11 deletions(-) create mode 100644 agent/cache-types/norace_test.go create mode 100644 agent/cache-types/race_test.go diff --git a/agent/cache-types/connect_ca_leaf.go b/agent/cache-types/connect_ca_leaf.go index 325423bec6..86807f316f 100644 --- a/agent/cache-types/connect_ca_leaf.go +++ b/agent/cache-types/connect_ca_leaf.go @@ -9,9 +9,10 @@ import ( "sync/atomic" "time" - "github.com/hashicorp/consul/lib" "github.com/mitchellh/hashstructure" + "github.com/hashicorp/consul/lib" + "github.com/hashicorp/consul/agent/cache" "github.com/hashicorp/consul/agent/connect" "github.com/hashicorp/consul/agent/consul" @@ -466,11 +467,7 @@ func (c *ConnectCALeaf) Fetch(opts cache.FetchOptions, req cache.Request) (cache func activeRootHasKey(roots *structs.IndexedCARoots, currentSigningKeyID string) bool { for _, ca := range roots.Roots { if ca.Active { - if ca.SigningKeyID == currentSigningKeyID { - return true - } - // Found the active CA but it has changed - return false + return ca.SigningKeyID == currentSigningKeyID } } // Shouldn't be possible since at least one root should be active. diff --git a/agent/cache-types/connect_ca_leaf_test.go b/agent/cache-types/connect_ca_leaf_test.go index 9dfa9763e1..20520ebc41 100644 --- a/agent/cache-types/connect_ca_leaf_test.go +++ b/agent/cache-types/connect_ca_leaf_test.go @@ -10,14 +10,14 @@ import ( "testing" "time" - "github.com/hashicorp/consul/sdk/testutil/retry" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" "github.com/hashicorp/consul/agent/cache" "github.com/hashicorp/consul/agent/connect" "github.com/hashicorp/consul/agent/consul" "github.com/hashicorp/consul/agent/structs" - "github.com/stretchr/testify/mock" - "github.com/stretchr/testify/require" + "github.com/hashicorp/consul/sdk/testutil/retry" ) func TestCalculateSoftExpire(t *testing.T) { @@ -147,6 +147,9 @@ func TestCalculateSoftExpire(t *testing.T) { // Test that after an initial signing, new CA roots (new ID) will // trigger a blocking query to execute. func TestConnectCALeaf_changingRoots(t *testing.T) { + if testingRace { + t.Skip("fails with -race because caRoot.Active is modified concurrently") + } t.Parallel() require := require.New(t) @@ -693,6 +696,9 @@ func TestConnectCALeaf_CSRRateLimiting(t *testing.T) { // This test runs multiple concurrent callers watching different leaf certs and // tries to ensure that the background root watch activity behaves correctly. func TestConnectCALeaf_watchRootsDedupingMultipleCallers(t *testing.T) { + if testingRace { + t.Skip("fails with -race because caRoot.Active is modified concurrently") + } t.Parallel() rpc := TestRPC(t) diff --git a/agent/cache-types/norace_test.go b/agent/cache-types/norace_test.go new file mode 100644 index 0000000000..cac95de9dc --- /dev/null +++ b/agent/cache-types/norace_test.go @@ -0,0 +1,5 @@ +// +build !race + +package cachetype + +const testingRace = false diff --git a/agent/cache-types/race_test.go b/agent/cache-types/race_test.go new file mode 100644 index 0000000000..29774bf679 --- /dev/null +++ b/agent/cache-types/race_test.go @@ -0,0 +1,5 @@ +// +build race + +package cachetype + +const testingRace = true diff --git a/agent/cache-types/testing.go b/agent/cache-types/testing.go index fcffe45a95..461d8948b7 100644 --- a/agent/cache-types/testing.go +++ b/agent/cache-types/testing.go @@ -4,8 +4,9 @@ import ( "reflect" "time" - "github.com/hashicorp/consul/agent/cache" "github.com/mitchellh/go-testing-interface" + + "github.com/hashicorp/consul/agent/cache" ) // TestRPC returns a mock implementation of the RPC interface. @@ -23,7 +24,8 @@ func TestFetchCh( t testing.T, typ cache.Type, opts cache.FetchOptions, - req cache.Request) <-chan interface{} { + req cache.Request, +) <-chan interface{} { resultCh := make(chan interface{}) go func() { result, err := typ.Fetch(opts, req)