Merge #157804 #157919 #157920

157804: backfill: skip external tenant under deadlock in vector backfill test r=yuzefovich a=yuzefovich

`TestVectorIndexMergingDuringBackfillWithPrefix` is rather intensive, and we've seen a couple of failures due to general overload in the external-process mode under deadlock, so we'll just disable that mode under deadlock. Alternative idea I had was to exempt the tenant from rate limiting, but I wasn't able to reproduce the failure on gceworker, so I couldn't verify that it worked and decided to not go with it.

Fixes: #156995.
Release note: None

157919: roachprod: fix service registry unit test name collision r=golgeek a=DarrylWong

We were previously generating cluster names with Uint32, which is not sufficient enough to prevent naming collisions after 1000 iterations. Instead, use a RandString of length 10.

Fixes: #157884
Release note: none

157920: storage,server: add debug separated value retrieval profile endpoint r=RaduBerinde a=jbowens

Add a new debug endpoint that returns a text-formatted representation of all the stack traces that performed a retrieval of a separated value while the profile was being collected. This provides better observability into what code paths are suffering more expensive value retrievals.

Epic: none
Informs: #146575
Release note: none

Co-authored-by: Yahor Yuzefovich <[email protected]>
Co-authored-by: DarrylWong <[email protected]>
Co-authored-by: Jackson Owens <[email protected]>

Author: 4 people

pkg/roachprod/install/services_test.go

@@ -138,7 +138,7 @@ func (t *serviceRegistryTest) makeVM(clusterName string, i int) vm.VM {
 }
 
 func (t *serviceRegistryTest) newCluster(nodeCount int) *SyncedCluster {
-	clusterName := fmt.Sprintf("cluster-%d", t.rng.Uint32())
+	clusterName := fmt.Sprintf("cluster-%s", randutil.RandString(t.rng, 10, randutil.PrintableKeyAlphabet))
 	c := &SyncedCluster{
 		Cluster: cloud.Cluster{
 			Name: clusterName,

pkg/server/debug/server.go

@@ -6,13 +6,15 @@
 package debug
 
 import (
+	"context"
 	"expvar"
 	"fmt"
 	"io"
 	"net/http"
 	"net/http/pprof"
 	"strconv"
 	"strings"
+	"time"
 
 	"github.com/cockroachdb/cockroach/pkg/base/serverident"
 	"github.com/cockroachdb/cockroach/pkg/kv/kvserver"
@@ -277,6 +279,29 @@ func (ds *Server) RegisterEngines(engines []storage.Engine) error {
 					fmt.Fprintf(w, "error analyzing LSM at %s: %v", dir, err)
 				}
 			})
+		ds.mux.HandleFunc(fmt.Sprintf("/debug/storage/%d/profiles/separated-value-retrievals", storeID),
+			func(w http.ResponseWriter, req *http.Request) {
+				dur := 30 * time.Second
+				if secsStr := req.Header.Get("seconds"); secsStr != "" {
+					secs, err := strconv.ParseInt(secsStr, 10, 64)
+					if err != nil {
+						http.Error(w, "error parsing seconds", http.StatusBadRequest)
+						return
+					}
+					dur = time.Duration(secs) * time.Second
+				}
+				ctx := req.Context()
+				ctx, cancel := context.WithTimeout(ctx, dur)
+				defer cancel()
+				profile, err := eng.ProfileSeparatedValueRetrievals(ctx)
+				if err != nil {
+					http.Error(w, "error profiling separated value retrievals", http.StatusInternalServerError)
+					return
+				}
+				w.Header().Set("Content-Type", "text/plain")
+				w.WriteHeader(http.StatusOK)
+				fmt.Fprint(w, profile.String())
+			})
 	}
 	return nil
 }

pkg/sql/backfill/backfill_test.go

@@ -346,6 +346,13 @@ func TestVectorIndexMergingDuringBackfillWithPrefix(t *testing.T) {
 	blockBackfill := make(chan struct{})
 	backfillBlocked := make(chan struct{})
 
+	var testTenant base.DefaultTestTenantOptions
+	if syncutil.DeadlockEnabled {
+		// The cluster can get overloaded under deadlock with external process
+		// mode.
+		testTenant = base.TestSkipForExternalProcessMode()
+	}
+
 	ctx := context.Background()
 	srv, db, _ := serverutils.StartServer(t, base.TestServerArgs{
 		Knobs: base.TestingKnobs{
@@ -357,6 +364,7 @@ func TestVectorIndexMergingDuringBackfillWithPrefix(t *testing.T) {
 				},
 			},
 		},
+		DefaultTestTenant: testTenant,
 	})
 	defer srv.Stopper().Stop(ctx)
 	sqlDB := sqlutils.MakeSQLRunner(db)

pkg/storage/engine.go

@@ -26,6 +26,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/util/uuid"
 	"github.com/cockroachdb/errors"
 	"github.com/cockroachdb/pebble"
+	"github.com/cockroachdb/pebble/metrics"
 	"github.com/cockroachdb/pebble/rangekey"
 	"github.com/cockroachdb/pebble/vfs"
 	"github.com/cockroachdb/redact"
@@ -926,6 +927,9 @@ type Engine interface {
 	Capacity() (roachpb.StoreCapacity, error)
 	// Properties returns the low-level properties for the engine's underlying storage.
 	Properties() roachpb.StoreProperties
+	// ProfileSeparatedValueRetrievals collects a profile of the engine's
+	// separated value retrievals. It stops when the context is done.
+	ProfileSeparatedValueRetrievals(ctx context.Context) (*metrics.ValueRetrievalProfile, error)
 	// Compact forces compaction over the entire database.
 	Compact(ctx context.Context) error
 	// Env returns the filesystem environment used by the Engine.

pkg/storage/pebble.go

@@ -2136,6 +2136,19 @@ func (p *Pebble) GetEnvStats() (*fs.EnvStats, error) {
 	return stats, nil
 }
 
+// ProfileSeparatedValueRetrievals collects a profile of the engine's
+// separated value retrievals. It stops when the context is done.
+func (p *Pebble) ProfileSeparatedValueRetrievals(
+	ctx context.Context,
+) (*metrics.ValueRetrievalProfile, error) {
+	stop, err := p.db.RecordSeparatedValueRetrievals()
+	if err != nil {
+		return nil, err
+	}
+	<-ctx.Done()
+	return stop(), nil
+}
+
 // GetAuxiliaryDir implements the Engine interface.
 func (p *Pebble) GetAuxiliaryDir() string {
 	return p.auxDir