Merge cockroachdb#70330 cockroachdb#72353 cockroachdb#72417 cockroach…

…db#72430 cockroachdb#72432

70330: util/log: add buffer sink decorator r=knz a=rauchenstein

Previously, only the file sink had buffering, and in that case it is
built into the sink.  It's important to add buffering to network sinks
for various reasons -- reducing network chatter, and making the
networking call itself asynchronous so the log call returns with very
low latency.

This change adds a buffering decorator so that buffering can be added to
any log sink with little or no development effort, and allowing
buffering to be configured in a uniform way.

Release note (cli change): Add buffering to log sinks. This can be
configured with the new "buffering" field on any log sink provided via
the "--log" or "--log-config-file" flags.

Release justification: This change is safe because it is a no-op without
a configuration change specifically enabling it.

72353: *: fix improperly wrapped errors r=otan,RaduBerinde,stevendanna a=rafiss

refs cockroachdb#42510

I'm working on a linter that detects errors that are not wrapped
correctly, and it discovered these.

Release note: None

72417: sql: add unit tests for creating default privileges r=ajwerner a=RichardJCai

Adding some unit test coverage so we don't hit bugs like this again.
cockroachdb#72322

Release note: None

72430: kvserver: use wrapper type for Store.mu.replicas r=erikgrinaker a=tbg

This simplifies lots of callers and it will also make it easier to work
on cockroachdb#72374, where this map will start containing more than one type as
value.

Release note: None


72432: roachprod: fix `roachprod start` ignoring --binary flag r=[rail,tbg] a=healthy-pod

Merging cockroachdb#71660 introduced a bug where roachprod ignores --binary
flag when running `roachprod start`.

This patch reverts to the old way of setting config.Binary as a quick solution to the bug.

Release note: None

Fixes cockroachdb#72425 cockroachdb#72420 cockroachdb#72373 cockroachdb#72372

Co-authored-by: Jay Rauchenstein <[email protected]>
Co-authored-by: Raphael 'kena' Poss <[email protected]>
Co-authored-by: Rafi Shamim <[email protected]>
Co-authored-by: richardjcai <[email protected]>
Co-authored-by: Richard Cai <[email protected]>
Co-authored-by: Tobias Grieger <[email protected]>
Co-authored-by: Ahmad Abedalqader <[email protected]>

build/bazelutil/check.sh

@@ -34,6 +34,7 @@ pkg/util/log/channels.go://go:generate go run gen/main.go logpb/log.proto channe
 pkg/util/log/channels.go://go:generate go run gen/main.go logpb/log.proto log_channels.go log_channels_generated.go
 pkg/util/log/channels.go://go:generate go run gen/main.go logpb/log.proto logging.md ../../../docs/generated/logging.md
 pkg/util/log/channels.go://go:generate go run gen/main.go logpb/log.proto severity.go severity/severity_generated.go
+pkg/util/log/sinks.go://go:generate mockgen -package=log -source=sinks.go -destination=mock_generated.go -mock_names=logSink=MockLogSink logSink
 pkg/util/timeutil/zoneinfo.go://go:generate go run gen/main.go
 "
 

docs/generated/logsinks.md

@@ -86,6 +86,7 @@ Configuration options shared across all sink types:
 | `redactable` | whether to keep redaction markers in the sink's output. The presence of redaction markers makes it possible to strip sensitive data reliably. |
 | `exit-on-error` | whether the logging system should terminate the process if an error is encountered while writing to this sink. |
 | `auditable` | translated to tweaks to the other settings for this sink during validation. For example, it enables `exit-on-error` and changes the format of files from `crdb-v1` to `crdb-v1-count`. |
