From c834bd84e974293fa5324d1a92cf00d8ab9f9141 Mon Sep 17 00:00:00 2001 From: Oliver Tan Date: Thu, 18 Mar 2021 11:47:29 +1100 Subject: [PATCH 1/2] utilccl: fix race with TestingEnterprise[Disabled|Enabled] This seems to race: ``` WARNING: DATA RACE Write at 0x00000bd3e950 by goroutine 38: github.com/cockroachdb/cockroach/pkg/ccl/utilccl.TestingDisableEnterprise() /go/src/github.com/cockroachdb/cockroach/pkg/ccl/utilccl/license_check.go:61 +0x59 github.com/cockroachdb/cockroach/pkg/ccl/multiregionccl_test.TestMultiRegionAfterEnterpriseDisabled() /go/src/github.com/cockroachdb/cockroach/pkg/ccl/multiregionccl/multiregion_test.go:75 +0x48e github.com/cockroachdb/cockroach/pkg/testutils/testcluster.(*TestCluster).Start() /go/src/github.com/cockroachdb/cockroach/pkg/testutils/testcluster/testcluster.go:338 +0x8eb github.com/cockroachdb/cockroach/pkg/testutils/testcluster.(*TestCluster).Start() /go/src/github.com/cockroachdb/cockroach/pkg/testutils/testcluster/testcluster.go:295 +0x2dc github.com/cockroachdb/cockroach/pkg/testutils/serverutils.StartNewTestCluster() /go/src/github.com/cockroachdb/cockroach/pkg/testutils/serverutils/test_cluster_shim.go:210 +0x19b github.com/cockroachdb/cockroach/pkg/ccl/multiregionccl/multiregionccltestutils.TestingCreateMultiRegionCluster() /go/src/github.com/cockroachdb/cockroach/pkg/ccl/multiregionccl/multiregionccltestutils/testutils.go:50 +0x537 github.com/cockroachdb/cockroach/pkg/ccl/multiregionccl_test.TestMultiRegionAfterEnterpriseDisabled() /go/src/github.com/cockroachdb/cockroach/pkg/ccl/multiregionccl/multiregion_test.go:56 +0x1c4 testing.tRunner() /usr/local/go/src/testing/testing.go:1123 +0x202 Previous read at 0x00000bd3e950 by goroutine 918: github.com/cockroachdb/cockroach/pkg/ccl/utilccl.CheckEnterpriseEnabled() /go/src/github.com/cockroachdb/cockroach/pkg/ccl/utilccl/license_check.go:71 +0x54 github.com/cockroachdb/cockroach/pkg/ccl/kvccl/kvfollowerreadsccl.checkEnterpriseEnabled() /go/src/github.com/cockroachdb/cockroach/pkg/ccl/kvccl/kvfollowerreadsccl/followerreads.go:75 +0xab github.com/cockroachdb/cockroach/pkg/ccl/kvccl/kvfollowerreadsccl.checkFollowerReadsEnabled() /go/src/github.com/cockroachdb/cockroach/pkg/ccl/kvccl/kvfollowerreadsccl/followerreads.go:82 +0x79 github.com/cockroachdb/cockroach/pkg/ccl/kvccl/kvfollowerreadsccl.canSendToFollower() /go/src/github.com/cockroachdb/cockroach/pkg/ccl/kvccl/kvfollowerreadsccl/followerreads.go:127 +0x64 github.com/cockroachdb/cockroach/pkg/kv/kvclient/kvcoord.(*DistSender).sendToReplicas() /go/src/github.com/cockroachdb/cockroach/pkg/kv/kvclient/kvcoord/dist_sender.go:1778 +0x441 ``` This is fixed by loading the value with atomic. Release note: None --- pkg/ccl/utilccl/license_check.go | 27 +++++++++++++++++++-------- 1 file changed, 19 insertions(+), 8 deletions(-) diff --git a/pkg/ccl/utilccl/license_check.go b/pkg/ccl/utilccl/license_check.go index b73629ddb3fa..b3e27afd149c 100644 --- a/pkg/ccl/utilccl/license_check.go +++ b/pkg/ccl/utilccl/license_check.go @@ -12,6 +12,7 @@ import ( "bytes" "context" "strings" + "sync/atomic" "time" "github.com/cockroachdb/cockroach/pkg/base" @@ -44,23 +45,33 @@ var enterpriseLicense = func() *settings.StringSetting { return s }() -var testingEnterpriseEnabled = false +// testingEnterprise determines whether the cluster is enabled +// or disabled for the purposes of testing. +// It should be loaded and stored using atomic as it can race with an +// in progress kv reader during TestingDisableEnterprise / +// TestingEnableEnterprise. +var testingEnterprise int32 + +const ( + testingEnterpriseDisabled = 0 + testingEnterpriseEnabled = 1 +) // TestingEnableEnterprise allows overriding the license check in tests. func TestingEnableEnterprise() func() { - before := testingEnterpriseEnabled - testingEnterpriseEnabled = true + before := atomic.LoadInt32(&testingEnterprise) + atomic.StoreInt32(&testingEnterprise, testingEnterpriseEnabled) return func() { - testingEnterpriseEnabled = before + atomic.StoreInt32(&testingEnterprise, before) } } // TestingDisableEnterprise allows re-enabling the license check in tests. func TestingDisableEnterprise() func() { - before := testingEnterpriseEnabled - testingEnterpriseEnabled = false + before := atomic.LoadInt32(&testingEnterprise) + atomic.StoreInt32(&testingEnterprise, testingEnterpriseDisabled) return func() { - testingEnterpriseEnabled = before + atomic.StoreInt32(&testingEnterprise, before) } } @@ -68,7 +79,7 @@ func TestingDisableEnterprise() func() { // feature is not enabled, including information or a link explaining how to // enable it. func CheckEnterpriseEnabled(st *cluster.Settings, cluster uuid.UUID, org, feature string) error { - if testingEnterpriseEnabled { + if atomic.LoadInt32(&testingEnterprise) == testingEnterpriseEnabled { return nil } return checkEnterpriseEnabledAt(st, timeutil.Now(), cluster, org, feature) From 8793b0fa06dc60f40150bb3e689a647bfcb9264b Mon Sep 17 00:00:00 2001 From: Oliver Tan Date: Thu, 18 Mar 2021 06:39:55 +1100 Subject: [PATCH 2/2] multiregionccl: unskip TestMultiRegionAfterEnterpriseDisabled Relevant fixes are now committed. Release note: None --- pkg/ccl/multiregionccl/multiregion_test.go | 25 +++++++++++----------- 1 file changed, 13 insertions(+), 12 deletions(-) diff --git a/pkg/ccl/multiregionccl/multiregion_test.go b/pkg/ccl/multiregionccl/multiregion_test.go index 571fba69eb40..85710015de04 100644 --- a/pkg/ccl/multiregionccl/multiregion_test.go +++ b/pkg/ccl/multiregionccl/multiregion_test.go @@ -16,7 +16,6 @@ import ( _ "github.com/cockroachdb/cockroach/pkg/ccl" "github.com/cockroachdb/cockroach/pkg/ccl/multiregionccl/multiregionccltestutils" "github.com/cockroachdb/cockroach/pkg/ccl/utilccl" - "github.com/cockroachdb/cockroach/pkg/testutils/skip" "github.com/cockroachdb/cockroach/pkg/util/leaktest" "github.com/cockroachdb/cockroach/pkg/util/log" "github.com/stretchr/testify/require" @@ -54,22 +53,24 @@ func TestMultiRegionAfterEnterpriseDisabled(t *testing.T) { defer log.Scope(t).Close(t) defer utilccl.TestingEnableEnterprise()() - skip.UnderRace(t, "#61163") - _, sqlDB, cleanup := multiregionccltestutils.TestingCreateMultiRegionCluster( t, 3 /* numServers */, base.TestingKnobs{}, nil, /* baseDir */ ) defer cleanup() - _, err := sqlDB.Exec(` -CREATE DATABASE test PRIMARY REGION "us-east1" REGIONS "us-east2"; -USE test; -CREATE TABLE t1 () LOCALITY GLOBAL; -CREATE TABLE t2 () LOCALITY REGIONAL BY TABLE; -CREATE TABLE t3 () LOCALITY REGIONAL BY TABLE IN "us-east2"; -CREATE TABLE t4 () LOCALITY REGIONAL BY ROW - `) - require.NoError(t, err) + for _, setupQuery := range []string{ + `CREATE DATABASE test PRIMARY REGION "us-east1" REGIONS "us-east2"`, + `USE test`, + `CREATE TABLE t1 () LOCALITY GLOBAL`, + `CREATE TABLE t2 () LOCALITY REGIONAL BY TABLE`, + `CREATE TABLE t3 () LOCALITY REGIONAL BY TABLE IN "us-east2"`, + `CREATE TABLE t4 () LOCALITY REGIONAL BY ROW`, + } { + t.Run(setupQuery, func(t *testing.T) { + _, err := sqlDB.Exec(setupQuery) + require.NoError(t, err) + }) + } defer utilccl.TestingDisableEnterprise()()