cli: add debug doctor command #52622

spaskob · 2020-08-11T11:41:52Z

This PR introduces two new debug commands:
/cockroach debug doctor cluster --url=<cluster_conn_string>
and
/cockroach debug doctor zipdir <debug-dir>

which will validate the descriptors of a live cluster or from an unzipped debug
directory respectfully. The commands print out all descriptor ids that were inspected
and the problems with specific ids if found.

Fixes #52077.
Informs #51153.

Release note: none.

cockroach-teamcity · 2020-08-11T11:42:00Z

This change is

knz

Reviewed 4 of 6 files at r1.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner and @spaskob)

pkg/sql/sqlbase/descriptor.go, line 221 at r1 (raw file):

		// CreateAsOfTime that it is now set.
		if t.Table.Adding() && t.Table.IsAs() && t.Table.CreateAsOfTime.IsEmpty() {
			log.Fatalf(context.TODO(), "table descriptor for %q (%d.%d) is in the "+

I wonder if it would make sense to error out with an assertion error instead of stopping the server in this condition.

pkg/sql/sqlbase/descriptor.go, line 238 at r1 (raw file):

		// are rendered in some other downstream setting we expect the timestamp to
		// be read. This is a hack we shouldn't need to do.
		log.Errorf(context.TODO(), "read table descriptor for %q (%d) without ModificationTime "+

I think the behavior (error vs fatal) should be dependent on a boolean. In a running server, this error should ensure the caller does not get a chance to use the descriptor. See my comment above.

spaskob

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @knz, and @spaskob)

pkg/sql/sqlbase/descriptor.go, line 221 at r1 (raw file):

Previously, knz (kena) wrote…

I wonder if it would make sense to error out with an assertion error instead of stopping the server in this condition.

@ajwerner suggested an approach to read the ts from crdb
_intenal and for debug.zip setting any non-zero ts makes the error go away. We could look into dumping the timestamp in debug.zip as well.

knz

A few general thoughts:

you don't need to invest too much into polish since this is a debug command.
however, you probably do want some tests. So I recommend some tests that injects invalid descriptors and verifies that the function finds the errors
the commit message (and PR description) should be scoped down to what this PR actually does. It's OK to talk about "future plans" in an issue (which is not filed yet as far as I can see, so you should file it and refer to it from the PR). It's not OK to talk about future plans in the commit message. The commit message should describe objectively what's contained in the commit.
the release note is IMHO premature since the work is not complete yet. If you do keep it, you need to start the release note with a description of what tool was introduced. Remember, the release note is pasted into docs without any reference to the PR or any title.

Reviewed 6 of 6 files at r2.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner and @spaskob)

pkg/sql/sqlbase/descriptor.go, line 221 at r1 (raw file):

Previously, spaskob (Spas Bojanov) wrote…

@ajwerner suggested an approach to read the ts from crdb
_intenal and for debug.zip setting any non-zero ts makes the error go away. We could look into dumping the timestamp in debug.zip as well.

The mvcc timestamp is now available from SQL, thanks to the changes by Rohan. Perhaps you can pick that up.

pkg/sql/sqlbase/descriptor.go, line 238 at r1 (raw file):

Previously, knz (kena) wrote…

I think the behavior (error vs fatal) should be dependent on a boolean. In a running server, this error should ensure the caller does not get a chance to use the descriptor. See my comment above.

I still stand by this point.

blathers-crl · 2020-08-13T14:12:10Z

Thank you for updating your pull request.

My owl senses detect your PR is good for review. Please keep an eye out for any test failures in CI.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.}

spaskob

Thanks for the general remarks! I updated the PR description and linked 2 relevant issues. I will be adding the tests shortly.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner and @knz)

pkg/sql/sqlbase/descriptor.go, line 221 at r1 (raw file):

Previously, knz (kena) wrote…

The mvcc timestamp is now available from SQL, thanks to the changes by Rohan. Perhaps you can pick that up.

Done.

pkg/sql/sqlbase/descriptor.go, line 238 at r1 (raw file):

Previously, knz (kena) wrote…

I still stand by this point.

This is unrelated to this PR anymore so I'd leave it to @ajwerner to decide if we need to file an issue to change this function prototype.

knz

This is pretty good for a MVP!
I just have a small suggestion below as to API.

I'd also like to renew my suggestion to unit test the Examine function in addition to the command-line test you already have, to test a larger variety of problem cases, and show the diversity of output messages that can be encountered.

I also recommend you rebase to recover from the flaky test that blocks your CI.

