From 9390f6f8a0ab6ab9015d855eb5ca5aabb5780c78 Mon Sep 17 00:00:00 2001 From: Gerrit91 Date: Tue, 19 Sep 2023 09:24:22 +0200 Subject: [PATCH] Another review with some improvements. --- cmd/internal/database/etcd/etcd.go | 2 +- .../database/meilisearch/meilisearch.go | 41 +++++++++------ cmd/internal/database/meilisearch/upgrade.go | 52 ++++++++++++------- cmd/internal/database/postgres/postgres.go | 2 +- cmd/internal/database/rethinkdb/rethinkdb.go | 2 +- 5 files changed, 61 insertions(+), 38 deletions(-) diff --git a/cmd/internal/database/etcd/etcd.go b/cmd/internal/database/etcd/etcd.go index 34f955e..d4e71c8 100644 --- a/cmd/internal/database/etcd/etcd.go +++ b/cmd/internal/database/etcd/etcd.go @@ -43,7 +43,7 @@ func New(log *zap.SugaredLogger, datadir, caCert, cert, key, endpoints, name str } } -// Check checks whether a backup needs to be restored or not, returns true if it needs a backup +// Check indicates whether a restore of the database is required or not. func (db *Etcd) Check(_ context.Context) (bool, error) { empty, err := utils.IsEmpty(db.datadir) if err != nil { diff --git a/cmd/internal/database/meilisearch/meilisearch.go b/cmd/internal/database/meilisearch/meilisearch.go index dafbdbb..2326b00 100644 --- a/cmd/internal/database/meilisearch/meilisearch.go +++ b/cmd/internal/database/meilisearch/meilisearch.go @@ -11,7 +11,6 @@ import ( "syscall" "time" - "github.com/avast/retry-go/v4" "github.com/meilisearch/meilisearch-go" "github.com/spf13/afero" "golang.org/x/sync/errgroup" @@ -83,10 +82,15 @@ func (db *Meilisearch) Backup(ctx context.Context) error { if err != nil { return fmt.Errorf("unable to find dump: %w", err) } - if len(dumps) == 0 { + if len(dumps) != 1 { return fmt.Errorf("did not find unique dump, found %d", len(dumps)) } + // we need to do a copy here and cannot simply rename as the file system is + // mounted by two containers. the dump is created in the database container, + // the copy is done in the backup-restore-sidecar container. os.Rename would + // lead to an error. + err = utils.Copy(afero.NewOsFs(), dumps[0], path.Join(constants.BackupDir, latestStableDump)) if err != nil { return fmt.Errorf("unable to move dump to latest: %w", err) @@ -110,7 +114,7 @@ func (db *Meilisearch) Backup(ctx context.Context) error { return nil } -// Check checks whether a backup needs to be restored or not, returns true if it needs a backup +// Check indicates whether a restore of the database is required or not. func (db *Meilisearch) Check(_ context.Context) (bool, error) { empty, err := utils.IsEmpty(db.datadir) if err != nil { @@ -159,7 +163,7 @@ func (db *Meilisearch) Recover(ctx context.Context) error { return fmt.Errorf("unable to recover %w", err) } - db.log.Infow("recovery done", "duration", time.Since(start)) + db.log.Infow("successfully restored meilisearch database", "duration", time.Since(start).String()) return nil } @@ -195,29 +199,32 @@ func (db *Meilisearch) importDump(ctx context.Context, dump string) error { return err } - db.log.Info("import of dump finished") + db.log.Info("execution of meilisearch finished without an error") return nil }) - // TODO big databases might take longer, not sure if 100 attempts are enough - // must check how long it take max with backoff ? - err = retry.Do(func() error { - restoreDB, err := New(db.log, db.datadir, "http://localhost:1", db.apikey) - if err != nil { - return fmt.Errorf("unable to create prober") - } + restoreDB, err := New(db.log, db.datadir, "http://localhost:1", db.apikey) + if err != nil { + return fmt.Errorf("unable to create prober") + } + + for { + time.Sleep(3 * time.Second) err = restoreDB.Probe(ctx) if err != nil { + db.log.Errorw("meilisearch is still restoring, continue probing for readiness...", "error", err) + return err - } + } else { + db.log.Infow("meilisearch started after importing the dump, stopping it again for takeover from the database container") - db.log.Infow("meilisearch started after importing the dump, stopping it again") + break + } + } - return cmd.Process.Signal(syscall.SIGINT) - }, retry.Attempts(100), retry.Context(ctx)) - if err != nil { + if err := cmd.Process.Signal(syscall.SIGINT); err != nil { return handleFailedRecovery(err) } diff --git a/cmd/internal/database/meilisearch/upgrade.go b/cmd/internal/database/meilisearch/upgrade.go index b336cdb..5e5aed5 100644 --- a/cmd/internal/database/meilisearch/upgrade.go +++ b/cmd/internal/database/meilisearch/upgrade.go @@ -33,10 +33,13 @@ func (db *Meilisearch) Upgrade(ctx context.Context) error { if err != nil { return err } + meilisearchVersion, err := db.getBinaryVersion(ctx) if err != nil { - return err + db.log.Errorw("unable to get binary version, skipping upgrade", "error", err) + return nil } + if dbVersion.String() == meilisearchVersion.String() { db.log.Infow("no version difference, no upgrade required", "database-version", dbVersion, "binary-version", meilisearchVersion) return nil @@ -55,29 +58,29 @@ func (db *Meilisearch) Upgrade(ctx context.Context) error { err = db.dumpWithOldBinary(ctx) if err != nil { - return err + return fmt.Errorf("unable to create dump with old meilisearch binary: %w", err) } oldVersionDataDir := strings.TrimRight(db.datadir, "/") + ".upgrade" err = os.Rename(db.datadir, oldVersionDataDir) if err != nil { - return fmt.Errorf("cannot move old version data dir out of the way: %w", err) + return fmt.Errorf("cannot move old version data dir out of the way, which could have happened due to a failed recovery attempt, consider manual cleanup: %w", err) } dump := path.Join(constants.BackupDir, latestStableDump) err = db.importDump(ctx, dump) if err != nil { - return err + return fmt.Errorf("unable to import dump with new meilisearch binary: %w", err) } err = os.RemoveAll(oldVersionDataDir) if err != nil { - return fmt.Errorf("unable cleanup old version data dir: %w", err) + db.log.Errorw("unable cleanup old version data dir, consider manual cleanup", "error", err) } - db.log.Infow("upgrade done and new data in place", "took", time.Since(start)) + db.log.Infow("meilisearch upgrade done and new data in place", "duration", time.Since(start)) return nil } @@ -136,6 +139,7 @@ func (db *Meilisearch) getDatabaseVersion(versionFile string) (*semver.Version, if err != nil { return nil, fmt.Errorf("unable to parse meilisearch binary version in %q: %w", string(versionBytes), err) } + return v, nil } @@ -183,27 +187,39 @@ func (db *Meilisearch) dumpWithOldBinary(ctx context.Context) error { return nil }) - err = retry.Do(func() error { - restoreDB, err := New(db.log, db.datadir, "http://localhost:1", db.apikey) - if err != nil { - return fmt.Errorf("unable to create prober") - } + restoreDB, err := New(db.log, db.datadir, "http://localhost:1", db.apikey) + if err != nil { + return fmt.Errorf("unable to create prober") + } - restoreDB.copyBinaryAfterBackup = false + restoreDB.copyBinaryAfterBackup = false - err = restoreDB.Backup(ctx) + err = retry.Do(func() error { + err = restoreDB.Probe(ctx) if err != nil { - return fmt.Errorf("unable to create dump from previous meilisearch version") - } + db.log.Errorw("meilisearch is still starting, continue probing for readiness...", "error", err) - db.log.Infow("taken dump from previous meilisearch version, stopping it again") + return err + } - return cmd.Process.Signal(syscall.SIGINT) + db.log.Infow("previous meilisearch started and is now ready for backup") + return nil }, retry.Context(ctx)) if err != nil { return err } + err = restoreDB.Backup(ctx) + if err != nil { + return fmt.Errorf("unable to create dump from previous meilisearch version") + } + + db.log.Infow("taken dump from previous meilisearch version, stopping it again") + + if err := cmd.Process.Signal(syscall.SIGINT); err != nil { + return err + } + err = g.Wait() if err != nil { // will probably work better in meilisearch v1.4.0: https://github.com/meilisearch/meilisearch/commit/eff8570f591fe32a6106087807e3fe8c18e8e5e4 @@ -214,7 +230,7 @@ func (db *Meilisearch) dumpWithOldBinary(ctx context.Context) error { } } - db.log.Info("successfully took dump from previous meilisearch database") + db.log.Info("successfully took dump with previous meilisearch version") return nil } diff --git a/cmd/internal/database/postgres/postgres.go b/cmd/internal/database/postgres/postgres.go index 4dc5fdf..383490d 100644 --- a/cmd/internal/database/postgres/postgres.go +++ b/cmd/internal/database/postgres/postgres.go @@ -46,7 +46,7 @@ func New(log *zap.SugaredLogger, datadir string, host string, port int, user str } } -// Check checks whether a backup needs to be restored or not, returns true if it needs a backup +// Check indicates whether a restore of the database is required or not. func (db *Postgres) Check(_ context.Context) (bool, error) { empty, err := utils.IsEmpty(db.datadir) if err != nil { diff --git a/cmd/internal/database/rethinkdb/rethinkdb.go b/cmd/internal/database/rethinkdb/rethinkdb.go index 620ade6..7928bab 100644 --- a/cmd/internal/database/rethinkdb/rethinkdb.go +++ b/cmd/internal/database/rethinkdb/rethinkdb.go @@ -55,7 +55,7 @@ func New(log *zap.SugaredLogger, datadir string, url string, passwordFile string } } -// Check checks whether a backup needs to be restored or not, returns true if it needs a backup +// Check indicates whether a restore of the database is required or not. func (db *RethinkDB) Check(_ context.Context) (bool, error) { empty, err := utils.IsEmpty(db.datadir) if err != nil {