Repository: cockroachdb/pebble
Pull request #486 (2020-01-10 16:59:03)

internal/metamorphic: use the strict MemFS and add an operation to re…
…set the DB

petermattis: 

<!-- Reviewable:start -->
This change is [<img src="https://reviewable.io/review_button.png" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/cockroachdb/pebble/486)
<!-- Reviewable:end -->


Patch:
diff --git a/internal/metamorphic/config.go b/internal/metamorphic/config.go
index d3c6e8eb..ddfd5f66 100644
--- a/internal/metamorphic/config.go
+++ b/internal/metamorphic/config.go
@@ -9,6 +9,8 @@ type opType int
 const (
 	batchAbort opType = iota
 	batchCommit
+	dbClose
+	dbRestart
 	iterClose
 	iterFirst
 	iterLast
@@ -47,9 +49,11 @@ type config struct {
 }
 
 var defaultConfig = config{
+	// dbClose is not in this list since it is deterministically generated once, at the end of the test.
 	ops: []int{
 		batchAbort:        5,
 		batchCommit:       5,
+		dbRestart:         2,
 		iterClose:         10,
 		iterFirst:         100,
 		iterLast:          100,
diff --git a/internal/metamorphic/generator.go b/internal/metamorphic/generator.go
index 5a7a0f80..4525f300 100644
--- a/internal/metamorphic/generator.go
+++ b/internal/metamorphic/generator.go
@@ -31,11 +31,12 @@ type generator struct {
 	liveBatches objIDSlice
 	// liveIters contains the live iterators.
 	liveIters objIDSlice
-	// liveReaders contains the DB, and any live indexed batches and snapshots.
+	// liveReaders contains the DB, and any live indexed batches and snapshots. The DB is always
+	// at index 0.
 	liveReaders objIDSlice
 	// liveSnapshots contains the live snapshots.
 	liveSnapshots objIDSlice
-	// liveWriters contains the DB, and any live batches.
+	// liveWriters contains the DB, and any live batches. The DB is always at index 0.
 	liveWriters objIDSlice
 
 	// Maps used to find associated objects during generation. These maps are not
@@ -49,7 +50,7 @@ type generator struct {
 	// or snapshots maps.
 	iters map[objID]objIDSet
 	// readerID -> reader iters: used to keep track of the open iterators on a
-	// reader. The iter set value will also be inexed by either the batches or
+	// reader. The iter set value will also be indexed by either the batches or
 	// snapshots maps. This map is the union of batches and snapshots maps.
 	readers map[objID]objIDSet
 	// snapshotID -> snapshot iters: used to keep track of the open iterators on
@@ -79,6 +80,7 @@ func generate(count uint64, cfg config) []op {
 	generators := []func(){
 		batchAbort:        g.batchAbort,
 		batchCommit:       g.batchCommit,
+		dbRestart:         g.dbRestart,
 		iterClose:         g.iterClose,
 		iterFirst:         g.iterFirst,
 		iterLast:          g.iterLast,
@@ -109,14 +111,7 @@ func generate(count uint64, cfg config) []op {
 		generators[deck.Int()]()
 	}
 
-	// Close any live iterators and snapshots, so that we can close the DB
-	// cleanly.
-	for len(g.liveIters) > 0 {
-		g.iterClose()
-	}
-	for len(g.liveSnapshots) > 0 {
-		g.snapshotClose()
-	}
+	g.dbClose()
 	return g.ops
 }
 
@@ -236,6 +231,38 @@ func (g *generator) batchCommit() {
 	})
 }
 
+func (g *generator) dbClose() {
+	// Close any live iterators and snapshots, so that we can close the DB
+	// cleanly.
+	for len(g.liveIters) > 0 {
+		g.iterClose()
+	}
+	for len(g.liveSnapshots) > 0 {
+		g.snapshotClose()
+	}
+	g.add(&closeOp{objID: makeObjID(dbTag, 0)})
+}
+
+func (g *generator) dbRestart() {
+	// Close any live iterators and snapshots, so that we can close the DB
+	// cleanly.
+	for len(g.liveIters) > 0 {
+		g.iterClose()
+	}
+	for len(g.liveSnapshots) > 0 {
+		g.snapshotClose()
+	}
+	// Close the batches.
+	for len(g.liveBatches) > 0 {
+		g.batchClose(g.liveBatches[0])
+	}
+	if len(g.liveReaders) != 1 || len(g.liveWriters) != 1 {
+		panic(fmt.Sprintf("unexpected counts: liveReaders %d, liveWriters: %d",
+			len(g.liveReaders), len(g.liveWriters)))
+	}
+	g.add(&dbRestartOp{})
+}
+
 func (g *generator) newIter() {
 	iterID := makeObjID(iterTag, g.init.iterSlots)
 	g.init.iterSlots++
diff --git a/internal/metamorphic/ops.go b/internal/metamorphic/ops.go
index 9654154d..46ed9410 100644
--- a/internal/metamorphic/ops.go
+++ b/internal/metamorphic/ops.go
@@ -6,6 +6,7 @@ package metamorphic
 
 import (
 	"fmt"
+	"os"
 	"strings"
 
 	"github.com/cockroachdb/pebble"
@@ -48,7 +49,7 @@ type applyOp struct {
 func (o *applyOp) run(t *test, h *history) {
 	b := t.getBatch(o.batchID)
 	w := t.getWriter(o.writerID)
-	err := w.Apply(b, pebble.NoSync)
+	err := w.Apply(b, t.writeOpts)
 	h.Recordf("%s // %v", o, err)
 	_ = b.Close()
 	t.clearObj(o.batchID)
@@ -82,7 +83,7 @@ type deleteOp struct {
 
 func (o *deleteOp) run(t *test, h *history) {
 	w := t.getWriter(o.writerID)
-	err := w.Delete(o.key, pebble.NoSync)
+	err := w.Delete(o.key, t.writeOpts)
 	h.Recordf("%s // %v", o, err)
 }
 
@@ -99,7 +100,7 @@ type deleteRangeOp struct {
 
 func (o *deleteRangeOp) run(t *test, h *history) {
 	w := t.getWriter(o.writerID)
-	err := w.DeleteRange(o.start, o.end, pebble.NoSync)
+	err := w.DeleteRange(o.start, o.end, t.writeOpts)
 	h.Recordf("%s // %v", o, err)
 }
 
@@ -116,7 +117,7 @@ type mergeOp struct {
 
 func (o *mergeOp) run(t *test, h *history) {
 	w := t.getWriter(o.writerID)
-	err := w.Merge(o.key, o.value, pebble.NoSync)
+	err := w.Merge(o.key, o.value, t.writeOpts)
 	h.Recordf("%s // %v", o, err)
 }
 
@@ -133,7 +134,7 @@ type setOp struct {
 
 func (o *setOp) run(t *test, h *history) {
 	w := t.getWriter(o.writerID)
-	err := w.Set(o.key, o.value, pebble.NoSync)
+	err := w.Set(o.key, o.value, t.writeOpts)
 	h.Recordf("%s // %v", o, err)
 }
 
@@ -179,7 +180,7 @@ type batchCommitOp struct {
 func (o *batchCommitOp) run(t *test, h *history) {
 	b := t.getBatch(o.batchID)
 	t.clearObj(o.batchID)
-	err := b.Commit(pebble.NoSync)
+	err := b.Commit(t.writeOpts)
 	h.Recordf("%s // %v", o, err)
 }
 
@@ -504,6 +505,23 @@ func (o *newSnapshotOp) String() string {
 	return fmt.Sprintf("%s = db.NewSnapshot()", o.snapID)
 }
 
+type dbRestartOp struct {
+}
+
+func (o *dbRestartOp) run(t *test, h *history) {
+	if err := t.restartDB(); err != nil {
+		h.Recordf("%s // %v", o, err)
+		fmt.Fprintf(os.Stderr, "terminating since dbRestartOp returned err %s\n", err)
+		h.failed = true
+	} else {
+		h.Recordf("%s", o)
+	}
+}
+
+func (o *dbRestartOp) String() string {
+	return fmt.Sprintf("db.Restart()")
+}
+
 func formatOps(ops []op) string {
 	var buf strings.Builder
 	for _, op := range ops {
diff --git a/internal/metamorphic/options.go b/internal/metamorphic/options.go
index f3ed3056..06932838 100644
--- a/internal/metamorphic/options.go
+++ b/internal/metamorphic/options.go
@@ -20,7 +20,7 @@ var comparer = func() pebble.Comparer {
 	return c
 }()
 
-func parseOptions(opts *pebble.Options, data string) error {
+func parseOptions(opts *testOptions, data string) error {
 	hooks := &pebble.ParseHooks{
 		NewFilterPolicy: func(name string) (pebble.FilterPolicy, error) {
 			if name == "none" {
@@ -28,8 +28,31 @@ func parseOptions(opts *pebble.Options, data string) error {
 			}
 			return bloom.FilterPolicy(10), nil
 		},
+		SkipUnknown: func(name string) bool {
+			switch name {
+			case "TestOptions":
+				return true
+			case "TestOptions.strictfs":
+				opts.strictFS = true
+				return true
+			default:
+				return false
+			}
+		},
+	}
+	err := opts.opts.Parse(data, hooks)
+	return err
+}
+
+func optionsToString(opts *testOptions) string {
+	str := opts.opts.String()
+	if opts.strictFS {
+		str += `
+[TestOptions]
+  strictfs=true
+`
 	}
-	return opts.Parse(data, hooks)
+	return str
 }
 
 func defaultOptions() *pebble.Options {
@@ -44,7 +67,12 @@ func defaultOptions() *pebble.Options {
 	return opts
 }
 
-func standardOptions() []*pebble.Options {
+type testOptions struct {
+	opts     *pebble.Options
+	strictFS bool
+}
+
+func standardOptions() []*testOptions {
 	// The index labels are not strictly necessary, but they make it easier to
 	// find which options correspond to a failure.
 	stdOpts := []string{
@@ -113,12 +141,19 @@ func standardOptions() []*pebble.Options {
 		16: `
 [Level "0"]
   filter_policy=none
+`,
+		// 1GB
+		17: `
+[Options]
+  bytes_per_sync=1073741824
+[TestOptions]
+  strictfs=true
 `,
 	}
 
-	opts := make([]*pebble.Options, len(stdOpts))
+	opts := make([]*testOptions, len(stdOpts))
 	for i := range opts {
-		opts[i] = defaultOptions()
+		opts[i] = &testOptions{opts: defaultOptions()}
 		if err := parseOptions(opts[i], stdOpts[i]); err != nil {
 			panic(err)
 		}
@@ -126,8 +161,10 @@ func standardOptions() []*pebble.Options {
 	return opts
 }
 
-func randomOptions(rng *rand.Rand) *pebble.Options {
+func randomOptions(rng *rand.Rand) *testOptions {
+	var testOpts = &testOptions{}
 	opts := defaultOptions()
+	opts.BytesPerSync = 1 << uint(rng.Intn(28))     // 1B - 256MB
 	opts.Cache = cache.New(1 << uint(rng.Intn(30))) // 1B - 1GB
 	opts.DisableWAL = rng.Intn(2) == 0
 	opts.L0CompactionThreshold = 1 + rng.Intn(100) // 1 - 100
@@ -149,5 +186,11 @@ func randomOptions(rng *rand.Rand) *pebble.Options {
 	lopts.IndexBlockSize = 1 << uint(rng.Intn(24)) // 1 - 16MB
 	lopts.TargetFileSize = 1 << uint(rng.Intn(28)) // 1 - 256MB
 	opts.Levels = []pebble.LevelOptions{lopts}
-	return opts
+
+	testOpts.opts = opts
+	testOpts.strictFS = rng.Intn(2) != 0
+	if testOpts.strictFS {
+		opts.DisableWAL = false
+	}
+	return testOpts
 }
diff --git a/internal/metamorphic/parser.go b/internal/metamorphic/parser.go
index 81d3d368..7214ec49 100644
--- a/internal/metamorphic/parser.go
+++ b/internal/metamorphic/parser.go
@@ -47,6 +47,8 @@ func opArgs(op op) (receiverID *objID, targetID *objID, args []interface{}) {
 		return &t.objID, nil, nil
 	case *batchCommitOp:
 		return &t.batchID, nil, nil
+	case *dbRestartOp:
+		return nil, nil, nil
 	case *deleteOp:
 		return &t.writerID, nil, []interface{}{&t.key}
 	case *deleteRangeOp:
@@ -91,7 +93,7 @@ func opArgs(op op) (receiverID *objID, targetID *objID, args []interface{}) {
 
 var methods = map[string]*methodInfo{
 	"Apply":           makeMethod(applyOp{}, dbTag, batchTag),
-	"Close":           makeMethod(closeOp{}, batchTag, iterTag, snapTag),
+	"Close":           makeMethod(closeOp{}, dbTag, batchTag, iterTag, snapTag),
 	"Commit":          makeMethod(batchCommitOp{}, batchTag),
 	"Delete":          makeMethod(deleteOp{}, dbTag, batchTag),
 	"DeleteRange":     makeMethod(deleteRangeOp{}, dbTag, batchTag),
@@ -107,6 +109,7 @@ var methods = map[string]*methodInfo{
 	"NewSnapshot":     makeMethod(newSnapshotOp{}, dbTag),
 	"Next":            makeMethod(iterNextOp{}, iterTag),
 	"Prev":            makeMethod(iterPrevOp{}, iterTag),
+	"Restart":         makeMethod(dbRestartOp{}, dbTag),
 	"SeekGE":          makeMethod(iterSeekGEOp{}, iterTag),
 	"SeekLT":          makeMethod(iterSeekLTOp{}, iterTag),
 	"SeekPrefixGE":    makeMethod(iterSeekPrefixGEOp{}, iterTag),
diff --git a/options.go b/options.go
index 9b4a72c7..6519ef80 100644
--- a/options.go
+++ b/options.go
@@ -787,7 +787,7 @@ func (o *Options) Parse(s string, hooks *ParseHooks) error {
 			}
 			return err
 		}
-		if hooks != nil && hooks.SkipUnknown != nil && hooks.SkipUnknown(section) {
+		if hooks != nil && hooks.SkipUnknown != nil && hooks.SkipUnknown(section+"."+key) {
 			return nil
 		}
 		return fmt.Errorf("pebble: unknown section: %q", section)


Test patch:
diff --git a/internal/metamorphic/meta_test.go b/internal/metamorphic/meta_test.go
index 07ec9284..86185179 100644
--- a/internal/metamorphic/meta_test.go
+++ b/internal/metamorphic/meta_test.go
@@ -77,7 +77,8 @@ func testMetaRun(t *testing.T, runDir string) {
 		t.Fatal(err)
 	}
 	opts := &pebble.Options{}
-	if err := parseOptions(opts, string(optionsData)); err != nil {
+	testOpts := &testOptions{opts: opts}
+	if err := parseOptions(testOpts, string(optionsData)); err != nil {
 		t.Fatal(err)
 	}
 
@@ -88,14 +89,18 @@ func testMetaRun(t *testing.T, runDir string) {
 
 	// Set up the filesystem to use for the test. Note that by default we use an
 	// in-memory FS.
-	if *disk {
+	if *disk && !testOpts.strictFS {
 		opts.FS = vfs.Default
 		if err := os.RemoveAll(opts.FS.PathJoin(runDir, "data")); err != nil {
 			t.Fatal(err)
 		}
 	} else {
 		opts.Cleaner = base.ArchiveCleaner{}
-		opts.FS = vfs.NewMem()
+		if testOpts.strictFS {
+			opts.FS = vfs.NewStrictMem()
+		} else {
+			opts.FS = vfs.NewMem()
+		}
 	}
 	if opts.WALDir != "" {
 		opts.WALDir = opts.FS.PathJoin(runDir, opts.WALDir)
@@ -115,17 +120,18 @@ func testMetaRun(t *testing.T, runDir string) {
 	h := newHistory(*failRE, writers...)
 
 	m := newTest(ops)
-	if err := m.init(h, opts.FS.PathJoin(runDir, "data"), opts); err != nil {
+	if err := m.init(h, opts.FS.PathJoin(runDir, "data"), testOpts); err != nil {
 		t.Fatal(err)
 	}
 	for m.step(h) {
 		if h.Failed() {
-			fmt.Fprintf(os.Stderr, "failure regex %q matched\n", *failRE)
+			if len(*failRE) > 0 {
+				fmt.Fprintf(os.Stderr, "failure regex %q matched\n", *failRE)
+			}
 			m.maybeSaveData()
 			os.Exit(1)
 		}
 	}
-	m.finish(h)
 
 	if *keep && !*disk {
 		m.maybeSaveData()
@@ -180,14 +186,15 @@ func TestMeta(t *testing.T) {
 	// Perform a particular test run with the specified options. The options are
 	// written to <run-dir>/OPTIONS and a child process is created to actually
 	// execute the test.
-	runOptions := func(t *testing.T, opts *pebble.Options) {
+	runOptions := func(t *testing.T, opts *testOptions) {
 		runDir := filepath.Join(metaDir, path.Base(t.Name()))
 		if err := os.MkdirAll(runDir, 0755); err != nil {
 			t.Fatal(err)
 		}
 
 		optionsPath := filepath.Join(runDir, "OPTIONS")
-		if err := ioutil.WriteFile(optionsPath, []byte(opts.String()), 0644); err != nil {
+		str := optionsToString(opts)
+		if err := ioutil.WriteFile(optionsPath, []byte(str), 0644); err != nil {
 			t.Fatal(err)
 		}
 
diff --git a/internal/metamorphic/test.go b/internal/metamorphic/test.go
index a8991d06..d813cf79 100644
--- a/internal/metamorphic/test.go
+++ b/internal/metamorphic/test.go
@@ -20,10 +20,12 @@ type test struct {
 	ops []op
 	idx int
 	// The DB the test is run on.
-	dir    string
-	db     *pebble.DB
-	opts   *pebble.Options
-	tmpDir string
+	dir       string
+	db        *pebble.DB
+	opts      *pebble.Options
+	testOpts  *testOptions
+	writeOpts *pebble.WriteOptions
+	tmpDir    string
 	// The slots for the batches, iterators, and snapshots. These are read and
 	// written by the ops to pass state from one op to another.
 	batches   []*pebble.Batch
@@ -37,9 +39,14 @@ func newTest(ops []op) *test {
 	}
 }
 
-func (t *test) init(h *history, dir string, opts *pebble.Options) error {
+func (t *test) init(h *history, dir string, testOpts *testOptions) error {
 	t.dir = dir
-	t.opts = opts.EnsureDefaults()
+	t.testOpts = testOpts
+	t.writeOpts = pebble.NoSync
+	if testOpts.strictFS {
+		t.writeOpts = pebble.Sync
+	}
+	t.opts = testOpts.opts.EnsureDefaults()
 	t.opts.Logger = h.Logger()
 	t.opts.EventListener = pebble.MakeLoggingEventListener(t.opts.Logger)
 	t.opts.DebugCheck = true
@@ -107,11 +114,54 @@ func (t *test) init(h *history, dir string, opts *pebble.Options) error {
 	if err = t.opts.FS.MkdirAll(t.tmpDir, 0755); err != nil {
 		return err
 	}
+	if t.testOpts.strictFS {
+		// Sync the whole directory path for the tmpDir, since restartDB() is executed during
+		// the test. That would reset MemFS to the synced state, which would make an unsynced
+		// directory disappear in the middle of the test. It is the responsibility of the test
+		// (not Pebble) to ensure that it can write the ssts that it will subsequently ingest
+		// into Pebble.
+		for {
+			f, err := t.opts.FS.OpenDir(dir)
+			if err != nil {
+				return err
+			}
+			if err = f.Sync(); err != nil {
+				return err
+			}
+			if err = f.Close(); err != nil {
+				return err
+			}
+			if len(dir) == 1 {
+				break
+			}
+			dir = t.opts.FS.PathDir(dir)
+			// TODO(sbhola): PathDir returns ".", which OpenDir() complains about. Fix.
+			if len(dir) == 1 {
+				dir = "/"
+			}
+		}
+	}
 
 	t.db = db
 	return nil
 }
 
+func (t *test) restartDB() error {
+	if !t.testOpts.strictFS {
+		return nil
+	}
+	fs := t.opts.FS.(*vfs.MemFS)
+	fs.SetIgnoreSyncs(true)
+	if err := t.db.Close(); err != nil {
+		return err
+	}
+	fs.ResetToSyncedState()
+	fs.SetIgnoreSyncs(false)
+	var err error
+	t.db, err = pebble.Open(t.dir, t.opts)
+	return err
+}
+
 // If an in-memory FS is being used, save the contents to disk.
 func (t *test) maybeSaveData() {
 	if t.opts.FS == vfs.Default {
@@ -132,12 +182,6 @@ func (t *test) step(h *history) bool {
 	return true
 }
 
-func (t *test) finish(h *history) {
-	db := t.db
-	t.db = nil
-	h.Recordf("db.Close() // %v", db.Close())
-}
-
 func (t *test) setBatch(id objID, b *pebble.Batch) {
 	if id.tag() != batchTag {
 		panic(fmt.Sprintf("invalid batch ID: %s", id))
@@ -162,7 +206,7 @@ func (t *test) setSnapshot(id objID, s *pebble.Snapshot) {
 func (t *test) clearObj(id objID) {
 	switch id.tag() {
 	case dbTag:
-		panic("cannot clear DB ID")
+		t.db = nil
 	case batchTag:
 		t.batches[id.slot()] = nil
 	case iterTag:
@@ -181,6 +225,8 @@ func (t *test) getBatch(id objID) *pebble.Batch {
 
 func (t *test) getCloser(id objID) io.Closer {
 	switch id.tag() {
+	case dbTag:
+		return t.db
 	case batchTag:
 		return t.batches[id.slot()]
 	case iterTag: