Skip to content

Commit

Permalink
cli: enforce inclusion of new crdb_internal tables in debug zip
Browse files Browse the repository at this point in the history 
This is achieved via a unit test. I went through all of the missing
tables and made judgement calls on whether they needed to be included.

This resulted in the inclusion of some obvious earlier omissions.

I also went through the crdb_internal tables and added a naming
convention and tried to add TODOs to all methods violating it.
It's a bad idea to rename any of these; we should probably alias
them to their desired name and leave the old name valid, somehow
without listing it in `SHOW TABLES FROM crdb_internal`.

Release note (bug fix): Data that was previously omitted from `debug
zip` is now included.
  • Loading branch information
@tbg
tbg committed Mar 28, 2019
1 parent 9f0384e commit 56d47ca
Show file tree
Hide file tree
Showing 4 changed files with 163 additions and 43 deletions.
76 changes: 67 additions & 9 deletions pkg/cli/cli_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import (
"path/filepath"
"reflect"
"regexp"
"sort"
"strconv"
"strings"
"testing"
Expand All @@ -47,6 +48,7 @@ import (
// register some workloads for TestWorkload
_ "github.com/cockroachdb/cockroach/pkg/workload/examples"
"github.com/pkg/errors"
"github.com/stretchr/testify/assert"
)

type cliTest struct {
Expand Down Expand Up @@ -1879,9 +1881,7 @@ Use "cockroach [command] --help" for more information about a command.
}
got := strings.Join(final, "\n")

if got != test.expected {
t.Errorf("got:\n%s\n----\nexpected:\n%s", got, test.expected)
}
assert.Equal(t, test.expected, got)
})
}
}
Expand Down Expand Up @@ -2288,6 +2288,54 @@ func TestJunkPositionalArguments(t *testing.T) {
}
}

// TestZipContainsAllInternalTables verifies that we don't add new internal tables
// without also taking them into account in a `debug zip`. If this test fails,
// add your table to either of the []string slices referenced in the test (which
// are used by `debug zip`) or add it as an exception after having verified that
// it indeed should not be collected (this is rare).
// NB: if you're adding a new one, you'll also have to update TestZip.
func TestZipContainsAllInternalTables(t *testing.T) {
defer leaktest.AfterTest(t)()

s, db, _ := serverutils.StartServer(t, base.TestServerArgs{})
defer s.Stopper().Stop(context.Background())

rows, err := db.Query(`
SELECT concat('crdb_internal.', table_name) as name FROM [ SHOW TABLES FROM crdb_internal ] WHERE
table_name NOT IN (
-- whitelisted tables that don't need to be in debug zip
'backward_dependencies',
'builtin_functions',
'create_statements',
'forward_dependencies',
'index_columns',
'table_columns',
'table_indexes',
'ranges',
'ranges_no_leases',
'predefined_comments',
'session_trace',
'session_variables',
'tables'
)
ORDER BY name ASC`)
assert.NoError(t, err)

var tables []string
for rows.Next() {
var table string
assert.NoError(t, rows.Scan(&table))
tables = append(tables, table)
}

var exp []string
exp = append(exp, debugZipTablesPerNode...)
exp = append(exp, debugZipTablesPerCluster...)
sort.Strings(exp)

assert.Equal(t, exp, tables)
}

