From 950ded454d2a6101090097f4e3bb60c722875497 Mon Sep 17 00:00:00 2001 From: vyzo Date: Wed, 28 Jul 2021 11:49:42 +0300 Subject: [PATCH 1/7] code cosmetics: rename variables for better readability and some comments --- blockstore/badger/blockstore.go | 69 +++++++++++++++++++-------------- 1 file changed, 39 insertions(+), 30 deletions(-) diff --git a/blockstore/badger/blockstore.go b/blockstore/badger/blockstore.go index 8e1a3a1ff8b..49951db6ed1 100644 --- a/blockstore/badger/blockstore.go +++ b/blockstore/badger/blockstore.go @@ -258,16 +258,16 @@ func (b *Blockstore) movingGC() error { b.moveCond.Broadcast() b.moveMx.Unlock() - var path string + var newPath string defer func() { b.lockMove() - db2 := b.dbNext + dbNext := b.dbNext b.dbNext = nil var state bsMoveState - if db2 != nil { + if dbNext != nil { state = moveStateCleanup } else { state = moveStateNone @@ -275,12 +275,13 @@ func (b *Blockstore) movingGC() error { b.unlockMove(state) - if db2 != nil { - err := db2.Close() + if dbNext != nil { + // the move failed and we have a left-over db; delete it. + err := dbNext.Close() if err != nil { log.Warnf("error closing badger db: %s", err) } - b.deleteDB(path) + b.deleteDB(newPath) b.lockMove() b.unlockMove(moveStateNone) @@ -296,63 +297,71 @@ func (b *Blockstore) movingGC() error { } if basePath == linkPath { - path = basePath + newPath = basePath } else { + // we do this dance to create a name adjacent to the current one, while avoiding clown + // shoes with multiple moves (i.e. we can't just take the basename of the linkPath, as it + // could have been created in a previous move and have the timestamp suffix, which would then + // perpetuate itself. name := filepath.Base(basePath) dir := filepath.Dir(linkPath) - path = filepath.Join(dir, name) + newPath = filepath.Join(dir, name) } - path = fmt.Sprintf("%s.%d", path, time.Now().UnixNano()) + newPath = fmt.Sprintf("%s.%d", newPath, time.Now().UnixNano()) - log.Infof("moving blockstore from %s to %s", b.opts.Dir, path) + log.Infof("moving blockstore from %s to %s", b.opts.Dir, newPath) opts := b.opts - opts.Dir = path - opts.ValueDir = path + opts.Dir = newPath + opts.ValueDir = newPath - db2, err := badger.Open(opts.Options) + dbNew, err := badger.Open(opts.Options) if err != nil { - return fmt.Errorf("failed to open badger blockstore in %s: %w", path, err) + return fmt.Errorf("failed to open badger blockstore in %s: %w", newPath, err) } b.lockMove() - b.dbNext = db2 + b.dbNext = dbNew b.unlockMove(moveStateMoving) log.Info("copying blockstore") err = b.doCopy(b.db, b.dbNext) if err != nil { - return fmt.Errorf("error moving badger blockstore to %s: %w", path, err) + return fmt.Errorf("error moving badger blockstore to %s: %w", newPath, err) } b.lockMove() - db1 := b.db + dbOld := b.db b.db = b.dbNext b.dbNext = nil b.unlockMove(moveStateCleanup) - err = db1.Close() + err = dbOld.Close() if err != nil { log.Warnf("error closing old badger db: %s", err) } - dbpath := b.opts.Dir - oldpath := fmt.Sprintf("%s.old.%d", dbpath, time.Now().Unix()) + // this is the canonical db path; this is where our db lives. + dbPath := b.opts.Dir - if err = os.Rename(dbpath, oldpath); err != nil { + // we first move the existing db out of the way, and only delete it after we have symlinked the + // new db to the canonical path + backupPath := fmt.Sprintf("%s.old.%d", dbPath, time.Now().Unix()) + if err = os.Rename(dbPath, backupPath); err != nil { // this is not catastrophic in the sense that we have not lost any data. // but it is pretty bad, as the db path points to the old db, while we are now using to the new // db; we can't continue and leave a ticking bomb for the next restart. // so a panic is appropriate and user can fix. - panic(fmt.Errorf("error renaming old badger db dir from %s to %s: %w; USER ACTION REQUIRED", dbpath, oldpath, err)) //nolint + panic(fmt.Errorf("error renaming old badger db dir from %s to %s: %w; USER ACTION REQUIRED", dbPath, backupPath, err)) //nolint } - if err = os.Symlink(path, dbpath); err != nil { + if err = os.Symlink(newPath, dbPath); err != nil { // same here; the db path is pointing to the void. panic and let the user fix. - panic(fmt.Errorf("error symlinking new badger db dir from %s to %s: %w; USER ACTION REQUIRED", path, dbpath, err)) //nolint + panic(fmt.Errorf("error symlinking new badger db dir from %s to %s: %w; USER ACTION REQUIRED", newPath, dbPath, err)) //nolint } - b.deleteDB(oldpath) + // the delete follows symlinks + b.deleteDB(backupPath) log.Info("moving blockstore done") return nil @@ -390,19 +399,19 @@ func (b *Blockstore) doCopy(from, to *badger.DB) error { func (b *Blockstore) deleteDB(path string) { // follow symbolic links, otherwise the data wil be left behind - lpath, err := filepath.EvalSymlinks(path) + linkPath, err := filepath.EvalSymlinks(path) if err != nil { log.Warnf("error resolving symlinks in %s", path) return } - log.Infof("removing data directory %s", lpath) - if err := os.RemoveAll(lpath); err != nil { - log.Warnf("error deleting db at %s: %s", lpath, err) + log.Infof("removing data directory %s", linkPath) + if err := os.RemoveAll(linkPath); err != nil { + log.Warnf("error deleting db at %s: %s", linkPath, err) return } - if path != lpath { + if path != linkPath { log.Infof("removing link %s", path) if err := os.Remove(path); err != nil { log.Warnf("error removing symbolic link %s", err) From 0740274b7c17dfb1857cdeb92494852c84dd4b22 Mon Sep 17 00:00:00 2001 From: vyzo Date: Wed, 28 Jul 2021 11:56:23 +0300 Subject: [PATCH 2/7] make relative links when the canonical and new db paths are in the same directory --- blockstore/badger/blockstore.go | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/blockstore/badger/blockstore.go b/blockstore/badger/blockstore.go index 49951db6ed1..3d186228327 100644 --- a/blockstore/badger/blockstore.go +++ b/blockstore/badger/blockstore.go @@ -355,7 +355,7 @@ func (b *Blockstore) movingGC() error { panic(fmt.Errorf("error renaming old badger db dir from %s to %s: %w; USER ACTION REQUIRED", dbPath, backupPath, err)) //nolint } - if err = os.Symlink(newPath, dbPath); err != nil { + if err = b.symlink(newPath, dbPath); err != nil { // same here; the db path is pointing to the void. panic and let the user fix. panic(fmt.Errorf("error symlinking new badger db dir from %s to %s: %w; USER ACTION REQUIRED", newPath, dbPath, err)) //nolint } @@ -367,6 +367,18 @@ func (b *Blockstore) movingGC() error { return nil } +// symlink creates a symlink from path to linkPath; the link is relative if the two are +// in the same directory +func (b *Blockstore) symlink(path, linkTo string) error { + pathDir := filepath.Dir(path) + linkDir := filepath.Dir(linkTo) + if pathDir == linkDir { + path = filepath.Base(path) + } + + return os.Symlink(path, linkTo) +} + // doCopy copies a badger blockstore to another, with an optional filter; if the filter // is not nil, then only cids that satisfy the filter will be copied. func (b *Blockstore) doCopy(from, to *badger.DB) error { From 297c9e2f7a915d78220146f385bc03919dfa70fb Mon Sep 17 00:00:00 2001 From: vyzo Date: Wed, 28 Jul 2021 11:56:35 +0300 Subject: [PATCH 3/7] extend test to check the validity of relative links --- blockstore/badger/blockstore_test.go | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/blockstore/badger/blockstore_test.go b/blockstore/badger/blockstore_test.go index ddfa6f28d35..d8ef5241b49 100644 --- a/blockstore/badger/blockstore_test.go +++ b/blockstore/badger/blockstore_test.go @@ -245,6 +245,21 @@ func testMove(t *testing.T, optsF func(string) Options) { checkBlocks() checkPath() + + // reopen the db to make sure our relative link works: + err = db.Close() + if err != nil { + t.Fatal(err) + } + + db, err = Open(optsF(dbPath)) + if err != nil { + t.Fatal(err) + } + + // db.Close() is already deferred + + checkBlocks() } func TestMoveNoPrefix(t *testing.T) { From fff1c0ae573387df3e6c5eaa1454b3bcb05c2dfc Mon Sep 17 00:00:00 2001 From: vyzo Date: Wed, 28 Jul 2021 16:15:39 +0300 Subject: [PATCH 4/7] improve detection of relative links --- blockstore/badger/blockstore.go | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/blockstore/badger/blockstore.go b/blockstore/badger/blockstore.go index 3d186228327..094b8eb6c5c 100644 --- a/blockstore/badger/blockstore.go +++ b/blockstore/badger/blockstore.go @@ -370,9 +370,17 @@ func (b *Blockstore) movingGC() error { // symlink creates a symlink from path to linkPath; the link is relative if the two are // in the same directory func (b *Blockstore) symlink(path, linkTo string) error { - pathDir := filepath.Dir(path) - linkDir := filepath.Dir(linkTo) - if pathDir == linkDir { + resolvedPathDir, err := filepath.EvalSymlinks(filepath.Dir(path)) + if err != nil { + return fmt.Errorf("error resolving links in %s: %w", path, err) + } + + resolvedLinkDir, err := filepath.EvalSymlinks(filepath.Dir(linkTo)) + if err != nil { + return fmt.Errorf("error resolving links in %s: %W", linkTo, err) + } + + if resolvedPathDir == resolvedLinkDir { path = filepath.Base(path) } From c65d72fbca1beda34285b8eeefba1b0fd10d4bfc Mon Sep 17 00:00:00 2001 From: vyzo Date: Wed, 28 Jul 2021 16:20:25 +0300 Subject: [PATCH 5/7] fix format specifier --- blockstore/badger/blockstore.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/blockstore/badger/blockstore.go b/blockstore/badger/blockstore.go index 094b8eb6c5c..dad37a6b1ff 100644 --- a/blockstore/badger/blockstore.go +++ b/blockstore/badger/blockstore.go @@ -377,7 +377,7 @@ func (b *Blockstore) symlink(path, linkTo string) error { resolvedLinkDir, err := filepath.EvalSymlinks(filepath.Dir(linkTo)) if err != nil { - return fmt.Errorf("error resolving links in %s: %W", linkTo, err) + return fmt.Errorf("error resolving links in %s: %w", linkTo, err) } if resolvedPathDir == resolvedLinkDir { From b75ff37448d0a65f105e004271c7c42ac82b5286 Mon Sep 17 00:00:00 2001 From: vyzo Date: Wed, 28 Jul 2021 17:11:04 +0300 Subject: [PATCH 6/7] fix typo Co-authored-by: Jakub Sztandera --- blockstore/badger/blockstore.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/blockstore/badger/blockstore.go b/blockstore/badger/blockstore.go index dad37a6b1ff..05e9048c90f 100644 --- a/blockstore/badger/blockstore.go +++ b/blockstore/badger/blockstore.go @@ -367,7 +367,7 @@ func (b *Blockstore) movingGC() error { return nil } -// symlink creates a symlink from path to linkPath; the link is relative if the two are +// symlink creates a symlink from path to linkTo; the link is relative if the two are // in the same directory func (b *Blockstore) symlink(path, linkTo string) error { resolvedPathDir, err := filepath.EvalSymlinks(filepath.Dir(path)) From 5e8b2c78608ba476ff083eac1b6d9c294848c02d Mon Sep 17 00:00:00 2001 From: vyzo Date: Thu, 29 Jul 2021 08:35:53 +0300 Subject: [PATCH 7/7] make symlink helper freestanding --- blockstore/badger/blockstore.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/blockstore/badger/blockstore.go b/blockstore/badger/blockstore.go index 05e9048c90f..a0b51d8df61 100644 --- a/blockstore/badger/blockstore.go +++ b/blockstore/badger/blockstore.go @@ -355,7 +355,7 @@ func (b *Blockstore) movingGC() error { panic(fmt.Errorf("error renaming old badger db dir from %s to %s: %w; USER ACTION REQUIRED", dbPath, backupPath, err)) //nolint } - if err = b.symlink(newPath, dbPath); err != nil { + if err = symlink(newPath, dbPath); err != nil { // same here; the db path is pointing to the void. panic and let the user fix. panic(fmt.Errorf("error symlinking new badger db dir from %s to %s: %w; USER ACTION REQUIRED", newPath, dbPath, err)) //nolint } @@ -369,7 +369,7 @@ func (b *Blockstore) movingGC() error { // symlink creates a symlink from path to linkTo; the link is relative if the two are // in the same directory -func (b *Blockstore) symlink(path, linkTo string) error { +func symlink(path, linkTo string) error { resolvedPathDir, err := filepath.EvalSymlinks(filepath.Dir(path)) if err != nil { return fmt.Errorf("error resolving links in %s: %w", path, err)