From 83122f9057272a3a770853a20afe766e0667ed92 Mon Sep 17 00:00:00 2001 From: Oleg Afanasyev Date: Wed, 20 Jul 2022 16:21:00 +0100 Subject: [PATCH] kvserver/reports: replication_constraint_stats use voter constraints Previously replication_constraint_stats were using zone constraints regardless of the fact that num_voters and a separate voter constraints were provided. This was causing report to have violations because range had less voters than replicas, but still were trying to check if we have as many voters as full replica constraints. This was causing ambiguity in reports as there were no violation and no way to figure out which constraint is being violated. This patch changes report to use voter constraints if they are provided or use voters and non voters if voter count is configured lower than replica count, but voter constraints are not provided. Release note (bug fix): table system.replication_constraint_stats is not showing erroneous voter contraint violations when num_voters is configured. Release justification: bugfix to an issue which also needs backporting. --- pkg/kv/kvclient/kvcoord/dist_sender.go | 2 + .../reports/constraint_stats_report.go | 232 ++++++++-- .../reports/constraint_stats_report_test.go | 422 +++++++++++++++++- .../reports/critical_localities_report.go | 6 +- .../critical_localities_report_test.go | 4 +- pkg/kv/kvserver/reports/reporter.go | 24 +- pkg/roachpb/metadata_replicas.go | 3 + 7 files changed, 626 insertions(+), 67 deletions(-) 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 {