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

SHOW BACKUP: Indicate if backup is multi region and how many regions. #79681

Closed
carlsongould opened this issue Apr 8, 2022 · 11 comments · Fixed by #88136
Closed

SHOW BACKUP: Indicate if backup is multi region and how many regions. #79681

carlsongould opened this issue Apr 8, 2022 · 11 comments · Fixed by #88136
Assignees
Labels
A-disaster-recovery C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) E-quick-win Likely to be a quick win for someone experienced. E-starter Might be suitable for a starter project for new employees or team members. good first issue T-disaster-recovery

Comments

@carlsongould
Copy link

carlsongould commented Apr 8, 2022

Summary

Working with a partner that has test data management as part of their portfolio. They were looking to identify if there a way to know if a previously taken backup is multi region?

For example:
Cluster A is multi-region and a cluster backup is created and writes to gcp cloud storage.
We restore to cluster B . Prior to doing so we want to validate if the backup is multi region or not AND to identify how many regions.

Currently SHOW BACKUP provides the following output fields:

  • database_name
  • parent_schema_name
  • object_name
  • object_type
  • backup_type
  • start_time
  • end_time
  • size_bytes
  • create_statement
  • is_full_cluster
  • path

Request

Can we add 2 columns to the output of SHOW BACKUP:
Column: is_multi_region Description: Indicate true/false if a cluster backup is multi region.
Column: has_regions Description: Indicate the number of regions for.a cluster backup.

Other methods:
Indicate the if a backup is multi region and number of regions in the path.
Example: //-/multi:non-multi/

Jira issue: CRDB-14976

@carlsongould carlsongould added the C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) label Apr 8, 2022
@blathers-crl
Copy link

blathers-crl bot commented May 25, 2022

cc @cockroachdb/bulk-io

@cockroachdb cockroachdb deleted a comment from mari-crl May 25, 2022
@livlobo livlobo added the E-quick-win Likely to be a quick win for someone experienced. label Jun 7, 2022
@livlobo livlobo added E-starter Might be suitable for a starter project for new employees or team members. good first issue labels Jun 28, 2022
@saichatla
Copy link

Can I try my hand at this?

@msbutler
Copy link
Collaborator

msbutler commented Jul 6, 2022

@saichatla thanks for volunteering to take this on! This is pretty straightforward to implement for database regions, but not for cluster regions; see multiregion docs for reference. The bulk team (responsible for backup), will chat about this issue in the next week or so and will provide further direction on next steps. Please hold tight for now!

@saichatla
Copy link

saichatla commented Jul 6, 2022

@msbutler Thank you! I will be waiting for your response.

@msbutler
Copy link
Collaborator

msbutler commented Jul 13, 2022

@saichatla Alright, we think this would be a good issue for you to pick up. It should only a day or 2 of work. Do you have time to work on this in the next two weeks? We'd like to get this in to our next release, which gets finalized in August. Let me know!

Here's the idea:

  • We want to add an additional column, called Regions, to the SHOW BACKUP return table which displays the ALTER DATABASE cmds that would create the multi region specs for that backed up database. For rows in the SHOW BACKUP return table that describe other objects (e.g. schema, table etc), this new column's value should be null. This column should appear if and only if a database in the backup is multiregion. We are not concerned about cluster level regions in this PR.
  • Here's an example of what the new column's value would be for a 3 region database:
ALTER DATABASE movr_4 PRIMARY REGION "aws-us-east-1"; ALTER DATABASE movr_4 ADD REGION "aws-eu-west-2"; ALTER DATABASE movr_4 ADD REGION "aws-eu-west-3";
  • In terms of implementation:
    • All of your work should occur in the pkg/ccl/backupccl/show.go
    • Check our restore_job for info on how to reason about multigiion info stored in database descriptors in the backup manifest (the backup metadata file-- the only thing used in SHOW BACKUP)

