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

streamingest: allow replicating system tenants to other tenants #122001

Merged
merged 2 commits into from
Apr 12, 2024

Conversation

dt
Copy link
Member

@dt dt commented Apr 9, 2024

@dt dt requested review from a team as code owners April 9, 2024 11:16
@dt dt requested review from msbutler and removed request for a team April 9, 2024 11:16
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Release note: none.
Epic: none.
@dt dt changed the title streamingest,bootstrap: allow replicating system tenants to other tenants streamingest: allow replicating system tenants to other tenants Apr 10, 2024
@dt
Copy link
Member Author

dt commented Apr 10, 2024

Now with a couple tests and RFAL.

@dt dt requested a review from stevendanna April 10, 2024 17:36
Copy link
Collaborator

@stevendanna stevendanna left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM but perhaps we should let @msbutler have a look as well since I already watched this code get written and might be a bit biased.

Copy link
Collaborator

@msbutler msbutler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! I'm glad all the span config streaming stuff works out of the box! Were you able to verify that the a user connected to the system tenant cannot directly write to the zone config table?

Also, it may be helpful to write out in the commit message some of the subtle ways that streaming from the system tenant is different than streaming from a main tenant (e.g. we'll replicate a zone configs table that will never get used after cutover).

@@ -34,6 +34,12 @@ import (
)

func makeTenantSpan(tenantID uint64) roachpb.Span {
// TODO(dt): remove conditional if we make MakeTenantSpan do this.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you add a docstring which explains why this is necessary?

Also, for a regular main tenant, do they have any non table data in their key space? I.e. if we replicate the key span keys.MakeTenantSpan(3), will that only ever contain table data?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or a regular main tenant, do they have any non table data in their key space

No.

replicate the key span keys.MakeTenantSpan(3), will that only ever contain table data

Correct.

@dt
Copy link
Member Author

dt commented Apr 12, 2024

the subtle ways that streaming from the system tenant is different... we'll replicate a zone configs table that will never get used after cutover

I think we overstated those differences in our chat; e.g. the zones table does get used, that is read by the reconciler and fed to span configs. After #122005 there really aren't many differences. It really is just that some jobs are only supposed to run in the system tenant so their owners may need to be aware of the fact they could end up in a non-system tenant and need to terminate themselves, a la the first commit here where we do that for our pcr job.

@dt dt added the backport-24.1.x Flags PRs that need to be backported to 24.1. label Apr 12, 2024
After cockroachdb#122005 there aren't really many differences between them. The
system tenant has a few tables that will be ignored by secondary tenants
but after that change they are included anyway and just ignored, so we
can also just replicate them and ignore them as well.

One thing to consider is jobs which only want to run in the system tenant:
these should exit if they find themselves running in another tenant, the
way the PCR ingestion job does.

Release note: none.
Epic: none.
@dt
Copy link
Member Author

dt commented Apr 12, 2024

TFTR!

bors r+

@craig
Copy link
Contributor

craig bot commented Apr 12, 2024

Build failed:

@dt
Copy link
Member Author

dt commented Apr 12, 2024

bors r+

@craig craig bot merged commit 81bb6ce into cockroachdb:master Apr 12, 2024
22 checks passed
Copy link

blathers-crl bot commented Apr 12, 2024

Encountered an error creating backports. Some common things that can go wrong:

  1. The backport branch might have already existed.
  2. There was a merge conflict.
  3. The backport branch contained merge commits.

You might need to create your backport manually using the backport tool.


error setting reviewers, but backport branch blathers/backport-release-24.1-122001 is ready: POST https://api.github.com/repos/cockroachdb/cockroach/pulls/122290/requested_reviewers: 422 Reviews may only be requested from collaborators. One or more of the teams you specified is not a collaborator of the cockroachdb/cockroach repository. []

Backport to branch 24.1.x failed. See errors above.


🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-24.1.x Flags PRs that need to be backported to 24.1.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants