server: evaluate decommission pre-checks

[AlexTalks]

This adds support for the evaluation of the decommission readiness of a
node (or set of nodes), by simulating their liveness to have the
DECOMMISSIONING status and utilizing the allocator to ensure that we are
able to perform any actions needed to repair the range. This supports a
"strict" mode, in which case we expect all ranges to only need
replacement or removal due to the decommissioning status, or a more
permissive "non-strict" mode, which allows for other actions needed, as
long as they do not encounter errors in finding a suitable allocation
target. The non-strict mode allows us to permit situations where a range
may have more than one action needed to repair it, such as a range that
needs to reach its replication factor before the decommissioning replica
can be replaced, or a range that needs to finalize an atomic replication
change.

Depends on #94024.

Part of #91568

[cockroach-teamcity]

This change is 

[kvoli]

Reviewed 2 of 3 files at r2, 8 of 8 files at r3, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @AlexTalks)

---

pkg/server/decommission.go line 51 at r3 (raw file):

type decommissionRangeCheckResult struct {
	desc         *roachpb.RangeDescriptor
	action       allocatorimpl.AllocatorAction

It would be nice if the allocator action didn't leak into the server pkg here. We discussed offline that this will only RPC the string.

---

pkg/server/decommission.go line 168 at r3 (raw file):

	nodeIDs []roachpb.NodeID,
	strictReadiness bool,
	collectTraces bool,

See below comment on trace overhead at high decommission replica count when this is enabled

---

pkg/server/decommission.go line 252 at r3 (raw file):

			}

			action, _, recording, rErr := evalStore.AllocatorCheckRange(ctx, &desc, overrideStorePool)

What is the expected mem overhead of collecting a trace recording on every range containing a replica on a decommissioning node?

I see that the check can pass in a list of Nodes that are to be decommissioned so it's foreseeable that you could be collecting a few hundred k traces (especially with multi-store).

---

pkg/server/decommission_test.go line 170 at r3 (raw file):

	hasAll := true
	for _, idx := range serverIdxs {
		hasAll = hasAll && desc.Replicas().HasReplicaOnNode(tc.Server(idx).NodeID())

nit: Why not just return early here?

for (...) {
    if !desc.Replicas().HasReplicaOnNode(tc.Server(idx).NodeID()) {
        return false
    }
}
return true

[AlexTalks]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @kvoli)

---

pkg/server/decommission.go line 51 at r3 (raw file):

Previously, kvoli (Austen) wrote…

It would be nice if the allocator action didn't leak into the server pkg here. We discussed offline that this will only RPC the string.

From the RPC, yes, but the action is used within the evaluation to determine a few things (see evaluateRangeCheckResult). In this return struct I can use a string of course, though it means the AllocatorAction will still be used in the server pkg.

---

pkg/server/decommission.go line 252 at r3 (raw file):

Previously, kvoli (Austen) wrote…

What is the expected mem overhead of collecting a trace recording on every range containing a replica on a decommissioning node?

I see that the check can pass in a list of Nodes that are to be decommissioned so it's foreseeable that you could be collecting a few hundred k traces (especially with multi-store).

This is a good point and was one of the things I most wanted input on in this review. Especially since right now, it's collecting traces always, and just returning them if collectTraces is on.

I think that we should collect traces only if requested, and we should also make sure we have a limit on the number of errors so that we can avoid huge memory overheads.

---

pkg/server/decommission_test.go line 170 at r3 (raw file):

Previously, kvoli (Austen) wrote…

nit: Why not just return early here?

for (...) {
    if !desc.Replicas().HasReplicaOnNode(tc.Server(idx).NodeID()) {
        return false
    }
}
return true

Can do that.

[AlexTalks]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @kvoli)

---

pkg/server/decommission.go line 51 at r3 (raw file):

Previously, AlexTalks (Alex Sarkesian) wrote…

From the RPC, yes, but the action is used within the evaluation to determine a few things (see evaluateRangeCheckResult). In this return struct I can use a string of course, though it means the AllocatorAction will still be used in the server pkg.

Done.

---

pkg/server/decommission.go line 168 at r3 (raw file):

Previously, kvoli (Austen) wrote…

See below comment on trace overhead at high decommission replica count when this is enabled

Done.

---

pkg/server/decommission.go line 252 at r3 (raw file):

Previously, AlexTalks (Alex Sarkesian) wrote…

This is a good point and was one of the things I most wanted input on in this review. Especially since right now, it's collecting traces always, and just returning them if collectTraces is on.

I think that we should collect traces only if requested, and we should also make sure we have a limit on the number of errors so that we can avoid huge memory overheads.

Done.

[kvoli]

Reviewed 6 of 6 files at r4, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @AlexTalks)

[AlexTalks]

bors r=kvoli

[craig]

Build failed (retrying...):

• Bazel Essential CI (Cockroach)

[AlexTalks]

bors r=kvoli

[craig]

Already running a review

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)