Reviewed 2 of 6 files at r1, 5 of 5 files at r3, 1 of 1 files at r4, 5 of 5 files at r5.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner and @spaskob)

pkg/sql/doctor/doctor.go, line 72 at r3 (raw file):

		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)

I recommend you replace the fmt.Printf by fmt.Fprintf and pass an io.Writer as argument to the function.

This will make your testing easier, as this way you can redirect the output to e.g. a bytes.Buffer and compare strings directly.

spaskob · 2020-08-19T07:01:49Z

PTAL, tests for doctor.Examine added.

knz

Cool, just a few nits and this is good to go.

Reviewed 3 of 8 files at r6, 3 of 5 files at r8, 4 of 5 files at r9.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @knz, and @spaskob)

pkg/cli/doctor.go, line 81 at r9 (raw file):

}

// RunClusterDoctor runs the doctors tool reading data from a live cluster.

nit: if you don't use the functions in a different package, then also un-export them.

pkg/cli/doctor.go, line 138 at r9 (raw file):

// RunZipDirDoctor runs the doctors tool reading data from a debug zip dir.
func RunZipDirDoctor(cmd *cobra.Command, args []string) (retErr error) {

ditto

pkg/sql/doctor/doctor.go, line 56 at r9 (raw file):

// Examine runs a suite of consistency checks over the descriptor table.
func Examine(descTable []DescriptorTableRow, verbose bool, stdout io.Writer) (bool, error) {

Our style recommendation is to name the return values when it's not clear what they mean.
Generally, if a function returns a bool and it's not a predicate (ie a function named "IsXXXX" where the bool clearly says yes/no), then it's unclear what the bool is about.

So the "bool" return here should probably be named "ok".

pkg/sql/doctor/doctor.go, line 67 at r9 (raw file):

		t := sqlbase.TableFromDescriptor(desc, hlc.Timestamp{})
		if t == nil {
			continue

I recommend a comment here to explain what it means to ignore the table descriptor.

pkg/sql/doctor/doctor.go, line 79 at r9 (raw file):

		if err := table.Validate(context.Background(), protoGetter, keys.SystemSQLCodec); err != nil {
			problemsFound = true
			fmt.Fprintf(stdout, "Table %3d: %s\n", table.ID, err.Error())

You don't need to call .Error() on err. Printf does it for you already.

pkg/sql/sqlbase/descriptor.go, line 239 at r9 (raw file):

		// be read. This is a hack we shouldn't need to do.
		log.Fatalf(context.TODO(), "read table descriptor for %q (%d) without ModificationTime "+
			"with zero MVCC timestamp; full descriptor:\n%s", GetDescriptorName(desc), GetDescriptorID(desc), desc.String())

FYI, printf-style formatting already calls .String() on objects.

The only reason why we sometimes call .String() explicitly is to avoid the object escaping to the heap, if it wasn't on the heap already.

In this particular case, you're receiving desc by reference, so it's already on the heap, so no need to call .String().

spaskob

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner and @knz)

pkg/cli/doctor.go, line 138 at r9 (raw file):

Previously, knz (kena) wrote…

ditto

Done.

pkg/sql/doctor/doctor.go, line 56 at r9 (raw file):

Previously, knz (kena) wrote…

Our style recommendation is to name the return values when it's not clear what they mean.
Generally, if a function returns a bool and it's not a predicate (ie a function named "IsXXXX" where the bool clearly says yes/no), then it's unclear what the bool is about.

So the "bool" return here should probably be named "ok".

Done.

pkg/sql/doctor/doctor.go, line 67 at r9 (raw file):

Previously, knz (kena) wrote…

I recommend a comment here to explain what it means to ignore the table descriptor.

Done.

pkg/sql/doctor/doctor.go, line 79 at r9 (raw file):

Previously, knz (kena) wrote…

You don't need to call .Error() on err. Printf does it for you already.

Done.

spaskob

PTAL

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner and @knz)

knz

nice :)

Reviewed 1 of 5 files at r12, 1 of 5 files at r13.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @ajwerner and @knz)

ajwerner

Reviewed 1 of 9 files at r10, 1 of 5 files at r12, 3 of 5 files at r13.
Reviewable status: complete! 2 of 0 LGTMs obtained (waiting on @ajwerner, @knz, and @spaskob)

pkg/sql/doctor/doctor.go, line 11 at r13 (raw file):

// licenses/APL.txt.

package doctor

nit: can you add a doc comment

