Merge #151365 #151668 #151796 #151810

151365: bazel: enable remote build on mesolite from linux r=rickystewart a=jlinder

We are merging EngFlow clusters. This transitions to using the mesolite cluster for running remote builds from gceworkers.

Part of: DEVINF-1489
Release note: None

151668: backupsink: increase test size to large r=msbutler a=jeffswenson

The test allocates a relatively large buffers and we have seen periodic ooms as a result.

Release note: None
Informs: #145892
Informs: #150623
Informs: #151665
Informs: #151032

151796: kvserver: add timeseries metric for value retrieval count r=annrpom a=annrpom

Epic: none

Release note: None

151810: scplan: unredact logs for unable to make progress error r=rafiss a=rafiss

We have seen this error in tests and Sentry reports. The details for the error do not show up when logs are redacted, so this change makes the details appear in the AssertionError itself. This also makes stageBuilder implement redact.SafeFormatter so that the error is formatted safely.

fixes #151769
informs #147315
Release note: None

Co-authored-by: James H. Linder <[email protected]>
Co-authored-by: Jeff Swenson <[email protected]>
Co-authored-by: Annie Pompa <[email protected]>
Co-authored-by: Rafi Shamim <[email protected]>

Author: 5 people

.bazelrc

@@ -147,17 +147,16 @@ build:engflowbase --remote_upload_local_results=false
 build:engflowbase --remote_download_toplevel
 build:engflowbase --test_env=GO_TEST_WRAP_TESTV=1
 build:engflowbase --config=lintonbuild
+build:engflowbase --remote_cache=grpcs://mesolite.cluster.engflow.com
+build:engflowbase --remote_executor=grpcs://mesolite.cluster.engflow.com
+build:engflowbase --bes_backend=grpcs://mesolite.cluster.engflow.com
 test:engflowbase --test_env=REMOTE_EXEC=1
 test:engflowbase --test_env=GOTRACEBACK=all
 build:engflow --config=engflowbase
-build:engflow --remote_cache=grpcs://tanzanite.cluster.engflow.com
-build:engflow --remote_executor=grpcs://tanzanite.cluster.engflow.com
-build:engflow --bes_backend=grpcs://tanzanite.cluster.engflow.com
-build:engflow --bes_results_url=https://tanzanite.cluster.engflow.com/invocation/
+build:engflow --bes_results_url=https://mesolite.cluster.engflow.com/invocations/private
+build:engflow --remote_instance_name=private
+build:engflow --bes_instance_name=private
 build:engflowpublic --config=engflowbase
-build:engflowpublic --remote_cache=grpcs://mesolite.cluster.engflow.com
-build:engflowpublic --remote_executor=grpcs://mesolite.cluster.engflow.com
-build:engflowpublic --bes_backend=grpcs://mesolite.cluster.engflow.com
 build:engflowpublic --bes_results_url=https://mesolite.cluster.engflow.com/invocation/
 
 try-import %workspace%/.bazelrc.user

dev

@@ -8,7 +8,7 @@ fi
 set -euo pipefail
 
 # Bump this counter to force rebuilding `dev` on all machines.
-DEV_VERSION=112
+DEV_VERSION=113
 
 THIS_DIR=$(cd "$(dirname "$0")" && pwd)
 BINARY_DIR=$THIS_DIR/bin/dev-versions

docs/generated/metrics/metrics.yaml

@@ -17366,6 +17366,14 @@ layers:
       unit: BYTES
       aggregation: AVG
       derivative: NONE
+    - name: storage.value_separation.value_retrieval.count
+      exported_name: storage_value_separation_value_retrieval_count
+      description: The number of value retrievals of values separated into blob files.
+      y_axis_label: Events
+      type: COUNTER
+      unit: COUNT
+      aggregation: AVG
+      derivative: NON_NEGATIVE_DERIVATIVE
     - name: storage.wal.bytes_in
       exported_name: storage_wal_bytes_in
       description: The number of logical bytes the storage engine has written to the WAL

