mmaprototype: small clean up doStructuralNormalization

[wenyihu6]

Epic: CRDB-55052
Release note: none

---

mmaprototype: add assertion on emptyVoterConstraintIndex

This commit adds assertions on emptyVoterConstraintIndex and
emptyConstraintIndex since there should only be one.

---

mmaprototype: clean up doStructuralNormalization

This commit refactors some code in doStructuralNormalization to
use min utility and also adds assertions on programming errors.

---

mmaprototype: use >= 0 for emptyVoterConstraintIndex

This commit fixes >= 0 when checking for whether there were
valid emptyVoterConstraintIndex and emptyConstraintIndex
since 0 is a valid index.

[blathers-crl]

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.}

[cockroach-teamcity]

This change is 

[tbg]

Thank you! I like the simplifications. Had a few suggestions for comments and improving coverage.

@tbg reviewed 1 of 1 files at r1, 1 of 1 files at r2, 1 of 1 files at r3, 1 of 1 files at r4, 1 of 1 files at r5, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @sumeerbhola)

---

pkg/kv/kvserver/allocator/mmaprototype/constraint.go line 563 at r2 (raw file):

		neededReplicas := conf.voterConstraints[emptyVoterConstraintIndex].numReplicas
		if voterConstraints[emptyVoterConstraintIndex].numReplicas != 0 {
			panic("programming error")

panic with errors.Assertionfailedf and should it contain information to figure out what went wrong?

just to make sure I understand: we're currently handling the "empty voter constraint index", and that one can't have num_replicas specified in the constraints apparently... but why not? Is there a helpful comment you could add?

---

pkg/kv/kvserver/allocator/mmaprototype/constraint.go line 435 at r1 (raw file):

	for i := range conf.voterConstraints {
		if len(conf.voterConstraints[i].constraints) == 0 {
			if emptyVoterConstraintIndex != -1 {

Here and below:

"invalid configuration with multiple empty constraints"? (since we can hit this both from voter and regular constraints)?

should these be errors.AssertionFailedf or can we hit these paths if users use "strange constraints" (i.e. there's no preliminary checking the caller has already expected to have done)?

If we hit this in practice, will it be clear what the offending constraints look like or should the error include them?

Also I'm realizing I don't even know what this empty constraints thing is about. Is there a helpful comment you can add, maybe on emptyConstraintIndex?

---

pkg/kv/kvserver/allocator/mmaprototype/constraint.go line 442 at r4 (raw file):

		for j := range conf.constraints {
			if len(conf.constraints[j].constraints) == 0 {
				if emptyConstraintIndex != -1 && emptyConstraintIndex != j {

Makes sense.

---

pkg/kv/kvserver/allocator/mmaprototype/constraint.go line 436 at r5 (raw file):

		if len(conf.voterConstraints[i].constraints) == 0 {
			if emptyVoterConstraintIndex != -1 {
				return errors.Errorf("invalid configurations with empty voter constraint")

we don't seem to have coverage for this case btw. Can you add a unit test?

---

pkg/kv/kvserver/allocator/mmaprototype/constraint.go line 443 at r5 (raw file):

			if len(conf.constraints[j].constraints) == 0 {
				if emptyConstraintIndex != -1 && emptyConstraintIndex != j {
					return errors.Errorf("invalid configurations with empty constraint")

Ditto on unit test (unless it's really painful)

[sumeerbhola]

@sumeerbhola reviewed 1 of 1 files at r1, 1 of 1 files at r2, 1 of 1 files at r3, 1 of 1 files at r4.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @wenyihu6)

---

pkg/kv/kvserver/allocator/mmaprototype/constraint.go line 562 at r2 (raw file):

	if emptyVoterConstraintIndex > 0 && emptyConstraintIndex > 0 {
		neededReplicas := conf.voterConstraints[emptyVoterConstraintIndex].numReplicas
		if voterConstraints[emptyVoterConstraintIndex].numReplicas != 0 {

This needs a comment.

// While iterating over the previous relationships, we explicitly did not touch the emptyVoterConstraintIndex, so the numReplica should still be 0.

For the remainder of the code here, consider adopting some version of #158382. Fine to do so in a later PR.

---

pkg/kv/kvserver/allocator/mmaprototype/constraint.go line 435 at r1 (raw file):

	for i := range conf.voterConstraints {
		if len(conf.voterConstraints[i].constraints) == 0 {
			if emptyVoterConstraintIndex != -1 {

reminder that we should de-dup the constraints conjunctions so this would be a panic. This is a limitation of our current normalization and not a user error.

Can happen in a later PR, but leave a TODO.

---

pkg/kv/kvserver/allocator/mmaprototype/constraint.go line 442 at r4 (raw file):

		for j := range conf.constraints {
			if len(conf.constraints[j].constraints) == 0 {
				if emptyConstraintIndex != -1 && emptyConstraintIndex != j {

isn't emptyConstraintIndex != j guaranteed to be true?