Merge #152915 #153033 #153139

152915: ptcache: fix error logging logic r=RaduBerinde a=RaduBerinde

Fix bug introduced in #144335 which accidentally suppresses this log.

Epic: none
Release note: None

153033: kvserver: return error properly for GetAggregatedStoreStats r=tbg a=wenyihu6

Epic: none
Release note: none

---
**kvserver: pass replication target rq.TransferLease**

Previously, rq.TransferLease was updated to take in replication target (node ID
and store ID) instead of just a store ID, but the log line formatting wasn’t
updated accordingly. This commit fixes the log formatting.

---
**kvserver: return error properly for GetAggregatedStoreStats**

Previously, GetAggregatedStoreStats would panic on error for store.Capacity
instead of returning it. This commit fixes the issue by properly returning the
error back to store.Descriptor.

---
**asim: correctly output rangeID**

Previously, the log labeled a value as a range but printed the storeID. This
commit fixes it to correctly print the rangeID.

153139: testcluster: fix TestDRPCEnabledRandomly for cluster with multiple nodes r=cthumuluru-crdb a=shubhamdhama

`` testcluster: fix TestDRPCEnabledRandomly for cluster with multiple nodes ``

This change fixes the behavior where it was deciding the DRPC enablement
for each node independently for TestDRPCEnabledRandomly option, leading to
a cluster startup with some nodes with DRPC enabled, and others not.

Now the DRPC option is resolved once at the cluster level and applied
consistently to all nodes, ensuring uniform DRPC configuration across
the entire test cluster.

`` server: fix DRPC option precedence for test-specific settings ``

Previously, when a test explicitly set TestDRPCDisabled but a global
DRPC override was configured, the global override would incorrectly
take precedence. This meant tests that explicitly disabled DRPC could
still end up running with DRPC enabled.

The fix also adds better logging to indicate when a global override
is being applied, making test behavior more transparent.

Epic: none
Release notes: none

Co-authored-by: Radu Berinde <[email protected]>
Co-authored-by: wenyihu6 <[email protected]>
Co-authored-by: Shubham Dhama <[email protected]>

Author: 4 people

pkg/base/test_server_args.go

