storage: fix and test a bogus source of replica divergence errors

pkg/cmd/roachtest/cluster.go

@@ -1041,39 +1041,75 @@ func (c *cluster) FailOnDeadNodes(ctx context.Context, t *test) {
 	})
 }
 
-// FailOnReplicaDivergence fails the test if
-// crdb_internal.check_consistency(true, '', '') indicates that any ranges'
-// replicas are inconsistent with each other.
-func (c *cluster) FailOnReplicaDivergence(ctx context.Context, t *test) {
-	if c.nodes < 1 {
-		return // unit tests
-	}
-	// TODO(tbg): n1 isn't necessarily online at this point. Try to sniff out
-	// a node that is.
-	db := c.Conn(ctx, 1)
-	defer db.Close()
-
-	c.l.Printf("running (fast) consistency checks")
+// CheckReplicaDivergenceOnDB runs a fast consistency check of the whole keyspace
+// against the provided db. If an inconsistency is found, it returns it in the
+// error. Note that this will swallow errors returned directly from the consistency
+// check since we know that such spurious errors are possibly without any relation
+// to the check having failed.
+func (c *cluster) CheckReplicaDivergenceOnDB(ctx context.Context, db *gosql.DB) error {
 	rows, err := db.QueryContext(ctx, `
 SELECT t.range_id, t.start_key_pretty, t.status, t.detail
 FROM
 crdb_internal.check_consistency(true, '', '') as t
 WHERE t.status NOT IN ('RANGE_CONSISTENT', 'RANGE_INDETERMINATE')`)
 	if err != nil {
-		c.l.Printf("%s", err)
-		return
+		// TODO(tbg): the checks can fail for silly reasons like missing gossiped
+		// descriptors, etc. -- not worth failing the test for. Ideally this would
+		// be rock solid.
+		c.l.Printf("consistency check failed with %v; ignoring", err)
+		return nil
 	}
+	var buf bytes.Buffer
 	for rows.Next() {
 		var rangeID int32
 		var prettyKey, status, detail string
 		if err := rows.Scan(&rangeID, &prettyKey, &status, &detail); err != nil {
-			c.l.Printf("%s", err)
-			break
+			return err
 		}
-		t.Fatalf("r%d (%s) is inconsistent: %s %s", rangeID, prettyKey, status, detail)
+		fmt.Fprintf(&buf, "r%d (%s) is inconsistent: %s %s\n", rangeID, prettyKey, status, detail)
 	}
 	if err := rows.Err(); err != nil {
-		c.l.Printf("%s", err)
+		return err
+	}
+
+	msg := buf.String()
+	if msg != "" {
+		return errors.New(msg)
+	}
+	return nil
+}
+
+// FailOnReplicaDivergence fails the test if
+// crdb_internal.check_consistency(true, '', '') indicates that any ranges'
+// replicas are inconsistent with each other. It uses the first node that
+// is up to run the query.
+func (c *cluster) FailOnReplicaDivergence(ctx context.Context, t *test) {
+	if c.nodes < 1 {
+		return // unit tests
+	}
+
+	// Find a live node to run against, if one exists.
+	var db *gosql.DB
+	for i := 1; i <= c.nodes; i++ {
+		db = c.Conn(ctx, i)
+		_, err := db.Exec(`SELECT 1`)
+		if err != nil {
+			_ = db.Close()
+			db = nil
+			continue
+		}
+		c.l.Printf("running (fast) consistency checks on node %d", i)
+		break
+	}
+	if db == nil {
+		c.l.Printf("no live node found, skipping consistency check")
+		return
+	}
+
+	defer db.Close()
+
+	if err := c.CheckReplicaDivergenceOnDB(ctx, db); err != nil {
+		t.Fatal(err)
 	}
 }
 

pkg/cmd/roachtest/version.go

@@ -24,6 +24,7 @@ import (
 
 	"github.com/cockroachdb/cockroach/pkg/util/binfetcher"
 	_ "github.com/lib/pq"
+	"github.com/pkg/errors"
 )
 
 func registerVersion(r *registry) {
@@ -95,7 +96,7 @@ func registerVersion(r *registry) {
 				// Make sure everyone is still running.
 				for i := 1; i <= nodes; i++ {
 					t.WorkerStatus("checking ", i)
-					db := c.Conn(ctx, 1)
+					db := c.Conn(ctx, i)
 					defer db.Close()
 					rows, err := db.Query(`SHOW DATABASES`)
 					if err != nil {
@@ -104,6 +105,10 @@ func registerVersion(r *registry) {
 					if err := rows.Close(); err != nil {
 						return err
 					}
+					// Regression test for #37425.
+					if err := c.CheckReplicaDivergenceOnDB(ctx, db); err != nil {
+						return errors.Wrapf(err, "node %d", i)
+					}
 				}
 				return nil
 			}

pkg/server/admin.go

@@ -19,6 +19,7 @@ import (
 	"context"
 	"encoding/json"
 	"fmt"
+	"os"
 	"sort"
 	"strconv"
 	"strings"
@@ -1353,12 +1354,12 @@ func (s *adminServer) Drain(req *serverpb.DrainRequest, stream serverpb.Admin_Dr
 		return nil
 	}
 
-	s.server.grpc.Stop()
-
 	go func() {
-		// The explicit closure here allows callers.Lookup() to return something
-		// sensible referring to this file (otherwise it ends up in runtime
-		// internals).
+		// TODO(tbg): why don't we stop the stopper first? Stopping the stopper
+		// first seems more reasonable since grpc.Stop closes the listener right
+		// away (and who knows whether gRPC-goroutines are tied up in some
+		// stopper task somewhere).
+		s.server.grpc.Stop()
 		s.server.stopper.Stop(ctx)
 	}()
 
@@ -1367,6 +1368,23 @@ func (s *adminServer) Drain(req *serverpb.DrainRequest, stream serverpb.Admin_Dr
 		return nil
 	case <-ctx.Done():
 		return ctx.Err()
+	case <-time.After(10 * time.Second):
+		// This is a hack to work around the problem in
+		// https://github.com/cockroachdb/cockroach/issues/37425#issuecomment-494336131
+		//
+		// There appear to be deadlock scenarios in which we don't manage to
+		// fully stop the grpc server (which implies closing the listener, i.e.
+		// seeming dead to the outside world) or don't manage to shut down the
+		// stopper (the evidence in #37425 is inconclusive which one it is).
+		//
+		// Other problems in this area are known, such as
+		// https://github.com/cockroachdb/cockroach/pull/31692
+		//
+		// The signal-based shutdown path uses a similar time-based escape hatch.
+		// Until we spend (potentially lots of time to) understand and fix this
+		// issue, this will serve us well.
+		os.Exit(1)
+		return errors.New("unreachable")
 	}
 }
 

pkg/sql/sem/builtins/generator_builtins.go

@@ -962,8 +962,10 @@ func (c *checkConsistencyGenerator) Start() error {
 			Key:    c.from,
 			EndKey: c.to,
 		},
-		Mode:     c.mode,
-		WithDiff: true,
+		Mode: c.mode,
+		// No meaningful diff can be created if we're checking the stats only,
+		// so request one only if a full check is run.
+		WithDiff: c.mode == roachpb.ChecksumMode_CHECK_FULL,
 	})
 	if err := c.db.Run(c.ctx, &b); err != nil {
 		return err

pkg/storage/batcheval/cmd_compute_checksum.go

@@ -42,7 +42,7 @@ func declareKeysComputeChecksum(
 // Version numbers for Replica checksum computation. Requests silently no-op
 // unless the versions are compatible.
 const (
-	ReplicaChecksumVersion    = 3
+	ReplicaChecksumVersion    = 4
 	ReplicaChecksumGCInterval = time.Hour
 )