From fb7ce48c15fb19fd76cd0c844c13e8e27b61ed0e Mon Sep 17 00:00:00 2001 From: "mergify[bot]" <37929162+mergify[bot]@users.noreply.github.com> Date: Thu, 7 Apr 2022 15:51:26 -0700 Subject: [PATCH] scmigrate: ensure target key is correctly renamed (backport #8276) (#8280) Prior to v0.35, the keys for seen-commit records included the applicable height. In v0.35 and beyond, we only keep the record for the latest height, and its key does not include the height. Update the seen-commit migration to ensure that the record we retain after migration is correctly renamed to omit the height from its key. Update the test cases to check for this condition after migrating. (cherry picked from commit f3858e52def8051575db7a0fea907e8f10bfb506) --- CHANGELOG_PENDING.md | 2 + go.mod | 1 + go.sum | 2 + scripts/keymigrate/migrate.go | 82 +++++++----------------------- scripts/keymigrate/migrate_test.go | 17 ++----- scripts/scmigrate/migrate.go | 51 ++++++++++++++++--- scripts/scmigrate/migrate_test.go | 26 +++++----- 7 files changed, 82 insertions(+), 99 deletions(-) diff --git a/CHANGELOG_PENDING.md b/CHANGELOG_PENDING.md index 1dcee4468..9f0c7ab4f 100644 --- a/CHANGELOG_PENDING.md +++ b/CHANGELOG_PENDING.md @@ -27,3 +27,5 @@ Special thanks to external contributors on this release: ### IMPROVEMENTS ### BUG FIXES + +- [cli] \#8276 scmigrate: ensure target key is correctly renamed. (@creachadair) diff --git a/go.mod b/go.mod index b870a0b4f..b42ae520a 100644 --- a/go.mod +++ b/go.mod @@ -8,6 +8,7 @@ require ( github.com/adlio/schema v1.3.0 github.com/btcsuite/btcd v0.22.0-beta github.com/btcsuite/btcutil v1.0.3-0.20201208143702-a53e38424cce + github.com/creachadair/taskgroup v0.3.2 github.com/facebookgo/ensure v0.0.0-20160127193407-b4ab57deab51 // indirect github.com/facebookgo/stack v0.0.0-20160209184415-751773369052 // indirect github.com/facebookgo/subset v0.0.0-20150612182917-8dac2c3c4870 // indirect diff --git a/go.sum b/go.sum index 79b17b880..ca26748c0 100644 --- a/go.sum +++ b/go.sum @@ -223,6 +223,8 @@ github.com/cpuguy83/go-md2man v1.0.10/go.mod h1:SmD6nW6nTyfqj6ABTjUi3V3JVMnlJmwc github.com/cpuguy83/go-md2man/v2 v2.0.0-20190314233015-f79a8a8ca69d/go.mod h1:maD7wRr/U5Z6m/iR4s+kqSMx2CaBsrgA7czyZG/E6dU= github.com/cpuguy83/go-md2man/v2 v2.0.0/go.mod h1:maD7wRr/U5Z6m/iR4s+kqSMx2CaBsrgA7czyZG/E6dU= github.com/cpuguy83/go-md2man/v2 v2.0.1/go.mod h1:tgQtvFlXSQOSOSIRvRPT7W67SCa46tRHOmNcaadrF8o= +github.com/creachadair/taskgroup v0.3.2 h1:zlfutDS+5XG40AOxcHDSThxKzns8Tnr9jnr6VqkYlkM= +github.com/creachadair/taskgroup v0.3.2/go.mod h1:wieWwecHVzsidg2CsUnFinW1faVN4+kq+TDlRJQ0Wbk= github.com/creack/pty v1.1.7/go.mod h1:lj5s0c3V2DBrqTV7llrYr5NG6My20zk30Fl46Y7DoTY= github.com/creack/pty v1.1.9/go.mod h1:oKZEueFk5CKHvIhNR5MUki03XCEU+Q6VDXinZuGJ33E= github.com/cyphar/filepath-securejoin v0.2.2/go.mod h1:FpkQEhXnPnOthhzymB7CGsFk2G9VLXONKD9G7QGMM+4= diff --git a/scripts/keymigrate/migrate.go b/scripts/keymigrate/migrate.go index 2061223ad..b53c5e98f 100644 --- a/scripts/keymigrate/migrate.go +++ b/scripts/keymigrate/migrate.go @@ -15,8 +15,8 @@ import ( "math/rand" "runtime" "strconv" - "sync" + "github.com/creachadair/taskgroup" "github.com/google/orderedcode" dbm "github.com/tendermint/tm-db" ) @@ -27,7 +27,7 @@ type ( ) func getAllLegacyKeys(db dbm.DB) ([]keyID, error) { - out := []keyID{} + var out []keyID iter, err := db.Iterator(nil, nil) if err != nil { @@ -43,11 +43,8 @@ func getAllLegacyKeys(db dbm.DB) ([]keyID, error) { continue } - // there's inconsistency around tm-db's handling of - // key copies. - nk := make([]byte, len(k)) - copy(nk, k) - out = append(out, nk) + // Make an explicit copy, since not all tm-db backends do. + out = append(out, []byte(string(k))) } if err = iter.Error(); err != nil { @@ -61,17 +58,6 @@ func getAllLegacyKeys(db dbm.DB) ([]keyID, error) { return out, nil } -func makeKeyChan(keys []keyID) <-chan keyID { - out := make(chan keyID, len(keys)) - defer close(out) - - for _, key := range keys { - out <- key - } - - return out -} - func keyIsLegacy(key keyID) bool { for _, prefix := range []keyID{ // core "store" @@ -111,7 +97,7 @@ func keyIsHash(key keyID) bool { return len(key) == 32 && !bytes.Contains(key, []byte("/")) } -func migarateKey(key keyID) (keyID, error) { +func migrateKey(key keyID) (keyID, error) { switch { case bytes.HasPrefix(key, keyID("H:")): val, err := strconv.Atoi(string(key[2:])) @@ -349,53 +335,23 @@ func Migrate(ctx context.Context, db dbm.DB) error { return err } - numWorkers := runtime.NumCPU() - wg := &sync.WaitGroup{} + var errs []string + g, start := taskgroup.New(func(err error) error { + errs = append(errs, err.Error()) + return err + }).Limit(runtime.NumCPU()) - errs := make(chan error, numWorkers) - - keyCh := makeKeyChan(keys) - - // run migrations. - for i := 0; i < numWorkers; i++ { - wg.Add(1) - go func() { - defer wg.Done() - for key := range keyCh { - err := replaceKey(db, key, migarateKey) - if err != nil { - errs <- err - } - - if ctx.Err() != nil { - return - } + for _, key := range keys { + key := key + start(func() error { + if err := ctx.Err(); err != nil { + return err } - }() + return replaceKey(db, key, migrateKey) + }) } - - // collect and process the errors. - errStrs := []string{} - signal := make(chan struct{}) - go func() { - defer close(signal) - for err := range errs { - if err == nil { - continue - } - errStrs = append(errStrs, err.Error()) - } - }() - - // Wait for everything to be done. - wg.Wait() - close(errs) - <-signal - - // check the error results - if len(errs) != 0 { - return fmt.Errorf("encountered errors during migration: %v", errStrs) + if g.Wait() != nil { + return fmt.Errorf("encountered errors during migration: %q", errs) } - return nil } diff --git a/scripts/keymigrate/migrate_test.go b/scripts/keymigrate/migrate_test.go index 21e9592fb..8f6f30808 100644 --- a/scripts/keymigrate/migrate_test.go +++ b/scripts/keymigrate/migrate_test.go @@ -117,7 +117,7 @@ func TestMigration(t *testing.T) { }) t.Run("Conversion", func(t *testing.T) { for kind, le := range legacyPrefixes { - nk, err := migarateKey(le) + nk, err := migrateKey(le) require.NoError(t, err, kind) require.False(t, keyIsLegacy(nk), kind) } @@ -159,7 +159,7 @@ func TestMigration(t *testing.T) { "UserKey3": []byte("foo/bar/baz/1.2/4"), } for kind, key := range table { - out, err := migarateKey(key) + out, err := migrateKey(key) require.Error(t, err, kind) require.Nil(t, out, kind) } @@ -177,7 +177,7 @@ func TestMigration(t *testing.T) { return nil, errors.New("hi") })) }) - t.Run("KeyDisapears", func(t *testing.T) { + t.Run("KeyDisappears", func(t *testing.T) { db := dbm.NewMemDB() key := keyID("hi") require.NoError(t, db.Set(key, []byte("world"))) @@ -215,17 +215,6 @@ func TestMigration(t *testing.T) { require.False(t, keyIsLegacy(key)) } }) - t.Run("ChannelConversion", func(t *testing.T) { - ch := makeKeyChan([]keyID{ - makeKey(t, "abc", int64(2), int64(42)), - makeKey(t, int64(42)), - }) - count := 0 - for range ch { - count++ - } - require.Equal(t, 2, count) - }) t.Run("Migrate", func(t *testing.T) { _, db := getLegacyDatabase(t) diff --git a/scripts/scmigrate/migrate.go b/scripts/scmigrate/migrate.go index e6eee3d95..ba3de2698 100644 --- a/scripts/scmigrate/migrate.go +++ b/scripts/scmigrate/migrate.go @@ -73,8 +73,6 @@ func sortMigrations(scData []toMigrate) { }) } -func getMigrationsToDelete(in []toMigrate) []toMigrate { return in[1:] } - func getAllSeenCommits(ctx context.Context, db dbm.DB) ([]toMigrate, error) { scKeyPrefix := makeKeyFromPrefix(prefixSeenCommit) iter, err := db.Iterator( @@ -117,6 +115,34 @@ func getAllSeenCommits(ctx context.Context, db dbm.DB) ([]toMigrate, error) { return scData, nil } +func renameRecord(ctx context.Context, db dbm.DB, keep toMigrate) error { + wantKey := makeKeyFromPrefix(prefixSeenCommit) + if bytes.Equal(keep.key, wantKey) { + return nil // we already did this conversion + } + + // This record's key has already been converted to the "new" format, we just + // now need to trim off the tail. + val, err := db.Get(keep.key) + if err != nil { + return err + } + + batch := db.NewBatch() + if err := batch.Delete(keep.key); err != nil { + return err + } + if err := batch.Set(wantKey, val); err != nil { + return err + } + werr := batch.Write() + cerr := batch.Close() + if werr != nil { + return werr + } + return cerr +} + func deleteRecords(ctx context.Context, db dbm.DB, scData []toMigrate) error { // delete all the remaining stale values in a single batch batch := db.NewBatch() @@ -141,20 +167,29 @@ func Migrate(ctx context.Context, db dbm.DB) error { scData, err := getAllSeenCommits(ctx, db) if err != nil { return fmt.Errorf("sourcing tasks to migrate: %w", err) + } else if len(scData) == 0 { + return nil // nothing to do } - // sort earliest->latest commits. + // Sort commits in decreasing order of height. sortMigrations(scData) - // trim the one we want to save: - scData = getMigrationsToDelete(scData) + // Keep and rename the newest seen commit, delete the rest. + // In TM < v0.35 we kept a last-seen commit for each height; in v0.35 we + // retain only the latest. + keep, remove := scData[0], scData[1:] - if len(scData) <= 1 { + if err := renameRecord(ctx, db, keep); err != nil { + return fmt.Errorf("renaming seen commit record: %w", err) + } + + if len(remove) == 0 { return nil } - // write the migration (remove ) - if err := deleteRecords(ctx, db, scData); err != nil { + // Remove any older seen commits. Prior to v0.35, we kept these records for + // all heights, but v0.35 keeps only the latest. + if err := deleteRecords(ctx, db, remove); err != nil { return fmt.Errorf("writing data: %w", err) } diff --git a/scripts/scmigrate/migrate_test.go b/scripts/scmigrate/migrate_test.go index abe12584d..900a15f85 100644 --- a/scripts/scmigrate/migrate_test.go +++ b/scripts/scmigrate/migrate_test.go @@ -1,6 +1,7 @@ package scmigrate import ( + "bytes" "context" "math/rand" "testing" @@ -104,15 +105,6 @@ func TestMigrations(t *testing.T) { assertWellOrderedMigrations(t, testData) }) }) - t.Run("GetMigrationsToDelete", func(t *testing.T) { - for i := 1; i < 100; i++ { - data := appendRandomMigrations([]toMigrate{}, i) - toMigrate := getMigrationsToDelete(data) - if len(data) != len(toMigrate)+1 { - t.Fatalf("migration prep did not save one document [original=%d migrations=%d]", len(data), len(toMigrate)) - } - } - }) t.Run("InvalidMigrations", func(t *testing.T) { if _, err := makeToMigrate(nil); err == nil { t.Fatal("should error for nil migrations") @@ -156,18 +148,24 @@ func TestMigrations(t *testing.T) { // safe to rerun t.Run(test, func(t *testing.T) { if err := Migrate(ctx, db); err != nil { - t.Fatal(err) + t.Fatalf("Migration failed: %v", err) } post, err := getAllSeenCommits(ctx, db) if err != nil { - t.Fatal(err) + t.Fatalf("Fetching seen commits: %v", err) } + if len(post) != 1 { - t.Fatal("migration was not successful") + t.Fatalf("Wrong number of commits: got %d, wanted 1", len(post)) } - if post[0].commit.Height != latestHeight { - t.Fatal("migration did not save correct document") + + wantKey := makeKeyFromPrefix(prefixSeenCommit) + if !bytes.Equal(post[0].key, wantKey) { + t.Errorf("Seen commit key: got %x, want %x", post[0].key, wantKey) + } + if got := post[0].commit.Height; got != latestHeight { + t.Fatalf("Wrong commit height after migration: got %d, wanted %d", got, latestHeight) } }) }