allocatorimpl,asim: fix replace constraints check fn #117900
Conversation
|
This change is |
kvoli
force-pushed
the
240117.replace-constraint-fix
branch
2 times, most recently
from
5a3f25f
to
7e5f08e
Compare
kvoli
added
backport-23.1.x
Introduce a `decommission_conformance` simulator test, which reproduces a simplified scenario of cockroachdb#117886. Informs: cockroachdb#117886 Release note: None
kvoli
force-pushed
the
240117.replace-constraint-fix
branch
2 times, most recently
from
25173e4
to
7d1c9b3
Compare
kvoli
changed the title
There was a problem hiding this comment.
Reviewed 1 of 1 files at r1, 4 of 4 files at r2, all commit messages.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @kvoli)
-- commits line 28 at r2:
Can we change this release note to talk more about the user-visible behavior instead of the implementation? For instance, instead of saying "replacement targets returned", we can talk about the decommissioning not getting stuck on a rebalance operation that was falsely determined to be unsafe.
kvoli
force-pushed
the
240117.replace-constraint-fix
branch
from
7d1c9b3
to
b28aa6a
Compare
There was a problem hiding this comment.
TYFTR
Reviewable status:
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
Can we change this release note to talk more about the user-visible behavior instead of the implementation? For instance, instead of saying "replacement targets returned", we can talk about the decommissioning not getting stuck on a rebalance operation that was falsely determined to be unsafe.
Fair point, I've updated to be closer to this language.
|
TYFTR bors r=nvanbenschoten. |
|
bors r- |
|
Canceled. |
Previously, it was possible for replica replacement to not return a valid target when some constraint was initially undersatisfied in addition to the replica being replaced satisfying a separate constraint. When this occurred, it could stall decommissioning a node, as replicas become stuck with the allocator returning no valid replacement target. Update the `replaceConstraintsCheck` function to correctly consider whether the replacement store satisfies the constraint when computing if the replacement is necessary. Fixes: cockroachdb#117886 Part of: cockroachdb#117891 Release note (bug fix): Decommissioning replicas which are part of a mis-replicated range will no longer get stuck on a rebalance operation that was falsely determined to be unsafe. This bug was introduced in 23.1.0.
kvoli
force-pushed
the
240117.replace-constraint-fix
branch
from
b28aa6a
to
3d731c5
Compare
|
Updated the unit test to only replace existing stores. |
There was a problem hiding this comment.
Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status:
|
TYFTR bors r=nvanbenschoten |
|
Build failed: |
|
Canceled with comment: Agent removed Retrying bors r=nvanbenschoten |
|
Build failed: |
|
bors r=nvanbenschoten |
|
Build succeeded: |
Previously, it was possible for replica replacement to not return a
valid target when some constraint was initially undersatisfied in
addition to the replica being replaced satisfying a separate constraint.
When this occurred, it could stall decommissioning a node, as replicas
become stuck with the allocator returning no valid replacement target.
Update the
replaceConstraintsCheckfunction to correctly considerwhether the replacement store satisfies the constraint when computing if
the replacement is necessary.
Fixes: #117886
Part of: #117891
Release note (bug fix): Decommissioning replicas which are part of a
mis-replicated range will no longer get stuck on a rebalance operation
that was falsely determined to be unsafe. This bug was introduced in
23.1.0.