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

Point in time recovery (part 1) #5160

Merged
merged 25 commits into from
Oct 7, 2019
Merged

Conversation

deepthi
Copy link
Member

@deepthi deepthi commented Sep 4, 2019

This PR replaces #4857

Partial fix for #4886

  • CreateKeyspace now takes three additional (optional) parameters
    • keyspace_type: NORMAL/SNAPSHOT
    • base_keyspace: keyspace that you are trying to recover
    • snapshot_time: for SNAPSHOT keyspaces, the time for which it is being recovered
  • when creating a keyspace, we copy the vschema from base_keyspace, and set require_explicit_routing to true
  • Vttablet recovery mode
    • init_keyspace: this should be a SNAPSHOT keyspace
    • init_shard: if this shard has already been deleted, it will be created
    • init_db_name_override: if this has been set on the base_keyspace's vttablets, it also needs to be set (to the same value) on the recovery keyspace's tablets. If it is not set it will be defaulted to vt_ + base_keyspace
    • snapshot_time will be used as a starting point to find a backup to restore (most recent backup from snapshot_time)
    • replication should not be setup when restoring a snapshot keyspace (all tablets should be run with -disable_active_reparents=true and -enable_replication_reporter=false)
  • Vtgate changes
    • tables and vindexes of SNAPSHOT keyspaces are not added to the uniqueTables and uniqueVindexes maps when building vschema. This is sufficient to hide them from the global routing.
  • It is not necessary to bring up a master tablet for PITR. All tablets can be replicas.
  • If a master is desired for any reason, it should be initialized using TabletExternallyReparented, not InitShardMaster or PlannedReparentShard.
  • How to access recovered data
    • use @replica followed by select ... from ks.table
    • use ks@replica or use ks:shard@replica followed by select .. from table
  • added unit tests for CreateKeyspace
  • integration tests to test recovery of vttablets + vtgate routing
    • base test with 1 active recovery keyspace
    • test with multiple active recovery keyspaces
    • test across resharding (recover keyspace from before a horizontal resharding event)
    • test recovery of a sharded keyspace
    • all tests test query routing by vtgate in addition to vttablet SERVING

Signed-off-by: deepthi [email protected]

@deepthi deepthi requested a review from sougou as a code owner September 4, 2019 21:37
@deepthi deepthi mentioned this pull request Sep 4, 2019
@deepthi deepthi requested review from enisoc and rafael September 4, 2019 21:39
@deepthi
Copy link
Member Author

deepthi commented Sep 5, 2019

@enisoc from our code walkthrough, I have these notes:

  • make sure initDbNameOverride is only computed if it is not already set
  • testcase that checks that if there's more than 1 backup the right one is selected
  • in basic testcase update data on master and check that there are different values returned by vtgate default routing vs recovery_keyspace routing
  • recovery on top of a sharded keyspace
  • what happens in sharded recovery if the original shard has been deleted? test manually integration test now covers this
  • maybe push down the decision of what to name the backup / using current time

The last one should be a separate cleanup PR rather than being mixed into this one.

All cases covered by integration tests have been tested manually with a specified snapshot_time (tests use current time for repeatability).

@deepthi deepthi force-pushed the ds-pitr2 branch 2 times, most recently from d679729 to d94e790 Compare September 9, 2019 16:56
Copy link
Contributor

@sougou sougou left a comment

Choose a reason for hiding this comment

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

Reviewed to topo and vschema side of things. Those part look good.
There's a merge conflict to be resolved.

@deepthi
Copy link
Member Author

deepthi commented Sep 12, 2019

I have rebased the branch to resolve the merge conflict.

Copy link
Member

@enisoc enisoc left a comment

Choose a reason for hiding this comment

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

I did a first pass and got through everything except the e2e tests. I'll look at those in the next pass.

