doctor: refactor for testing

Release note: none.

pkg/cli/doctor.go

@@ -14,6 +14,7 @@ import (
 	"bufio"
 	"database/sql/driver"
 	hx "encoding/hex"
+	"fmt"
 	"io"
 	"os"
 	"path"
@@ -51,7 +52,7 @@ var doctorZipDirCmd = &cobra.Command{
 Run doctor tool on system data from directory	created by unzipping debug.zip.
 `,
 	Args: cobra.ExactArgs(1),
-	RunE: RunZipDirDoctor,
+	RunE: runZipDirDoctor,
 }
 
 var doctorClusterCmd = &cobra.Command{
@@ -61,11 +62,24 @@ var doctorClusterCmd = &cobra.Command{
 Run doctor tool reading system data from a live cluster specified by --url.
 `,
 	Args: cobra.NoArgs,
-	RunE: MaybeDecorateGRPCError(RunClusterDoctor),
+	RunE: MaybeDecorateGRPCError(runClusterDoctor),
 }
 
-// RunClusterDoctor runs the doctors tool reading data from a live cluster.
-func RunClusterDoctor(cmd *cobra.Command, args []string) (retErr error) {
+func wrapExamine(descTable []doctor.DescriptorTableRow) error {
+	// TODO(spaskob): add --verbose flag.
+	valid, err := doctor.Examine(descTable, false, os.Stdout)
+	if err != nil {
+		return &cliError{exitCode: 2, cause: errors.Wrap(err, "examine failed")}
+	}
+	if !valid {
+		return &cliError{exitCode: 1, cause: errors.New("validation failed")}
+	}
+	fmt.Println("No problems found!")
+	return nil
+}
+
+// runClusterDoctor runs the doctors tool reading data from a live cluster.
+func runClusterDoctor(cmd *cobra.Command, args []string) (retErr error) {
 	sqlConn, err := makeSQLClient("cockroach doctor", useSystemDb)
 	if err != nil {
 		return errors.Wrap(err, "could not establish connection to cluster")
@@ -117,15 +131,11 @@ ORDER BY id`,
 		descTable = append(descTable, row)
 	}
 
-	// TODO(spaskob): add --verbose flag.
-	if err := doctor.Examine(descTable, false); err != nil {
-		return errors.Wrap(err, "examine failed")
-	}
-	return nil
+	return wrapExamine(descTable)
 }
 
-// RunZipDirDoctor runs the doctors tool reading data from a debug zip dir.
-func RunZipDirDoctor(cmd *cobra.Command, args []string) (retErr error) {
+// runZipDirDoctor runs the doctors tool reading data from a debug zip dir.
+func runZipDirDoctor(cmd *cobra.Command, args []string) (retErr error) {
 	// To make parsing user functions code happy.
 	_ = builtins.AllBuiltinNames
 
@@ -158,9 +168,5 @@ func RunZipDirDoctor(cmd *cobra.Command, args []string) (retErr error) {
 		descTable = append(descTable, doctor.DescriptorTableRow{ID: int64(i), DescBytes: descBytes, ModTime: ts})
 	}
 
-	// TODO(spaskob): add --verbose flag.
-	if err := doctor.Examine(descTable, false); err != nil {
-		return errors.Wrap(err, "examine failed")
-	}
-	return nil
+	return wrapExamine(descTable)
 }

pkg/cli/testdata/doctor/testcluster

@@ -3,3 +3,4 @@ doctor cluster
 debug doctor cluster
 Examining 32 descriptors...
 Table  52: parentID 50  does not exist
+ERROR: validation failed

pkg/cli/testdata/doctor/testzipdir

@@ -8,3 +8,4 @@ Table  55: parentID 52  does not exist
 Table  56: parentID 52  does not exist
 Table  57: parentID 52  does not exist
 Table  58: parentID 52  does not exist
+ERROR: validation failed

pkg/sql/doctor/doctor.go

@@ -8,11 +8,14 @@
 // by the Apache License, Version 2.0, included in the file
 // licenses/APL.txt.
 
+// Package doctor provides utilities for checking the consistency of cockroach
+// internal persisted metadata.
 package doctor
 
 import (
 	"context"
 	"fmt"
+	"io"
 
 	"github.com/cockroachdb/cockroach/pkg/keys"
 	"github.com/cockroachdb/cockroach/pkg/roachpb"
@@ -52,36 +55,35 @@ func NewProtoGetter(rows []DescriptorTableRow) (*sqlbase.MapProtoGetter, error)
 }
 
 // Examine runs a suite of consistency checks over the descriptor table.
-func Examine(descTable []DescriptorTableRow, verbose bool) error {
-	fmt.Printf("Examining %d descriptors...\n", len(descTable))
+func Examine(descTable []DescriptorTableRow, verbose bool, stdout io.Writer) (ok bool, err error) {
+	fmt.Fprintf(stdout, "Examining %d descriptors...\n", len(descTable))
 	protoGetter, err := NewProtoGetter(descTable)
 	if err != nil {
-		return err
+		return false, err
 	}
 	var problemsFound bool
 	for _, row := range descTable {
 		desc := protoGetter.Protos[string(key(row.ID))].(*descpb.Descriptor)
 		t := sqlbase.TableFromDescriptor(desc, hlc.Timestamp{})
 		if t == nil {
+			// So far we only examine table descriptors. We may add checks for other
+			// descriptors in later versions.
 			continue
 		}
 		ts := hlc.Timestamp{WallTime: timeutil.Now().UnixNano()}
 		sqlbase.MaybeSetDescriptorModificationTimeFromMVCCTimestamp(context.Background(), desc, ts)
 		table := sqlbase.NewImmutableTableDescriptor(*sqlbase.TableFromDescriptor(desc, ts))
 		if int64(table.ID) != row.ID {
-			fmt.Printf("Table %3d: different id in the descriptor: %d", row.ID, table.ID)
+			fmt.Fprintf(stdout, "Table %3d: different id in the descriptor: %d", row.ID, table.ID)
 			problemsFound = true
 			continue
 		}
 		if err := table.Validate(context.Background(), protoGetter, keys.SystemSQLCodec); err != nil {
 			problemsFound = true
-			fmt.Printf("Table %3d: %s\n", table.ID, err.Error())
+			fmt.Fprintf(stdout, "Table %3d: %s\n", table.ID, err)
 		} else if verbose {
-			fmt.Printf("Table %3d: validated\n", table.ID)
+			fmt.Fprintf(stdout, "Table %3d: validated\n", table.ID)
 		}
 	}
-	if !problemsFound {
-		fmt.Println("No problems found!")
-	}
-	return nil
+	return !problemsFound, nil
 }

pkg/sql/doctor/doctor_test.go

@@ -0,0 +1,81 @@
+// Copyright 2020 The Cockroach Authors.
+//
+// Use of this software is governed by the Business Source License
+// included in the file licenses/BSL.txt.
+//
+// As of the Change Date specified in that file, in accordance with
+// the Business Source License, use of this software will be governed
+// by the Apache License, Version 2.0, included in the file
+// licenses/APL.txt.
+
+package doctor_test
+
+import (
+	"bytes"
+	"fmt"
+	"testing"
+
+	"github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb"
+	"github.com/cockroachdb/cockroach/pkg/sql/doctor"
+	"github.com/cockroachdb/cockroach/pkg/util/leaktest"
+	"github.com/cockroachdb/cockroach/pkg/util/log"
+	"github.com/cockroachdb/cockroach/pkg/util/protoutil"
+	"github.com/stretchr/testify/require"
+)
+
+func TestExamine(t *testing.T) {
+	defer leaktest.AfterTest(t)()
+	defer log.Scope(t).Close(t)
+
+	toBytes := func(tableDesc *descpb.TableDescriptor) []byte {
+		desc := descpb.Descriptor{
+			Union: &descpb.Descriptor_Table{
+				Table: tableDesc,
+			},
+		}
+		res, err := protoutil.Marshal(&desc)
+		require.NoError(t, err)
+		return res
+	}
+
+	tests := []struct {
+		descTable []doctor.DescriptorTableRow
+		valid     bool
+		errStr    string
+		expected  string
+	}{
+		{
+			valid:    true,
+			expected: "Examining 0 descriptors...\n",
+		},
+		{
+			descTable: []doctor.DescriptorTableRow{{ID: 1, DescBytes: []byte("#$@#@#$#@#")}},
+			errStr:    "failed to unmarshal descriptor",
+			expected:  "Examining 1 descriptors...\n",
+		},
+		{
+			descTable: []doctor.DescriptorTableRow{{ID: 1, DescBytes: toBytes(&descpb.TableDescriptor{ID: 2})}},
+			errStr:    "",
+			expected:  "Examining 1 descriptors...\nTable   1: different id in the descriptor: 2",
+		},
+		{
+			descTable: []doctor.DescriptorTableRow{
+				{ID: 1, DescBytes: toBytes(&descpb.TableDescriptor{Name: "foo", ID: 1})},
+			},
+			expected: "Examining 1 descriptors...\nTable   1: invalid parent ID 0\n",
+		},
+	}
+
+	for i, test := range tests {
+		var buf bytes.Buffer
+		valid, err := doctor.Examine(test.descTable, false, &buf)
+		msg := fmt.Sprintf("Test %d failed!", i+1)
+		if test.errStr != "" {
+			require.Containsf(t, err.Error(), test.errStr, msg)
+		} else {
+			require.NoErrorf(t, err, msg)
+		}
+		require.Equalf(t, test.valid, valid, msg)
+		require.Equalf(t, test.expected, buf.String(), msg)
+	}
+}