storage: prevent node from restarting after consistency check fatal #42401

tbg · 2019-11-12T13:57:10Z

NB: I'll update the inconsistency roachtest with an end-to-end check on the restart protection.

Many deployments auto-restart crashed nodes unconditionally. As a
result, we are often pulled into inconsistency failures relatively late,
which results in a loss of information (in particular if the source of
the problem turns out to be one of the nodes that did not actually
crash); if the node that crashes is the one with the problem,
restarting it lets it serve data again (until the next time the
consistency checker nukes it), which is bad as well.

This commit introduces a "death rattle" - if a node is told to terminate
as the result of a consistency check, it will write a marker file that
prevents subsequent restarts of the node with an informative message
alerting the operator that there is a serious problem that needs to be
addressed.

Fixes #40442.

Release note (general change): nodes that have been terminated as the
result of a failed consistency check now refuse to restart, making it
more likely that the operator notices that there is a persistent issue
in a timely manner.

cockroach-teamcity · 2019-11-12T13:57:19Z

This change is

bdarnell

Do we want to do this for Replica.maybeSetCorrupt too?

Reviewed 7 of 7 files at r2.
Reviewable status: complete! 0 of 0 LGTMs obtained

Release note: None

Many deployments auto-restart crashed nodes unconditionally. As a result, we are often pulled into inconsistency failures relatively late, which results in a loss of information (in particular if the source of the problem turns out to be one of the nodes that did not actually crash); if the node that crashes *is* the one with the problem, restarting it lets it serve data again (until the next time the consistency checker nukes it), which is bad as well. This commit introduces a "death rattle" - if a node is told to terminate as the result of a consistency check, it will write a marker file that prevents subsequent restarts of the node with an informative message alerting the operator that there is a serious problem that needs to be addressed. We use the same strategy when a replica finds that its internal invariants are violated. Fixes cockroachdb#40442. Release note (general change): nodes that have been terminated as the result of a failed consistency check now refuse to restart, making it more likely that the operator notices that there is a persistent issue in a timely manner.

tbg · 2019-11-18T10:33:13Z

Good idea, done.

tbg · 2019-11-18T10:35:31Z

bors r=bdarnell

42401: storage: prevent node from restarting after consistency check fatal r=bdarnell a=tbg NB: I'll update the inconsistency roachtest with an end-to-end check on the restart protection. ---- Many deployments auto-restart crashed nodes unconditionally. As a result, we are often pulled into inconsistency failures relatively late, which results in a loss of information (in particular if the source of the problem turns out to be one of the nodes that did not actually crash); if the node that crashes *is* the one with the problem, restarting it lets it serve data again (until the next time the consistency checker nukes it), which is bad as well. This commit introduces a "death rattle" - if a node is told to terminate as the result of a consistency check, it will write a marker file that prevents subsequent restarts of the node with an informative message alerting the operator that there is a serious problem that needs to be addressed. Fixes #40442. Release note (general change): nodes that have been terminated as the result of a failed consistency check now refuse to restart, making it more likely that the operator notices that there is a persistent issue in a timely manner. Co-authored-by: Tobias Schottdorf <tobias.schottdorf@gmail.com>

craig · 2019-11-18T11:11:25Z

Build succeeded

GitHub CI (Cockroach)

