diff --git a/pkg/kv/kvclient/kvcoord/dist_sender.go b/pkg/kv/kvclient/kvcoord/dist_sender.go index 970ee9db4308..ca5a0382eb63 100644 --- a/pkg/kv/kvclient/kvcoord/dist_sender.go +++ b/pkg/kv/kvclient/kvcoord/dist_sender.go @@ -1926,6 +1926,8 @@ func (ds *DistSender) sendToReplicas( } // Filter the replicas to only those that are relevant to the routing policy. + // NB: When changing leaseholder policy constraint_status_report should be + // updated appropriately. var replicaFilter ReplicaSliceFilter switch ba.RoutingPolicy { case roachpb.RoutingPolicy_LEASEHOLDER: diff --git a/pkg/kv/kvserver/reports/constraint_stats_report.go b/pkg/kv/kvserver/reports/constraint_stats_report.go index a0a5318cb7f0..6b50490aadf8 100644 --- a/pkg/kv/kvserver/reports/constraint_stats_report.go +++ b/pkg/kv/kvserver/reports/constraint_stats_report.go @@ -70,6 +70,9 @@ const ( // Constraint means that the entry refers to a constraint (i.e. a member of // the constraints field in a zone config). Constraint ConstraintType = "constraint" + // VoterConstraint means that the entry refers to a voter_constraint (i.e. a + // member of voter_constraint field in a zone config). + VoterConstraint ConstraintType = "voter_constraint" // TODO(andrei): add leaseholder preference ) @@ -109,7 +112,7 @@ func (k ConstraintStatusKey) Less(other ConstraintStatusKey) bool { return true } if other.ViolationType.Less(k.ViolationType) { - return true + return false } return k.Constraint.Less(other.Constraint) } @@ -146,6 +149,9 @@ func (r ConstraintReport) ensureEntries(key ZoneKey, zone *zonepb.ZoneConfig) { for _, conjunction := range zone.Constraints { r.ensureEntry(key, Constraint, ConstraintRepr(conjunction.String())) } + for _, conjunction := range zone.VoterConstraints { + r.ensureEntry(key, VoterConstraint, ConstraintRepr(conjunction.String())) + } for i, sz := range zone.Subzones { szKey := ZoneKey{ZoneID: key.ZoneID, SubzoneID: base.SubzoneIDFromIndex(i)} r.ensureEntries(szKey, &sz.Config) @@ -336,6 +342,8 @@ func (r *replicationConstraintStatsReportSaver) upsertConstraintStatus( return nil } +type replicaPredicate func(r roachpb.ReplicaDescriptor) bool + // constraintConformanceVisitor is a visitor that, when passed to visitRanges(), // computes the constraint conformance report (i.e. the // system.replication_constraint_stats table). @@ -348,11 +356,11 @@ type constraintConformanceVisitor struct { report ConstraintReport visitErr bool - // prevZoneKey and prevConstraints maintain state from one range to the next. - // This state can be reused when a range is covered by the same zone config as - // the previous one. Reusing it speeds up the report generation. - prevZoneKey ZoneKey - prevConstraints []zonepb.ConstraintsConjunction + // Zone checker maintain a zone config state internally and can be reused when + // a range is covered by the same zone config as the previous one. Reusing it + // speeds up the report generation. + // It is recreated every time a range is processed with a different zone key. + zoneChecker constraintConformanceChecker } var _ rangeVisitor = &constraintConformanceVisitor{} @@ -408,6 +416,59 @@ func (v *constraintConformanceVisitor) reset(ctx context.Context) { } } +// constraintCheckPolicy defines a set of predicates that define subsets of +// replicas that are checked against conjunctions. +// In order for constraint type to satisfy, each subset of replicas needs to +// satisfy provided constraint conjunction. +// For example voter constraint should match both subset of replicas +// representing outgoing and incoming consensus. +type constraintCheckPolicy struct { + predicates map[zonepb.Constraint_Type][]replicaPredicate +} + +// getViolations finds constraint violations according to sets of predicates +// defined in policy. +func (p constraintCheckPolicy) getViolations( + r *roachpb.RangeDescriptor, + storeResolver StoreResolver, + conjunctions []zonepb.ConstraintsConjunction, +) (res []ConstraintRepr) { + + checkConstraints := func(cj zonepb.ConstraintsConjunction) (bool, ConstraintRepr) { + for _, cc := range cj.Constraints { + t := cc.Type + if t == zonepb.Constraint_DEPRECATED_POSITIVE { + t = zonepb.Constraint_REQUIRED + } + + // Check all store variants defined by policy. + for _, p := range p.predicates[t] { + rds := r.Replicas().FilterToDescriptors(p) + storeDescs := make([]roachpb.StoreDescriptor, len(rds)) + for i, r := range rds { + storeDescs[i] = storeResolver(r.StoreID) + } + // Run each constraint against all store variants to find violations. + replicasRequiredToMatch := int(cj.NumReplicas) + if replicasRequiredToMatch == 0 { + replicasRequiredToMatch = len(storeDescs) + } + if !constraintSatisfied(cc, replicasRequiredToMatch, storeDescs) { + return true, ConstraintRepr(cj.String()) + } + } + } + return false, "" + } + + for _, cj := range conjunctions { + if ok, repr := checkConstraints(cj); ok { + res = append(res, repr) + } + } + return res +} + // visitNewZone is part of the rangeVisitor interface. func (v *constraintConformanceVisitor) visitNewZone( ctx context.Context, r *roachpb.RangeDescriptor, @@ -418,68 +479,165 @@ func (v *constraintConformanceVisitor) visitNewZone( }() // Find the applicable constraints, which may be inherited. + var numVoters int var constraints []zonepb.ConstraintsConjunction + var voterConstraints []zonepb.ConstraintsConjunction var zKey ZoneKey _, err := visitZones(ctx, r, v.cfg, ignoreSubzonePlaceholders, func(_ context.Context, zone *zonepb.ZoneConfig, key ZoneKey) bool { if zone.Constraints == nil { return false } + // Check num voters and only set it if it is different from num replicas. + var numReplicas int32 + if zone.NumReplicas != nil { + numReplicas = *zone.NumReplicas + } + if zone.NumVoters != nil && numReplicas != *zone.NumVoters { + numVoters = int(*zone.NumVoters) + } constraints = zone.Constraints + voterConstraints = zone.VoterConstraints zKey = key return true }) if err != nil { return errors.Wrap(err, "unexpected error visiting zones") } - v.prevZoneKey = zKey - v.prevConstraints = constraints - v.countRange(ctx, r, zKey, constraints) + v.zoneChecker = constraintConformanceChecker{ + zoneKey: zKey, + numVoters: numVoters, + constraints: constraints, + voterConstraints: voterConstraints, + storeResolver: v.storeResolver, + report: &v.report, + } + v.zoneChecker.checkZone(ctx, r) return nil } -// visitSameZone is part of the rangeVisitor interface. func (v *constraintConformanceVisitor) visitSameZone( ctx context.Context, r *roachpb.RangeDescriptor, ) { - v.countRange(ctx, r, v.prevZoneKey, v.prevConstraints) + v.zoneChecker.checkZone(ctx, r) +} + +type constraintConformanceChecker struct { + zoneKey ZoneKey + numVoters int + constraints []zonepb.ConstraintsConjunction + voterConstraints []zonepb.ConstraintsConjunction + + storeResolver StoreResolver + report *ConstraintReport +} + +// visitSameZone is part of the rangeVisitor interface. +func (v *constraintConformanceChecker) checkZone(ctx context.Context, r *roachpb.RangeDescriptor) { + // replicaConstraintsAllVoters are applied to replica constraints when number + // of voters are not specified e.g. equal to number of replicas implicitly or + // equal to number of voters explicitly. + var replicaConstraintsAllVoters = constraintCheckPolicy{ + predicates: map[zonepb.Constraint_Type][]replicaPredicate{ + zonepb.Constraint_REQUIRED: { + isInIncomingQuorumOrNonVoter, isInOutgoingQuorumOrNonVoter, + }, + zonepb.Constraint_PROHIBITED: { + isAny, + }, + }, + } + + // replicaConstraintsWithNonVoters are applied to replica constraints when + // number of voters are explicitly specified and is below total number of + // replicas. + var replicaConstraintsWithNonVoters = constraintCheckPolicy{ + predicates: map[zonepb.Constraint_Type][]replicaPredicate{ + // Note that required predicate are replicas that we can route reads to + // hence LEARNER and VOTER_DEMOTING and VOTER_OUTGOING are excluded. Even + // if DEMOTING/OUTGOING replicas can still serve reads we don't route to + // them and they should disappear momentarily, so we keep constraint + // checks in sync with routing logic in DistSender. + zonepb.Constraint_REQUIRED: { + isInIncomingQuorumOrNonVoter, + }, + zonepb.Constraint_PROHIBITED: { + isAny, + }, + }, + } + + // voterConstraints are applied when voter constraints are explicitly specified + // in zone config to voter constraints. + var voterConstraints = constraintCheckPolicy{ + predicates: map[zonepb.Constraint_Type][]replicaPredicate{ + zonepb.Constraint_REQUIRED: { + isInIncomingQuorum, isInOutgoingQuorum, + }, + zonepb.Constraint_PROHIBITED: { + isVoter, + }, + }, + } + + if v.numVoters != 0 { + v.countRange(ctx, r, v.zoneKey, Constraint, replicaConstraintsWithNonVoters, + v.constraints) + } else { + v.countRange(ctx, r, v.zoneKey, Constraint, replicaConstraintsAllVoters, v.constraints) + } + v.countRange(ctx, r, v.zoneKey, VoterConstraint, voterConstraints, v.voterConstraints) } -func (v *constraintConformanceVisitor) countRange( +func (v *constraintConformanceChecker) countRange( ctx context.Context, r *roachpb.RangeDescriptor, key ZoneKey, + t ConstraintType, + policy constraintCheckPolicy, constraints []zonepb.ConstraintsConjunction, ) { - storeDescs := v.storeResolver(r) - violated := getViolations(ctx, storeDescs, constraints) - for _, c := range violated { - v.report.AddViolation(key, Constraint, c) + for _, violation := range policy.getViolations(r, v.storeResolver, constraints) { + v.report.AddViolation(key, t, violation) } } -// getViolations returns the list of constraints violated by a range. The range -// is represented by the descriptors of the replicas' stores. -func getViolations( - ctx context.Context, - storeDescs []roachpb.StoreDescriptor, - constraintConjunctions []zonepb.ConstraintsConjunction, -) []ConstraintRepr { - var res []ConstraintRepr - // Evaluate all zone constraints for the stores (i.e. replicas) of the given range. - for _, conjunction := range constraintConjunctions { - replicasRequiredToMatch := int(conjunction.NumReplicas) - if replicasRequiredToMatch == 0 { - replicasRequiredToMatch = len(storeDescs) - } - for _, c := range conjunction.Constraints { - if !constraintSatisfied(c, replicasRequiredToMatch, storeDescs) { - res = append(res, ConstraintRepr(conjunction.String())) - break - } - } +func isAny(_ roachpb.ReplicaDescriptor) bool { + return true +} + +func isInIncomingQuorumOrNonVoter(r roachpb.ReplicaDescriptor) bool { + return isInIncomingQuorum(r) || isNonVoter(r) +} + +func isInOutgoingQuorumOrNonVoter(r roachpb.ReplicaDescriptor) bool { + return isInOutgoingQuorum(r) || isNonVoter(r) +} + +func isNonVoter(r roachpb.ReplicaDescriptor) bool { + return r.GetType() == roachpb.NON_VOTER +} + +func isVoter(r roachpb.ReplicaDescriptor) bool { + return isInOutgoingQuorum(r) || isInIncomingQuorum(r) +} + +func isInIncomingQuorum(r roachpb.ReplicaDescriptor) bool { + switch r.GetType() { + case roachpb.VOTER_FULL, roachpb.VOTER_INCOMING: + return true + default: + return false + } +} + +func isInOutgoingQuorum(r roachpb.ReplicaDescriptor) bool { + switch r.GetType() { + case roachpb.VOTER_FULL, roachpb.VOTER_OUTGOING, roachpb.VOTER_DEMOTING_NON_VOTER, roachpb.VOTER_DEMOTING_LEARNER: + return true + default: + return false } - return res } // constraintSatisfied checks that a range (represented by its replicas' stores) diff --git a/pkg/kv/kvserver/reports/constraint_stats_report_test.go b/pkg/kv/kvserver/reports/constraint_stats_report_test.go index 550c1b06fb3b..2a0db1c31b7b 100644 --- a/pkg/kv/kvserver/reports/constraint_stats_report_test.go +++ b/pkg/kv/kvserver/reports/constraint_stats_report_test.go @@ -261,6 +261,145 @@ func TestConformanceReport(t *testing.T) { }, }, }, + { + name: "voter constraints violations", + baseReportTestCase: baseReportTestCase{ + defaultZone: zone{voters: 3}, + schema: []database{ + { + name: "db1", + tables: []table{{name: "t1"}, {name: "t2"}}, + // The database has a zone requesting everything to be on SSDs. + zone: &zone{ + voters: 2, + // The first conjunction will be satisfied; the second won't. + constraints: `{"+region=us,+dc=dc1":1,"+region=us,+dc=dc2":1}`, + voterConstraints: `{"+region=us,+dc=dc2":1}`, + }, + }, + }, + splits: []split{ + {key: "/Table/t1", stores: "1 2"}, + }, + nodes: []node{ + {id: 1, locality: "region=us,dc=dc1", stores: []store{{id: 1}}}, + {id: 2, locality: "region=us,dc=dc3", stores: []store{{id: 2}}}, + }, + }, + exp: []constraintEntry{ + { + object: "db1", + constraint: "+region=us,+dc=dc1:1", + constraintType: Constraint, + numRanges: 0, + }, + { + object: "db1", + constraint: "+region=us,+dc=dc2:1", + constraintType: Constraint, + numRanges: 1, + }, + { + object: "db1", + constraint: "+region=us,+dc=dc2:1", + constraintType: VoterConstraint, + numRanges: 1, + }, + }, + }, + { + name: "learner constraint", + baseReportTestCase: baseReportTestCase{ + defaultZone: zone{voters: 3}, + schema: []database{ + { + name: "db1", + tables: []table{{name: "t1"}, {name: "t2"}}, + zone: &zone{ + // We have learners in both of those stores, but they should + // be ignored for the sake of inclusion, but accounted for + // for the sake of exclusion + constraints: `{"-dc=dc1","+dc=dc5"}`, + }, + }, + }, + splits: []split{ + {key: "/Table/t1", stores: "1l 2 3 4 5l"}, + }, + nodes: []node{ + {id: 1, locality: "region=us,dc=dc1", stores: []store{{id: 1}}}, + {id: 2, locality: "region=us,dc=dc2", stores: []store{{id: 2}}}, + {id: 3, locality: "region=us,dc=dc3", stores: []store{{id: 3}}}, + {id: 4, locality: "region=us,dc=dc4", stores: []store{{id: 4}}}, + {id: 5, locality: "region=us,dc=dc5", stores: []store{{id: 5}}}, + }, + }, + exp: []constraintEntry{ + { + object: "db1", + constraint: "-dc=dc1", + constraintType: Constraint, + numRanges: 1, + }, + { + object: "db1", + constraint: "+dc=dc5", + constraintType: Constraint, + numRanges: 1, + }, + }, + }, + { + name: "quorum constraints", + baseReportTestCase: baseReportTestCase{ + schema: []database{ + { + name: "db1", + tables: []table{{name: "t1"}, {name: "t2"}}, + zone: &zone{ + voters: 3, + constraints: `{"+region=us":3}`, + voterConstraints: `{"+dc=dc1":1,"+dc=dc2":1,"+dc=dc3":1}`, + }, + }, + }, + splits: []split{ + {key: "/Table/t1", stores: "1 2 3o 4i"}, + }, + nodes: []node{ + {id: 1, locality: "region=us,dc=dc1", stores: []store{{id: 1}}}, + {id: 2, locality: "region=us,dc=dc2", stores: []store{{id: 2}}}, + {id: 3, locality: "region=us,dc=dc3", stores: []store{{id: 3}}}, + {id: 4, locality: "region=us,dc=dc3", stores: []store{{id: 4}}}, + }, + }, + exp: []constraintEntry{ + { + object: "db1", + constraint: "+region=us:3", + constraintType: Constraint, + numRanges: 0, + }, + { + object: "db1", + constraint: "+dc=dc1:1", + constraintType: VoterConstraint, + numRanges: 0, + }, + { + object: "db1", + constraint: "+dc=dc2:1", + constraintType: VoterConstraint, + numRanges: 0, + }, + { + object: "db1", + constraint: "+dc=dc3:1", + constraintType: VoterConstraint, + numRanges: 0, + }, + }, + }, } for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { @@ -304,7 +443,10 @@ func runConformanceReportTest(t *testing.T, tc conformanceConstraintTestCase) { ConstraintStatus: v, } } - require.Equal(t, expRows, gotRows) + sort.Slice(expRows, func(i, j int) bool { + return expRows[i].ConstraintStatusKey.Less(expRows[j].ConstraintStatusKey) + }) + require.EqualValues(t, expRows, gotRows) } type zone struct { @@ -316,6 +458,8 @@ type zone struct { nonVoters int32 // "" means unset. "[]" means empty. constraints string + // "" means unset. "[]" means empty. + voterConstraints string } func (z zone) toZoneConfig() zonepb.ZoneConfig { @@ -332,6 +476,14 @@ func (z zone) toZoneConfig() zonepb.ZoneConfig { cfg.Constraints = constraintsList.Constraints cfg.InheritedConstraints = false } + if z.voterConstraints != "" { + var constraintsList zonepb.ConstraintsList + if err := yaml.UnmarshalStrict([]byte(z.voterConstraints), &constraintsList); err != nil { + panic(err) + } + cfg.VoterConstraints = constraintsList.Constraints + cfg.InheritedConstraints = false + } return *cfg } @@ -845,20 +997,14 @@ func compileTestCase(tc baseReportTestCase) (compiledTestCase, error) { nodeLocalities[nodeDesc.NodeID] = nodeDesc.Locality } allLocalities := expandLocalities(nodeLocalities) - storeResolver := func(r *roachpb.RangeDescriptor) []roachpb.StoreDescriptor { - replicas := r.Replicas().FilterToDescriptors(func(_ roachpb.ReplicaDescriptor) bool { - return true - }) - stores := make([]roachpb.StoreDescriptor, len(replicas)) - for i, rep := range replicas { - for _, desc := range allStores { - if rep.StoreID == desc.StoreID { - stores[i] = desc - break - } + storeResolver := func(id roachpb.StoreID) (desc roachpb.StoreDescriptor) { + for _, s := range allStores { + if id == s.StoreID { + desc = s + break } } - return stores + return desc } nodeChecker := func(nodeID roachpb.NodeID) bool { for _, n := range tc.nodes { @@ -1199,3 +1345,253 @@ func (b *systemConfigBuilder) addDBDesc(id int, dbDesc catalog.DatabaseDescripto } b.kv = append(b.kv, roachpb.KeyValue{Key: k, Value: v}) } + +type replicaInfo struct { + t roachpb.ReplicaType + // Node attrs and localities set on StoreDescriptor that replica resolver + // will provide. + a string +} + +func TestConstraintMatching(t *testing.T) { + for _, d := range []struct { + name string + config zone + replicas []replicaInfo + violations []string + }{ + { + name: "no voters set:violates incoming consensus", + config: zone{ + constraints: `{"+dc=us-west-1":1,"+dc=us-east-1":2}`, + }, + replicas: []replicaInfo{ + {roachpb.VOTER_FULL, "dc=us-west-1"}, + {roachpb.VOTER_FULL, "dc=us-east-1"}, + {roachpb.VOTER_OUTGOING, "dc=us-east-1"}, + {roachpb.VOTER_INCOMING, "dc=us-east-2"}, + }, + violations: []string{ + "zone:0,0 type:constraint constraint:+dc=us-east-1:2=1", + }, + }, + { + name: "no voters set:violates outgoing consensus", + config: zone{ + constraints: `{"+dc=us-west-1":1,"+dc=us-east-1":2}`, + }, + replicas: []replicaInfo{ + {roachpb.VOTER_FULL, "dc=us-west-1"}, + {roachpb.VOTER_FULL, "dc=us-east-1"}, + {roachpb.VOTER_OUTGOING, "dc=us-east-2"}, + {roachpb.VOTER_INCOMING, "dc=us-east-1"}, + }, + violations: []string{ + "zone:0,0 type:constraint constraint:+dc=us-east-1:2=1", + }, + }, + { + name: "no voters set:per-replica constraints with unconstrained replicas", + config: zone{ + constraints: `{"+dc=us-west-1":1,"+dc=us-east-1":1}`, + }, + replicas: []replicaInfo{ + {roachpb.VOTER_FULL, "dc=us-west-1"}, + {roachpb.VOTER_FULL, "dc=us-west-2"}, + {roachpb.VOTER_FULL, "dc=us-east-1"}, + }, + }, + { + name: "no voters set:prohibited constraint", + config: zone{ + constraints: `["-dc=us-west-1"]`, + }, + replicas: []replicaInfo{ + {roachpb.VOTER_FULL, "dc=us-east-1"}, + {roachpb.VOTER_FULL, "dc=us-west-2"}, + {roachpb.LEARNER, "dc=us-west-1"}, + }, + violations: []string{ + "zone:0,0 type:constraint constraint:-dc=us-west-1=1", + }, + }, + { + name: "voters set:violates incoming consensus", + config: zone{ + voters: 3, + nonVoters: 2, + constraints: `{"+dc=us-west-1":1,"+dc=us-east-1":2}`, + }, + replicas: []replicaInfo{ + {roachpb.VOTER_FULL, "dc=us-west-1"}, + {roachpb.VOTER_FULL, "dc=us-east-1"}, + {roachpb.NON_VOTER, "dc=us-west-2"}, + {roachpb.VOTER_INCOMING, "dc=us-east-2"}, + {roachpb.NON_VOTER, "dc=us-west-2"}, + }, + violations: []string{ + "zone:0,0 type:constraint constraint:+dc=us-east-1:2=1", + }, + }, + { + name: "voters set:violates prohibited", + config: zone{ + voters: 3, + nonVoters: 2, + constraints: `{"-dc=us-east-2"}`, + }, + replicas: []replicaInfo{ + {roachpb.VOTER_FULL, "dc=us-west-1"}, + {roachpb.VOTER_FULL, "dc=us-east-1"}, + {roachpb.NON_VOTER, "dc=us-east-1"}, + {roachpb.VOTER_INCOMING, "dc=us-east-2"}, + {roachpb.NON_VOTER, "dc=us-east-1"}, + }, + violations: []string{ + "zone:0,0 type:constraint constraint:-dc=us-east-2=1", + }, + }, + { + name: "voters set:violates prohibited non voter", + config: zone{ + voters: 3, + nonVoters: 2, + constraints: `{"-dc=us-east-2"}`, + }, + replicas: []replicaInfo{ + {roachpb.VOTER_FULL, "dc=us-west-1"}, + {roachpb.VOTER_FULL, "dc=us-east-1"}, + {roachpb.NON_VOTER, "dc=us-east-1"}, + {roachpb.VOTER_INCOMING, "dc=us-east-1"}, + {roachpb.NON_VOTER, "dc=us-east-2"}, + }, + violations: []string{ + "zone:0,0 type:constraint constraint:-dc=us-east-2=1", + }, + }, + { + name: "voter constraints:violates incoming", + config: zone{ + voters: 3, + nonVoters: 2, + voterConstraints: `{"+dc=us-west-1":1,"+dc=us-east-1":2}`, + }, + replicas: []replicaInfo{ + {roachpb.VOTER_FULL, "dc=us-west-1"}, + {roachpb.VOTER_FULL, "dc=us-east-1"}, + {roachpb.VOTER_OUTGOING, "dc=us-east-1"}, + {roachpb.NON_VOTER, "dc=us-east-1"}, + {roachpb.VOTER_INCOMING, "dc=us-east-2"}, + {roachpb.NON_VOTER, "dc=us-east-2"}, + }, + violations: []string{ + "zone:0,0 type:voter_constraint constraint:+dc=us-east-1:2=1", + }, + }, + { + name: "voter constraints:violates outgoing", + config: zone{ + voters: 3, + nonVoters: 2, + voterConstraints: `{"+dc=us-west-1":1,"+dc=us-east-1":2}`, + }, + replicas: []replicaInfo{ + {roachpb.VOTER_FULL, "dc=us-west-1"}, + {roachpb.VOTER_FULL, "dc=us-east-1"}, + {roachpb.VOTER_OUTGOING, "dc=us-east-2"}, + {roachpb.NON_VOTER, "dc=us-east-1"}, + {roachpb.VOTER_INCOMING, "dc=us-east-1"}, + {roachpb.NON_VOTER, "dc=us-east-2"}, + }, + violations: []string{ + "zone:0,0 type:voter_constraint constraint:+dc=us-east-1:2=1", + }, + }, + { + name: "voter constraints:violates prohibited", + config: zone{ + voters: 3, + nonVoters: 2, + voterConstraints: `{"-dc=us-east-1"}`, + }, + replicas: []replicaInfo{ + {roachpb.VOTER_FULL, "dc=us-west-1"}, + {roachpb.VOTER_FULL, "dc=us-east-1"}, + {roachpb.NON_VOTER, "dc=us-east-1"}, + {roachpb.VOTER_INCOMING, "dc=us-east-2"}, + {roachpb.NON_VOTER, "dc=us-east-2"}, + }, + violations: []string{ + "zone:0,0 type:voter_constraint constraint:-dc=us-east-1=1", + }, + }, + { + name: "voter constraints:ignores prohibited non-voters", + config: zone{ + voters: 3, + nonVoters: 2, + voterConstraints: `{"-dc=us-east-2"}`, + }, + replicas: []replicaInfo{ + {roachpb.VOTER_FULL, "dc=us-west-1"}, + {roachpb.VOTER_FULL, "dc=us-east-1"}, + {roachpb.NON_VOTER, "dc=us-east-2"}, + {roachpb.VOTER_INCOMING, "dc=us-east-1"}, + {roachpb.NON_VOTER, "dc=us-east-2"}, + }, + }, + } { + t.Run(d.name, func(t *testing.T) { + resolver := func(sid roachpb.StoreID) roachpb.StoreDescriptor { + var storeDesc roachpb.StoreDescriptor + storeDesc.StoreID = sid + for _, al := range strings.Split(d.replicas[sid-1].a, ",") { + tag := strings.Split(al, "=") + if len(tag) == 1 { + storeDesc.Attrs.Attrs = append(storeDesc.Attrs.Attrs, tag[0]) + } else { + storeDesc.Node.Locality.Tiers = append(storeDesc.Node.Locality.Tiers, + roachpb.Tier{Key: tag[0], Value: tag[1]}) + } + } + return storeDesc + } + report := make(ConstraintReport) + zc := d.config.toZoneConfig() + var voters int32 + if zc.NumVoters != nil { + voters = *zc.NumVoters + } + checker := constraintConformanceChecker{ + zoneKey: ZoneKey{}, + numVoters: int(voters), + constraints: zc.Constraints, + voterConstraints: zc.VoterConstraints, + storeResolver: resolver, + report: &report, + } + replicas := make([]roachpb.ReplicaDescriptor, len(d.replicas)) + for i, r := range d.replicas { + replicaType := r.t + replicas[i] = roachpb.ReplicaDescriptor{ + ReplicaID: roachpb.ReplicaID(i + 1), + StoreID: roachpb.StoreID(i + 1), + NodeID: roachpb.NodeID(i + 1), + Type: &replicaType, + } + } + rd := roachpb.RangeDescriptor{ + RangeID: roachpb.RangeID(1), + InternalReplicas: replicas, + } + checker.checkZone(context.Background(), &rd) + var violations []string + for k, v := range *checker.report { + if v.FailRangeCount > 0 { + violations = append(violations, fmt.Sprintf("%s=%d", k.String(), v.FailRangeCount)) + } + } + require.EqualValues(t, d.violations, violations) + }) + } +} diff --git a/pkg/kv/kvserver/reports/critical_localities_report.go b/pkg/kv/kvserver/reports/critical_localities_report.go index 58bf6bab0795..2730c58bcdac 100644 --- a/pkg/kv/kvserver/reports/critical_localities_report.go +++ b/pkg/kv/kvserver/reports/critical_localities_report.go @@ -372,7 +372,11 @@ func (v *criticalLocalitiesVisitor) visitSameZone(ctx context.Context, r *roachp func (v *criticalLocalitiesVisitor) countRange( ctx context.Context, zoneKey ZoneKey, r *roachpb.RangeDescriptor, ) { - stores := v.storeResolver(r) + replicas := r.Replicas().Descriptors() + stores := make([]roachpb.StoreDescriptor, len(replicas)) + for i, r := range replicas { + stores[i] = v.storeResolver(r.StoreID) + } // Collect all the localities of all the replicas. Note that we collect // "expanded" localities: if a replica has a multi-tier locality like diff --git a/pkg/kv/kvserver/reports/critical_localities_report_test.go b/pkg/kv/kvserver/reports/critical_localities_report_test.go index 2a669dd11b69..391d5fce26bd 100644 --- a/pkg/kv/kvserver/reports/critical_localities_report_test.go +++ b/pkg/kv/kvserver/reports/critical_localities_report_test.go @@ -101,8 +101,8 @@ func TestCriticalLocalitiesReport(t *testing.T) { // All the learners are dead, but learners don't matter. So only reg1 // is critical for this range. {key: "/Table/t5", stores: "1 2 3 4l 5l 6l 7l"}, - // Joint-consensus case. Here 1,2,3 are part of the outgoing quorum and - // 1,4,8 are part of the incoming quorum. 4 and 5 are dead, which + // Joint-consensus case. Here 1,2,4 are part of the outgoing quorum and + // 1,5,8 are part of the incoming quorum. 4 and 5 are dead, which // makes all the other nodes critical. So localities "reg1", // "reg1,az1", "reg1,az=2" and "reg8" are critical for this range. {key: "/Table/t6", stores: "1 2o 4o 5i 8i"}, diff --git a/pkg/kv/kvserver/reports/reporter.go b/pkg/kv/kvserver/reports/reporter.go index f886e94049ba..5c20d915e245 100644 --- a/pkg/kv/kvserver/reports/reporter.go +++ b/pkg/kv/kvserver/reports/reporter.go @@ -194,19 +194,15 @@ func (stats *Reporter) update( } allStores := stats.storePool.GetStores() - var getStoresFromGossip StoreResolver = func( - r *roachpb.RangeDescriptor, - ) []roachpb.StoreDescriptor { - storeDescs := make([]roachpb.StoreDescriptor, len(r.Replicas().VoterDescriptors())) + var storesFromGossip StoreResolver = func( + id roachpb.StoreID, + ) roachpb.StoreDescriptor { // We'll return empty descriptors for stores that gossip doesn't have a // descriptor for. These stores will be considered to satisfy all // constraints. // TODO(andrei): note down that some descriptors were missing from gossip // somewhere in the report. - for i, repl := range r.Replicas().VoterDescriptors() { - storeDescs[i] = allStores[repl.StoreID] - } - return storeDescs + return allStores[id] } isLiveMap := stats.liveness.GetIsLiveMap() @@ -224,10 +220,10 @@ func (stats *Reporter) update( // Create the visitors that we're going to pass to visitRanges() below. constraintConfVisitor := makeConstraintConformanceVisitor( - ctx, stats.latestConfig, getStoresFromGossip) + ctx, stats.latestConfig, storesFromGossip) localityStatsVisitor := makeCriticalLocalitiesVisitor( ctx, nodeLocalities, stats.latestConfig, - getStoresFromGossip, isNodeLive) + storesFromGossip, isNodeLive) replicationStatsVisitor := makeReplicationStatsVisitor(ctx, stats.latestConfig, isNodeLive) // Iterate through all the ranges. @@ -511,10 +507,10 @@ func getZoneByID(id config.ObjectID, cfg *config.SystemConfig) (*zonepb.ZoneConf return zone, nil } -// StoreResolver is a function resolving a range to a store descriptor for each -// of the replicas. Empty store descriptors are to be returned when there's no -// information available for the store. -type StoreResolver func(*roachpb.RangeDescriptor) []roachpb.StoreDescriptor +// StoreResolver is a function resolving a store descriptor by its id. Empty +// store descriptors are to be returned when there's no information available +// for the store. +type StoreResolver func(roachpb.StoreID) roachpb.StoreDescriptor // rangeVisitor abstracts the interface for range iteration implemented by all // report generators. diff --git a/pkg/roachpb/metadata_replicas.go b/pkg/roachpb/metadata_replicas.go index 903a58cf0b2f..ceba5948fa3d 100644 --- a/pkg/roachpb/metadata_replicas.go +++ b/pkg/roachpb/metadata_replicas.go @@ -550,6 +550,9 @@ var errReplicaCannotHoldLease = errors.Errorf("replica cannot hold lease") // leaseHolderRemovalAllowed is intended to check if the cluster version is // EnableLeaseHolderRemoval or higher. // TODO(shralex): remove this flag in 23.1 +// NB: This logic should be in sync with constraint_stats_report as report +// will check voter constraint violations. When changing this method, you need +// to update replica filter in report to keep it correct. func CheckCanReceiveLease( wouldbeLeaseholder ReplicaDescriptor, replDescs ReplicaSet, leaseHolderRemovalAllowed bool, ) error {