*: fix bugs related to invalid use of PrefixEnd to compute tenant end key#110241
Merged
craig[bot] merged 6 commits intocockroachdb:masterfrom Sep 8, 2023
Merged
*: fix bugs related to invalid use of PrefixEnd to compute tenant end key#110241craig[bot] merged 6 commits intocockroachdb:masterfrom
craig[bot] merged 6 commits intocockroachdb:masterfrom
Conversation
Ever since cockroachdb#104974 we have decided that the exclusive end key of a virtual keyspace is at "current tenant ID + 1". For this to be possible without special cases, it's important that this key is always computable. For this, we must ensure that no tenant can ever use an ID equal to MaxUint64. This patch achieves it. Release note: None
Prior to this patch, the RPC authz code was restricting requests from secondary tenants to the range /Tenant/ID ... /Tenant/ID/PrefixEnd However, meanwhile, ever since cockroachdb#104974 was merged, the tenant keyspace is actually defined as the range /Tenant/ID ... /Tenant/<ID+1> Under the hood, ".PrefixEnd()" computes the "prefix end" key by incrementing the last byte. Therefore, for most integer values of ID, /Tenant/ID/PrefixEnd and /Tenant/<ID+1> are equivalent. So the mismatch above goes unnoticed until ... we run into an ID value for which they are not equivalent. An example such value is ID 511. This ID encodes to a sequence of bytes such that the .PrefixEnd() key is larger than the /Tenant/511 prefix, but strictly smaller than /Tenant/512, which is the actual exclusive end key for that virtual cluster. Because of this mismatch, the RPC code would reject any request from tenant 511 where the end key was /Tenant/512, even though it should have accepted them. This patch fixes it. Release note: None
Prior to this patch, the streaming code was using `.PrefixEnd()` to compute the exclusive end key of a tenant's keyspace. As established earlier, this is incorrect, the actual end key is `/Tenant/<ID+1>`. When the tenant ID is 109, 511 or other values for which the value encoding needs escape bytes, `.PrefixEnd()` compares smaller than the keyspace end key. (We are not currently putting any data in the range /Tenant/ID/PrefixEnd ... /Tenant/<ID+1> so this did not actually result in a replication bug. There is no release note needed.) Release note: None
Prior to this patch, TenantRanges was incorrectly scanning up to tenantPrefix.PrefixEnd() to collect range descriptors. Since the actual end key may be different, this resulted in the endpoint potentially missing out on the last range of a tenant keyspace, between /Tenant/ID/PrefixEnd and /Tenant/<ID+1>. Thankfully, as of this writing no data is ever written in this part of the keyspace so this bug was not visible in practice. Release note: None
Prior to this patch, the GC logic was incorrectly scanning up to tenantPrefix.PrefixEnd() to delete tenant data. Since the actual end key may be different, this resulted in GC potentially failing to delete the last portion of a tenant keyspace, between /Tenant/ID/PrefixEnd and /Tenant/<ID+1>. Thankfully, as of this writing no data is ever written in this part of the keyspace so this bug was not visible in practice. Release note: None
These are the last few mistaken uses of .PrefixEnd(). Release note: None
Member
stevendanna
approved these changes
Sep 8, 2023
Collaborator
stevendanna
left a comment
There was a problem hiding this comment.
Thanks for the very clear commit messages and for cleaning this up.
Contributor
Author
|
TFYR! bors r=stevendanna |
Contributor
|
Build succeeded: |
|
Encountered an error creating backports. Some common things that can go wrong:
You might need to create your backport manually using the backport tool. error creating merge commit from 62399d7 to blathers/backport-release-23.1-110241: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict [] you may need to manually resolve merge conflicts with the backport tool. Backport to branch 23.1.x failed. See errors above. 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Ever since #104974 we have decided that the exclusive end key of a virtual keyspace is at "current tenant ID + 1", not "start key / PrefixEnd".
This PR completes the investigation requested in #104928 and fixes some bugs in the process.
See individual commits for details.
Fixes #104928.
Fixes #110051.
Epic: CRDB-26091