Merge #110191 #110275

110191: kvprober: make the planner thread-safe to fix test-only race r=nvanbenschoten a=joshimhoff

#109564

**kvprober: make the planner thread-safe to fix test-only race**

This commit makes the planner thread-safe, in order to fix a test-only data race. This commit also un-skips the test that was skipped, when this bug was discovered.

Fixes: #109564
Release note: None.

110275: tenantcostclient: relax expectation in RU estimate test r=yuzefovich a=yuzefovich

In #106769 we tightened the RU estimate test to use 0.05 allowance, but we just saw a case where the difference was on the order of 0.2, so this commit bumps the allowed delta to 0.25 (which is still much better than 0.75 before #106769).

Fixes: #109732.

Release note: None

Co-authored-by: Josh Imhoff <[email protected]>
Co-authored-by: Yahor Yuzefovich <[email protected]>

pkg/ccl/multitenantccl/tenantcostclient/query_ru_estimate_test.go

@@ -195,7 +195,7 @@ func TestEstimateQueryRUConsumption(t *testing.T) {
 	// Check the estimated RU aggregate for all the queries against the actual
 	// measured RU consumption for the tenant.
 	tenantMeasuredRUs = getTenantRUs() - tenantStartRUs
-	const deltaFraction = 0.05
+	const deltaFraction = 0.25
 	allowedDelta := tenantMeasuredRUs * deltaFraction
 	require.InDeltaf(t, tenantMeasuredRUs, tenantEstimatedRUs, allowedDelta,
 		"estimated RUs (%d) were not within %f RUs of the expected value (%f)",

pkg/kv/kvprober/kvprober_integration_test.go

@@ -42,7 +42,6 @@ func TestProberDoesReadsAndWrites(t *testing.T) {
 	defer log.Scope(t).Close(t)
 
 	skip.UnderShort(t)
-	skip.WithIssue(t, 109564)
 
 	ctx := context.Background()
 

pkg/kv/kvprober/planner.go

@@ -19,6 +19,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/kv"
 	"github.com/cockroachdb/cockroach/pkg/roachpb"
 	"github.com/cockroachdb/cockroach/pkg/settings/cluster"
+	"github.com/cockroachdb/cockroach/pkg/util/syncutil"
 	"github.com/cockroachdb/cockroach/pkg/util/timeutil"
 	"github.com/cockroachdb/errors"
 )
@@ -34,8 +35,6 @@ type Step struct {
 
 // planner abstracts deciding on ranges to probe. It is public to integration
 // test from kvprober_test.
-//
-// planner is NOT thread-safe.
 type planner interface {
 	// Next returns a Step for the prober to execute on. A Step is a decision on
 	// on what range to probe next. Executing on a series of Steps returned by
@@ -54,6 +53,14 @@ type planner interface {
 type meta2Planner struct {
 	db       *kv.DB
 	settings *cluster.Settings
+
+	// The production kvprober as of Sep 7th 2023 does not need this mutex, as
+	// there is one planner per probe loop we run. At the same time, making the
+	// planner thread-safe makes for easier testing in kvprober_integration_test.go.
+	// Given that in this context there is no meaningful performance cost to
+	// synchronizing with a mutex, it makes sense to do it for the test use case
+	// specifically.
+	mu syncutil.Mutex
 	// cursor points to a key in meta2 at which scanning should resume when new
 	// plans are needed.
 	cursor roachpb.Key
@@ -148,6 +155,10 @@ func newMeta2Planner(
 //     number, we pay a higher CPU cost less often; if it's set to a low number,
 //     we pay a smaller CPU cost more often.
 func (p *meta2Planner) next(ctx context.Context) (Step, error) {
+	// See comment where mu is defined.
+	p.mu.Lock()
+	defer p.mu.Unlock()
+
 	if len(p.plan) == 0 {
 		// Protect CRDB from planning executing too often, due to either issues
 		// with CRDB (meta2 unavailability) or bugs in kvprober.