kvserver: 10-byte discrepancy in SysBytes
#93896
Comments
erikgrinaker
added
C-bug
craig bot
pushed a commit
that referenced
this issue
Dec 19, 2022
93837: backupccl: deflake TestClusterRestoreFailCleanup r=stevendanna a=msbutler This test occasionally flakes due to #86806. To prevent the flakiness, this patch manually sets the kv.bulkio.write_metadata_sst.enabled cluster setting to false. When #86806 gets addressed, this patch should be reverted. Epic: None Release note: None 93897: kvnemesis: ignore `SysBytes:10` MVCC stats discrepancy r=erikgrinaker a=erikgrinaker Resolves #93890. Touches #93896. Touches #93312. Touches #86542. Epic: none Release note: None Co-authored-by: Michael Butler <[email protected]> Co-authored-by: Erik Grinaker <[email protected]>
|
Another instance here: #95282 |
|
cc @cockroachdb/replication |
|
I've dug into some kvnemesis failures -- it reproduces within ~20 minutes or so, but rarely with the same seed. Adding some logging at the spots that mutate Will try to find out where they're coming from. |
|
Grabbed a stack trace during these events, dumping it below since I have to run now. |
This was referenced Mar 13, 2023
|
I think the above was a red herring. This seems to be caused by range merges: after disabling range merges in kvnemesis, I haven't seen a failure in 10.000 runs (it used to fail after about 2000). This is pure speculation, but I wonder if it could have something to do with the below code, which takes the stats computed during the RHS subsume request and then subtracts the range-local replicated data computed via the current batch. I'm going to try subtracting during subsume evaluation, which should avoid any races. cockroach/pkg/kv/kvserver/batcheval/cmd_end_transaction.go Lines 1285 to 1292 in 9aa4f34 |
|
That seems to have been it, passed 10k runs with the following patch. We should find out why -- either the batch we read through contains range-local writes to the RHS, or the subsume isn't preventing writes to the RHS range-local span (which seems bad). diff --git a/pkg/kv/kvserver/batcheval/cmd_end_transaction.go b/pkg/kv/kvserver/batcheval/cmd_end_transaction.go
index 09fbecaaf7e..6ec1184ae8a 100644
--- a/pkg/kv/kvserver/batcheval/cmd_end_transaction.go
+++ b/pkg/kv/kvserver/batcheval/cmd_end_transaction.go
@@ -1282,15 +1282,6 @@ func mergeTrigger(
}
ms.Subtract(msRangeKeyDelta)
- {
- ridPrefix := keys.MakeRangeIDReplicatedPrefix(merge.RightDesc.RangeID)
- sysMS, err := storage.ComputeStats(batch, ridPrefix, ridPrefix.PrefixEnd(), 0 /* nowNanos */)
- if err != nil {
- return result.Result{}, err
- }
- ms.Subtract(sysMS)
- }
-
var pd result.Result
pd.Replicated.Merge = &kvserverpb.Merge{
MergeTrigger: *merge,
diff --git a/pkg/kv/kvserver/batcheval/cmd_subsume.go b/pkg/kv/kvserver/batcheval/cmd_subsume.go
index 05dcc8ddb02..885b1a8cd3d 100644
--- a/pkg/kv/kvserver/batcheval/cmd_subsume.go
+++ b/pkg/kv/kvserver/batcheval/cmd_subsume.go
@@ -135,6 +135,14 @@ func Subsume(
reply.LeaseAppliedIndex = cArgs.EvalCtx.GetLeaseAppliedIndex()
reply.FreezeStart = cArgs.EvalCtx.Clock().NowAsClockTimestamp()
+ // Remove the replicated range-local stats.
+ ridPrefix := keys.MakeRangeIDReplicatedPrefix(desc.RangeID)
+ sysMS, err := storage.ComputeStats(readWriter, ridPrefix, ridPrefix.PrefixEnd(), 0 /* nowNanos */)
+ if err != nil {
+ return result.Result{}, err
+ }
+ reply.MVCCStats.Subtract(sysMS)
+
// Collect a read summary from the RHS leaseholder to ship to the LHS
// leaseholder. This is used to instruct the LHS on how to update its
// timestamp cache to ensure that no future writes are allowed to invalidate |
|
I think this has to be related to resolution of the RHS range descriptor intent. Here's the list of stats updates to the RHS system keys of a merge with incorrect stats around a subsume (the first put is the initial split): The RHS of the merge is considered local during the commit of the merge trigger: cockroach/pkg/kv/kvserver/batcheval/cmd_end_transaction.go Lines 569 to 574 in 86a5852 |
|
On second thought, this can't be the RHS range descriptor intent, because that's not a range ID-local key, it's a range-local key. The range ID-local keys are these: Lines 1034 to 1071 in 0b2c235 The range applied state key is written below Raft, but it's omitted by I think this is likely the range lease, which is a range ID-local key. Looking at a previous case, notice that we're evaluating a range lease for the RHS r82 after we've subsumed it, but before we commit the merge:
cockroach/pkg/kv/kvserver/concurrency/concurrency_manager.go Lines 386 to 390 in 736a67e Here's another repro, for good measure: In fact, it appears to have been the subsume request itself that fired off the lease extension, to extend an expiration lease in the last half of the lease interval (notice the "not acquiring latches"): I'm calling it. I'll submit a PR that computes the range ID-local stats delta during the subsume itself, so that it's consistent with |
Submitted #99017, but I don't think it's sufficient to compute this during subsume evaluation, because that can still race with an MVCC stats update being applied. |
craig bot
pushed a commit
that referenced
this issue
Mar 21, 2023
98225: ui: show normalized CPU Usage metric on Node Map r=koorosh a=koorosh Before, Node map (on Overview page) displayed current system and user CPU usage that didn't represent the same data as CPU Percent metric on Metrics page. Now, Node Map displays the same metric to provide users consistent information. Release note (admin ui, bug fix): show normalized CPU usage on Node Map. Resolve: #87664 98985: storage: scan local keyspace in `TestMVCCHistories` r=erikgrinaker a=erikgrinaker This patch processes the local keyspace along with the user keyspace in `TestMVCCHistories`, which is useful for MVCC stats tests of system keys (e.g. `SysBytes`). This was motivated by tracking down an observed discrepancy in `SysBytes`, but that hasn't borne fruit yet. Touches #93896. Epic: none Release note: None 99050: ccl/sqlproxyccl/acl: fix TestParsingErrorHandling flake r=adityamaru,jaylim-crl a=pjtatlow TestParsingErrorHandling asserts that the error count metric is updated correctly when reading a file fails or succeeds, and the files are checked at a regular interval. For the tests that interval is set to 100ms, and we waited 200ms to ensure the metric would be updated, but that seems to not be reliable. This change increases the wait to 500ms which should ensure the file is re-read before we check the value of the error metics. Fixes #98839 Co-authored-by: Andrii Vorobiov <[email protected]> Co-authored-by: Erik Grinaker <[email protected]> Co-authored-by: PJ Tatlow <[email protected]>
jbowens
added a commit
to jbowens/cockroach
that referenced
this issue
May 31, 2023
Previously, the checks=true configuration of the clearrange roachtest would fatal on stats divergences. Due to cockroachdb#93896, this test can fatal with a SysBytes divergence. This is fixed on master, but the fix will not be backported to 23.1 or 22.2. Disable the enforcement of consistent stats on this branch. Fixes cockroachdb#104078. Informs cockroachdb#104011. Epic: none Release note: none Release justification: non-production code changes
jbowens
added a commit
to jbowens/cockroach
that referenced
this issue
May 31, 2023
Previously, the checks=true configuration of the clearrange roachtest would fatal on stats divergences. Due to cockroachdb#93896, this test can fatal with a SysBytes divergence. This is fixed on master, but the fix will not be backported to 23.1 or 22.2. Disable the enforcement of consistent stats on this branch. Fixes cockroachdb#104011. Epic: none Release note: none Release justification: non-production code changes
itsbilal
added a commit
to itsbilal/cockroach
that referenced
this issue
Aug 15, 2023
Previously, the clearrange roachtest was the only place anywhere in the CockroachDB codebase where we would assert on MVCC stats matching between replicas. This would trip up and fail the clearrange roachtest even in known cases of MVCC stats mismatches. This change removes the code to assert on stats mismatches with consistency checks, but retains the clearrange roachtest's use of aggressive consistency checks, so mismatches in checksums computed on data in each replica will continue to fatal the test. Related to cockroachdb#93896. Fixes cockroachdb#108726. Epic: none Release note: None
craig bot
pushed a commit
that referenced
this issue
Aug 16, 2023
108673: kvstreamer: minor cleanup and unification r=yuzefovich a=yuzefovich This commit performs some minor cleanup to unify the code a bit between GetResp and ScanResp. The only change that is not a noop is the fact that we're now `nil`ing out `ResumeSpan` on GetResponses as well as on ScanResponses when SingleRowLookup hint is `false`. Originally, we were unsetting it for all ScanResponses but in 6343df3 this was lost making the behavior different based on the hint; GetResponses never had this `nil`ing out in the first place. The rationale for actually unsetting the ResumeSpan for both types of the responses is somewhat weak (not confusing the Streamer's user (which doesn't actually inspect the ResumeSpan field) as well as to allow for GC of the keys sooner), but it's better for this behavior to be unified. Epic: None Release note: None 108786: roachtest: allow stats mismatches in clearrange roachtest r=RaduBerinde a=itsbilal Previously, the clearrange roachtest was the only place anywhere in the CockroachDB codebase where we would assert on MVCC stats matching between replicas. This would trip up and fail the clearrange roachtest even in known cases of MVCC stats mismatches. This change removes the code to assert on stats mismatches with consistency checks, but retains the clearrange roachtest's use of aggressive consistency checks, so mismatches in checksums computed on data in each replica will continue to fatal the test. Related to #93896. Fixes #108726. Epic: none Release note: None Co-authored-by: Yahor Yuzefovich <[email protected]> Co-authored-by: Bilal Akhtar <[email protected]>
|
Re-opening this given the PR description of #99017,. |
arulajmani
added a commit
to arulajmani/cockroach
that referenced
this issue
Sep 26, 2024
We started ignoring these in 398dfc7. However, that commit was effectively reverted when we addressed cockroachdb#93896 in 2d855d3. However, like 2d855d3 notes in its description, this was best effort. As such, this can still cause test flakes (e.g. cockroachdb#131187). This patch effectively resurrects 398dfc7, but also additionally quietens `SysBytes:-10` failure mdoes. Epic: none Release note: None
craig bot
pushed a commit
that referenced
this issue
Sep 27, 2024
131446: kvnemesis: ignore SysBytes:{,-}10 MVCC stats discrepancy r=pav-kv a=arulajmani
We started ignoring these in 398dfc7. However, that commit was effectively reverted when we addressed #93896 in 2d855d3. However, like 2d855d3 notes in its description, this was best effort. As such, this can still cause test flakes (e.g. #131187).
This patch effectively resurrects 398dfc7, but also additionally quietens `SysBytes:-10` failure mdoes.
Epic: none
Release note: None
Co-authored-by: Arul Ajmani <[email protected]>
blathers-crl bot
pushed a commit
that referenced
this issue
Sep 27, 2024
We started ignoring these in 398dfc7. However, that commit was effectively reverted when we addressed #93896 in 2d855d3. However, like 2d855d3 notes in its description, this was best effort. As such, this can still cause test flakes (e.g. #131187). This patch effectively resurrects 398dfc7, but also additionally quietens `SysBytes:-10` failure mdoes. Epic: none Release note: None
blathers-crl bot
pushed a commit
that referenced
this issue
Sep 27, 2024
We started ignoring these in 398dfc7. However, that commit was effectively reverted when we addressed #93896 in 2d855d3. However, like 2d855d3 notes in its description, this was best effort. As such, this can still cause test flakes (e.g. #131187). This patch effectively resurrects 398dfc7, but also additionally quietens `SysBytes:-10` failure mdoes. Epic: none Release note: None
blathers-crl bot
pushed a commit
that referenced
this issue
Sep 27, 2024
We started ignoring these in 398dfc7. However, that commit was effectively reverted when we addressed #93896 in 2d855d3. However, like 2d855d3 notes in its description, this was best effort. As such, this can still cause test flakes (e.g. #131187). This patch effectively resurrects 398dfc7, but also additionally quietens `SysBytes:-10` failure mdoes. Epic: none Release note: None
cthumuluru-crdb
pushed a commit
to cthumuluru-crdb/cockroach
that referenced
this issue
Oct 1, 2024
We started ignoring these in 398dfc7. However, that commit was effectively reverted when we addressed cockroachdb#93896 in 2d855d3. However, like 2d855d3 notes in its description, this was best effort. As such, this can still cause test flakes (e.g. cockroachdb#131187). This patch effectively resurrects 398dfc7, but also additionally quietens `SysBytes:-10` failure mdoes. Epic: none Release note: None
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
We occasionally see 10-byte discrepancies in
SysBytes, e.g. in these test failures:The source of these discrepancies is unknown. We should find out what it is.
#93897 ignores this discrepancy in KVNemesis. We should remove that exception when this is fixed.
Jira issue: CRDB-22591
The text was updated successfully, but these errors were encountered: