Moving GC for badger #6854

vyzo · 2021-07-23T20:17:18Z

Cherry-picked from #6728.

This adds the option to perform moving GC on a badger blockstore; this effectively reclaims all space.
It is also hooked into the splitstore, to perform moving GC once every 20 compactions (about once a week, user configurable).

Rationale: badger supports online GC, which is quite fast but ultimately ineffective at reclaiming space. It has been observed that the hotstore size slowly creeps up, despite frequent garbage collections.
This addresses the situation with real moving GC.

vyzo · 2021-07-23T20:28:25Z

dunno what's up with the gen-check failure, there is no information there.

magik6k · 2021-07-23T21:03:13Z

dunno what's up with the gen-check failure, there is no information there.

When you expand the second-to-last thing on circle it shows you the diff. The usual solution is to just run make gen which should regenerate whatever it's complaining about (in this case config doc-gen stuff which just landed in #6848 - so just running make cfgdoc-gen should also be enough, and faster)

node/config/types.go

vyzo · 2021-07-24T17:21:35Z

Effectiveness of moving GC:

Before moving GC, after about a week with online GC in every compaction:

132G    ./datastore/splitstore/hot.badger

An hour after moving GC:

57G     ./datastore/splitstore/hot.badger.1627141269

vyzo · 2021-07-25T06:00:42Z

rebased on master.

the great spellchecker strikes again

we can't really continue and leave a ticking bomb for the next restart; the user might not see it.

it was there to support a potential CopyTo interface; but we'll cross that bridge when we get there.

vyzo · 2021-07-27T09:12:31Z

Changes per review comments (quite a few!):

Updated the interface to use functional options.
Renamed MovingGC to FullGC in the nomenclature.
Removed the target path option and made the new path resolve links.
Dropped the filter from the copy; we are not currently using it anyway.
Implemented the copy using the streaming interface.
Made path manipulation failures that require action panic; that seems to be the only sane way to proceed.
Made the state variables typed.
Renamed db2 to dbNext in the data structure.
Used EvalSymlinks where appropriate

vyzo · 2021-07-27T09:14:12Z

ok, this is ready for the next round of review; in the meantime I'll deploy it in my discard node to get some timings and real-world burn.

vyzo · 2021-07-27T11:33:11Z

Timings are looking really good:

2021-07-27T11:31:33.039Z        INFO    splitstore      splitstore/splitstore_gc.go:30  garbage collecting hotstore done        {"took": 535.387156281}

previously it was taking about 40min; that's 4.5 times faster.

jennijuju · 2021-07-27T19:14:14Z

We really want this in, however, it's not a hard blocker for v1.11.1

Stebalien

Mostly naming comments but also some questions around file names.

TL;DR: When I have 2 databases and 4 different paths (old, new, link target, backup), the names really matter.

blockstore/badger/blockstore.go

Stebalien · 2021-07-27T23:36:18Z

blockstore/badger/blockstore.go

