FDB: Disk based snapshot and restore - Version 2 #1733

senthil-ram · 2019-06-20T17:14:13Z

Currently, we have a disk based snapshot and restore mechanism implemented in FDB6, but, there are few restrictions with the current implementation, namely:

Works only in environments where the disk snapshot request completes in < 5 seconds.
Status of snapshot request needs to be verified externally by parsing through the trace events.

Requirements:

Ability for FDB snapshot command to work on systems where the disk snapshot command may take few 10s of seconds or up to a minute.
Ability to determine the success or failure of the snapshot request and communicate it to the user.

Non Requirements:

Ability to support multi-region snapshot and restore

more details here:
https://docs.google.com/document/d/1Udgrpqu2D4DNEv1_2id9E7CQVDkisaTtK31FybVgNYQ/edit?usp=sharing

mpilman

I am not quite through with my review yet. As a general comment: please check your excessive use of state variables. I didn't check all of them and I think a lot of them are unnecessary in the old code. This is bad style, makes your code slower, uses more memory, and makes understanding the code much harder (as state variables are not scoped).

I also added a few other comments

fdbclient/NativeAPI.actor.cpp

fdbserver/DataDistribution.actor.cpp

fdbserver/FDBExecHelper.actor.cpp

fdbclient/MasterProxyInterface.h

fdbclient/NativeAPI.actor.cpp

fdbserver/DataDistribution.actor.cpp

fdbserver/MasterProxyServer.actor.cpp

fdbserver/OldTLogServer_6_0.actor.cpp

fdbserver/worker.actor.cpp

tests/restarting/from_6.2.0/SnapCycleRestart-2.txt

alexmiller-apple

I've run out of time about half way through.

@mpilman, could you please do a closer read through and review of this? I'm still going to be deeply lacking time while my own 6.2 work is unfinished.

fdbserver/DataDistribution.actor.cpp

fdbserver/FDBExecHelper.actor.cpp

fdbclient/NativeAPI.actor.cpp

fdbclient/ManagementAPI.actor.cpp

alexmiller-apple · 2019-07-18T20:21:53Z

fdbclient/ManagementAPI.actor.h

 // execute payload in 'snapCmd' on all the coordinators, TLogs and
 // storage nodes
-ACTOR Future<Void> mgmtSnapCreate(Database cx, StringRef snapCmd);
+ACTOR Future<Void> mgmtSnapCreate(Database cx, StringRef snapCmd, int version);


For future reference, as this is about to be removed anyway, int version is a great opportunity to use an enum class instead of an int.

fdbserver/Knobs.cpp

fdbserver/QuietDatabase.actor.cpp

fdbserver/SimulatedCluster.actor.cpp

fdbserver/TLogInterface.h

fdbserver/DataDistribution.actor.cpp

fdbserver/QuietDatabase.actor.cpp

fdbserver/SimulatedCluster.actor.cpp

fdbserver/TLogInterface.h

fdbserver/WorkerInterface.actor.h

flow/genericactors.actor.h

senthil-ram · 2019-07-22T19:14:46Z

Following are the items that we know of that will be addressed in 6.2 or later:

disable DD now uses a persistent state and an operator has to unset it if the CLI fails in-between the snap request. This will be addressed in a subsequent PR in 6.2 time-frame to keep the disable DD as a in-memory state.
To detect if there was a recovery - currently code checks the client DB id and monitor the server DB info on the DataDistributor. But, Markus and I were thinking that it is possible that there is a possibility of a recovery that goes un-noticed and ending up with a bad coordinated state. One improvement there is to comparing the read generation id of the coordinated state before and after snapshots.
Markus pointed out that coordinators need not be workers and for that case we need to find a different mechanism to snapshot the coordinators (for eg: write the coordinated state into tlog/storage snapshots and re-populated it after restore). This will be mostly addressed beyond 6.2.

senthil-ram · 2019-07-22T21:54:21Z

This PR is dependent on a bug fix in setDDMode which is in this PR: #1866

alexmiller-apple · 2019-07-23T07:00:58Z

I'm not sure why your CI got stuck. Let's see if this works in one line...

@fdb-build, run bindings please, run cmake linux please, run documentation please, run linux please.

alexmiller-apple

I did a quick skim while building, and this seems fine to merge.

I'm kicking off some extra correctness:
all tests = 20190723-004450-alexmiller-4034900cf15fc655
snap only = 20190723-004752-alexmiller-4d476ce0d0cc6fc7

fdbserver/DataDistribution.actor.cpp

Co-Authored-By: Alex Miller <[email protected]>

senthil-ram · 2019-07-23T15:26:47Z

I'm not sure why your CI got stuck. Let's see if this works in one line...

@fdb-build, run bindings please, run cmake linux please, run documentation please, run linux please.

It compiled and tested just fine in our environments. With the commit that removed the commented line of code - it cleared CI. Sounds like CI getting struck for unknown issue.

senthil-ram · 2019-07-23T16:40:25Z

I did a quick skim while building, and this seems fine to merge.

I'm kicking off some extra correctness:
all tests = 20190723-004450-alexmiller-4034900cf15fc655
snap only = 20190723-004752-alexmiller-4d476ce0d0cc6fc7

Few snap tests will fail, because the dependency fix #1866 is not yet merged.

senthil-ram · 2019-07-25T02:34:47Z

Fixed the issue #1675

senthil-ram force-pushed the snapv2 branch 2 times, most recently from 918aac0 to 00e2a30 Compare June 20, 2019 20:53

alexmiller-apple self-assigned this Jun 21, 2019

alexmiller-apple requested review from alexmiller-apple and mpilman June 21, 2019 00:57

mpilman suggested changes Jun 27, 2019

View reviewed changes

senthil-ram force-pushed the snapv2 branch from 00e2a30 to f50da3f Compare July 15, 2019 20:50

mpilman suggested changes Jul 16, 2019

View reviewed changes

mpilman reviewed Jul 16, 2019

View reviewed changes

tests/restarting/from_6.2.0/SnapCycleRestart-2.txt Show resolved Hide resolved

senthil-ram mentioned this pull request Jul 19, 2019

setDDMode should set moveKeysLockWriteKey #1866

Merged

alexmiller-apple reviewed Jul 19, 2019

View reviewed changes

mpilman suggested changes Jul 21, 2019

View reviewed changes

senthil-ram force-pushed the snapv2 branch from f50da3f to 8eea3e1 Compare July 22, 2019 18:53

mpilman approved these changes Jul 23, 2019

View reviewed changes

sramamoorthy added 14 commits July 22, 2019 18:09

snap v2: fdbclient related changes

acd43e7

snap v2: master proxy related changes

89ae2d1

snap v2: DD changes - snapshot orchestration logic

a681568

snap v2: TLog related changes

ea6fbe7

snap v2: worker and other helper related changes

6f1e14a

snap v2: knobs for max snap create timeout

408f632

snap v2: restore related changes for the simulator

cc35084

snap v2: test files changes

d5073e5

snap v2: implement get only local storage workers

f48bfb4

timeoutError that takes Future<Error<T>> as input

4b79d54

tryGetReply instead of getReply for ddSnapReq

f05b401

storage worker to throw in case of failures

bf3c44b

enable/disable pop req with UID mis-match to fail

da4ef7c

includeCancelled for workerSnapCreate

40cc51d

sramamoorthy added 17 commits July 22, 2019 18:14

includeCancelled for tLogSnapCreate

0eb5432

do not pop txsTag

466f384

disable dd during snap and enable in restore

76aa6c0

adjut versions to handle KCV > recoveryVersion

4599a44

timeout snapshotdatabase call

5f2d18c

Snap*Restart in ver 2 and buggify off in restore

4d29fb9

few minor fixes

6f993cf

snap create timeout for client call

234457e

enable DD mode in restore based on test spec

ceac993

bug fix in timeout & header file re-arrange in DD

2b09f06

make snap v2 the default version

738c6a5

Few cosmetic edits and fixes

438a5a2

increase snaptime out to 15s for simulator

f4bc463

snapshot only local tlogs and not the satellite

6dc3c42

code review comments and merge to master changes

e5f9b09

get rid of all timeouts and other changes

5e45897

remove un-necessary state variables in getCoord

5f43289

senthil-ram force-pushed the snapv2 branch from 1b20993 to 5f43289 Compare July 23, 2019 02:01

alexmiller-apple reviewed Jul 23, 2019

View reviewed changes

fdbserver/DataDistribution.actor.cpp Outdated Show resolved Hide resolved

Update fdbserver/DataDistribution.actor.cpp

78139fc

Co-Authored-By: Alex Miller <[email protected]>

senthil-ram mentioned this pull request Jul 23, 2019

Remove Snap V1 code #1885

Merged

alexmiller-apple merged commit edeec8a into apple:master Jul 24, 2019

FDB: Disk based snapshot and restore - Version 2 #1733

FDB: Disk based snapshot and restore - Version 2 #1733

Uh oh!

Conversation

senthil-ram commented Jun 20, 2019

Uh oh!

mpilman left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

alexmiller-apple left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

alexmiller-apple Jul 18, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

senthil-ram commented Jul 22, 2019

Uh oh!

senthil-ram commented Jul 22, 2019

Uh oh!

alexmiller-apple commented Jul 23, 2019

Uh oh!

alexmiller-apple left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

senthil-ram commented Jul 23, 2019

Uh oh!

senthil-ram commented Jul 23, 2019

Uh oh!

senthil-ram commented Jul 25, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants