It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR?

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

This change is 

@renatolabs @pav-kv Submitting this for an initial look. Will need a few tests and tweaks.

@renatolabs Do you have any thoughts on this before I wrap it up?

On second thought, I wonder if we should just do something simpler here, which won't require any API changes: add an envvar (e.g. COCKROACH_INTERNAL_CHECK_CONSISTENCY_VIA_QUEUE) which will change crdb_internal.check_consistency() to use CHECK_VIA_QUEUE, causing it to take checkpoints and terminate nodes if it finds any inconsistencies.

Wdyt @pav-kv?

@erikgrinaker
The current implementation takes checkpoints and doesn’t kill the node, right? If we flip to VIA_QUEUE, it’ll kill the node. Is it ok? If not, COCKROACH_INTERNAL_CHECK_CONSISTENCY_VIA_QUEUE would need to be honoured both to set VIA_QUEUE, and to modify its interpretation. This happens on 2 different nodes (the sender and the receiver below raft).

Upd: ah no, we can just not set the Terminate field, which is all on the sender.

I figured terminating the node seemed fine, since we were going to fail the test anyway.

I guess that prevents us from collecting debug.zip data from the node, so maybe we do want to keep it running -- but if the inconsistency happened to be caught by the normal consistency queue, then we'd be in this situation anyway.

It just seems like a much more light-weight change -- I'm not sure I like modifying the public APIs just for this.

If terminating is fine, then perhaps an env var is fine. Who would set the var?

We'd have to set it for all roachtests.

Can we use some of the static constants instead of env var? Like the buildutil.CrdbTestBuild. So that all tests/roachtests automatically change the meaning of a full check.

Another benefit of a static test-only check is that it can actually be done across nodes. So we can terminate or not terminate nodes as we like.

Roachtests use arbitrary binaries, in principle we don't control whether they're built with CrdbTestBuild or otherwise (consider e.g. running roachtest locally).

all tests/roachtests automatically change the meaning of a full check

I don't think we want to do this. For example, it will break all tests of crdb_internal.check_consistency(), since they'll unconditionally fatal.

I don't think we want to do this. For example, it will break all tests of crdb_internal.check_consistency(), since they'll unconditionally fatal.

We may choose to never fatal nodes in tests. Similarly to how we mock out the fatal behaviour in unit tests.

Also, it doesn't have to be a build flag. Can be any semi-static indicator that we're in a [specific] roachtest, similar to testing knobs for the unit tests.

Also, it doesn't have to be a build flag. Can be any semi-static indicator that we're in a [specific] roachtest, similar to testing knobs for the unit tests.

How can we do this? Roachtests use an arbitrary CRDB binary, they can only control runtime flags like envvars, not build-time flags.

Right, then env var is the answer. I was thinking of some "code level" env vars basically, but it appears we don't have them.

Ok, I've added COCKROACH_INTERNAL_CHECK_CONSISTENCY_FATAL instead, and adapted the code. I need to add a test for the checkpoint artifacts collection and have a final look over everything, but otherwise I think this should be good now.

This should be ready for final review now. Became a fair bit simpler.

I need to add a test for the checkpoint artifacts collection

Doesn't look like we have a test harness in place for roachtest artifacts collection, and I don't want to start building one now, so I dropped this. Verified manually with the inconsistency test that this works as expected.

@renatolabs I don't think there should be anything controversial here, so I'll merge this, but let me know if you have further comments and I'll address them separately.

TFTRs!

bors r+

Build succeeded:

• Bazel Essential CI (Cockroach)

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 f9ebbde to blathers/backport-release-23.2-117235: 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.}