+		panic(fmt.Errorf("error renaming old badger db dir from %s to %s: %w; USER ACTION REQUIRED", dbpath, oldpath, err)) //nolint
+	}
+
+	if err = os.Symlink(path, dbpath); err != nil {


What if I wasn't using symlinks?

is that a problem?
The issue with not using symlinks is moving across filesystems, which can take a long time.

Basically, what if I had the database at "foo". Now I'll end up with a symlink at "foo" pointing to a random directory.

If I started out with plain directories, I'd rather end up with plain directories. It's not critical, but the current version is going to confuse users.

They initialize their repo.

They see where their hotstore is.

The hotstore gets moved and replaced by a symlink.

Hrm, it complicates things to try and keep it symlink-free, as we'll need two versions of things.

We should probably be clear to users that the splitstore owns the splitstore directory; they shouldn't muck with it.
The coldstore is another matter though.

Fair enough.

Stebalien · 2021-07-27T23:39:08Z

blockstore/badger/blockstore.go

+	dbpath := b.opts.Dir
+	oldpath := fmt.Sprintf("%s.old.%d", dbpath, time.Now().Unix())
+
+	if err = os.Rename(dbpath, oldpath); err != nil {


Won't this just rename the symlink? Don't we need to move the actual file?

We need integration tests for this.

Ah, I see... we are renaming the symlink intentionally. Why not just delete it then delete linkPath?

yeah, it's fine -- we'll delete deep in deleteDB which follows symlinks.

Also, I don't think we need an integration test for this, we can test pretty much everything with unit tests as the changes are pretty self-contained.

What do you want to add to the unit test?

yeah, it's fine -- we'll delete deep in deleteDB which follows symlinks.

It's fine, but a bit weird.

What do you want to add to the unit test?

The unit test is fine. I thought there was a bug here which would have been caught by a basic test. I revisited this because I then did look at the test, and it clearly checked this case.

Your tests are good.

Stebalien · 2021-07-28T00:07:01Z

Other feedback: The renaming logic with the symlink dance is strange. We should be preserving symlinks if the user is using them, and not using symlinks if the user isn't already using them.

Not all filesystems support them.
Are we sure we're not creating absolute symlinks? That could cause trouble when moving repos.

vyzo · 2021-07-28T05:32:00Z

Not all filesystems support them.

Wait, is there a realistic situation where lotus would run in a filesystem that doesn't support symlinks?
Even windows support them these days.

Are we sure we're not creating absolute symlinks? That could cause trouble when moving repos.

Gah, you are right about this. We are creating absolute links, I'll fix in follow up.

Stebalien · 2021-07-28T05:43:30Z

Wait, is there a realistic situation where lotus would run in a filesystem that doesn't support symlinks?

Well... probably not. Given that NFS does. Although someone probably does it anyways...

Sorry, I've had some recent encounters with symlinks.

vyzo · 2021-07-28T08:58:42Z

Follw up in #6905

vyzo requested review from Stebalien and raulk July 23, 2021 20:17

vyzo requested a review from a team as a code owner July 23, 2021 20:17

vyzo added epic/splitstore team/ignite Issues and PRs being tracked by Team Ignite at Protocol Labs labels Jul 23, 2021

magik6k reviewed Jul 23, 2021

View reviewed changes

node/config/types.go Outdated Show resolved Hide resolved

vyzo force-pushed the feat/moving-gc branch from e597f7c to 4a9c498 Compare July 25, 2021 06:00

vyzo mentioned this pull request Jul 25, 2021

Splitstore: Implement cold object reification #6868

Closed

vyzo added 18 commits July 26, 2021 15:04

implement BlockstoreMover in badger

b741d61

use pooled slices for the copy

001c04f

add MoveTo test

aec1268

fix lotus-shed

4715b1f

fix lint

4b0b37a

the great spellchecker strikes again

fix copy: flush discards the transaction

608a9f8

follow symbolic links when deleting old dbs

f2c7b08

add some more logging around move

524564e

don't heap allocate the cond, just set L

5cf6fdf

make moveTo a private method

9450996

add options to BlockstoreGC trait

a843665

do moving GC if the user asks for it

c747f2f

do hotstore moving GC in splitstore with a user-specified frequency

fb39862

add config option for splitstore moving gc frequency

5acae50

document moving GC frequency option in README

b1f60e8

use CollectGarbage in blockstore move test, as it is the real interface

21e7c18

add docstrings for splitstore config

4cdb34e

make cfgdoc-gen

20f93a5

vyzo added 7 commits July 27, 2021 10:27

panic if we fail to correctly setup the db paths.

649fc62

we can't really continue and leave a ticking bomb for the next restart; the user might not see it.

remove filter from doCopy; it's not used

0baeec0

it was there to support a potential CopyTo interface; but we'll cross that bridge when we get there.

use a slab allocator for the copy

59aebba

use the badger streaming interface in doCopy

b82f953

make state constants typed

bb2d999

remove db2 to dbNext

c21c413

update README

8809707

vyzo added 2 commits July 27, 2021 12:12

Merge branch 'master' into feat/moving-gc

da3e36a

make cfgdoc-gen

7c19524

raulk approved these changes Jul 27, 2021

View reviewed changes

Merge branch 'master' into feat/moving-gc

7c28ff3

jennijuju added this to the 1.11.1 milestone Jul 27, 2021

jennijuju added P1 P1: Must be resolved P2 P2: Should be resolved and removed P1 P1: Must be resolved labels Jul 27, 2021

vyzo merged commit 6751185 into master Jul 27, 2021

vyzo deleted the feat/moving-gc branch July 27, 2021 20:27

vyzo restored the feat/moving-gc branch July 27, 2021 20:27

vyzo deleted the feat/moving-gc branch July 27, 2021 20:27

Stebalien reviewed Jul 27, 2021

View reviewed changes

vyzo mentioned this pull request Jul 28, 2021

Moving GC Followup #6905

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Moving GC for badger #6854

Moving GC for badger #6854

vyzo commented Jul 23, 2021 •

edited

Loading

vyzo commented Jul 23, 2021

magik6k commented Jul 23, 2021

vyzo commented Jul 24, 2021

vyzo commented Jul 25, 2021

vyzo commented Jul 27, 2021

vyzo commented Jul 27, 2021

vyzo commented Jul 27, 2021 •

edited

Loading

jennijuju commented Jul 27, 2021 •

edited

Loading

Stebalien left a comment

Stebalien Jul 27, 2021

vyzo Jul 28, 2021

Stebalien Jul 28, 2021

vyzo Jul 28, 2021

Stebalien Jul 28, 2021

Stebalien Jul 27, 2021

Stebalien Jul 27, 2021

vyzo Jul 28, 2021

Stebalien Jul 28, 2021

Stebalien commented Jul 28, 2021

vyzo commented Jul 28, 2021

Stebalien commented Jul 28, 2021

vyzo commented Jul 28, 2021

Moving GC for badger #6854

Moving GC for badger #6854

Conversation

vyzo commented Jul 23, 2021 • edited Loading

vyzo commented Jul 23, 2021

magik6k commented Jul 23, 2021

vyzo commented Jul 24, 2021

vyzo commented Jul 25, 2021

vyzo commented Jul 27, 2021

vyzo commented Jul 27, 2021

vyzo commented Jul 27, 2021 • edited Loading

jennijuju commented Jul 27, 2021 • edited Loading

Stebalien 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

Choose a reason for hiding this comment

Stebalien commented Jul 28, 2021

vyzo commented Jul 28, 2021

Stebalien commented Jul 28, 2021

vyzo commented Jul 28, 2021

vyzo commented Jul 23, 2021 •

edited

Loading

vyzo commented Jul 27, 2021 •

edited

Loading

jennijuju commented Jul 27, 2021 •

edited

Loading