This commit intorduces two new debug commands: `/cockroach debug doctor cluster --url=<cluster_conn_string>` and `/cockroach debug doctor zipdir <debug-dir>` which validate the descriptors read from a live cluster or from an unzipped debug zip directory. The commands print out all descriptor ids that were inspected and the problems with specific ids if found. Fixes cockroachdb#52077. Informs cockroachdb#51153. Release note: none.

We were not testing correctly for the length of the argument list which could result in segfault. Release note: none.

Release note: none.

spaskob · 2020-08-20T12:40:02Z

bors r+

craig · 2020-08-20T13:29:18Z

Build succeeded:

GitHub CI (Cockroach)

spaskob requested a review from ajwerner August 11, 2020 11:41

spaskob requested a review from a team as a code owner August 11, 2020 11:41

knz reviewed Aug 11, 2020

View reviewed changes

spaskob force-pushed the tool branch from 9e738f4 to 8157c8a Compare August 13, 2020 10:08

spaskob changed the title ~~(WIP)cli: add debug doctor command~~ cli: add debug doctor command Aug 13, 2020

spaskob force-pushed the tool branch from 8157c8a to c4273fd Compare August 13, 2020 10:11

spaskob commented Aug 13, 2020

View reviewed changes

knz reviewed Aug 13, 2020

View reviewed changes

spaskob force-pushed the tool branch from c4273fd to 1666ce1 Compare August 13, 2020 14:12

blathers-crl bot added the O-community Originated from the community label Aug 13, 2020

spaskob force-pushed the tool branch from 1666ce1 to 07041a9 Compare August 14, 2020 06:33

spaskob commented Aug 14, 2020

View reviewed changes

spaskob force-pushed the tool branch 3 times, most recently from 925c6d8 to 0117928 Compare August 18, 2020 10:46

knz reviewed Aug 18, 2020

View reviewed changes

spaskob force-pushed the tool branch 4 times, most recently from 9d5ce0b to 3d86acc Compare August 19, 2020 07:01

knz reviewed Aug 19, 2020

View reviewed changes

spaskob force-pushed the tool branch from 3d86acc to 1559ff2 Compare August 19, 2020 08:05

spaskob commented Aug 19, 2020

View reviewed changes

knz approved these changes Aug 19, 2020

View reviewed changes

ajwerner approved these changes Aug 19, 2020

View reviewed changes

spaskob force-pushed the tool branch 3 times, most recently from c15f28f to 6fba86e Compare August 20, 2020 08:11

Spas Bojanov added 4 commits August 20, 2020 11:43

cli: fix a test bug

b4c7f6e

We were not testing correctly for the length of the argument list which could result in segfault. Release note: none.

cli: add tests for doctor

64d0749

Release note: none.

doctor: refactor for testing

c2e07e3

Release note: none.

spaskob force-pushed the tool branch from 6fba86e to c2e07e3 Compare August 20, 2020 08:44

craig bot merged commit 24c69e8 into cockroachdb:master Aug 20, 2020

spaskob mentioned this pull request Oct 5, 2020

Share the code between implementation of doctor tool and crdb_internal.invalid_objects #55219

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cli: add debug doctor command #52622

cli: add debug doctor command #52622

spaskob commented Aug 11, 2020 •

edited

Loading

cockroach-teamcity commented Aug 11, 2020

knz left a comment

spaskob left a comment

knz left a comment

blathers-crl bot commented Aug 13, 2020

spaskob left a comment

knz left a comment

spaskob commented Aug 19, 2020

knz left a comment

spaskob left a comment

spaskob left a comment

knz left a comment

ajwerner left a comment

spaskob commented Aug 20, 2020

craig bot commented Aug 20, 2020

cli: add debug doctor command #52622

cli: add debug doctor command #52622

Conversation

spaskob commented Aug 11, 2020 • edited Loading

cockroach-teamcity commented Aug 11, 2020

knz left a comment

Choose a reason for hiding this comment

spaskob left a comment

Choose a reason for hiding this comment

knz left a comment

Choose a reason for hiding this comment

blathers-crl bot commented Aug 13, 2020

spaskob left a comment

Choose a reason for hiding this comment

knz left a comment

Choose a reason for hiding this comment

spaskob commented Aug 19, 2020

knz left a comment

Choose a reason for hiding this comment

spaskob left a comment

Choose a reason for hiding this comment

spaskob left a comment

Choose a reason for hiding this comment

knz left a comment

Choose a reason for hiding this comment

ajwerner left a comment

Choose a reason for hiding this comment

spaskob commented Aug 20, 2020

craig bot commented Aug 20, 2020

spaskob commented Aug 11, 2020 •

edited

Loading