+| `buffering` | configures buffering for this log sink, or NONE to explicitly disable. See the [common buffering configuration](#buffering-config) section for details.  |
 
 
 
@@ -168,6 +169,7 @@ Configuration options shared across all sink types:
 | `redactable` | whether to keep redaction markers in the sink's output. The presence of redaction markers makes it possible to strip sensitive data reliably. |
 | `exit-on-error` | whether the logging system should terminate the process if an error is encountered while writing to this sink. |
 | `auditable` | translated to tweaks to the other settings for this sink during validation. For example, it enables `exit-on-error` and changes the format of files from `crdb-v1` to `crdb-v1-count`. |
+| `buffering` | configures buffering for this log sink, or NONE to explicitly disable. See the [common buffering configuration](#buffering-config) section for details.  |
 
 
 
@@ -233,6 +235,7 @@ Configuration options shared across all sink types:
 | `redactable` | whether to keep redaction markers in the sink's output. The presence of redaction markers makes it possible to strip sensitive data reliably. |
 | `exit-on-error` | whether the logging system should terminate the process if an error is encountered while writing to this sink. |
 | `auditable` | translated to tweaks to the other settings for this sink during validation. For example, it enables `exit-on-error` and changes the format of files from `crdb-v1` to `crdb-v1-count`. |
+| `buffering` | configures buffering for this log sink, or NONE to explicitly disable. See the [common buffering configuration](#buffering-config) section for details.  |
 
 
 
@@ -291,6 +294,7 @@ Configuration options shared across all sink types:
 | `redactable` | whether to keep redaction markers in the sink's output. The presence of redaction markers makes it possible to strip sensitive data reliably. |
 | `exit-on-error` | whether the logging system should terminate the process if an error is encountered while writing to this sink. |
 | `auditable` | translated to tweaks to the other settings for this sink during validation. For example, it enables `exit-on-error` and changes the format of files from `crdb-v1` to `crdb-v1-count`. |
+| `buffering` | configures buffering for this log sink, or NONE to explicitly disable. See the [common buffering configuration](#buffering-config) section for details.  |
 
 
 
@@ -370,3 +374,36 @@ Likewise:
     channels: {INFO: all except ops,health}
 
 etc.
+
+
+
+<a name="buffering-config">
+
+## Common buffering configuration
+
+Buffering may be configured with the following fields. It may also be explicitly
+set to "NONE" to disable buffering. Example configuration:
+
+    file-defaults:
+       dir: logs
+       buffering:
+          max-staleness: 20s
+          flush-trigger-size: 25KB
+    sinks:
+       file-groups:
+          health:
+             channels: HEALTH
+             buffering:
+                max-staleness: 5s  # Override max-staleness for this sink.
+          ops:
+             channels: OPS
+             buffering: NONE  # Disable buffering for this sink.
+
+
+| Field | Description |
+|--|--|
+| `max-staleness` | the maximum time a log message will sit in the buffer before a flush is triggered. |
+| `flush-trigger-size` | the number of bytes that will trigger the buffer to flush. |
+| `max-in-flight` | the maximum number of buffered flushes before messages start being dropped. |
+
+

pkg/cli/log_flags_test.go

@@ -39,7 +39,8 @@ func TestSetupLogging(t *testing.T) {
 		`filter: INFO, ` +
 		`format: json-fluent-compact, ` +
 		`redactable: true, ` +
-		`exit-on-error: false}`
+		`exit-on-error: false, ` +
+		`buffering: NONE}`
 	const defaultHTTPConfig = `http-defaults: {` +
 		`method: POST, ` +
 		`unsafe-tls: false, ` +
@@ -48,7 +49,8 @@ func TestSetupLogging(t *testing.T) {
 		`filter: INFO, ` +
 		`format: json-compact, ` +
 		`redactable: true, ` +
-		`exit-on-error: false}`
+		`exit-on-error: false, ` +
+		`buffering: NONE}`
 	stdFileDefaultsRe := regexp.MustCompile(
 		`file-defaults: \{` +
 			`dir: (?P<path>[^,]+), ` +
@@ -57,21 +59,24 @@ func TestSetupLogging(t *testing.T) {
 			`buffered-writes: true, ` +
 			`filter: INFO, ` +
 			`format: crdb-v2, ` +
-			`redactable: true\}`)
+			`redactable: true, ` +
+			`buffering: NONE\}`)
 	fileDefaultsNoMaxSizeRe := regexp.MustCompile(
 		`file-defaults: \{` +
 			`dir: (?P<path>[^,]+), ` +
 			`file-permissions: "0644", ` +
 			`buffered-writes: true, ` +
 			`filter: INFO, ` +
 			`format: crdb-v2, ` +
-			`redactable: true\}`)
+			`redactable: true, ` +
+			`buffering: NONE\}`)
 	const fileDefaultsNoDir = `file-defaults: {` +
 		`file-permissions: "0644", ` +
 		`buffered-writes: true, ` +
 		`filter: INFO, ` +
 		`format: crdb-v2, ` +
-		`redactable: true}`
+		`redactable: true, ` +
+		`buffering: NONE}`
 	const defaultLogDir = `PWD/cockroach-data/logs`
 	stdCaptureFd2Re := regexp.MustCompile(
 		`capture-stray-errors: \{` +
@@ -85,7 +90,8 @@ func TestSetupLogging(t *testing.T) {
 			`buffered-writes: (?P<buf>[^,]+), ` +
 			`filter: INFO, ` +
 			`format: (?P<format>[^,]+), ` +
-			`redactable: true\}`)
+			`redactable: true, ` +
+			`buffering: NONE\}`)
 	telemetryFileCfgRe := regexp.MustCompile(
 		`\{channels: \{INFO: \[TELEMETRY\]\}, ` +
 			`dir: (?P<path>[^,]+), ` +
@@ -95,18 +101,21 @@ func TestSetupLogging(t *testing.T) {
 			`buffered-writes: true, ` +
 			`filter: INFO, ` +
 			`format: crdb-v2, ` +
-			`redactable: true\}`)
+			`redactable: true, ` +
+			`buffering: NONE\}`)
 
 	stderrCfgRe := regexp.MustCompile(
 		`stderr: {channels: \{(?P<level>[^:]+): all\}, ` +
 			`filter: [^,]+, ` +
 			`format: crdb-v2-tty, ` +
-			`redactable: (?P<redactable>[^}]+)}`)
+			`redactable: (?P<redactable>[^}]+), ` +
+			`buffering: NONE}`)
 
 	stderrCfgNoneRe := regexp.MustCompile(
 		`stderr: {filter: NONE, ` +
 			`format: crdb-v2-tty, ` +
-			`redactable: (?P<redactable>[^}]+)}`)
+			`redactable: (?P<redactable>[^}]+), ` +
+			`buffering: NONE}`)
 
 	wd, err := os.Getwd()
 	if err != nil {

pkg/cli/testdata/logflags

@@ -482,7 +482,8 @@ file-permissions: "0644",
 buffered-writes: true,
 filter: INFO,
 format: crdb-v2,
-redactable: true}},
+redactable: true,
+buffering: NONE}},
 <stderrEnabledWarningNoRedaction>}}
 
 # For servers, --logtostderr overrides the threshold and keeps

