kvserver: enqueue decom ranges at an interval behind a setting #130117
Conversation
![blathers[crl]](https://avatars.githubusercontent.com/in/59700?s=40&u=373d9101a12edc3847d2fb6ae78e3adcfb4ca41e&v=4){kind=avatar}
|
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 |
kvoli
force-pushed
the
240904.enqueue-decommissioning-replicas-2
branch
2 times, most recently
from
a289462
to
0b8287f
Compare
Introduce the `ranges.decommissioning` gauge metric, which represents the number of ranges with at least one replica on a decommissioning node. The metric is reported by the leaseholder, or if there is no valid leaseholder, the first live replica in the descriptor, similar to (under|over)-replication metrics. The metric can be used to approximately identify the distribution of decommissioning work remaining across nodes, as the leaseholder replica is responsible for triggering the replacement of decommissioning replicas for its own range. Informs: cockroachdb#130085 Release note (ops change): The `ranges.decommissioning` metric is added, representing the number of ranges which have a replica on a decommissioning node.
kvoli
force-pushed
the
240904.enqueue-decommissioning-replicas-2
branch
2 times, most recently
from
7fbf34e
to
e5a15f1
Compare
kvoli
changed the title
kvoli
force-pushed
the
240904.enqueue-decommissioning-replicas-2
branch
from
e5a15f1
to
4b6600c
Compare
kvoli
added
backport-23.1.x
There was a problem hiding this comment.
The patch itself looks good to merge and backport, esp given it's default off. Do you see a world where we turn this default on and subsume the logic introduced in #80993 into maybeEnqueueProblemRange? It should be doable with a bit of tweaking, right?
Separately, it feels a bit unsatisfying that there's a bug lurking here somewhere which we haven't found.
Reviewed 5 of 5 files at r1, 5 of 5 files at r2, all commit messages.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @kvoli)
pkg/kv/kvserver/replica.go line 2581 at r2 (raw file):
// NOT be called unless the range is known to require action e.g., // decommissioning|underreplicated. func (r *Replica) maybeEnqueueProblemRange(
Do we want to add some commentary about the motivation for introducing this method and why it isn't needed in the bug free case (which will also explain to readers why it is default off)?
When `kv.enqueue_in_replicate_queue_on_problem.interval` is set to a positive non-zero value, leaseholder replicas of ranges which have decommissioning replicas will be enqueued into the replicate queue every `kv.enqueue_in_replicate_queue_on_problem.interval` interval. When `kv.enqueue_in_replicate_queue_on_problem.interval` is set to 0, no enqueueing on decommissioning will take place, outside of the regular replica scanner. A recommended value for users enabling the enqueue (non-zero), is at least 15 minutes e.g., ``` SET CLUSTER SETTING kv.enqueue_in_replicate_queue_on_problem.interval='900s' ``` Resolves: cockroachdb#130085 Informs: cockroachdb#130199 Release note: None
kvoli
force-pushed
the
240904.enqueue-decommissioning-replicas-2
branch
from
4b6600c
to
637cd5d
Compare
There was a problem hiding this comment.
TYFTR!
Do you see a world where we turn this default on and subsume the logic introduced in #80993 into
maybeEnqueueProblemRange? It should be doable with a bit of tweaking, right?
I do see a world where that occurs, probably on a new async task goroutine which iterates over every replica on the node at most every 30s-60s+ (adding a validator to the setting to prevent configuring less than that interval).
Separately, it feels a bit unsatisfying that there's a bug lurking here somewhere which we haven't found.
Indeed, I'd much rather we fix the bug than ship this patch realistically unless we can reproduce the problem ourselves that seems unlikely. I filed #130199 for tracking.
Reviewable status:
pkg/kv/kvserver/replica.go line 2581 at r2 (raw file):
Previously, arulajmani (Arul Ajmani) wrote…
Do we want to add some commentary about the motivation for introducing this method and why it isn't needed in the bug free case (which will also explain to readers why it is default off)?
Good idea, I've added it here and also filed the issue for the bug #130199.
|
bors r=arulajmani |
|
Based on the specified backports for this PR, I applied new labels to the following linked issue(s). Please adjust the labels as needed to match the branches actually affected by the issue(s), including adding any known older branches. Issue #130085: branch-release-23.1, branch-release-23.2, branch-release-24.1, branch-release-24.2. 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
|
Encountered an error creating backports. Some common things that can go wrong:
You might need to create your backport manually using the backport tool. error creating merge commit from a98fe59 to blathers/backport-release-23.1-130117: 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 a98fe59 to blathers/backport-release-23.2-130117: 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. error creating merge commit from a98fe59 to blathers/backport-release-24.1-130117: 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 24.1.x failed. See errors above. error creating merge commit from a98fe59 to blathers/backport-release-24.2-130117: 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 24.2.x failed. See errors above. 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
This will be fun. I'll cherry-pick the commits onto the release branches and sort out the conflicts tomorrow. |
Introduce the
ranges.decommissioninggauge metric, which representsthe number of ranges with at least one replica on a decommissioning
node.
The metric is reported by the leaseholder, or if there is no valid
leaseholder, the first live replica in the descriptor, similar to
(under|over)-replication metrics.
The metric can be used to approximately identify the distribution of
decommissioning work remaining across nodes, as the leaseholder replica
is responsible for triggering the replacement of decommissioning
replicas for its own range.
Informs: #130085
Release note (ops change): The
ranges.decommissioningmetric is added,representing the number of ranges which have a replica on a
decommissioning node.
When
kv.enqueue_in_replicate_queue_on_problem.intervalis set to apositive non-zero value, leaseholder replicas of ranges which are
underreplicated will be enqueued into the replicate queue every
kv.enqueue_in_replicate_queue_on_problem.intervalinterval.When
kv.enqueue_in_replicate_queue_on_problem.intervalis set to 0,no enqueueing on underreplication will take place, outside of the
regular replica scanner.
A recommended value for users enabling the enqueue (non-zero), is 15
minutes e.g.,
Resolves: #130085
Release note (ops change): The
ranges.decommissioningmetric is added,representing the number of ranges which have a replica on a
decommissioning node.