typeDesc := typedesc.NewBuilder(t.TypeDesc()).BuildImmutableType()
if typeDesc.GetKind() == descpb.TypeDescriptor_MULTIREGION_ENUM {
// Check to see if we've found more than one multi-region enum on any
// given database.
if id, ok := mrEnumsFound[typeDesc.GetParentID()]; ok {
return errors.AssertionFailedf(
"unexpectedly found more than one MULTIREGION_ENUM (IDs = %d, %d) "+
"on database %d during restore", id, typeDesc.GetID(), typeDesc.GetParentID())
}
mrEnumsFound[typeDesc.GetParentID()] = typeDesc.GetID()
if db, ok := dbsByID[typeDesc.GetParentID()]; ok {
desc := db.DatabaseDesc()
if desc.RegionConfig == nil {
return errors.AssertionFailedf(
"found MULTIREGION_ENUM on non-multi-region database %s", desc.Name)
}
// Update the RegionEnumID to record the new multi-region enum ID.
desc.RegionConfig.RegionEnumID = t.GetID()
// If we're not in a cluster restore, rebuild the database-level zone
// configuration.
if details.DescriptorCoverage != tree.AllDescriptors {

  • Check out for how we construct the sql cmds returned when the user runs SHOW BACKUP ... with privileges

func showPrivileges(descriptor *descpb.Descriptor) string {
var privStringBuilder strings.Builder
b := descbuilder.NewBuilder(descriptor)
if b == nil {
return ""
}
var objectType privilege.ObjectType
switch b.DescriptorType() {
case catalog.Database:
objectType = privilege.Database
case catalog.Table:
objectType = privilege.Table
case catalog.Type:
objectType = privilege.Type
case catalog.Schema:
objectType = privilege.Schema
default:
return ""
}
privDesc := b.BuildImmutable().GetPrivileges()
if privDesc == nil {
return ""
}
for _, userPriv := range privDesc.Show(objectType) {
privs := userPriv.Privileges
if len(privs) == 0 {
continue
}
privStringBuilder.WriteString("GRANT ")
for j, priv := range privs {
if j != 0 {
privStringBuilder.WriteString(", ")
}
privStringBuilder.WriteString(priv.Kind.String())
}
privStringBuilder.WriteString(" ON ")
privStringBuilder.WriteString(strings.ToUpper(string(objectType)) + " ")
privStringBuilder.WriteString(descpb.GetDescriptorName(descriptor))
privStringBuilder.WriteString(" TO ")
privStringBuilder.WriteString(userPriv.User.SQLIdentifier())
privStringBuilder.WriteString("; ")
}
return privStringBuilder.String()

  • when the feature is ready for testing, I'd create a new test data file in ccl/backupccl/testdata/show/multiregion. You can run the data driven test by running ./dev test pkg/ccl/backupccl --filter TestDataDriven

Happy coding!

@saichatla
Copy link

@msbutler Yep! I will get it done ASAP!

@msbutler
Copy link
Collaborator

msbutler commented Aug 5, 2022

@saichatla how's this going? We may need to pick this up to slip it into our next release unless you have a PR ready.

@msbutler
Copy link
Collaborator

msbutler commented Aug 5, 2022

Sorry to hear about the personal stuff. Again, we're happy to take over from here, if you don't have capacity. Please just let us know!

Within the pkg/ccl/backupccl/testdata/ directory, I think you should create a show subdirectory with a file called multiregion.

@saichatla
Copy link

Sorry to hear about the personal stuff. Again, we're happy to take over from here, if you don't have capacity. Please just let us know!

Within the pkg/ccl/backupccl/testdata/ directory, I think you should create a show subdirectory with a file called multiregion.

If you want to take over that is fine, since this is on a time crunch. @msbutler I did not expect to be dealing with some of the things I have been dealing with.

@msbutler
Copy link
Collaborator

msbutler commented Aug 5, 2022

Cool, I'll think we'll take over from here!

@msbutler
Copy link
Collaborator

hey @saichatla. After speaking again with the team, we've decided this issue is no longer high priority and under a time crunch. Please let me know if you'd like to give it a try, and I'll reassign you!

Sorry for all the admin back and forth.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-disaster-recovery C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) E-quick-win Likely to be a quick win for someone experienced. E-starter Might be suitable for a starter project for new employees or team members. good first issue T-disaster-recovery
Projects
No open projects
Archived in project
Development

Successfully merging a pull request may close this issue.

6 participants