Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

kvserver/reports: SREs can't use replication reports to check application of tenant zone configs #82881

Closed
knz opened this issue Jun 14, 2022 · 4 comments
Labels
A-kv-replication Relating to Raft, consensus, and coordination. A-multitenancy Related to multi-tenancy A-zone-configs C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. T-kv KV Team

Comments

@knz
Copy link
Contributor

knz commented Jun 14, 2022

Describe the problem

The code in kvserver/reports/reporter.go, zoneResolver, uses keys.TODOSQLCodec to reverse-engineer which zone configs apply to which ranges.

This means the code is not able to distinguish data for secondary tenants.

Expected behavior

The replication reports should check the tenant-specific zone configs applying to each secondary tenant.

Presumably, the replication reports should not use the zone config data in SystemConfigSpan any more, and instead use the new span config infrastructure.

Slack discussion here.

UX considerations

The primary goal of this issue is to make the cross-tenant replication works in the system tenant.

We can have a separate discussion about whether we want per-tenant replication reports as well. This is out of scope here.

Epic CRDB-10630

Jira issue: CRDB-16708

@knz knz added C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. A-kv-replication Relating to Raft, consensus, and coordination. A-multitenancy Related to multi-tenancy T-kv-replication labels Jun 14, 2022
@blathers-crl
Copy link

blathers-crl bot commented Jun 14, 2022

cc @cockroachdb/replication

@knz
Copy link
Contributor Author

knz commented Jun 14, 2022

We also see a use of keys.TODOSQLCodec in testutils/keysutils/pretty_scanner.go, parseTableKeysAsAscendingInts().
This test code is (as far as I can see) only used by the replication report tests.

This can be modified to take the codec as argument, once that is determined in the replication report code.

@vy-ton
Copy link
Contributor

vy-ton commented Jun 27, 2022

We can have a separate discussion about whether we want per-tenant replication reports as well. This is out of scope here.

I believe this is covered by https://cockroachlabs.atlassian.net/browse/CRDB-10791. FYI @thtruo

@knz
Copy link
Contributor Author

knz commented Mar 30, 2023

We have a better description of the work in the following issue: #100004

Closing this one as duplicate.

@knz knz closed this as completed Mar 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-kv-replication Relating to Raft, consensus, and coordination. A-multitenancy Related to multi-tenancy A-zone-configs C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. T-kv KV Team
Projects
None yet
Development

No branches or pull requests

2 participants