Prior to this patch, the code in `runStartSQL` was using a different sequence of steps than in `runStart`, even for those steps that are beneficial to run in both cases. This commit fixes that. In particular it adds the following missing bits to `cockroach mt start-sql`: - it fixes support for the (test-only) `COCKROACH_EXPERIMENTAL_LINEARIZABLE` env var. (from cockroachdb#4754) - it adds a tracing span for the startup code. (from cockroachdb#8712) - it properly supports `--listening-url-file`. (from cockroachdb#15468) - it properly does sanitization of `--external-io-dir`. (from cockroachdb#19725) - it sets the proper log severity level for gRPC. (from cockroachdb#20308) - it reports the command-line and env config to logs. (from cockroachdb#21344) - it prevents startup if there is a `_CRITICAL_ALERT.txt` file in the store directory. (from cockroachdb#42401) - sets the umask for newly created file to remove "other" permission bits. This was a security team request originally. (from cockroachdb#44043) - it sets `GOMAXPROCS` from current cgroup limits. (from cockroachdb#57390) - it stops the server early if the storage disk is full. (from cockroachdb#66893) - it fixes support for the `COCKROACH_DISABLE_SPAN_CONFIGS` config env var. (from cockroachdb#73876) To review this commit, it is useful to open the files `start.go` and `mt_start_sql.go` side-by-side, which clarifies how much closer they have become to each other. Looking at the `mt_start_sql.go` changes without that context likely makes the review more difficult. A later commit will actually merge the two code paths together so there is just one thing to maintain. Release note: None

90176: cli/start: unify code between `cockroach start` and `cockroach mt start-sql` r=andreimatei a=knz Fixes #89974. Fixes #90831. Fixes #90524. This PR merges the server initialization code between `cockroach start` and `cockroach mt start-sql`. In doing so, it brings `cockroach mt start-sql` closer to what we expect from proper CockroachDB server processes: - it fixes support for the (test-only) `COCKROACH_EXPERIMENTAL_LINEARIZABLE` env var. (from #4754) - it adds a tracing span for the startup code. (from #8712!!) - it properly supports `--listening-url-file`. (from #15468) - it properly does sanitization of `--external-io-dir`. (from #19725) - it sets the proper log severity level for gRPC. (from #20308) - it reports the command-line and env config to logs. (from #21344) - it prevents startup if there is a `_CRITICAL_ALERT.txt` file in the store directory. (from #42401) - sets the umask for newly created file to remove "other" permission bits. This was a security team request originally. (from #44043) - it recovers support for `DISABLE_STARTING_BACKGROUND_JOBS`. (from #44786) - it sets `GOMAXPROCS` from current cgroup limits. (from #57390) - it stops the server early if the storage disk is full. (from #66893) - it fixes support for the `COCKROACH_DISABLE_SPAN_CONFIGS` config env var. (from #73876) See the individual commit for details. 90660: sql: add contention_events to cluster_execution_insights r=j82w a=j82w The original contention column will remain to make query operations faster. The events are being put into a json column because it's possible there could be multiple blocking events for a single statement. The json column avoids the complexity of adding another table and keeping it in sync with the insights table. The table can be joined with index_columns and tables to get the database name, table name, and index name that contention occurred on. This does not contain the blocking statement information, and the blocking fingerprint id. closes: #88561 Release note (sql change): Adds contention_events to cluster_execution_insights. This is used to see which transaction is blocking the specific statement. 90719: opgen: added a bool field in struct opgen.transition r=Xiang-Gu a=Xiang-Gu This PR adds a bool field in struct opgen.transition that indicates whether it results from a `equiv(xx)` transition spec in the opgen file. It will be useful for a test where we need to find the inital status on a adding/dropping path. Without such a change, it can be problematic if we have a `equiv(xx)` spec as the first transition. E.g. ``` ToAbsent( PUBLIC, equiv(VALIDATED), to(WRITE_ONLY), to(ABSENT), ) ``` Without this change, the inital status will confusingly be `VALIDATED`, and the next status will be `PUBLIC`. With this change, the initial status will be `PUBLIC`, and the next status will be `WRITE_ONLY`. We also added some comments when we make transitions from the specs. Epic: None Release note: None 90865: sql: use bare name string of new pk to compare with pk name when altering primary key r=chengxiong-ruan a=chengxiong-ruan Fixes #90836 Release note (sql change): previously, the `DROP CONSTRAINT, ADD CONSTRAINT` in one trick to have a new primary key without moving old primary key to be a secondary index didn't work if the primary key name is a reserved SQL keyword. A `constraint already exists` error was returned. This patch fixed the bug, the trick now also works with primary key named as reserved keywords. Co-authored-by: Raphael 'kena' Poss <knz@thaumogen.net> Co-authored-by: j82w <jwilley@cockroachlabs.com> Co-authored-by: Xiang Gu <xiang@cockroachlabs.com> Co-authored-by: Chengxiong Ruan <chengxiongruan@gmail.com>

tbg requested a review from bdarnell November 12, 2019 13:57

tbg requested a review from a team as a code owner November 12, 2019 13:57

bdarnell approved these changes Nov 12, 2019

View reviewed changes

tbg added 2 commits November 18, 2019 11:32

cli: fix typo

7dbdd94

Release note: None

tbg force-pushed the deathrattle-4 branch from 6412113 to ba6699b Compare November 18, 2019 10:32

craig bot merged commit ba6699b into cockroachdb:master Nov 18, 2019

tbg deleted the deathrattle-4 branch November 18, 2019 11:13

tbg mentioned this pull request Nov 18, 2019

backport-19.2: terminate nodes in minority on consistency failure #42149

Merged

knz mentioned this pull request Oct 18, 2022

cli/start: unify code between cockroach start and cockroach mt start-sql #90176

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

storage: prevent node from restarting after consistency check fatal #42401

storage: prevent node from restarting after consistency check fatal #42401

tbg commented Nov 12, 2019 •

edited

cockroach-teamcity commented Nov 12, 2019

bdarnell left a comment

tbg commented Nov 18, 2019 •

edited

tbg commented Nov 18, 2019

craig bot commented Nov 18, 2019

storage: prevent node from restarting after consistency check fatal #42401

storage: prevent node from restarting after consistency check fatal #42401

Conversation

tbg commented Nov 12, 2019 • edited

cockroach-teamcity commented Nov 12, 2019

bdarnell left a comment

Choose a reason for hiding this comment

tbg commented Nov 18, 2019 • edited

tbg commented Nov 18, 2019

craig bot commented Nov 18, 2019

Build succeeded

tbg commented Nov 12, 2019 •

edited

tbg commented Nov 18, 2019 •

edited