pkg/backup/backupsink/BUILD.bazel

@@ -32,6 +32,7 @@ go_library(
 
 go_test(
     name = "backupsink_test",
+    size = "large",
     srcs = [
         "file_sst_sink_test.go",
         "sst_sink_key_writer_test.go",

pkg/cmd/dev/doctor.go

@@ -30,7 +30,7 @@ const (
 	// doctorStatusVersion is the current "version" of the status checks
 	// performed by `dev doctor``. Increasing it will force doctor to be re-run
 	// before other dev commands can be run.
-	doctorStatusVersion = 11
+	doctorStatusVersion = 12
 
 	noCacheFlag     = "no-cache"
 	interactiveFlag = "interactive"
@@ -276,8 +276,15 @@ Make sure one of the following lines is in the file %s/.bazelrc.user:
 			if d.checkUsingConfig(cfg.workspace, "dev") {
 				return "In --remote mode, you cannot use the `dev` build configuration."
 			}
-			if !d.checkLinePresenceInBazelRcUser(cfg.workspace, "build:engflow --jobs=200") {
-				return fmt.Sprintf("Make sure the following line is in %s/.bazelrc.user: build:engflow --jobs=200", cfg.workspace)
+			requiredLines := []string{
+				"build:engflow --jobs=200",
+				"build:engflow --credential_helper=mesolite.cluster.engflow.com=engflow_auth",
+				"build:engflow --remote_execution_priority=-1",
+			}
+			for _, line := range requiredLines {
+				if !d.checkLinePresenceInBazelRcUser(cfg.workspace, line) {
+					return fmt.Sprintf("Make sure the following line is in %s/.bazelrc.user: %s", cfg.workspace, line)
+				}
 			}
 			return ""
 		},
@@ -304,13 +311,19 @@ Make sure one of the following lines is in the file %s/.bazelrc.user:
 					return err
 				}
 			}
-			if !d.checkLinePresenceInBazelRcUser(cfg.workspace, "build:engflow --jobs=200") {
-				err := d.addLineToBazelRcUser(cfg.workspace, "build:engflow --jobs=200")
-				if err != nil {
-					return err
+			requiredLines := []string{
+				"build:engflow --jobs=200",
+				"build:engflow --credential_helper=mesolite.cluster.engflow.com=engflow_auth",
+				"build:engflow --remote_execution_priority=-1",
+			}
+			for _, line := range requiredLines {
+				if !d.checkLinePresenceInBazelRcUser(cfg.workspace, line) {
+					err := d.addLineToBazelRcUser(cfg.workspace, line)
+					if err != nil {
+						return err
+					}
 				}
 			}
-
 			return nil
 		},
 		remoteOnly: true,
@@ -359,13 +372,16 @@ slightly slower and introduce a noticeable delay in first-time build setup.`
 		},
 	},
 	{
-		name: "engflow_certificates",
+		name: "engflow_auth",
 		check: func(d *dev, ctx context.Context, cfg doctorConfig) string {
-			if !d.checkLinePresenceInBazelRcUser(cfg.workspace, "build:engflow --tls_client_certificate=") {
-				return fmt.Sprintf("Must specify the --tls_client_certificate to use for EngFlow builds in %s/.bazelrc.user. This is a line of the form: `build:engflow --tls_client_certificate=/path/to/file`.", cfg.workspace)
+			if !d.checkFileExistsUnderHomedir(".config/engflow_auth/tokens/mesolite.cluster.engflow.com") {
+				return "Make sure you've installed engflow_auth (https://github.com/EngFlow/auth), added it to your PATH, and login using `engflow_auth login -store=file https://mesolite.cluster.engflow.com`."
+			}
+			if d.checkLinePresenceInBazelRcUser(cfg.workspace, "build:engflow --tls_client_certificate=") {
+				return fmt.Sprintf("Please remove the --tls_client_certificate line from %s/.bazelrc.user. It is no longer necessary when using engflow_auth.", cfg.workspace)
 			}
-			if !d.checkLinePresenceInBazelRcUser(cfg.workspace, "build:engflow --tls_client_key=") {
-				return fmt.Sprintf("Must specify the --tls_client_key to use for EngFlow builds in %s/.bazelrc.user. This is a line of the form: `build:engflow --tls_client_key=/path/to/file`.", cfg.workspace)
+			if d.checkLinePresenceInBazelRcUser(cfg.workspace, "build:engflow --tls_client_key=") {
+				return fmt.Sprintf("Please remove the --tls_client_key line from %s/.bazelrc.user. It is no longer necessary when using engflow_auth.", cfg.workspace)
 			}
 			return ""
 		},
@@ -924,3 +940,18 @@ func (d *dev) removeAllPrefixesInFile(filename, prefixToRemove string) error {
 	outStr = strings.TrimSpace(outStr) + "\n"
 	return d.os.WriteFile(filename, outStr)
 }
+
+// checkFileExistsUnderHomedir checks whether the given file or path is present
+// under the home directory. If it is, this function returns true. Otherwise,
+// it returns false.
+func (d *dev) checkFileExistsUnderHomedir(expectedFile string) bool {
+	homeDir, err := os.UserHomeDir()
+	if err != nil {
+		return false
+	}
+	exists, err := d.os.Exists(filepath.Join(homeDir, expectedFile))
+	if err != nil {
+		return false
+	}
+	return exists
+}

pkg/cmd/dev/io/os/os.go

@@ -296,6 +296,26 @@ func (o *OS) IsDir(dirname string) (bool, error) {
 	return strconv.ParseBool(strings.TrimSpace(output))
 }
 
+// Exists wraps around os.Stat, which returns the os.FileInfo of the named
+// path. Exists returns true if and only if it exists. If there is an error,
+// it will return false and the error.
+func (o *OS) Exists(filename string) (bool, error) {
+	command := fmt.Sprintf("cat %s", filename)
+	if !o.knobs.silent {
+		o.logger.Print(command)
+	}
+
+	output, _ := o.Next(command, func() (output string, err error) {
+		_, err = os.Stat(filename)
+		exists := !errors.Is(err, fs.ErrNotExist)
+		if exists && err != nil {
+			return "false", err
+		}
+		return strconv.FormatBool(exists), nil
+	})
+	return strconv.ParseBool(strings.TrimSpace(output))
+}
+
 // ReadFile wraps around os.ReadFile, reading a file from disk and
 // returning the contents.
 func (o *OS) ReadFile(filename string) (string, error) {

pkg/kv/kvserver/metrics.go

@@ -2675,6 +2675,12 @@ Note that the measurement does not include the duration for replicating the eval
 		Measurement: "Bytes",
 		Unit:        metric.Unit_BYTES,
 	}
+	metaValueSeparationValueRetrievalCount = metric.Metadata{
+		Name:        "storage.value_separation.value_retrieval.count",
+		Help:        "The number of value retrievals of values separated into blob files.",
+		Measurement: "Events",
+		Unit:        metric.Unit_COUNT,
+	}
 	metaWALBytesWritten = metric.Metadata{
 		Name:        "storage.wal.bytes_written",
 		Help:        "The number of bytes the storage engine has written to the WAL",
@@ -3002,18 +3008,19 @@ type StoreMetrics struct {
 	CompressionUnknownBytes *metric.Gauge
 	CompressionOverallCR    *metric.GaugeFloat64
 
-	categoryIterMetrics              pebbleCategoryIterMetricsContainer
-	categoryDiskWriteMetrics         pebbleCategoryDiskWriteMetricsContainer
-	ValueSeparationBytesReferenced   *metric.Gauge
-	ValueSeparationBytesUnreferenced *metric.Gauge
-	ValueSeparationBlobFileCount     *metric.Gauge
-	ValueSeparationBlobFileSize      *metric.Gauge
-	WALBytesWritten                  *metric.Counter
-	WALBytesIn                       *metric.Counter
-	WALFailoverSwitchCount           *metric.Counter
-	WALFailoverPrimaryDuration       *metric.Counter
-	WALFailoverSecondaryDuration     *metric.Counter
-	WALFailoverWriteAndSyncLatency   *metric.ManualWindowHistogram
+	categoryIterMetrics                pebbleCategoryIterMetricsContainer
+	categoryDiskWriteMetrics           pebbleCategoryDiskWriteMetricsContainer
+	ValueSeparationBytesReferenced     *metric.Gauge
+	ValueSeparationBytesUnreferenced   *metric.Gauge
+	ValueSeparationBlobFileCount       *metric.Gauge
+	ValueSeparationBlobFileSize        *metric.Gauge
+	ValueSeparationValueRetrievalCount *metric.Counter
+	WALBytesWritten                    *metric.Counter
+	WALBytesIn                         *metric.Counter
+	WALFailoverSwitchCount             *metric.Counter
+	WALFailoverPrimaryDuration         *metric.Counter
+	WALFailoverSecondaryDuration       *metric.Counter
+	WALFailoverWriteAndSyncLatency     *metric.ManualWindowHistogram
 
 	RdbCheckpoints *metric.Gauge
 
@@ -3740,15 +3747,16 @@ func newStoreMetrics(histogramWindow time.Duration) *StoreMetrics {
 		categoryDiskWriteMetrics: pebbleCategoryDiskWriteMetricsContainer{
 			registry: storeRegistry,
 		},
-		ValueSeparationBytesReferenced:   metric.NewGauge(metaValueSeparationBytesReferenced),
-		ValueSeparationBytesUnreferenced: metric.NewGauge(metaValueSeparationBytesUnreferenced),
-		ValueSeparationBlobFileCount:     metric.NewGauge(metaValueSeparationBlobFileCount),
-		ValueSeparationBlobFileSize:      metric.NewGauge(metaValueSeparationBlobFileSize),
-		WALBytesWritten:                  metric.NewCounter(metaWALBytesWritten),
-		WALBytesIn:                       metric.NewCounter(metaWALBytesIn),
-		WALFailoverSwitchCount:           metric.NewCounter(metaStorageWALFailoverSwitchCount),
-		WALFailoverPrimaryDuration:       metric.NewCounter(metaStorageWALFailoverPrimaryDuration),
-		WALFailoverSecondaryDuration:     metric.NewCounter(metaStorageWALFailoverSecondaryDuration),
+		ValueSeparationBytesReferenced:     metric.NewGauge(metaValueSeparationBytesReferenced),
+		ValueSeparationBytesUnreferenced:   metric.NewGauge(metaValueSeparationBytesUnreferenced),
+		ValueSeparationBlobFileCount:       metric.NewGauge(metaValueSeparationBlobFileCount),
+		ValueSeparationBlobFileSize:        metric.NewGauge(metaValueSeparationBlobFileSize),
+		ValueSeparationValueRetrievalCount: metric.NewCounter(metaValueSeparationValueRetrievalCount),
+		WALBytesWritten:                    metric.NewCounter(metaWALBytesWritten),
+		WALBytesIn:                         metric.NewCounter(metaWALBytesIn),
+		WALFailoverSwitchCount:             metric.NewCounter(metaStorageWALFailoverSwitchCount),
+		WALFailoverPrimaryDuration:         metric.NewCounter(metaStorageWALFailoverPrimaryDuration),
+		WALFailoverSecondaryDuration:       metric.NewCounter(metaStorageWALFailoverSecondaryDuration),
 		WALFailoverWriteAndSyncLatency: metric.NewManualWindowHistogram(
 			metaStorageWALFailoverWriteAndSyncLatency,
 			pebble.FsyncLatencyBuckets,
@@ -4187,6 +4195,7 @@ func (sm *StoreMetrics) updateEngineMetrics(m storage.Metrics) {
 	sm.ValueSeparationBytesUnreferenced.Update(int64(m.BlobFiles.ValueSize - m.BlobFiles.ReferencedValueSize))
 	sm.ValueSeparationBlobFileCount.Update(int64(m.BlobFiles.LiveCount))
 	sm.ValueSeparationBlobFileSize.Update(int64(m.BlobFiles.LiveSize))
+	sm.ValueSeparationValueRetrievalCount.Update(int64(m.Iterator.ValueRetrievalCount))
 	// NB: `UpdateIfHigher` is used here since there is a race in pebble where
 	// sometimes the WAL is rotated but metrics are retrieved prior to the update
 	// to BytesIn to account for the previous WAL.

pkg/sql/schemachanger/scplan/internal/scstage/build.go

@@ -269,9 +269,10 @@ func buildPostCommitStages(bc buildContext, bs buildState) (stages []Stage) {
 			var trace []string
 			bs.trace = &trace
 			sb = bc.makeStageBuilder(bs)
-			panic(errors.WithDetailf(
-				errors.AssertionFailedf("unable to make progress"),
-				"terminal state:\n%s\nrule trace:\n%s", sb, strings.Join(trace, "\n")))
+			panic(errors.AssertionFailedf(
+				"unable to make progress\nterminal state:\n%v\nrule trace:\n%s",
+				sb, redact.SafeString(strings.Join(trace, "\n")),
+			))
 		}
 		build(sb)
 	}
@@ -728,15 +729,22 @@ func (sb stageBuilder) hasAnyNonRevertibleOps() bool {
 
 // String returns a string representation of the stageBuilder.
 func (sb stageBuilder) String() string {
-	var str strings.Builder
+	return redact.StringWithoutMarkers(sb)
+}
+
+// SafeFormat implements redact.SafeFormatter.
+func (sb stageBuilder) SafeFormat(p redact.SafePrinter, verb rune) {
 	for _, t := range sb.current {
-		str.WriteString(" - ")
-		str.WriteString(screl.NodeString(t.n))
-		str.WriteString("\n")
+		p.SafeString(" - ")
+		if err := screl.FormatNode(p, t.n); err != nil {
+			p.UnsafeString(screl.NodeString(t.n))
+		}
+		p.SafeString("\n")
 	}
-	return str.String()
 }
 
+var _ redact.SafeFormatter = stageBuilder{}
+
 // computeExtraOps generates extra operations to decorate a stage with.
 // These are typically job-related.
 //

pkg/storage/engine.go

@@ -1303,6 +1303,9 @@ type AggregatedIteratorStats struct {
 	// ExternalSteps, it's a good indication that there's an accumulation of
 	// garbage within the LSM (NOT MVCC garbage).
 	InternalSteps int
+	// ValueRetrievalCount is the total count of value retrievals of values
+	// separated into blob files.
+	ValueRetrievalCount uint64
 }
 
 // AggregatedBatchCommitStats hold cumulative stats summed over all the

pkg/storage/pebble.go

@@ -1420,6 +1420,7 @@ func (p *Pebble) aggregateIterStats(stats IteratorStats) {
 	p.iterStats.ExternalSteps += stats.Stats.ForwardStepCount[pebble.InterfaceCall] + stats.Stats.ReverseStepCount[pebble.InterfaceCall]
 	p.iterStats.InternalSeeks += stats.Stats.ForwardSeekCount[pebble.InternalIterCall] + stats.Stats.ReverseSeekCount[pebble.InternalIterCall]
 	p.iterStats.InternalSteps += stats.Stats.ForwardStepCount[pebble.InternalIterCall] + stats.Stats.ReverseStepCount[pebble.InternalIterCall]
+	p.iterStats.ValueRetrievalCount += stats.Stats.InternalStats.SeparatedPointValue.CountFetched
 }
 
 func (p *Pebble) aggregateBatchCommitStats(stats BatchCommitStats) {