func TestZip(t *testing.T) {
defer leaktest.AfterTest(t)()

Expand All @@ -2306,16 +2354,28 @@ writing ` + os.DevNull + `
debug/liveness.json
debug/settings.json
debug/reports/problemranges.json
debug/crdb_internal.cluster_queries.txt
debug/crdb_internal.cluster_sessions.txt
debug/crdb_internal.cluster_settings.txt
debug/crdb_internal.jobs.txt
debug/crdb_internal.kv_node_status.txt
debug/crdb_internal.kv_store_status.txt
debug/crdb_internal.schema_changes.txt
debug/crdb_internal.partitions.txt
debug/crdb_internal.zones.txt
debug/nodes/1/status.json
debug/nodes/1/crdb_internal.feature_usage.txt
debug/nodes/1/crdb_internal.gossip_alerts.txt
debug/nodes/1/crdb_internal.gossip_liveness.txt
debug/nodes/1/crdb_internal.gossip_network.txt
debug/nodes/1/crdb_internal.gossip_nodes.txt
debug/nodes/1/crdb_internal.leases.txt
debug/nodes/1/crdb_internal.node_statement_statistics.txt
debug/nodes/1/crdb_internal.node_build_info.txt
debug/nodes/1/crdb_internal.node_metrics.txt
debug/nodes/1/crdb_internal.gossip_alerts.txt
debug/nodes/1/queries.txt
debug/nodes/1/sessions.txt
debug/nodes/1/crdb_internal.node_queries.txt
debug/nodes/1/crdb_internal.node_runtime_info.txt
debug/nodes/1/crdb_internal.node_sessions.txt
debug/nodes/1/details.json
debug/nodes/1/gossip.json
debug/nodes/1/stacks.txt
Expand Down Expand Up @@ -2360,9 +2420,7 @@ writing ` + os.DevNull + `
debug/schema/system/zones.json
`

if out != expected {
t.Errorf("expected:\n%s\ngot:\n%s", expected, out)
}
assert.Equal(t, expected, out)
}

func TestWorkload(t *testing.T) {
Expand Down
6 changes: 6 additions & 0 deletions pkg/cli/flags.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -231,6 +231,12 @@ func init() {
if strings.HasPrefix(flag.Name, "lightstep_") {
flag.Hidden = true
}
if strings.HasPrefix(flag.Name, "httptest.") {
// If we test the cli commands in tests, we may end up transitively
// importing httptest, for example via `testify/assert`. Make sure
// it doesn't show up in the output or it will confuse tests.
flag.Hidden = true
}
switch flag.Name {
case logflags.NoRedirectStderrName:
flag.Hidden = true
Expand Down
76 changes: 43 additions & 33 deletions pkg/cli/zip.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,41 @@ cluster to be live.
RunE: MaybeDecorateGRPCError(runDebugZip),
}

// Tables containing cluster-wide info that are collected in a debug zip.
var debugZipTablesPerCluster = []string{
"crdb_internal.cluster_queries",
"crdb_internal.cluster_sessions",
"crdb_internal.cluster_settings",

"crdb_internal.jobs",

"crdb_internal.kv_node_status",
"crdb_internal.kv_store_status",

"crdb_internal.schema_changes",
"crdb_internal.partitions",
"crdb_internal.zones",
}

// Tables collected from each node in a debug zip.
var debugZipTablesPerNode = []string{
"crdb_internal.feature_usage",

"crdb_internal.gossip_alerts",
"crdb_internal.gossip_liveness",
"crdb_internal.gossip_network",
"crdb_internal.gossip_nodes",

"crdb_internal.leases",

"crdb_internal.node_statement_statistics",
"crdb_internal.node_build_info",
"crdb_internal.node_metrics",
"crdb_internal.node_queries",
"crdb_internal.node_runtime_info",
"crdb_internal.node_sessions",
}

type zipper struct {
f *os.File
z *zip.Writer
Expand Down Expand Up @@ -235,36 +270,10 @@ func runDebugZip(cmd *cobra.Command, args []string) error {
}
}

type queryAndName struct {
query, name string
}

// These are run only once because they return cluster-wide information.
perClusterQueries := []queryAndName{
{"SELECT * FROM crdb_internal.jobs;", "crdb_internal.jobs"},
{"SELECT * FROM crdb_internal.schema_changes;", "crdb_internal.schema_changes"},
}

// These are run against every node because they reflect local state.
//
// TODO(tbg): ideally we'd discriminate between both kinds of internal tables
// based on some naming schema and then we wouldn't have to keep as many
// explicit lists here, however we've failed to do so in the past and
// now it'll take some effort.
// PS: I may not even have gotten all of them right.
perNodeQueries := []queryAndName{
{"SELECT * FROM crdb_internal.gossip_liveness;", "crdb_internal.gossip_liveness"},
{"SELECT * FROM crdb_internal.gossip_network;", "crdb_internal.gossip_network"},
{"SELECT * FROM crdb_internal.gossip_nodes;", "crdb_internal.gossip_nodes"},
{"SELECT * FROM crdb_internal.node_metrics;", "crdb_internal.node_metrics"},
{"SELECT * FROM crdb_internal.gossip_alerts;", "crdb_internal.gossip_alerts"},
{"SHOW QUERIES;", "queries"},
{"SHOW SESSIONS;", "sessions"},
}

for _, item := range perClusterQueries {
if err := dumpTableDataForZip(z, sqlConn, item.query, base+"/"+item.name+".txt"); err != nil {
return errors.Wrap(err, item.name)
for _, table := range debugZipTablesPerCluster {
query := fmt.Sprintf(`SELECT * FROM %s`, table)
if err := dumpTableDataForZip(z, sqlConn, query, base+"/"+table+".txt"); err != nil {
return errors.Wrap(err, table)
}
}

Expand Down Expand Up @@ -293,9 +302,10 @@ func runDebugZip(cmd *cobra.Command, args []string) error {
return err
}

for _, item := range perNodeQueries {
if err := dumpTableDataForZip(z, curSQLConn, item.query, prefix+"/"+item.name+".txt"); err != nil {
return errors.Wrap(err, item.name)
for _, table := range debugZipTablesPerNode {
query := fmt.Sprintf(`SELECT * FROM %s`, table)
if err := dumpTableDataForZip(z, curSQLConn, query, prefix+"/"+table+".txt"); err != nil {
return errors.Wrap(err, table)
}
}

Expand Down
Loading

0 comments on commit 56d47ca

Please sign in to comment.