cockroachdb / cockroachdb/cockroach

kvserver: allocator and node vitality have conflicting suspect states

Open
#132,879 0 comments 0 reactions 0 assignees View on GitHub
branch-release-23.2 branch-release-24.1 branch-release-24.2 branch-release-24.3 C-bug O-perturbation T-kv
Dominant language
Go
Stars
32.5k
Forks
4.1k
PR merge metrics
PR metrics pending

Description

Prior to 23.2, the concept of a store (or node by extension) being `suspect` was only found in the store pool and allocator[^1].

In 23.2, `NodeVitality` was introduced, which also introduced the concept of `suspect`, however did not also introduce a `suspect` related state, reusing `NodeLivenessStatus_UNAVAILABLE`.

The storepool also now calls into `NodeVitality` to determine if a node is alive, dead, unavailable, decommissioning or unknown, where previously it called `NodeLiveness`.

When `NodeVitality` determines that a node should be considered `suspect` by having last received a store descriptor from that node more than the `gossip.StoreTTL` ago, in addition to another 30s (default `server.time_after_store_suspect` duration).

`NodeVitality` then returns `NodeLivenessStatus_UNAVAILABLE` from:

https://github.com/cockroachdb/cockroach/blob/a0f39e7ac9574756063bc90bba6bc532b45c33d4/pkg/kv/kvserver/liveness/livenesspb/liveness.go#L429-L429

In the store pool however, the suspect condition is based on the last time `NodeLivenessStatus_UNAVAILABLE` was returned. This implies that the suspect duration may be double counted[^2] in some instances, while also not correctly accounting for allocator storepool calls which wish to still include suspect nodes in consideration:

https://github.com/cockroachdb/cockroach/blob/a0f39e7ac9574756063bc90bba6bc532b45c33d4/pkg/kv/kvserver/allocator/allocatorimpl/allocator.go#L996-L1003

Which is relied upon for correct `AllocatorAction` quorum/availability determination and `ChangeReplicasTxn`:

https://github.com/cockroachdb/cockroach/blob/a0f39e7ac9574756063bc90bba6bc532b45c33d4/pkg/kv/kvserver/replica_command.go#L1217-L1223

We should rectify these inconsistencies and fully assess the impact of them, not otherwise stated here.

[^1]: https://github.com/cockroachdb/cockroach/blob/7002082b08ad63d1c4f8596b1ece693361268c75/pkg/kv/kvserver/allocator/storepool/store_pool.go#L54-L74
[^2]: https://github.com/cockroachdb/cockroach/blob/a0f39e7ac9574756063bc90bba6bc532b45c33d4/pkg/kv/kvserver/allocator/storepool/store_pool.go#L189-L194

Jira issue: CRDB-43317

Epic CRDB-37526

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.