cli/doctor, doctor: use right jobs table, skip dropped descs

[annrpom]

cli/doctor: doctor should read from the right jobs table

In #97762, we started writing a job's payload (and progress)
information to the system.jobs_info table. As a result, we
had to change the parts of our code that relied on the system.jobs
table to use crdb_internal.system_jobs instead (since that table
would inaccurately report that some payloads were NULL).
This change did not occur for the in-memory representation of the jobs
table created by debug doctor -- which can result in missing job
false-positives. This patch updates debug doctor's representation of
the jobs table by referring to crdb_internal.system_jobs instead.

Epic: none
Fixes: #122675

Release note: None

---

doctor: skip validation for dropped descriptors

In some cases, dropped descriptors appear in our system.descriptors
table with dangling job mutations without an associated job.
This patch teaches debug doctor examine to skip validation
on such dropped descriptors.

Epic: none

Fixes: #123477
Fixes: #122956

Release note: none

[cockroach-teamcity]

This change is 

[annrpom]

this is RFAL -- i had a question about mimicking the json file version (system.jobs.json)

[fqazi]

Reviewed 14 of 14 files at r1, 2 of 3 files at r2.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @annrpom)

---

pkg/cli/doctor.go line 558 at r1 (raw file):

		}, 0)
		// TODO(before merge): when does this happen?
		if err := parseJSONFile(zipDirPath, "system.jobs.json", &jobsTableJSON); err != nil {

On master / 24.1 a debug zip will format tables as JSON, so we need to account for that

---

pkg/cli/doctor_test.go line 214 at r2 (raw file):

// TestDoctorClusterDropped tests that debug doctor examine will avoid validating dropped descriptors.
func TestDoctorClusterDropped(t *testing.T) {

Could we just combine it with the one above? I think it will be a bit cleaner since it will be a different path and toggling between the two flag states.

[annrpom]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @fqazi)

---

pkg/cli/doctor.go line 558 at r1 (raw file):

Previously, fqazi (Faizan Qazi) wrote…

On master / 24.1 a debug zip will format tables as JSON, so we need to account for that

faizan and i had a talk offline and found out that the original PR that changed debug zip output to JSON format was partially reverted(38cf825) so that the format is still the beloved tsv that we see today -- with an option to specify JSON formatting by doing:

cockroach debug zip debug-json.zip --format=json

just did a little update to ensure that JSON formatting is supported properly

---

pkg/cli/doctor_test.go line 214 at r2 (raw file):

Previously, fqazi (Faizan Qazi) wrote…

Could we just combine it with the one above? I think it will be a bit cleaner since it will be a different path and toggling between the two flag states.

I tried seeing how this one would pan out, but it's not looking too hot annrpom@9eb1aec

I think we could merge these two tests together if we did something like

// setup for 1st one:
create table
unsafe upsert
run debug doctor examine

// followed by setup for 2nd one:
unsafe upsert -- adding dropped fields
run debug doctor examine

However, I am getting the error linked in the commit above when I try this; also, would

cockroach/pkg/sql/repair.go

Lines 101 to 104 in 48ebfd1

	if existing.IsUncommittedVersion() {
	return pgerror.Newf(pgcode.ObjectNotInPrerequisiteState,
	"cannot modify a modified descriptor (%d) with UnsafeUpsertDescriptor", id)
	}

not block me in any way? I am probably missing something here, wdyt 🙇 ?

[fqazi]

Reviewed 2 of 3 files at r3, 5 of 5 files at r4, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @annrpom)

---

pkg/cli/doctor_test.go line 214 at r2 (raw file):

Previously, annrpom (annie pompa) wrote…

I tried seeing how this one would pan out, but it's not looking too hot annrpom@9eb1aec

I think we could merge these two tests together if we did something like

// setup for 1st one:
create table
unsafe upsert
run debug doctor examine

// followed by setup for 2nd one:
unsafe upsert -- adding dropped fields
run debug doctor examine

However, I am getting the error linked in the commit above when I try this; also, would

cockroach/pkg/sql/repair.go

Lines 101 to 104 in 48ebfd1

	if existing.IsUncommittedVersion() {
	return pgerror.Newf(pgcode.ObjectNotInPrerequisiteState,
	"cannot modify a modified descriptor (%d) with UnsafeUpsertDescriptor", id)
	}

not block me in any way? I am probably missing something here, wdyt 🙇 ?

Yeah, its okay to leave it separate then.

[rafiss]

nice work! looks like there's one issue in the new test.

=== RUN   TestDoctorClusterDropped/examine
[debug --host=127.0.0.1:39133 --insecure=false --certs-dir=/var/lib/engflow/worker/work/0/exec/_tmp/1855398a359288315eef18ccd1f2c1dd/cli-test2994948257 doctor examine cluster]
    datadriven.go:144: 
        /var/lib/engflow/worker/work/0/exec/bazel-out/k8-fastbuild/bin/pkg/cli/cli_test_/cli_test.runfiles/com_github_cockroachdb_cockroach/pkg/cli/testdata/doctor/test_examine_cluster_dropped:1:
         
        expected:
        debug doctor examine cluster
        Examining 57 descriptors and 58 namespace entries...
        Examining 2 jobs...
        No problems found!
        
        found:
        debug doctor examine cluster
        Examining 62 descriptors and 63 namespace entries...
          ParentID 100, ParentSchemaID 101: namespace entry "foo" (104): descriptor is being dropped
        Examining 7 jobs...
        ERROR: validation failed

also, could you take care of adding the correct backport labels? (is it just 24.1 or 23.2 as well?)

[annrpom]

TFTR! ('-')7

backporting all the way back to 23.1 because that looks like when we stopped writing payload/progress to system.jobs #99458

bors r=fqazi

[craig]

Build succeeded:

• lint
• acceptance
• macos_amd64_build
• linux_amd64_build
• linux_amd64_fips_build
• check_generated_code
• macos_arm64_build
• docker_image_amd64
• unit_tests
• local_roachtest_fips
• local_roachtest
• linux_arm64_build
• examples_orms
• windows_build

[blathers-crl]

Encountered an error creating backports. Some common things that can go wrong:

1. The backport branch might have already existed.
2. There was a merge conflict.
3. The backport branch contained merge commits.

You might need to create your backport manually using the backport tool.

---

error creating merge commit from 2566787 to blathers/backport-release-23.1-123298: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 23.1.x failed. See errors above.

---

error creating merge commit from 2566787 to blathers/backport-release-23.2-123298: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 23.2.x failed. See errors above.

---

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