@@ -235,9 +235,12 @@ type SlimTestServerConfig struct {
 type DefaultTestDRPCOption uint8
 
 const (
+	// TestDRPCUnset represents an uninitialized or invalid DRPC option.
+	TestDRPCUnset DefaultTestDRPCOption = iota
+
 	// TestDRPCDisabled disables DRPC; all inter-node connectivity will use gRPC
 	// only.
-	TestDRPCDisabled DefaultTestDRPCOption = iota
+	TestDRPCDisabled
 
 	// TestDRPCEnabled enables DRPC. Some services may still use gRPC if they
 	// have not yet migrated to DRPC.

pkg/kv/kvserver/asim/state/impl.go

@@ -1089,7 +1089,7 @@ func (s *state) applyLoad(rng *rng, le workload.LoadEvent) {
 func (s *state) RangeUsageInfo(rangeID RangeID, storeID StoreID) allocator.RangeUsageInfo {
 	r, ok := s.Range(rangeID)
 	if !ok {
-		panic(fmt.Sprintf("no leaseholder store found for range %d", storeID))
+		panic(fmt.Sprintf("no leaseholder store found for range %d", rangeID))
 	}
 
 	if _, ok = r.Replica(storeID); !ok {

pkg/kv/kvserver/load/node_capacity_provider.go

@@ -24,7 +24,7 @@ type StoresStatsAggregator interface {
 	// GetAggregatedStoreStats returns the total cpu usage across all stores and
 	// the count of stores. If useCached is true, it uses the cached store
 	// descriptor instead of computing new ones. Implemented by Stores.
-	GetAggregatedStoreStats(useCached bool) (aggregatedCPUUsage int64, totalStoreCount int32)
+	GetAggregatedStoreStats(useCached bool) (aggregatedCPUUsage int64, totalStoreCount int32, err error)
 }
 
 // NodeCapacityProvider reports node-level cpu usage and capacity by sampling
@@ -82,8 +82,11 @@ func (n *NodeCapacityProvider) Run(ctx context.Context) {
 // capacity and aggregated store-level cpu usage. If useCached is true, it will
 // use cached store descriptors to aggregate the sum of store-level cpu
 // capacity.
-func (n *NodeCapacityProvider) GetNodeCapacity(useCached bool) roachpb.NodeCapacity {
-	storesCPURate, numStores := n.stores.GetAggregatedStoreStats(useCached)
+func (n *NodeCapacityProvider) GetNodeCapacity(useCached bool) (roachpb.NodeCapacity, error) {
+	storesCPURate, numStores, err := n.stores.GetAggregatedStoreStats(useCached)
+	if err != nil {
+		return roachpb.NodeCapacity{}, err
+	}
 	// TODO(wenyihu6): may be unexpected to caller that useCached only applies to
 	// the stores stats but not runtime load monitor. We can change
 	// runtimeLoadMonitor to also fetch updated stats.
@@ -95,7 +98,7 @@ func (n *NodeCapacityProvider) GetNodeCapacity(useCached bool) roachpb.NodeCapac
 		NumStores:           numStores,
 		NodeCPURateCapacity: cpuCapacityNanoPerSec,
 		NodeCPURateUsage:    cpuUsageNanoPerSec,
-	}
+	}, nil
 }
 
 // runtimeLoadMonitor polls cpu usage and capacity stats of the node

pkg/kv/kvserver/load/node_capacity_provider_test.go

@@ -25,8 +25,8 @@ type mockStoresStatsAggregator struct {
 
 func (m *mockStoresStatsAggregator) GetAggregatedStoreStats(
 	_ bool,
-) (totalCPUUsage int64, totalStoreCount int32) {
-	return m.cpuUsage, m.storeCount
+) (totalCPUUsage int64, totalStoreCount int32, _ error) {
+	return m.cpuUsage, m.storeCount, nil
 }
 
 // TestNodeCapacityProvider tests the basic functionality of the
@@ -51,7 +51,8 @@ func TestNodeCapacityProvider(t *testing.T) {
 
 	// Provider should have valid stats.
 	testutils.SucceedsSoon(t, func() error {
-		nc := provider.GetNodeCapacity(false)
+		nc, err := provider.GetNodeCapacity(false)
+		require.NoError(t, err)
 		if nc.NodeCPURateUsage == 0 || nc.NodeCPURateCapacity == 0 || nc.StoresCPURate == 0 {
 			return errors.Newf(
 				"CPU usage or capacity is 0: node cpu rate usage %v, node cpu rate capacity %v, stores cpu rate %v",
@@ -62,7 +63,8 @@ func TestNodeCapacityProvider(t *testing.T) {
 
 	cancel()
 	// GetNodeCapacity should still return valid stats after cancellation.
-	nc := provider.GetNodeCapacity(false)
+	nc, err := provider.GetNodeCapacity(false)
+	require.NoError(t, err)
 	require.Greater(t, nc.NodeCPURateCapacity, int64(0))
 	require.Greater(t, nc.NodeCPURateUsage, int64(0))
 }

pkg/kv/kvserver/protectedts/ptcache/cache.go

@@ -188,8 +188,12 @@ func (c *Cache) periodicallyRefreshProtectedtsCache(ctx context.Context) {
 				return
 			}
 			res := future.WaitForResult(ctx)
-			if res.Err != nil && ctx.Err() != nil {
-				log.Dev.Errorf(ctx, "failed to refresh protected timestamps: %v", res.Err)
+			if res.Err != nil {
+				// We expect an error if the context was cancelled, we don't want to log
+				// in that case.
+				if ctx.Err() == nil {
+					log.Dev.Errorf(ctx, "failed to refresh protected timestamps: %v", res.Err)
+				}
 			}
 			timer.Reset(protectedts.PollInterval.Get(&c.settings.SV))
 			lastReset = timeutil.Now()

pkg/kv/kvserver/replicate_queue.go

@@ -1125,7 +1125,7 @@ func (rq *replicateQueue) TransferLease(
 	rangeUsageInfo allocator.RangeUsageInfo,
 ) error {
 	rq.metrics.TransferLeaseCount.Inc(1)
-	log.KvDistribution.Infof(ctx, "transferring lease to s%d", target)
+	log.KvDistribution.Infof(ctx, "transferring lease to %v", target)
 	// Inform allocator sync that the change has been applied which applies
 	// changes to store pool and inform mma.
 	changeID := rq.as.NonMMAPreTransferLease(
@@ -1139,7 +1139,7 @@ func (rq *replicateQueue) TransferLease(
 	rq.as.PostApply(changeID, err == nil /*success*/)
 
 	if err != nil {
-		return errors.Wrapf(err, "%s: unable to transfer lease to s%d", rlm, target)
+		return errors.Wrapf(err, "%s: unable to transfer lease to %v", rlm, target)
 	}
 	return nil
 }

pkg/kv/kvserver/store.go

@@ -3295,14 +3295,19 @@ func (s *Store) Descriptor(ctx context.Context, useCached bool) (*roachpb.StoreD
 		return nil, err
 	}
 
+	nc, err := s.nodeCapacityProvider.GetNodeCapacity(useCached)
+	if err != nil {
+		return nil, err
+	}
+
 	// Initialize the store descriptor.
 	return &roachpb.StoreDescriptor{
 		StoreID:      s.Ident.StoreID,
 		Attrs:        s.Attrs(),
 		Node:         *s.nodeDesc,
 		Capacity:     capacity,
 		Properties:   s.Properties(),
-		NodeCapacity: s.nodeCapacityProvider.GetNodeCapacity(useCached),
+		NodeCapacity: nc,
 	}, nil
 }
 

pkg/kv/kvserver/stores.go

@@ -338,15 +338,19 @@ func (ls *Stores) GetStoreMetricRegistry(storeID roachpb.StoreID) *metric.Regist
 
 // GetAggregatedStoreStats returns the aggregated cpu usage across all stores and
 // the count of stores.
-func (ls *Stores) GetAggregatedStoreStats(useCached bool) (storesCPURate int64, numStores int32) {
-	_ = ls.VisitStores(func(s *Store) error {
+func (ls *Stores) GetAggregatedStoreStats(
+	useCached bool,
+) (storesCPURate int64, numStores int32, _ error) {
+	if err := ls.VisitStores(func(s *Store) error {
 		c, err := s.Capacity(context.Background(), useCached)
 		if err != nil {
-			panic(err)
+			return err
 		}
 		storesCPURate += int64(c.CPUPerSecond)
 		numStores++
 		return nil
-	})
-	return storesCPURate, numStores
+	}); err != nil {
+		return 0, 0, err
+	}
+	return storesCPURate, numStores, nil
 }

pkg/server/testserver.go

@@ -333,6 +333,10 @@ func makeTestConfigFromParams(params base.TestServerArgs) Config {
 		cfg.TestingKnobs.AdmissionControlOptions = &admission.Options{}
 	}
 
+	if params.DefaultDRPCOption == base.TestDRPCEnabled {
+		rpc.ExperimentalDRPCEnabled.Override(context.Background(), &st.SV, true)
+	}
+
 	return cfg
 }
 

pkg/testutils/serverutils/test_server_shim.go

@@ -24,10 +24,8 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/kv"
 	"github.com/cockroachdb/cockroach/pkg/multitenant/tenantcapabilitiespb"
 	"github.com/cockroachdb/cockroach/pkg/roachpb"
-	"github.com/cockroachdb/cockroach/pkg/rpc"
 	"github.com/cockroachdb/cockroach/pkg/security/securitytest"
 	"github.com/cockroachdb/cockroach/pkg/security/username"
-	"github.com/cockroachdb/cockroach/pkg/settings/cluster"
 	"github.com/cockroachdb/cockroach/pkg/testutils/pgurlutils"
 	"github.com/cockroachdb/cockroach/pkg/testutils/skip"
 	"github.com/cockroachdb/cockroach/pkg/util/envutil"
@@ -274,11 +272,9 @@ func StartServerOnlyE(t TestLogger, params base.TestServerArgs) (TestServerInter
 	ctx := context.Background()
 	allowAdditionalTenants := params.DefaultTestTenant.AllowAdditionalTenants()
 
-	// Update the flags with the actual decision as to whether we should
-	// start the service for a default test tenant.
+	// Update the flags with the actual decisions for test configuration.
 	params.DefaultTestTenant = ShouldStartDefaultTestTenant(t, params.DefaultTestTenant)
-
-	TryEnableDRPCSetting(ctx, t, &params)
+	params.DefaultDRPCOption = ShouldEnableDRPC(ctx, t, params.DefaultDRPCOption)
 
 	s, err := NewServer(params)
 	if err != nil {
@@ -541,13 +537,16 @@ func WaitForTenantCapabilities(
 	}
 }
 
-// TryEnableDRPCSetting determines whether to enable the DRPC cluster setting
-// based on the `TestServerArgs.DefaultDRPCOption` and updates the
-// `TestServerArgs.Settings` based on that.
-func TryEnableDRPCSetting(ctx context.Context, t TestLogger, args *base.TestServerArgs) {
-	option := args.DefaultDRPCOption
-	if option == base.TestDRPCDisabled && globalDefaultDRPCOptionOverride.isSet {
+// ShouldEnableDRPC determines the final DRPC option based on the input
+// option and any global overrides, resolving random choices to a concrete
+// enabled/disabled state.
+func ShouldEnableDRPC(
+	ctx context.Context, t TestLogger, option base.DefaultTestDRPCOption,
+) base.DefaultTestDRPCOption {
+	var logSuffix string
+	if option == base.TestDRPCUnset && globalDefaultDRPCOptionOverride.isSet {
 		option = globalDefaultDRPCOptionOverride.value
+		logSuffix = " (override by TestingGlobalDRPCOption)"
 	}
 	enableDRPC := false
 	switch option {
@@ -557,12 +556,11 @@ func TryEnableDRPCSetting(ctx context.Context, t TestLogger, args *base.TestServ
 		rng, _ := randutil.NewTestRand()
 		enableDRPC = rng.Intn(2) == 0
 	}
-	if !enableDRPC {
-		return
-	}
 
-	if args.Settings == nil {
-		args.Settings = cluster.MakeClusterSettings()
+	if enableDRPC {
+		t.Log("DRPC is enabled" + logSuffix)
+		return base.TestDRPCEnabled
 	}
-	rpc.ExperimentalDRPCEnabled.Override(ctx, &args.Settings.SV, true)
+
+	return base.TestDRPCDisabled
 }