constraint: tighten inputs to CheckConjunction

Previously, `ConjunctionsCheck` took a store descriptor as an input.
However, it only needed to know the store/node attributes and locality.
Some upcoming callers (lease acquisition) can't easily construct a full
store descriptor since the locking order would cause deadlocks.

This patch changes it to only take the attributes and locality instead
of the entire store descriptor, and renames it to `CheckConjunction()`.
It also adds `CheckStoreConjunction()` as a convenience method that
takes a store descriptor, and migrates all existing callers.

Epic: none
Release note: None

Author: erikgrinaker

pkg/kv/kvserver/allocator/allocatorimpl/allocator.go

@@ -2827,7 +2827,7 @@ func (a Allocator) PreferredLeaseholders(
 			if !ok {
 				continue
 			}
-			if constraint.ConjunctionsCheck(storeDesc, preference.Constraints) {
+			if constraint.CheckStoreConjunction(storeDesc, preference.Constraints) {
 				preferred = append(preferred, repl)
 			}
 		}

pkg/kv/kvserver/allocator/allocatorimpl/allocator_scorer.go

@@ -2010,7 +2010,7 @@ func allocateConstraintsCheck(
 	}
 
 	for i, constraints := range analyzed.Constraints {
-		if constraintsOK := constraint.ConjunctionsCheck(
+		if constraintsOK := constraint.CheckStoreConjunction(
 			store, constraints.Constraints,
 		); constraintsOK {
 			valid = true
@@ -2050,9 +2050,7 @@ func replaceConstraintsCheck(
 	for i, constraints := range analyzed.Constraints {
 		matchingStores := analyzed.SatisfiedBy[i]
 		satisfiedByExistingStore := containsStore(matchingStores, existingStore.StoreID)
-		satisfiedByCandidateStore := constraint.ConjunctionsCheck(
-			store, constraints.Constraints,
-		)
+		satisfiedByCandidateStore := constraint.CheckStoreConjunction(store, constraints.Constraints)
 		if satisfiedByCandidateStore {
 			valid = true
 		}
@@ -2147,7 +2145,7 @@ func rebalanceFromConstraintsCheck(
 	// satisfied by existing replicas or that is only fully satisfied because of
 	// fromStoreID, then it's necessary.
 	for i, constraints := range analyzed.Constraints {
-		if constraintsOK := constraint.ConjunctionsCheck(
+		if constraintsOK := constraint.CheckStoreConjunction(
 			store, constraints.Constraints,
 		); constraintsOK {
 			valid = true

pkg/kv/kvserver/allocator/base.go

@@ -76,7 +76,7 @@ func IsStoreValid(
 		return true
 	}
 	for _, subConstraints := range constraints {
-		if constraintsOK := constraint.ConjunctionsCheck(
+		if constraintsOK := constraint.CheckStoreConjunction(
 			store, subConstraints.Constraints,
 		); constraintsOK {
 			return true

pkg/kv/kvserver/asim/queue/allocator_replica.go

@@ -96,7 +96,7 @@ func (sr *SimulatorReplica) LeaseViolatesPreferences(context.Context) bool {
 		return false
 	}
 	for _, preference := range conf.LeasePreferences {
-		if constraint.ConjunctionsCheck(storeDesc, preference.Constraints) {
+		if constraint.CheckStoreConjunction(storeDesc, preference.Constraints) {
 			return false
 		}
 	}

pkg/kv/kvserver/constraint/analyzer.go

@@ -68,7 +68,7 @@ func AnalyzeConstraints(
 			// is a much more stable failure state than frantically moving everything
 			// off such a node.
 			store, ok := storeResolver.GetStoreDescriptor(repl.StoreID)
-			if !ok || ConjunctionsCheck(store, subConstraints.Constraints) {
+			if !ok || CheckStoreConjunction(store, subConstraints.Constraints) {
 				result.SatisfiedBy[i] = append(result.SatisfiedBy[i], store.StoreID)
 				result.Satisfies[store.StoreID] = append(result.Satisfies[store.StoreID], i)
 			}
@@ -80,18 +80,31 @@ func AnalyzeConstraints(
 	return result
 }
 
-// ConjunctionsCheck checks a store against a single set of constraints (out of
-// the possibly numerous sets that apply to a range), returning true iff the
-// store matches the constraints. The constraints are AND'ed together; a store
-// matches the conjunction if it matches all of them.
-func ConjunctionsCheck(store roachpb.StoreDescriptor, constraints []roachpb.Constraint) bool {
+// CheckConjunction checks the given attributes and locality tags against all
+// the given constraints. Every constraint must be satisfied by any
+// attribute/tier, i.e. they are ANDed together.
+func CheckConjunction(
+	storeAttrs, nodeAttrs roachpb.Attributes,
+	nodeLocality roachpb.Locality,
+	constraints []roachpb.Constraint,
+) bool {
 	for _, constraint := range constraints {
-		// StoreMatchesConstraint returns whether a store matches the given constraint.
-		hasConstraint := roachpb.StoreMatchesConstraint(store, constraint)
-		if (constraint.Type == roachpb.Constraint_REQUIRED && !hasConstraint) ||
-			(constraint.Type == roachpb.Constraint_PROHIBITED && hasConstraint) {
+		matchesConstraint := roachpb.MatchesConstraint(storeAttrs, nodeAttrs, nodeLocality, constraint)
+		if (constraint.Type == roachpb.Constraint_REQUIRED && !matchesConstraint) ||
+			(constraint.Type == roachpb.Constraint_PROHIBITED && matchesConstraint) {
 			return false
 		}
 	}
 	return true
 }
+
+// CheckStoreConjunction checks a store against a single set of constraints (out of
+// the possibly numerous sets that apply to a range), returning true iff the
+// store matches the constraints. The constraints are AND'ed together; a store
+// matches the conjunction if it matches all of them.
+func CheckStoreConjunction(
+	storeDesc roachpb.StoreDescriptor, constraints []roachpb.Constraint,
+) bool {
+	return CheckConjunction(
+		storeDesc.Attrs, storeDesc.Node.Attrs, storeDesc.Node.Locality, constraints)
+}

pkg/kv/kvserver/replica_range_lease.go

@@ -1551,7 +1551,7 @@ func (r *Replica) LeaseViolatesPreferences(ctx context.Context) bool {
 		return false
 	}
 	for _, preference := range conf.LeasePreferences {
-		if constraint.ConjunctionsCheck(*storeDesc, preference.Constraints) {
+		if constraint.CheckStoreConjunction(*storeDesc, preference.Constraints) {
 			return false
 		}
 	}

pkg/roachpb/span_config.go

@@ -18,12 +18,12 @@ import (
 	"github.com/cockroachdb/errors"
 )
 
-// StoreMatchesConstraint returns whether a store's attributes or node's
-// locality match the constraint's spec. It notably ignores whether the
-// constraint is required, prohibited, positive, or otherwise.
-func StoreMatchesConstraint(store StoreDescriptor, c Constraint) bool {
+// MatchesConstraint return whether the given attributes and locality tags match
+// the constraint's spec. It ignores whether the constraint is required,
+// prohibited, positive, or otherwise.
+func MatchesConstraint(storeAttrs, nodeAttrs Attributes, nodeLocality Locality, c Constraint) bool {
 	if c.Key == "" {
-		for _, attrs := range []Attributes{store.Attrs, store.Node.Attrs} {
+		for _, attrs := range []Attributes{storeAttrs, nodeAttrs} {
 			for _, attr := range attrs.Attrs {
 				if attr == c.Value {
 					return true
@@ -32,7 +32,7 @@ func StoreMatchesConstraint(store StoreDescriptor, c Constraint) bool {
 		}
 		return false
 	}
-	for _, tier := range store.Node.Locality.Tiers {
+	for _, tier := range nodeLocality.Tiers {
 		if c.Key == tier.Key && c.Value == tier.Value {
 			return true
 		}