kvserver: fix incorrect handling of ExportRequests that are pre-empted by the CPU limiter #97886
Assignees
Labels
A-disaster-recovery
branch-release-23.1
Used to mark GA and release blockers, technical advisories, and bugs for 23.1
C-bug
Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior.
release-blocker
Indicates a release-blocker. Use with branch-release-2x.x label to denote which branch is blocked.
T-disaster-recovery
Comments
adityamaru
added
C-bug
|
cc @cockroachdb/disaster-recovery |
stevendanna
added a commit
to stevendanna/cockroach
that referenced
this issue
Mar 2, 2023
In cockroachdb#97886, we changed ExportRequest such that it returns resume spans that result from the elastic CPU limiter all the way to the caller. This has at least two problems: 1) In a mixed-version state, the caller might not yet know how to handle resume spans. This could result in incomplete responses erroneous being used as if they were full responses. 2) The DistSender inspects a request to determine whether it may stop early. If it shouldn't be able to stop early, then the request is split up, possibly sent in parallel, and all responses are combined. The code which combines responses asserts that neither side has a resume span. As a result, we've seen failures such as crdb_internal.fingerprint(): combining /Tenant/2/Table/106/1/-{8403574544142222370/0-37656332809536692} with /Tenant/{2/Table/106/1/436440321206557763/0-3} since the change was made. Here, we add a new request header field to allow callers to indicate whether they are prepared to accept resume spans. Further, we add that new field into the logic in DistSender which decides how to process requests. The downside here is that crdb_internal.fingerprint won't have its requests sent in parallel. Release note: None Epic: none
stevendanna
added a commit
to stevendanna/cockroach
that referenced
this issue
Mar 7, 2023
In cockroachdb#97886, we changed ExportRequest such that it returns resume spans that result from the elastic CPU limiter all the way to the caller. This has at least two problems: 1) In a mixed-version state, the caller might not yet know how to handle resume spans. This could result in incomplete responses erroneously being used as if they were full responses. 2) The DistSender inspects a request to determine whether it may stop early. If it shouldn't be able to stop early, then the request is split up, possibly sent in parallel, and all responses are combined. The code which combines responses asserts that neither side has a resume span. As a result, we've seen failures such as crdb_internal.fingerprint(): combining /Tenant/2/Table/106/1/-{8403574544142222370/0-37656332809536692} with /Tenant/{2/Table/106/1/436440321206557763/0-3} since the change was made. Here, we add a new request header field to allow callers to indicate whether they are prepared to accept resume spans. Further, we add that new field into the logic in DistSender which decides how to process requests. Now, if ReturnElasticCPUResumeSpans is set, the DistSender will no longer send requests in parallel and knows to expect a possible early exit. The downside here is that crdb_internal.fingerprint won't have its requests sent in parallel. Release note: None Epic: none
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
In #96691 we enabled the elastic CPU limiter for all users of ExportRequests. Since then we have thought of two bugs that were an oversight:
As surfaced by roachtest: c2c/tpcc/warehouses=500/duration=10/cutover=5 failed - source fingerprint scan fails #97693, if the client does not set a TargetBytes on the ExportRequest then DistSender may split the request if it targets >1 range. These requests are then sent in parallel and the responses are combined before returning to the client. The logic that combines the response headers does not expect both the LHS and RHS to have a resume span. With elastic CPU limiting enabled this is very much possible and results in the ExportRequest erroring out https://github.com/cockroachdb/cockroach/blob/master/pkg/kv/kvpb/api.go#L323-L329.
In a mixed version cluster ExportRequest clients running on older nodes do not know to handle resume spans. Thus, if a span gets pre-empted by the elastic CPU limiter then the client will not resend the remaining span to be exported.
To address 2) we plan to add a bool to the
AdmissionHeaderthat indicates whether or not the client is capable of handling paginated ExportRequests. This field will default to false if the client is on older nodes. If the field is true, we will return from kvserver to the client on exceeding CPULimit. If false, we will fall back to retrying the ExportRequest on the server side, as was the case before #96691.To address 1), we will teach DistSender to not parallelize ExportRequests that are sent with the newly added
AdmissionHeaderbool set to true. This is a short-term fix but we expect the loss of parallelization to only affectcrdb_internal.fingerprintsince the other call sites are all on the descriptors table which is usually on a single range. A longer-term fix will be to teach DistSender to combine the responses of parallel requests into a slice of resume spans and then have the client handle multiple, non-overlapping resume spans.cc: @stevendanna
Jira issue: CRDB-24946
Epic CRDB-21944
The text was updated successfully, but these errors were encountered: