feat: backup and restore sqlite database #21584

williamhbaker · 2021-06-01T20:49:15Z

Closes #21568
Partially closes #21567

Adds the following endpoints to the API:

api/v2/restore/sql: This endpoint will receive a new sqlite database to replace the existing sqlite database, intended to act as a mechanism for restoring backed up data.
api/v2/backup/metadata (partial): Will return a multipart/form-data response containing the bolt and sqlite database files. There is also a placeholder in the response to include a manifest describing the buckets & shards in the bolt metadata.

Future work will include updating the openapi specification to include these new endpoints, and completing the implementation for the manifest response at api/v2/backup/metadata. Once that is complete, the existing api/v2/backup/kv endpoint will be updated to return an error response and direct the client to the api/v2/backup/metadata endpoint.

danxmoran

Awesome progress! Some initial thoughts

danxmoran · 2021-06-02T12:52:01Z

http/backup_service.go

+	baseName := time.Now().UTC().Format(influxdb.BackupFilenamePattern)
+
+	formWriter := multipart.NewWriter(w)
+	w.Header().Set("Content-Type", formWriter.FormDataContentType())


Could we use multipart/mixed instead, to match the API doc? All the descriptions I can find of multipart/form-data make it sounds like it's intended for uploads (i.e. this).

Yep! That makes more sense. Just have to be a little more explicit since the multipart package seems to really be set up for multipart/form-data by default. I updated the content type to specifically be multipart/mixed and everything still seems to work alright.

danxmoran · 2021-06-02T12:53:26Z

http/backup_service.go

+		writeFn   func(io.Writer) error
+	}{
+		{
+			"bolt",


The API doc currently has this as boltdb, so we'll need to match it for codegen.

Higher-level Q: I'm wondering if we should use kv and sql as the part names instead of bolt and sqlite, to match the API endpoints & pseudo-abstract over the specific DB formats used. What do you think?

I don't have a strong feeling, but having the parts named kv and sql seems like an improvement. I went ahead and updated the part names to kv and sql...it might make things a little more clear when parsing the response based on the endpoints, like you said.

👍 I've got a WIP branch to finish the openapi spec for the manifest & the restore endpoints, I'll update the part names as part of it.

danxmoran · 2021-06-02T12:54:43Z

http/backup_service.go

+			"sqlite",
+			fmt.Sprintf("%s.sqlite", baseName),
+			func(fw io.Writer) error {
+				return h.SqlBackupRestoreService.BackupSqlStore(ctx, fw)


Is there any way we could take a lock across both KV and SQL before running these commands? If not there's still a window where inconsistencies could creep in.

I added locking mechanisms. It required a fair amount of modifications, but seems to work.

williamhbaker · 2021-06-02T16:44:26Z

storage/engine.go

@@ -318,13 +320,22 @@ func (e *Engine) DeleteBucketRangePredicate(ctx context.Context, orgID, bucketID
 	return e.tsdbStore.DeleteSeriesWithPredicate(bucketID.String(), min, max, pred)
 }

+// LockKVStore locks the KV store as well as the engine in preparation for doing a backup.
+func (e *Engine) LockKVStore() {
+	e.mu.Lock()


Locking the engine here might be overkill, but it was easy enough to do and might be prevent some edge issues

williamhbaker · 2021-06-02T17:04:43Z

http/backup_service.go

+			},
+		},
+		{
+			"buckets",


Update this part to be named "buckets". It will have a file name with a .json extension like 20210602T171102Z.json

danxmoran · 2021-06-02T19:53:56Z

sqlite/sqlite.go

+	}
+	defer os.RemoveAll(tempDir)
+
+	destPath := tempDir + "/" + DefaultFilename


I'm not sure if this will cause problems on Windows, could we use filepath.Join instead?

Good catch, made the change!

danxmoran · 2021-06-02T19:54:57Z

sqlite/sqlite.go

+	}
+
+	// perform the backup
+	_, err = bk.Step(-1)


Step(-1) means "backup everything"?

Yep, there's a pretty good explanation here: https://www.sqlite.org/backup.html

It's possible to use specific numbers instead of -1 if you want be able to pause the backup and give other writes a chance to work in while you are doing the backup, which we definitely don't want to do, so -1 just locks the db for writes and does the entire backup in one shot.

danxmoran · 2021-06-02T19:55:53Z

sqlite/sqlite.go

+	}
+	defer os.RemoveAll(tempDir)
+
+	tempFileName := fmt.Sprintf("%s/%s", tempDir, DefaultFilename)


Same here, could we filepath.Join?

Made this change as well. Also found it funny how I did the exact same thing two different ways in the same file 😆 - now it's consistent if nothing else.

danxmoran · 2021-06-02T19:58:43Z

http/backup_service.go

+	h.BackupService.LockKVStore()
+	defer h.BackupService.UnlockKVStore()
+
+	h.SqlBackupRestoreService.LockSqlStore()


Is there a chance we could end up w/ a deadlock here where the KV store is locked & waiting on sql, and some other process has sql locked & is waiting on KV?

I don't think so - at least not right now. The only other thing that uses the sql database is the notebooks service. Notebooks will get the write lock when modifying a notebook. The only way the modification of a notebook should interact with the KV is reading data from it during the request to get permissions info. This requires getting a read lock, and the KV store is locked here with a read lock as well. There currently isn't anything you can do with the sql database that would require a write to the KV. Except for backups, I guess!

So I can't think of a scenario where there would be a deadlock, really due to the fact that the two databases are almost completely decoupled. At least for notebooks...and annotations should work the same way. If we start doing things which do writes to both databases simultaneously I could see it being a problem. Then there could be a deadlock if the external request starts right between these two locks. So...hopefully we don't ever do that 😸.

williamhbaker added 3 commits June 1, 2021 15:07

feat: new metadata backup endpoint

0a691dd

chore: renamed sqlbackup to sqlbackuprestore

2ec0d6f

feat: added restore/sql API endpoint

f5771f9

williamhbaker force-pushed the wb-metadata-backup-21567 branch from 8f9308c to f5771f9 Compare June 2, 2021 01:13

danxmoran reviewed Jun 2, 2021

View reviewed changes

williamhbaker marked this pull request as ready for review June 2, 2021 13:28

williamhbaker added 3 commits June 2, 2021 09:43

fix: content-type is multipart/mixed, part names are kv and sql

e336849

fix: changed multipart manifest to buckets and made it .json

bd7a363

feat: added lock for backing up sqlite and bolt dbs

5038b98

williamhbaker commented Jun 2, 2021

View reviewed changes

williamhbaker added 2 commits June 2, 2021 15:38

fix: use read lock instead of write lock on kv during backup

b5def3f

chore: updated comments

a9409a3

danxmoran reviewed Jun 2, 2021

View reviewed changes

fix: use filepath.Join for temp dirs

64cff32

danxmoran approved these changes Jun 2, 2021

View reviewed changes

Merge branch 'master' into wb-metadata-backup-21567

78ced52

williamhbaker merged commit b0ea3b6 into master Jun 2, 2021

williamhbaker deleted the wb-metadata-backup-21567 branch June 2, 2021 23:07

williamhbaker mentioned this pull request Jun 3, 2021

feat: metadata backup returns bucket manifests #21601

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: backup and restore sqlite database #21584

feat: backup and restore sqlite database #21584

williamhbaker commented Jun 1, 2021 •

edited

Loading

danxmoran left a comment

danxmoran Jun 2, 2021

williamhbaker Jun 2, 2021

danxmoran Jun 2, 2021

williamhbaker Jun 2, 2021

danxmoran Jun 2, 2021

danxmoran Jun 2, 2021

williamhbaker Jun 2, 2021

williamhbaker Jun 2, 2021

williamhbaker Jun 2, 2021 •

edited

Loading

danxmoran Jun 2, 2021

williamhbaker Jun 2, 2021

danxmoran Jun 2, 2021

williamhbaker Jun 2, 2021

danxmoran Jun 2, 2021

williamhbaker Jun 2, 2021

danxmoran Jun 2, 2021

williamhbaker Jun 2, 2021

feat: backup and restore sqlite database #21584

feat: backup and restore sqlite database #21584

Conversation

williamhbaker commented Jun 1, 2021 • edited Loading

danxmoran left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

williamhbaker Jun 2, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

williamhbaker commented Jun 1, 2021 •

edited

Loading

williamhbaker Jun 2, 2021 •

edited

Loading