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

backupccl: support RESTORE inside tenant #58908

Merged
merged 2 commits into from
Feb 16, 2021

Conversation

pbardea
Copy link
Contributor

@pbardea pbardea commented Jan 13, 2021

This commit adds support for RESTORE inside tenants. A RESTORE performed
by a tenant will produce a DistSQL plan that is planned on a single
node.

Since we don't make note during the backup if we're backing up inside a tenant
or not, we check for every key that we rekey if we're in a new tenant. This is a bit
wasteful -- it would be nice if we could make this determination when we create
the rekeyer.

I suppose that if we can make the assumption that all keys will lie in the same
tenant-space, we can do some checks on the first key and save our learnings
to avoid repetitive work on every key.

Closes #47970.

Release note: None

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@pbardea pbardea force-pushed the restore-tenant branch 3 times, most recently from 322598b to a0fe7b4 Compare January 13, 2021 06:13
@pbardea pbardea marked this pull request as ready for review January 13, 2021 06:18
@pbardea pbardea requested review from a team and dt and removed request for a team January 13, 2021 06:18
@pbardea pbardea force-pushed the restore-tenant branch 2 times, most recently from 60d8264 to 8b87c88 Compare January 13, 2021 16:18
@pbardea pbardea force-pushed the restore-tenant branch 5 times, most recently from f97b068 to b3d2cfb Compare February 3, 2021 20:27
backupCodec := keys.SystemSQLCodec
if len(sqlDescs) != 0 {
if len(latestBackupManifest.Spans) != 0 {
_, tenantID, err := keys.DecodeTenantPrefix(latestBackupManifest.Spans[0].Key)
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we should confirm that all spans, or at least the last span, has the same tenant ID? just thinking to be defensive about backups that contain spans from multiple tenants, where it is unclear that the first is the right one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. Although I think that a backup that has no tenant targets would ensure that the entire keyspace covered by manifest.Spans belongs to 1 tenant?

@pbardea pbardea force-pushed the restore-tenant branch 3 times, most recently from 7a9af33 to aabf4aa Compare February 3, 2021 22:34
This commit adds the ability for the key rewriter to rekey keys from one
tenant (including the system tenant), into another.

Release note: None
This commit adds support for RESTORE inside tenants. A RESTORE performed
by a tenant will produce a DistSQL plan that is planned on a single
node.

Release note: None
@pbardea
Copy link
Contributor Author

pbardea commented Feb 4, 2021

At this point, this PR supports restoring backups taken from tenants into other tenants (but not the system tenant). In order to support restoring backups taken in tenant into the system tenant, we'll need to pass in more information into the rekeyer to tell it if we want to rekey the tenant or not.

I think that what is currently supported addresses enough of a use-case to be merged on its own, and can address rekeying to the system tenant in a separate PR.

All to say, this should be RFAL.

@pbardea pbardea requested a review from dt February 4, 2021 13:42
@pbardea pbardea closed this Feb 4, 2021
@pbardea pbardea reopened this Feb 4, 2021
@pbardea
Copy link
Contributor Author

pbardea commented Feb 16, 2021

Friendly ping

@pbardea
Copy link
Contributor Author

pbardea commented Feb 16, 2021

TFTR!
bors r+

@craig
Copy link
Contributor

craig bot commented Feb 16, 2021

Build succeeded:

@craig craig bot merged commit 57220aa into cockroachdb:master Feb 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bulkio: BACKUP/RESTORE/IMPORT for use by tenants
3 participants