pkg/cmd/cmpconn/conn.go

@@ -249,7 +249,7 @@ ReadRows:
 				first = vals
 			} else {
 				if err := CompareVals(first, vals); err != nil {
-					return false, fmt.Errorf("compare %s to %s:\n%v", firstName, name, err)
+					return false, errors.Wrapf(err, "compare %s to %s", firstName, name)
 				}
 			}
 		}

pkg/cmd/docgen/extract/extract.go

@@ -58,7 +58,7 @@ func GenerateRRJar(jar string, bnf []byte) ([]byte, error) {
 
 	out, err := cmd.CombinedOutput()
 	if err != nil {
-		return nil, fmt.Errorf("%s: %s", err, out)
+		return nil, fmt.Errorf("%w: %s", err, out)
 	}
 	return out, nil
 }

pkg/cmd/roachprod-stress/main.go

@@ -79,7 +79,7 @@ func run() error {
 	{
 		fi, err := os.Stat(pkg)
 		if err != nil {
-			return fmt.Errorf("the pkg flag %q is not a directory relative to the current working directory: %v", pkg, err)
+			return errors.Wrapf(err, "the pkg flag %q is not a directory relative to the current working directory", pkg)
 		}
 		if !fi.Mode().IsDir() {
 			return fmt.Errorf("the pkg flag %q is not a directory relative to the current working directory", pkg)
@@ -88,7 +88,7 @@ func run() error {
 		// Verify that the test binary exists.
 		fi, err = os.Stat(localTestBin)
 		if err != nil {
-			return fmt.Errorf("test binary %q does not exist: %v", localTestBin, err)
+			return errors.Wrapf(err, "test binary %q does not exist", localTestBin)
 		}
 		if !fi.Mode().IsRegular() {
 			return fmt.Errorf("test binary %q is not a file", localTestBin)
@@ -113,19 +113,19 @@ func run() error {
 	}
 	if *flagFailure != "" {
 		if _, err := regexp.Compile(*flagFailure); err != nil {
-			return fmt.Errorf("bad failure regexp: %s", err)
+			return errors.Wrap(err, "bad failure regexp")
 		}
 	}
 	if *flagIgnore != "" {
 		if _, err := regexp.Compile(*flagIgnore); err != nil {
-			return fmt.Errorf("bad ignore regexp: %s", err)
+			return errors.Wrap(err, "bad ignore regexp")
 		}
 	}
 
 	cmd := exec.Command("roachprod", "status", cluster)
 	out, err := cmd.CombinedOutput()
 	if err != nil {
-		return fmt.Errorf("%v\n%s", err, out)
+		return errors.Wrapf(err, "%s", out)
 	}
 	nodes := strings.Count(string(out), "\n") - 1
 
@@ -160,15 +160,15 @@ func run() error {
 		tmpPath := "testdata" + strconv.Itoa(rand.Int())
 		cmd = exec.Command("roachprod", "run", cluster, "--", "rm", "-rf", testdataPath)
 		if output, err := cmd.CombinedOutput(); err != nil {
-			return fmt.Errorf("failed to remove old testdata: %v:\n%s", err, output)
+			return errors.Wrapf(err, "failed to remove old testdata:\n%s", output)
 		}
 		cmd = exec.Command("roachprod", "put", cluster, testdataPath, tmpPath)
 		if err := cmd.Run(); err != nil {
-			return fmt.Errorf("failed to copy testdata: %v", err)
+			return errors.Wrap(err, "failed to copy testdata")
 		}
 		cmd = exec.Command("roachprod", "run", cluster, "mv", tmpPath, testdataPath)
 		if err := cmd.Run(); err != nil {
-			return fmt.Errorf("failed to move testdata: %v", err)
+			return errors.Wrap(err, "failed to move testdata")
 		}
 	}
 	testBin := filepath.Join(pkg, localTestBin)
@@ -310,7 +310,7 @@ func run() error {
 				}
 				return err
 			default:
-				return fmt.Errorf("unexpected context error: %v", err)
+				return errors.Wrap(err, "unexpected context error")
 			}
 		}
 	}

pkg/cmd/roachprod/main.go

@@ -63,22 +63,21 @@ destroy the cluster.
 }
 
 var (
-	numNodes           int
-	numRacks           int
-	username           string
-	dryrun             bool
-	destroyAllMine     bool
-	destroyAllLocal    bool
-	extendLifetime     time.Duration
-	wipePreserveCerts  bool
-	listDetails        bool
-	listJSON           bool
-	listMine           bool
-	listPattern        string
-	sqlCockroachBinary = "cockroach"
-	secure             = false
-	extraSSHOptions    = ""
-	nodeEnv            = []string{
+	numNodes          int
+	numRacks          int
+	username          string
+	dryrun            bool
+	destroyAllMine    bool
+	destroyAllLocal   bool
+	extendLifetime    time.Duration
+	wipePreserveCerts bool
+	listDetails       bool
+	listJSON          bool
+	listMine          bool
+	listPattern       string
+	secure            = false
+	extraSSHOptions   = ""
+	nodeEnv           = []string{
 		"COCKROACH_ENABLE_RPC_COMPRESSION=false",
 		"COCKROACH_UI_RELEASE_NOTES_SIGNUP_DISMISSED=true",
 	}
@@ -795,7 +794,7 @@ var sqlCmd = &cobra.Command{
 	Long:  "Run `cockroach sql` on a remote cluster.\n",
 	Args:  cobra.MinimumNArgs(1),
 	Run: wrap(func(cmd *cobra.Command, args []string) error {
-		return roachprod.SQL(clusterOpts(args[0]), sqlCockroachBinary, args[1:])
+		return roachprod.SQL(clusterOpts(args[0]), args[1:])
 	}),
 }
 
@@ -1134,7 +1133,7 @@ func main() {
 			fallthrough
 		case sqlCmd:
 			cmd.Flags().StringVarP(
-				&sqlCockroachBinary, "binary", "b", "cockroach",
+				&config.Binary, "binary", "b", config.Binary,
 				"the remote cockroach binary to use")
 			fallthrough
 		case pgurlCmd, adminurlCmd:

pkg/cmd/roachtest/tests/canary.go

@@ -26,6 +26,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/cmd/roachtest/option"
 	"github.com/cockroachdb/cockroach/pkg/cmd/roachtest/test"
 	"github.com/cockroachdb/cockroach/pkg/util/retry"
+	"github.com/cockroachdb/errors"
 )
 
 // This file contains common elements for all 3rd party test suite roachtests.
@@ -131,7 +132,7 @@ func repeatRunE(
 		}
 		return nil
 	}
-	return fmt.Errorf("all attempts failed for %s due to error: %s", operation, lastError)
+	return errors.Wrapf(lastError, "all attempts failed for %s", operation)
 }
 
 // repeatRunWithBuffer is the same function as c.RunWithBuffer but with an
@@ -164,7 +165,7 @@ func repeatRunWithBuffer(
 		}
 		return lastResult, nil
 	}
-	return nil, fmt.Errorf("all attempts failed for %s, with error: %s\n%s", operation, lastError, lastResult)
+	return nil, errors.Wrapf(lastError, "all attempts failed for %s\n%s", operation, lastResult)
 }
 
 // repeatGitCloneE is the same function as c.GitCloneE but with an automatic
@@ -193,7 +194,7 @@ func repeatGitCloneE(
 		}
 		return nil
 	}
-	return fmt.Errorf("could not clone %s due to error: %s", src, lastError)
+	return errors.Wrapf(lastError, "could not clone %s", src)
 }
 
 // repeatGetLatestTag fetches the latest (sorted) tag from a github repo.
@@ -290,7 +291,7 @@ func repeatGetLatestTag(
 
 		return releaseTags[len(releaseTags)-1].tag, nil
 	}
-	return "", fmt.Errorf("could not get tags from %s, due to error: %s", url, lastError)
+	return "", errors.Wrapf(lastError, "could not get tags from %s", url)
 }
 
 // gitCloneWithRecurseSubmodules clones a git repo from src into dest and checks out origin's

pkg/kv/kvserver/BUILD.bazel

@@ -83,6 +83,7 @@ go_library(
         "store_rebalancer.go",
         "store_remove_replica.go",
         "store_replica_btree.go",
+        "store_replicas_by_rangeid.go",
         "store_send.go",
         "store_snapshot.go",
         "store_split.go",

pkg/kv/kvserver/helpers_test.go

@@ -21,7 +21,6 @@ import (
 	"math/rand"
 	"testing"
 	"time"
-	"unsafe"
 
 	circuit "github.com/cockroachdb/circuitbreaker"
 	"github.com/cockroachdb/cockroach/pkg/kv"
@@ -515,21 +514,20 @@ func WriteRandomDataToRange(
 }
 
 func WatchForDisappearingReplicas(t testing.TB, store *Store) {
-	m := make(map[int64]struct{})
+	m := make(map[roachpb.RangeID]struct{})
 	for {
 		select {
 		case <-store.Stopper().ShouldQuiesce():
 			return
 		default:
 		}
 
-		store.mu.replicas.Range(func(k int64, v unsafe.Pointer) bool {
-			m[k] = struct{}{}
-			return true
+		store.mu.replicasByRangeID.Range(func(repl *Replica) {
+			m[repl.RangeID] = struct{}{}
 		})
 
 		for k := range m {
-			if _, ok := store.mu.replicas.Load(k); !ok {
+			if _, ok := store.mu.replicasByRangeID.Load(k); !ok {
 				t.Fatalf("r%d disappeared from Store.mu.replicas map", k)
 			}
 		}