go/vt/mysqlctl/backup.go Outdated Show resolved Hide resolved
go/vt/mysqlctl/backup.go Outdated Show resolved Hide resolved
go/vt/mysqlctl/backup.go Outdated Show resolved Hide resolved
go/vt/mysqlctl/backupengine.go Outdated Show resolved Hide resolved
go/vt/mysqlctl/backupengine.go Outdated Show resolved Hide resolved
go/vt/vttablet/tabletmanager/restore.go Outdated Show resolved Hide resolved
go/vt/vttablet/tabletmanager/restore.go Outdated Show resolved Hide resolved
go/vt/vttablet/tabletmanager/restore.go Show resolved Hide resolved
proto/topodata.proto Outdated Show resolved Hide resolved
proto/vschema.proto Outdated Show resolved Hide resolved
@deepthi
Copy link
Member Author

deepthi commented Sep 22, 2019

I have either responded to or resolved all the review comments. In addition, I have cleaned up the params handling and pushed down the backup dir & name computation down into mysqlctl.Backup.

Copy link

@setassociative setassociative left a comment

Choose a reason for hiding this comment

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

I lack some context here that I'd ideally have but on an initial read this feels good. I expect there will be non-trivial changes if you pursue moving the snapshot time out of the keyspace proto and into the local metadata table so will make another pass if that lands.

I did have a couple of broad questions that didn't make sense to attach to specific lines of code:

If a master is desired for any reason, it should be initialized using TabletExternallyReparented, not InitShardMaster or PlannedReparentShard.

What is the failure mode of using InitShardMaster or PlannedReparentShard?

And:

If a tablet points at a keyspace of type SNAPSHOT should we default to -disable_active_reparents and -enable_replication_reporter=false instead of requiring them to be passed as flags? It seems like that having them independently configurable doesn't buy us much

go/vt/mysqlctl/backup.go Outdated Show resolved Hide resolved
go/vt/mysqlctl/backup.go Outdated Show resolved Hide resolved
go/vt/mysqlctl/backup.go Outdated Show resolved Hide resolved
go/vt/mysqlctl/backupengine.go Show resolved Hide resolved
go/vt/mysqlctl/backupengine.go Outdated Show resolved Hide resolved
go/vt/mysqlctl/backupengine.go Outdated Show resolved Hide resolved
go/vt/mysqlctl/backupengine.go Outdated Show resolved Hide resolved
go/vt/vtctl/vtctl.go Show resolved Hide resolved
go/vt/vtctl/vtctl.go Outdated Show resolved Hide resolved
go/cmd/vtbackup/vtbackup.go Show resolved Hide resolved
@deepthi
Copy link
Member Author

deepthi commented Sep 29, 2019

If a master is desired for any reason, it should be initialized using TabletExternallyReparented, not InitShardMaster or PlannedReparentShard.

What is the failure mode of using InitShardMaster or PlannedReparentShard?

InitShardMaster should only be used when there is no data, because it forces replica positions to match the master. It might work in this case since all replicas in a shard are expected to be at the same position, but is not recommended in general.
PlannedReparentShard cannot be run on a shard that currently has no master.

If a tablet points at a keyspace of type SNAPSHOT should we default to -disable_active_reparents and -enable_replication_reporter=false instead of requiring them to be passed as flags? It seems like that having them independently configurable doesn't buy us much

We had thought about turning off this behavior internally and that seemed to add unnecessary complexity to the code, but setting the flag values is a cleaner approach that we could add.

- if there's more than 1 backup the correct one should be chosen
- update data on base keyspace after backup and ensure recovery keyspace does
not see the change
- recover a sharded keyspace

Signed-off-by: deepthi <[email protected]>
…up, fix bug in FindBackup for snapshot, misc review changes

Signed-off-by: deepthi <[email protected]>
…cord, more params cleanup and refactoring from reviews

Signed-off-by: deepthi <[email protected]>
Copy link

@setassociative setassociative left a comment

Choose a reason for hiding this comment

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

two tiny nits

go/vt/vttablet/tabletmanager/restore.go Outdated Show resolved Hide resolved
go/vt/mysqlctl/backupengine.go Outdated Show resolved Hide resolved
@deepthi deepthi changed the title Point in time recovery Point in time recovery (part 1) Oct 7, 2019
Copy link
Member

@enisoc enisoc left a comment

Choose a reason for hiding this comment

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

LGTM

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.

4 participants