From cce98d1c425bb823528f2d10ec05aa0b62df3048 Mon Sep 17 00:00:00 2001 From: Wenyi Date: Thu, 15 Jun 2023 04:24:45 -0400 Subject: [PATCH] kv: refactor CompareWithLocality to use enum MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Prior to this commit, `CompareWithLocality` returned two boolean values indicating whether two localities were cross-region and cross-zone. However, this required callers to perform additional cross-comparison of these boolean values to make meaningful metrics updates. To simplify this, this commit introduces a new enum type `LocalityComparisonType`. It provides four locality comparison results: cross region, same region cross zone, same region same zone, and undefined (indicating error behavior). This refactoring allow the caller to directly use the comparison result without additional operations. In addition, this commit also updates the logic to classify activities between different regions as cross-regional, regardless of the zone tiers’ configuration. Initially, cross-region but same-zone tiers activities were flagged as misconfiguration. After some discussion, we have decided that regions should be non-overlapping. Hence, same zone tiers from different regions should still be considered as different zones. Part of: https://github.com/cockroachdb/cockroach/issues/103983 Release note: None --- pkg/kv/kvclient/kvcoord/dist_sender.go | 86 ++++++++-------- .../allocator/storepool/store_pool.go | 15 +-- pkg/kv/kvserver/replica_command.go | 2 +- pkg/kv/kvserver/store_snapshot.go | 62 ++++++------ pkg/roachpb/metadata.go | 31 +++--- pkg/roachpb/metadata.proto | 16 +++ pkg/roachpb/metadata_test.go | 74 +++++++------- pkg/server/node.go | 98 +++++++++---------- 8 files changed, 189 insertions(+), 195 deletions(-) diff --git a/pkg/kv/kvclient/kvcoord/dist_sender.go b/pkg/kv/kvclient/kvcoord/dist_sender.go index f243945bcc04..e962a94fbc71 100644 --- a/pkg/kv/kvclient/kvcoord/dist_sender.go +++ b/pkg/kv/kvclient/kvcoord/dist_sender.go @@ -2296,12 +2296,12 @@ func (ds *DistSender) sendToReplicas( if ds.BatchRequestInterceptor != nil { ds.BatchRequestInterceptor(ba) } - shouldIncCrossRegion, shouldIncCrossZone := ds.checkAndUpdateBatchRequestMetrics(ctx, ba) + comparisonResult := ds.checkAndUpdateCrossLocalityBatchMetrics(ctx, ba) br, err = transport.SendNext(ctx, ba) if ds.BatchResponseInterceptor != nil { ds.BatchResponseInterceptor(br) } - ds.checkAndUpdateBatchResponseMetrics(br, shouldIncCrossRegion, shouldIncCrossZone) + ds.updateCrossLocalityBatchMetrics(br, comparisonResult) ds.maybeIncrementErrCounters(br, err) if err != nil { @@ -2557,71 +2557,67 @@ func (ds *DistSender) sendToReplicas( } } -// isCrossRegionCrossZoneBatch returns (bool, bool) - indicating if the given -// batch request is cross-region and cross-zone respectively. -func (ds *DistSender) isCrossRegionCrossZoneBatch( +// getCrossLocalityComparison compares the localities of the current node and +// the destination range node to determine if the given batch request is +// cross-region and cross-zone. +func (ds *DistSender) getCrossLocalityComparison( ctx context.Context, ba *kvpb.BatchRequest, -) (bool, bool) { +) roachpb.LocalityComparisonType { gatewayNodeDesc, err := ds.nodeDescs.GetNodeDescriptor(ba.GatewayNodeID) if err != nil { log.VEventf(ctx, 2, "failed to perform look up for node descriptor %s", err) - return false, false + return roachpb.LocalityComparisonType_UNDEFINED } destinationNodeDesc, err := ds.nodeDescs.GetNodeDescriptor(ba.Replica.NodeID) if err != nil { log.VEventf(ctx, 2, "failed to perform look up for node descriptor %s", err) - return false, false + return roachpb.LocalityComparisonType_UNDEFINED } - isCrossRegion, regionErr, isCrossZone, zoneErr := gatewayNodeDesc.Locality.IsCrossRegionCrossZone(destinationNodeDesc.Locality) + + comparisonResult, regionErr, zoneErr := gatewayNodeDesc.Locality.CompareWithLocality(destinationNodeDesc.Locality) if regionErr != nil { - log.VEventf(ctx, 2, "%v", regionErr) + log.VEventf(ctx, 2, "unable to determine if batch is cross region %v", regionErr) } if zoneErr != nil { - log.VEventf(ctx, 2, "%v", zoneErr) + log.VEventf(ctx, 2, "unable to determine if batch is cross zone %v", zoneErr) } - return isCrossRegion, isCrossZone + return comparisonResult } -// checkAndUpdateBatchRequestMetrics updates the batch requests metrics in a -// more meaningful way. Cross-region metrics monitor activities across different -// regions. Cross-zone metrics monitor cross-zone activities within the same -// region or in cases where region tiers are not configured. The check result is -// returned here to avoid redundant check for metrics updates after receiving -// batch responses. -func (ds *DistSender) checkAndUpdateBatchRequestMetrics( +// checkAndUpdateCrossLocalityBatchMetrics updates the batch requests metrics in +// a more meaningful way. Cross-region metrics monitor activities across +// different regions. Cross-zone metrics monitor cross-zone activities within +// the same region or in cases where region tiers are not configured. The +// locality comparison result is returned here to avoid redundant check for +// metrics updates after receiving batch responses. +func (ds *DistSender) checkAndUpdateCrossLocalityBatchMetrics( ctx context.Context, ba *kvpb.BatchRequest, -) (shouldIncCrossRegion bool, shouldIncCrossZone bool) { +) roachpb.LocalityComparisonType { ds.metrics.ReplicaAddressedBatchRequestBytes.Inc(int64(ba.Size())) - isCrossRegion, isCrossZone := ds.isCrossRegionCrossZoneBatch(ctx, ba) - if isCrossRegion { - if !isCrossZone { - log.VEventf(ctx, 2, "unexpected: cross region but same zone") - } else { - ds.metrics.CrossRegionBatchRequestBytes.Inc(int64(ba.Size())) - shouldIncCrossRegion = true - } - } else { - if isCrossZone { - ds.metrics.CrossZoneBatchRequestBytes.Inc(int64(ba.Size())) - shouldIncCrossZone = true - } - } - return shouldIncCrossRegion, shouldIncCrossZone + comparisonResult := ds.getCrossLocalityComparison(ctx, ba) + switch comparisonResult { + case roachpb.LocalityComparisonType_CROSS_REGION: + ds.metrics.CrossRegionBatchRequestBytes.Inc(int64(ba.Size())) + case roachpb.LocalityComparisonType_SAME_REGION_CROSS_ZONE: + ds.metrics.CrossZoneBatchRequestBytes.Inc(int64(ba.Size())) + case roachpb.LocalityComparisonType_SAME_REGION_SAME_ZONE: + // No metrics or error reporting. + } + return comparisonResult } -// checkAndUpdateBatchResponseMetrics updates the batch response metrics based -// on the shouldIncCrossRegion and shouldIncCrossZone parameters. These -// parameters are determined during the initial check for batch requests. The -// underlying assumption is that if requests were cross-region or cross-zone, -// the response should be as well. -func (ds *DistSender) checkAndUpdateBatchResponseMetrics( - br *kvpb.BatchResponse, shouldIncCrossRegion bool, shouldIncCrossZone bool, +// updateCrossLocalityBatchMetrics updates the batch response metrics based on +// the comparisonResult parameter determined during the initial batch requests +// check. The underlying assumption is that the response should match the +// cross-region or cross-zone nature of the request. +func (ds *DistSender) updateCrossLocalityBatchMetrics( + br *kvpb.BatchResponse, comparisonResult roachpb.LocalityComparisonType, ) { ds.metrics.ReplicaAddressedBatchResponseBytes.Inc(int64(br.Size())) - if shouldIncCrossRegion { + switch comparisonResult { + case roachpb.LocalityComparisonType_CROSS_REGION: ds.metrics.CrossRegionBatchResponseBytes.Inc(int64(br.Size())) - } - if shouldIncCrossZone { + case roachpb.LocalityComparisonType_SAME_REGION_CROSS_ZONE: ds.metrics.CrossZoneBatchResponseBytes.Inc(int64(br.Size())) } } diff --git a/pkg/kv/kvserver/allocator/storepool/store_pool.go b/pkg/kv/kvserver/allocator/storepool/store_pool.go index 286bcce4e18a..17a8d405e449 100644 --- a/pkg/kv/kvserver/allocator/storepool/store_pool.go +++ b/pkg/kv/kvserver/allocator/storepool/store_pool.go @@ -1344,22 +1344,11 @@ func (sp *StorePool) GetNodeLocalityString(nodeID roachpb.NodeID) string { return sp.getNodeLocalityWithString(nodeID).str } -// getNodeLocality returns the locality information for the given node. -func (sp *StorePool) getNodeLocality(nodeID roachpb.NodeID) roachpb.Locality { +// GetNodeLocality returns the locality information for the given node. +func (sp *StorePool) GetNodeLocality(nodeID roachpb.NodeID) roachpb.Locality { return sp.getNodeLocalityWithString(nodeID).locality } -// IsCrossRegionCrossZone takes in two replicas and compares the locality of -// them based on their replica node IDs. It returns (bool, error, bool, error) -// where the boolean values indicate whether the two replicas' nodes are in -// different regions, different zones, along with any lookup errors. -func (sp *StorePool) IsCrossRegionCrossZone( - firstReplica roachpb.ReplicaDescriptor, secReplica roachpb.ReplicaDescriptor, -) (bool, error, bool, error) { - return sp.getNodeLocality(firstReplica.NodeID).IsCrossRegionCrossZone( - sp.getNodeLocality(secReplica.NodeID)) -} - // IsStoreReadyForRoutineReplicaTransfer returns true iff the store's node is // live (as indicated by its `NodeLivenessStatus`) and thus a legal candidate // to receive a replica. diff --git a/pkg/kv/kvserver/replica_command.go b/pkg/kv/kvserver/replica_command.go index 8ba1d7f34ff9..af2b6fef7a73 100644 --- a/pkg/kv/kvserver/replica_command.go +++ b/pkg/kv/kvserver/replica_command.go @@ -3152,7 +3152,7 @@ func (r *Replica) followerSendSnapshot( r.store.metrics.DelegateSnapshotSendBytes.Inc(inc) } r.store.metrics.RangeSnapshotSentBytes.Inc(inc) - r.store.updateCrossLocalitySnapshotMetrics( + r.store.checkAndUpdateCrossLocalitySnapshotMetrics( ctx, req.CoordinatorReplica, req.RecipientReplica, inc, true /* isSent */) switch header.Priority { diff --git a/pkg/kv/kvserver/store_snapshot.go b/pkg/kv/kvserver/store_snapshot.go index f8bce0e375e6..d2ac642b3418 100644 --- a/pkg/kv/kvserver/store_snapshot.go +++ b/pkg/kv/kvserver/store_snapshot.go @@ -973,58 +973,52 @@ func (s *Store) checkSnapshotOverlapLocked( return nil } -// shouldIncrementCrossLocalitySnapshotMetrics returns (bool, bool) - indicating -// if the two given replicas are cross-region and cross-zone respectively. -func (s *Store) shouldIncrementCrossLocalitySnapshotMetrics( +// getCrossLocalityComparison compares the localities of the two given replicas. +func (s *Store) getCrossLocalityComparison( ctx context.Context, firstReplica roachpb.ReplicaDescriptor, secReplica roachpb.ReplicaDescriptor, -) (bool, bool) { - isCrossRegion, regionErr, isCrossZone, zoneErr := s.cfg.StorePool.IsCrossRegionCrossZone( - firstReplica, secReplica) +) roachpb.LocalityComparisonType { + firstLocality := s.cfg.StorePool.GetNodeLocality(firstReplica.NodeID) + secLocality := s.cfg.StorePool.GetNodeLocality(secReplica.NodeID) + comparisonResult, regionErr, zoneErr := firstLocality.CompareWithLocality(secLocality) if regionErr != nil { log.VEventf(ctx, 2, "unable to determine if snapshot is cross region %v", regionErr) } if zoneErr != nil { log.VEventf(ctx, 2, "unable to determine if snapshot is cross zone %v", zoneErr) } - return isCrossRegion, isCrossZone + return comparisonResult } -// updateCrossLocalitySnapshotMetrics updates the snapshot metrics in a more -// meaningful way. Cross-region metrics monitor activities across different -// regions. Cross-zone metrics monitor any cross-zone activities within the same -// region or if the region tiers are not configured. -func (s *Store) updateCrossLocalitySnapshotMetrics( +// checkAndUpdateCrossLocalitySnapshotMetrics updates the snapshot metrics in +// a more meaningful way. Cross-region metrics monitor activities across +// different regions. Cross-zone metrics monitor cross-zone activities within +// the same region or in cases where region tiers are not configured. +func (s *Store) checkAndUpdateCrossLocalitySnapshotMetrics( ctx context.Context, firstReplica roachpb.ReplicaDescriptor, secReplica roachpb.ReplicaDescriptor, inc int64, isSent bool, ) { - isCrossRegion, isCrossZone := s.shouldIncrementCrossLocalitySnapshotMetrics(ctx, firstReplica, secReplica) + comparisonResult := s.getCrossLocalityComparison(ctx, firstReplica, secReplica) if isSent { - if isCrossRegion { - if !isCrossZone { - log.VEventf(ctx, 2, "unexpected: cross region but same zone") - } else { - s.metrics.RangeSnapShotCrossRegionSentBytes.Inc(inc) - } - } else { - if isCrossZone { - s.metrics.RangeSnapShotCrossZoneSentBytes.Inc(inc) - } + switch comparisonResult { + case roachpb.LocalityComparisonType_CROSS_REGION: + s.metrics.RangeSnapShotCrossRegionSentBytes.Inc(inc) + case roachpb.LocalityComparisonType_SAME_REGION_CROSS_ZONE: + s.metrics.RangeSnapShotCrossZoneSentBytes.Inc(inc) + case roachpb.LocalityComparisonType_SAME_REGION_SAME_ZONE: + // No metrics or error reporting. } } else { // isReceived - if isCrossRegion { - if !isCrossZone { - log.VEventf(ctx, 2, "unexpected: cross region but same zone") - } else { - s.metrics.RangeSnapShotCrossRegionRcvdBytes.Inc(inc) - } - } else { - if isCrossZone { - s.metrics.RangeSnapShotCrossZoneRcvdBytes.Inc(inc) - } + switch comparisonResult { + case roachpb.LocalityComparisonType_CROSS_REGION: + s.metrics.RangeSnapShotCrossRegionRcvdBytes.Inc(inc) + case roachpb.LocalityComparisonType_SAME_REGION_CROSS_ZONE: + s.metrics.RangeSnapShotCrossZoneRcvdBytes.Inc(inc) + case roachpb.LocalityComparisonType_SAME_REGION_SAME_ZONE: + // No metrics or error reporting. } } } @@ -1145,7 +1139,7 @@ func (s *Store) receiveSnapshot( recordBytesReceived := func(inc int64) { s.metrics.RangeSnapshotRcvdBytes.Inc(inc) - s.updateCrossLocalitySnapshotMetrics( + s.checkAndUpdateCrossLocalitySnapshotMetrics( ctx, header.RaftMessageRequest.FromReplica, header.RaftMessageRequest.ToReplica, inc, false /* isSent */) switch header.Priority { diff --git a/pkg/roachpb/metadata.go b/pkg/roachpb/metadata.go index ee56469552e1..91d2b1427c19 100644 --- a/pkg/roachpb/metadata.go +++ b/pkg/roachpb/metadata.go @@ -675,14 +675,11 @@ func (l Locality) getFirstRegionFirstZone() ( return firstRegionValue, hasRegion, firstZoneKey, firstZoneValue, hasZone } -// IsCrossRegionCrossZone returns multiple values containing: -// 1. A boolean value indicating if this and the provided locality are -// cross-region. -// 2. Error indicating if either locality does not have a "region" tier key. -// 3. A boolean value indicating if this and the provided locality are -// cross-zone. -// 4. Error indicating if either locality does not have a "zone" tier key or if -// the first "zone" tier keys used by two localities are different. +// CompareWithLocality returns the comparison result between this and the +// provided other locality along with any lookup errors. Possible errors include +// 1. if either locality does not have a "region" tier key. 2. if either +// locality does not have a "zone" tier key or if the first "zone" tier keys +// used by two localities are different. // // Limitation: // - It is unfortunate that the tier key is hardcoded here. Ideally, we would @@ -699,14 +696,14 @@ func (l Locality) getFirstRegionFirstZone() ( // a single function to avoid overhead. If you are adding additional locality // tiers comparisons, it is recommended to handle them within one tier list // iteration. -func (l Locality) IsCrossRegionCrossZone( +func (l Locality) CompareWithLocality( other Locality, -) (isCrossRegion bool, regionErr error, isCrossZone bool, zoneErr error) { +) (_ LocalityComparisonType, regionErr error, zoneErr error) { firstRegionValue, hasRegion, firstZoneKey, firstZone, hasZone := l.getFirstRegionFirstZone() firstRegionValueOther, hasRegionOther, firstZoneKeyOther, firstZoneOther, hasZoneOther := other.getFirstRegionFirstZone() - isCrossRegion = firstRegionValue != firstRegionValueOther - isCrossZone = firstZone != firstZoneOther + isCrossRegion := firstRegionValue != firstRegionValueOther + isCrossZone := firstZone != firstZoneOther if !hasRegion || !hasRegionOther { isCrossRegion = false @@ -718,7 +715,15 @@ func (l Locality) IsCrossRegionCrossZone( zoneErr = errors.Errorf("localities must have a valid zone tier key for cross-zone comparison") } - return isCrossRegion, regionErr, isCrossZone, zoneErr + if isCrossRegion { + return LocalityComparisonType_CROSS_REGION, regionErr, zoneErr + } else { + if isCrossZone { + return LocalityComparisonType_SAME_REGION_CROSS_ZONE, regionErr, zoneErr + } else { + return LocalityComparisonType_SAME_REGION_SAME_ZONE, regionErr, zoneErr + } + } } // SharedPrefix returns the number of this locality's tiers which match those of diff --git a/pkg/roachpb/metadata.proto b/pkg/roachpb/metadata.proto index 01aebf22f1a7..c8cbfc7824f5 100644 --- a/pkg/roachpb/metadata.proto +++ b/pkg/roachpb/metadata.proto @@ -434,6 +434,22 @@ message Tier { optional string value = 2 [(gogoproto.nullable) = false]; } +// LocalityComparisonType represents different types of comparison results that +// indicate the relationship between two localities. +enum LocalityComparisonType { + // CROSS_REGION indicates that the two localities have different region tiers. + CROSS_REGION = 0; + // SAME_REGION_CROSS_ZONE indicates that the two localities have the same + // region tiers but different zone tiers. + SAME_REGION_CROSS_ZONE = 1; + // SAME_REGION_SAME_ZONE indicates that the two localities have same region + // and same zone tiers. + SAME_REGION_SAME_ZONE = 2; + // UNDEFINED represents an undefined comparison result, indicating error + // behavior. + UNDEFINED = 3; +} + message Version { option (gogoproto.equal) = true; option (gogoproto.goproto_stringer) = false; diff --git a/pkg/roachpb/metadata_test.go b/pkg/roachpb/metadata_test.go index de8940fbfaed..cc95cff2c4fa 100644 --- a/pkg/roachpb/metadata_test.go +++ b/pkg/roachpb/metadata_test.go @@ -221,7 +221,7 @@ func TestLocalityMatches(t *testing.T) { } } -func TestLocalityIsCrossRegionCrossZone(t *testing.T) { +func TestLocalityCompareWithLocality(t *testing.T) { regionErrStr := "localities must have a valid region tier key for cross-region comparison" zoneErrStr := "localities must have a valid zone tier key for cross-zone comparison" @@ -251,82 +251,81 @@ func TestLocalityIsCrossRegionCrossZone(t *testing.T) { for _, tc := range []struct { l string other string - isCrossRegion bool - isCrossZone bool + localityType LocalityComparisonType crossRegionErr string crossZoneErr string }{ // -------- Part 1: check for different zone tier alternatives -------- // Valid tier keys, same regions and same zones. {l: "region=us-west,zone=us-west-1", other: "region=us-west,zone=us-west-1", - isCrossRegion: false, isCrossZone: false, crossRegionErr: "", crossZoneErr: ""}, + localityType: LocalityComparisonType_SAME_REGION_SAME_ZONE, crossRegionErr: "", crossZoneErr: ""}, // Valid tier keys, different regions and different zones. {l: "region=us-west,zone=us-west-1", other: "region=us-east,zone=us-west-2", - isCrossRegion: true, isCrossZone: true, crossRegionErr: "", crossZoneErr: ""}, + localityType: LocalityComparisonType_CROSS_REGION, crossRegionErr: "", crossZoneErr: ""}, // Valid tier keys, different regions and different zones. {l: "region=us-west,availability-zone=us-west-1", other: "region=us-east,availability-zone=us-east-1", - isCrossRegion: true, isCrossZone: true, crossRegionErr: "", crossZoneErr: ""}, + localityType: LocalityComparisonType_CROSS_REGION, crossRegionErr: "", crossZoneErr: ""}, // Valid tier keys, same regions and different zones. {l: "region=us-west,az=us-west-1", other: "region=us-west,other-keys=us,az=us-east-1", - isCrossRegion: false, isCrossZone: true, crossRegionErr: "", crossZoneErr: ""}, + localityType: LocalityComparisonType_SAME_REGION_CROSS_ZONE, crossRegionErr: "", crossZoneErr: ""}, // Invalid zone tier key and different regions. {l: "region=us-west,availability-zone=us-west-1", other: "region=us-east,zone=us-east-1", - isCrossRegion: true, isCrossZone: false, crossRegionErr: "", crossZoneErr: zoneErrStr}, + localityType: LocalityComparisonType_CROSS_REGION, crossRegionErr: "", crossZoneErr: zoneErrStr}, // Valid zone tier key (edge case), different zones and regions. {l: "region=us-west,zone=us-west-1", other: "region=us-east,zone=us-west-2,az=us-west-1", - isCrossRegion: true, isCrossZone: true, crossRegionErr: "", crossZoneErr: ""}, + localityType: LocalityComparisonType_CROSS_REGION, crossRegionErr: "", crossZoneErr: ""}, // Missing zone tier key and different regions. {l: "region=us-west,zone=us-west-1", other: "region=us-east", - isCrossRegion: true, isCrossZone: false, crossRegionErr: "", crossZoneErr: zoneErrStr}, + localityType: LocalityComparisonType_CROSS_REGION, crossRegionErr: "", crossZoneErr: zoneErrStr}, // Different region and different zones with non-unique & invalid zone tier key. {l: "region=us-west,zone=us-west-1,az=us-west-2", other: "az=us-west-1,region=us-west,zone=us-west-1", - isCrossRegion: false, isCrossZone: false, crossRegionErr: "", crossZoneErr: zoneErrStr}, + localityType: LocalityComparisonType_SAME_REGION_SAME_ZONE, crossRegionErr: "", crossZoneErr: zoneErrStr}, // Different regions and different zones with non-unique & valid zone tier key. {l: "region=us-west,az=us-west-2,zone=us-west-1", other: "region=us-west,az=us-west-1", - isCrossRegion: false, isCrossZone: true, crossRegionErr: "", crossZoneErr: ""}, + localityType: LocalityComparisonType_SAME_REGION_CROSS_ZONE, crossRegionErr: "", crossZoneErr: ""}, // Invalid region tier key and different zones. {l: "country=us,zone=us-west-1", other: "country=us,zone=us-west-2", - isCrossRegion: false, isCrossZone: true, crossRegionErr: regionErrStr, crossZoneErr: ""}, + localityType: LocalityComparisonType_SAME_REGION_CROSS_ZONE, crossRegionErr: regionErrStr, crossZoneErr: ""}, // Missing region tier key and different zones. {l: "az=us-west-1", other: "region=us-east,az=us-west-2", - isCrossRegion: false, isCrossZone: true, crossRegionErr: regionErrStr, crossZoneErr: ""}, + localityType: LocalityComparisonType_SAME_REGION_CROSS_ZONE, crossRegionErr: regionErrStr, crossZoneErr: ""}, // Invalid region and zone tier key. {l: "invalid-key=us-west,zone=us-west-1", other: "region=us-east,invalid-key=us-west-1", - isCrossRegion: false, isCrossZone: false, crossRegionErr: regionErrStr, crossZoneErr: zoneErrStr}, + localityType: LocalityComparisonType_SAME_REGION_SAME_ZONE, crossRegionErr: regionErrStr, crossZoneErr: zoneErrStr}, // Invalid region and zone tier key. {l: "country=us,dc=us-west-2", other: "country=us,dc=us-west-2", - isCrossRegion: false, isCrossZone: false, crossRegionErr: regionErrStr, crossZoneErr: zoneErrStr}, + localityType: LocalityComparisonType_SAME_REGION_SAME_ZONE, crossRegionErr: regionErrStr, crossZoneErr: zoneErrStr}, // -------- Part 2: single region, single zone -------- // One: (both) Two: (region) {l: makeLocalityStr(firstRegionStr, firstZoneStr), other: makeLocalityStr(secRegionStr, ""), - isCrossRegion: true, isCrossZone: false, crossRegionErr: "", crossZoneErr: zoneErrStr}, + localityType: LocalityComparisonType_CROSS_REGION, crossRegionErr: "", crossZoneErr: zoneErrStr}, // One: (both) Two: (zone) {l: makeLocalityStr(firstRegionStr, firstZoneStr), other: makeLocalityStr("", secZoneStr), - isCrossRegion: false, isCrossZone: true, crossRegionErr: regionErrStr, crossZoneErr: ""}, + localityType: LocalityComparisonType_SAME_REGION_CROSS_ZONE, crossRegionErr: regionErrStr, crossZoneErr: ""}, // One: (region) Two: (region) {l: makeLocalityStr(firstRegionStr, ""), other: makeLocalityStr(secRegionStr, ""), - isCrossRegion: true, isCrossZone: false, crossRegionErr: "", crossZoneErr: zoneErrStr}, + localityType: LocalityComparisonType_CROSS_REGION, crossRegionErr: "", crossZoneErr: zoneErrStr}, // One: (zone) Two: (zone) {l: makeLocalityStr("", firstZoneStr), other: makeLocalityStr("", secZoneStr), - isCrossRegion: false, isCrossZone: true, crossRegionErr: regionErrStr, crossZoneErr: ""}, + localityType: LocalityComparisonType_SAME_REGION_CROSS_ZONE, crossRegionErr: regionErrStr, crossZoneErr: ""}, // One: (region) Two: (zone) {l: makeLocalityStr(firstRegionStr, ""), other: makeLocalityStr("", secZoneStr), - isCrossRegion: false, isCrossZone: false, crossRegionErr: regionErrStr, crossZoneErr: zoneErrStr}, + localityType: LocalityComparisonType_SAME_REGION_SAME_ZONE, crossRegionErr: regionErrStr, crossZoneErr: zoneErrStr}, // One: (both) Two: (both) {l: makeLocalityStr(firstRegionStr, firstZoneStr), other: makeLocalityStr(secRegionStr, secZoneStr), - isCrossRegion: true, isCrossZone: true, crossRegionErr: "", crossZoneErr: ""}, + localityType: LocalityComparisonType_CROSS_REGION, crossRegionErr: "", crossZoneErr: ""}, // One: (none) Two: (none) {l: makeLocalityStr("", ""), other: makeLocalityStr("", ""), - isCrossRegion: false, isCrossZone: false, crossRegionErr: regionErrStr, crossZoneErr: zoneErrStr}, + localityType: LocalityComparisonType_SAME_REGION_SAME_ZONE, crossRegionErr: regionErrStr, crossZoneErr: zoneErrStr}, // One: (region) Two: (none) {l: makeLocalityStr(firstRegionStr, ""), other: makeLocalityStr("", ""), - isCrossRegion: false, isCrossZone: false, crossRegionErr: regionErrStr, crossZoneErr: zoneErrStr}, + localityType: LocalityComparisonType_SAME_REGION_SAME_ZONE, crossRegionErr: regionErrStr, crossZoneErr: zoneErrStr}, // One: (zone) Two: (none) {l: makeLocalityStr("", firstZoneStr), other: makeLocalityStr("", ""), - isCrossRegion: false, isCrossZone: false, crossRegionErr: regionErrStr, crossZoneErr: zoneErrStr}, + localityType: LocalityComparisonType_SAME_REGION_SAME_ZONE, crossRegionErr: regionErrStr, crossZoneErr: zoneErrStr}, // One: (both) Two: (none) {l: makeLocalityStr(firstRegionStr, firstZoneStr), other: makeLocalityStr("", ""), - isCrossRegion: false, isCrossZone: false, crossRegionErr: regionErrStr, crossZoneErr: zoneErrStr}, + localityType: LocalityComparisonType_SAME_REGION_SAME_ZONE, crossRegionErr: regionErrStr, crossZoneErr: zoneErrStr}, } { t.Run(fmt.Sprintf("%s-crosslocality-%s", tc.l, tc.other), func(t *testing.T) { var l Locality @@ -334,24 +333,21 @@ func TestLocalityIsCrossRegionCrossZone(t *testing.T) { require.NoError(t, l.Set(tc.l)) require.NoError(t, other.Set(tc.other)) type localities struct { - isCrossRegion bool - isCrossZone bool + localityType LocalityComparisonType crossRegionErr string crossZoneErr string } - isCrossRegion, crossRegionErr, isCrossZone, crossZoneErr := l.IsCrossRegionCrossZone(other) - crossRegionErrStr := "" - if crossRegionErr != nil { - crossRegionErrStr = crossRegionErr.Error() + localityType, regionErr, zoneErr := l.CompareWithLocality(other) + regionErrStr := "" + if regionErr != nil { + regionErrStr = regionErr.Error() } - crossZoneErrStr := "" - if crossZoneErr != nil { - crossZoneErrStr = crossZoneErr.Error() + zoneErrStr := "" + if zoneErr != nil { + zoneErrStr = zoneErr.Error() } - actual := localities{isCrossRegion, isCrossZone, - crossRegionErrStr, crossZoneErrStr} - expected := localities{tc.isCrossRegion, tc.isCrossZone, - tc.crossRegionErr, tc.crossZoneErr} + actual := localities{localityType, regionErrStr, zoneErrStr} + expected := localities{tc.localityType, tc.crossRegionErr, tc.crossZoneErr} require.Equal(t, expected, actual) }) } diff --git a/pkg/server/node.go b/pkg/server/node.go index ad35e68ad0b5..11fa232a403d 100644 --- a/pkg/server/node.go +++ b/pkg/server/node.go @@ -1325,82 +1325,80 @@ func (n *Node) batchInternal( return br, nil } -// isCrossRegionCrossZoneBatch returns (bool, bool) - indicating if the given -// batch request is cross-region and cross-zone respectively. -func (n *Node) isCrossRegionCrossZoneBatch( +// getCrossLocalityComparison compares the localities of the gateway node and +// the current node to determine if the given batch request is cross-region and +// cross-zone. +func (n *Node) getCrossLocalityComparison( ctx context.Context, ba *kvpb.BatchRequest, -) (bool, bool) { +) roachpb.LocalityComparisonType { gossip := n.storeCfg.Gossip if gossip == nil { log.VEventf(ctx, 2, "gossip is not configured") - return false, false + return roachpb.LocalityComparisonType_UNDEFINED } gatewayNodeDesc, err := gossip.GetNodeDescriptor(ba.GatewayNodeID) if err != nil { log.VEventf(ctx, 2, - "failed to perform look up for node descriptor %+v", err) - return false, false + "failed to perform look up for node descriptor %v", err) + return roachpb.LocalityComparisonType_UNDEFINED } - isCrossRegion, regionErr, isCrossZone, zoneErr := n.Descriptor.Locality. - IsCrossRegionCrossZone(gatewayNodeDesc.Locality) + comparisonResult, regionErr, zoneErr := n.Descriptor.Locality.CompareWithLocality(gatewayNodeDesc.Locality) if regionErr != nil { - log.VEventf(ctx, 2, "%v", regionErr) + log.VEventf(ctx, 2, "unable to determine if batch is cross region %v", regionErr) } if zoneErr != nil { - log.VEventf(ctx, 2, "%v", zoneErr) + log.VEventf(ctx, 2, "unable to determine if batch is cross zone %v", zoneErr) } - return isCrossRegion, isCrossZone + return comparisonResult } -// checkAndUpdateBatchRequestMetrics updates the batch requests metrics in a -// more meaningful way. Cross-region metrics monitor activities across different -// regions. Cross-zone metrics monitor cross-zone activities within the same -// region or in cases where region tiers are not configured. The check result is -// returned here to avoid redundant check for metrics updates after receiving -// batch responses. -func (n *Node) checkAndUpdateBatchRequestMetrics( +// checkAndUpdateCrossLocalityBatchMetrics updates the batch requests metrics in +// a more meaningful way. Cross-region metrics monitor activities across +// different regions. Cross-zone metrics monitor cross-zone activities within +// the same region or in cases where region tiers are not configured. The +// locality comparison result is returned here to avoid redundant check for +// metrics updates after receiving batch responses. +func (n *Node) checkAndUpdateCrossLocalityBatchMetrics( ctx context.Context, ba *kvpb.BatchRequest, shouldIncrement bool, -) (shouldIncCrossRegion bool, shouldIncCrossZone bool) { +) roachpb.LocalityComparisonType { if !shouldIncrement { - return false, false + // shouldIncrement is set to false using testing knob in specific tests to + // filter out metrics changes caused by irrelevant batch requests. + return roachpb.LocalityComparisonType_UNDEFINED } n.metrics.BatchRequestsBytes.Inc(int64(ba.Size())) - isCrossRegion, isCrossZone := n.isCrossRegionCrossZoneBatch(ctx, ba) - if isCrossRegion { - if !isCrossZone { - log.VEventf(ctx, 2, "unexpected: cross region but same zone") - } else { - n.metrics.CrossRegionBatchRequestBytes.Inc(int64(ba.Size())) - shouldIncCrossRegion = true - } - } else { - if isCrossZone { - n.metrics.CrossZoneBatchRequestBytes.Inc(int64(ba.Size())) - shouldIncCrossZone = true - } - } - return shouldIncCrossRegion, shouldIncCrossZone -} - -// checkAndUpdateBatchResponseMetrics updates the batch response metrics based -// on the shouldIncCrossRegion and shouldIncCrossZone parameters. These -// parameters are determined during the initial check for batch requests. The -// underlying assumption is that if requests were cross-region or cross-zone, -// the response should be as well. -func (n *Node) checkAndUpdateBatchResponseMetrics( - br *kvpb.BatchResponse, shouldIncCrossRegion bool, shouldIncCrossZone bool, shouldIncrement bool, + comparisonResult := n.getCrossLocalityComparison(ctx, ba) + switch comparisonResult { + case roachpb.LocalityComparisonType_CROSS_REGION: + n.metrics.CrossRegionBatchRequestBytes.Inc(int64(ba.Size())) + case roachpb.LocalityComparisonType_SAME_REGION_CROSS_ZONE: + n.metrics.CrossZoneBatchRequestBytes.Inc(int64(ba.Size())) + case roachpb.LocalityComparisonType_SAME_REGION_SAME_ZONE: + // No metrics or error reporting. + } + return comparisonResult +} + +// updateCrossLocalityBatchMetrics updates the batch response metrics based on +// the comparisonResult parameter determined during the initial batch requests +// check. The underlying assumption is that the response should match the +// cross-region or cross-zone nature of the request. +func (n *Node) updateCrossLocalityBatchMetrics( + br *kvpb.BatchResponse, comparisonResult roachpb.LocalityComparisonType, shouldIncrement bool, ) { if !shouldIncrement { + // shouldIncrement is set to false using testing knob in specific tests to + // filter out metrics changes caused by irrelevant batch requests. return } n.metrics.BatchResponsesBytes.Inc(int64(br.Size())) - if shouldIncCrossRegion { + switch comparisonResult { + case roachpb.LocalityComparisonType_CROSS_REGION: n.metrics.CrossRegionBatchResponseBytes.Inc(int64(br.Size())) - } - if shouldIncCrossZone { + case roachpb.LocalityComparisonType_SAME_REGION_CROSS_ZONE: n.metrics.CrossZoneBatchResponseBytes.Inc(int64(br.Size())) } } @@ -1426,7 +1424,7 @@ func (n *Node) Batch(ctx context.Context, args *kvpb.BatchRequest) (*kvpb.BatchR // requests that are irrelevant to our tests. shouldIncrement = fn(args) } - shouldIncCrossRegion, shouldIncCrossZone := n.checkAndUpdateBatchRequestMetrics(ctx, args, shouldIncrement) + comparisonResult := n.checkAndUpdateCrossLocalityBatchMetrics(ctx, args, shouldIncrement) // NB: Node.Batch is called directly for "local" calls. We don't want to // carry the associated log tags forward as doing so makes adding additional @@ -1484,7 +1482,7 @@ func (n *Node) Batch(ctx context.Context, args *kvpb.BatchRequest) (*kvpb.BatchR // requests that are irrelevant to our tests. shouldIncrement = fn(br) } - n.checkAndUpdateBatchResponseMetrics(br, shouldIncCrossRegion, shouldIncCrossZone, shouldIncrement) + n.updateCrossLocalityBatchMetrics(br, comparisonResult, shouldIncrement) if buildutil.CrdbTestBuild && br.Error != nil && n.testingErrorEvent != nil { n.testingErrorEvent(ctx, args, errors.DecodeError(ctx, br.Error.EncodedError)) }