From 0b9450e356fffd6b4ab060a169fd5fa6a3a604e0 Mon Sep 17 00:00:00 2001 From: kisfenyo Date: Sun, 14 Jun 2026 10:02:58 +0200 Subject: [PATCH] F17: per-app restore now replays the captured .sql DB dump (.sql wins) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The restore paths (RestoreFromRecoveryUnit + the RestoreApp fallback) repopulated Docker volume tars but NEVER replayed the captured -.sql dump, so DB-resident data (e.g. rows in a DB whose data dir is a bind mount) did not come back — the romm marker round-trip in the audit lost the row. New appbackup.ImportDump (read-side counterpart to DumpOne) replays a .sql/.sql.gz into the running DB using the live container's OWN discovered credentials (no env threading; reuses DiscoveredDB + getMariaDBPassword). backup.reimportDBDumps orchestrates it AFTER volume restore + stack bring-up, so the logical dump WINS over any volume-tar copy of the DB (operator-chosen precedence). pg_dump --clean --if-exists and mariadb-dump (default --add-drop-table) make replay idempotent; psql ON_ERROR_STOP=1 surfaces real import errors. Also: volume-restore per-volume failures and DB-import failures now SURFACE (the restore returns an error) instead of a swallowed WARN, so a failed data restore cannot read as success. Tests (restore_db_test.go, injectable discover/import seams): imports when dump+DB present, failure surfaces, no-dump skips discovery, dump-but-no-matching-DB is a non-fatal skip. Live DB round-trip to be validated post-deploy. --- controller/internal/appbackup/dbdump.go | 108 +++++++++++++++ .../internal/backup/appbackup_bridge.go | 5 + controller/internal/backup/backup.go | 5 + controller/internal/backup/restore.go | 31 ++++- controller/internal/backup/restore_db.go | 90 +++++++++++++ controller/internal/backup/restore_db_test.go | 125 ++++++++++++++++++ controller/internal/backup/restore_unit.go | 16 ++- 7 files changed, 376 insertions(+), 4 deletions(-) create mode 100644 controller/internal/backup/restore_db.go create mode 100644 controller/internal/backup/restore_db_test.go diff --git a/controller/internal/appbackup/dbdump.go b/controller/internal/appbackup/dbdump.go index b96eed2..c2ad46a 100644 --- a/controller/internal/appbackup/dbdump.go +++ b/controller/internal/appbackup/dbdump.go @@ -2,6 +2,7 @@ package appbackup import ( "bufio" + "compress/gzip" "context" "fmt" "io" @@ -513,6 +514,113 @@ func populateDBEnv(ctx context.Context, db *DiscoveredDB) error { return nil } +// ImportDump replays a (possibly gzipped) SQL dump into a RUNNING database container — the read-side +// counterpart to DumpOne (F17). It reuses the per-engine clients (psql / mariadb) and the DiscoveredDB's +// OWN credentials (discovered from the live container env), so the caller needs no external env map. The +// container must already be running (the restore flow brings the stack up first); ImportDump briefly +// waits for the engine to accept connections, then pipes the dump in. The backup dumps are produced with +// DROP/CREATE (pg_dump --clean --if-exists; mariadb-dump's default --add-drop-table), so a replay fully +// reconstructs the captured logical state. +func ImportDump(ctx context.Context, db DiscoveredDB, dumpPath string, logger *log.Logger, debug bool) error { + if err := waitDBReady(ctx, db, 30*time.Second); err != nil { + return fmt.Errorf("waiting for %s (%s) readiness: %w", db.ContainerName, db.DBType, err) + } + + f, err := os.Open(dumpPath) + if err != nil { + return fmt.Errorf("opening dump %s: %w", dumpPath, err) + } + defer f.Close() + + var reader io.Reader = f + if strings.HasSuffix(dumpPath, ".gz") { + gr, err := gzip.NewReader(f) + if err != nil { + return fmt.Errorf("opening gzip %s: %w", dumpPath, err) + } + defer gr.Close() + reader = gr + } + + impCtx, cancel := context.WithTimeout(ctx, 30*time.Minute) + defer cancel() + + var cmd *exec.Cmd + switch db.DBType { + case DBTypePostgres: + user := db.DBUser + if user == "" { + user = "postgres" + } + dbName := db.DBName + if dbName == "" { + dbName = user + } + // ON_ERROR_STOP=1: a real import error must FAIL (and surface), not silently half-apply. + cmd = exec.CommandContext(impCtx, "docker", "exec", "-i", db.ContainerID, + "psql", "-v", "ON_ERROR_STOP=1", "-U", user, "-d", dbName) + case DBTypeMariaDB: + password := getMariaDBPassword(impCtx, db.ContainerID) + if password == "" { + return fmt.Errorf("could not determine MariaDB root password for %s", db.ContainerName) + } + cmd = exec.CommandContext(impCtx, "docker", "exec", "-i", db.ContainerID, + "mariadb", "-u", "root", "-p"+password, db.DBName) + default: + return fmt.Errorf("unsupported DB type: %s", db.DBType) + } + + cmd.Stdin = reader + var stderr strings.Builder + cmd.Stderr = &stderr + if debug && logger != nil { + logger.Printf("[DEBUG] [backup] ImportDump: importing %s into %s (%s)", dumpPath, db.ContainerName, db.DBType) + } + if err := cmd.Run(); err != nil { + msg := strings.TrimSpace(stderr.String()) + if len(msg) > 300 { + msg = msg[:300] + } + return fmt.Errorf("%s import into %s failed: %s — %w", db.DBType, db.ContainerName, msg, err) + } + if logger != nil { + logger.Printf("[INFO] [backup] Imported DB dump %s into %s (%s)", filepath.Base(dumpPath), db.ContainerName, db.DBType) + } + return nil +} + +// waitDBReady polls until the database accepts connections (pg_isready / mariadb-admin ping). +func waitDBReady(ctx context.Context, db DiscoveredDB, timeout time.Duration) error { + deadline := time.Now().Add(timeout) + for { + c, cancel := context.WithTimeout(ctx, 5*time.Second) + var cmd *exec.Cmd + switch db.DBType { + case DBTypePostgres: + user := db.DBUser + if user == "" { + user = "postgres" + } + cmd = exec.CommandContext(c, "docker", "exec", db.ContainerID, "pg_isready", "-U", user) + case DBTypeMariaDB: + pw := getMariaDBPassword(c, db.ContainerID) + cmd = exec.CommandContext(c, "docker", "exec", db.ContainerID, "mariadb-admin", "ping", "-u", "root", "-p"+pw) + default: + cancel() + return fmt.Errorf("unsupported DB type: %s", db.DBType) + } + err := cmd.Run() + cancel() + if err == nil { + return nil + } + if time.Now().After(deadline) { + return fmt.Errorf("timeout after %s", timeout) + } + time.Sleep(2 * time.Second) + } +} + func getMariaDBPassword(ctx context.Context, containerID string) string { cmd := exec.CommandContext(ctx, "docker", "inspect", containerID, "--format", "{{range .Config.Env}}{{println .}}{{end}}") diff --git a/controller/internal/backup/appbackup_bridge.go b/controller/internal/backup/appbackup_bridge.go index c5e3243..a919b83 100644 --- a/controller/internal/backup/appbackup_bridge.go +++ b/controller/internal/backup/appbackup_bridge.go @@ -59,6 +59,11 @@ func DumpOne(ctx context.Context, db DiscoveredDB, dumpDir string, logger *log.L return appbackup.DumpOne(ctx, db, dumpDir, logger, debug) } +// ImportDump replays a captured .sql dump back into a running DB container (F17 restore path). +func ImportDump(ctx context.Context, db DiscoveredDB, dumpPath string, logger *log.Logger, debug bool) error { + return appbackup.ImportDump(ctx, db, dumpPath, logger, debug) +} + func ValidateDump(filePath string, dbType DBType) DumpValidation { return appbackup.ValidateDump(filePath, dbType) } diff --git a/controller/internal/backup/backup.go b/controller/internal/backup/backup.go index 4f89312..a491918 100644 --- a/controller/internal/backup/backup.go +++ b/controller/internal/backup/backup.go @@ -31,6 +31,11 @@ type Manager struct { // tier2Notify, if set, is called after each Tier 2 copy (success: err==nil) for notifications. tier2Notify func(stackName, destLabel string, dur time.Duration, err error) + // F17 restore seams — overridable in tests so the .sql re-import orchestration can be unit-tested + // without Docker. Default to the real DiscoverDatabases / ImportDump (lazy-init in reimportDBDumps). + discoverDBs func(ctx context.Context) ([]DiscoveredDB, error) + importDBDump func(ctx context.Context, db DiscoveredDB, dumpPath string) error + mu sync.Mutex lastDBDump *DBDumpStatus running bool diff --git a/controller/internal/backup/restore.go b/controller/internal/backup/restore.go index 28ad564..eec105d 100644 --- a/controller/internal/backup/restore.go +++ b/controller/internal/backup/restore.go @@ -56,12 +56,17 @@ func (m *Manager) RestoreApp(stackName, snapshotID string) error { m.logger.Printf("[WARN] RESTORE could not stop %s: %v (proceeding anyway)", stackName, err) } + // F17: surface a data-restore failure instead of swallowing it. We still bring the app back up so it + // isn't left dead, but the error is returned at the end so a failed restore can't read as success. + var dataErr error + // Populate Docker volumes from restored tars if m.isDebug() { m.logger.Printf("[DEBUG] RestoreApp: step 2/3 — restoring Docker volumes for %s", stackName) } if err := m.restoreDockerVolumes(stackName, drivePath); err != nil { - m.logger.Printf("[WARN] RESTORE volume restore failed for %s: %v (continuing)", stackName, err) + m.logger.Printf("[ERROR] RESTORE volume restore failed for %s: %v", stackName, err) + dataErr = err } // Restart the app @@ -72,11 +77,23 @@ func (m *Manager) RestoreApp(stackName, snapshotID string) error { m.logger.Printf("[WARN] RESTORE could not restart %s after restore: %v", stackName, err) } + // F17: replay the captured .sql dump into the now-running DB (the legacy path never did this, so + // DB-resident data did not come back). Runs after volume restore so the dump WINS over any tar copy. + if _, err := m.reimportDBDumpsCtx(stackName, m.namespaceRoot(drivePath)); err != nil { + m.logger.Printf("[ERROR] RESTORE DB re-import failed for %s: %v", stackName, err) + if dataErr == nil { + dataErr = err + } + } + // Verify app started successfully if err := m.waitForHealthy(stackName, 90*time.Second); err != nil { m.logger.Printf("[WARN] [backup] Restore completed but app health check failed: %v", err) } + if dataErr != nil { + return fmt.Errorf("restore of %s completed with data errors: %w", stackName, dataErr) + } m.logger.Printf("[INFO] RESTORE completed: stack=%s", stackName) return nil } @@ -93,6 +110,7 @@ func (m *Manager) restoreDockerVolumes(stackName, drivePath string) error { } var restored int + var failed []string for _, entry := range entries { if entry.IsDir() || !strings.HasSuffix(entry.Name(), ".tar") { continue @@ -106,7 +124,8 @@ func (m *Manager) restoreDockerVolumes(stackName, drivePath string) error { // Create fresh volume if out, err := exec.Command("docker", "volume", "create", volName).CombinedOutput(); err != nil { - m.logger.Printf("[WARN] [backup] Failed to create volume %s: %s — %v", volName, strings.TrimSpace(string(out)), err) + m.logger.Printf("[ERROR] [backup] Failed to create volume %s: %s — %v", volName, strings.TrimSpace(string(out)), err) + failed = append(failed, volName) continue } @@ -120,7 +139,8 @@ func (m *Manager) restoreDockerVolumes(stackName, drivePath string) error { cancel() if err != nil { - m.logger.Printf("[WARN] [backup] Failed to populate volume %s: %s — %v", volName, strings.TrimSpace(string(out)), err) + m.logger.Printf("[ERROR] [backup] Failed to populate volume %s: %s — %v", volName, strings.TrimSpace(string(out)), err) + failed = append(failed, volName) continue } @@ -133,6 +153,11 @@ func (m *Manager) restoreDockerVolumes(stackName, drivePath string) error { if restored > 0 { m.logger.Printf("[INFO] [backup] Restored %d Docker volume(s) for %s", restored, stackName) } + // F17: a per-volume failure used to be a swallowed WARN; surface it so the restore is reported as + // failed rather than silently partial. + if len(failed) > 0 { + return fmt.Errorf("failed to restore %d volume(s): %v", len(failed), failed) + } return nil } diff --git a/controller/internal/backup/restore_db.go b/controller/internal/backup/restore_db.go new file mode 100644 index 0000000..6d8ad06 --- /dev/null +++ b/controller/internal/backup/restore_db.go @@ -0,0 +1,90 @@ +package backup + +import ( + "context" + "fmt" + "os" + "path/filepath" + "time" +) + +// reimportDBDumps replays the captured per-app .sql dumps back into the app's now-running database +// container(s) — the F17 fix. The per-app backup captures a logical SQL dump (DumpOne → +// -.sql) but the legacy restore only repopulated Docker volume tars and NEVER replayed +// the dump, so DB-resident data (e.g. rows in a DB whose data dir is a bind mount, not a named volume) +// did not come back. This runs AFTER volume restore + stack bring-up, so the dump WINS over any +// volume-tar copy of the DB (the operator-chosen precedence: the consistent logical dump is authoritative). +// +// It uses the live container's OWN discovered credentials (DiscoveredDB), so no env threading is needed. +// A dump whose DB container is not found is logged and skipped; an actual import FAILURE is returned +// (surfaced, not swallowed) so a failed data restore cannot read as success. +func (m *Manager) reimportDBDumps(ctx context.Context, stackName, nsRoot string) (int, error) { + dumpDir := AppDBDumpPath(nsRoot, stackName) + entries, err := os.ReadDir(dumpDir) + if err != nil { + if os.IsNotExist(err) { + return 0, nil // no DB dumps for this app + } + return 0, fmt.Errorf("reading db-dump dir: %w", err) + } + hasDump := false + for _, e := range entries { + if !e.IsDir() && filepath.Ext(e.Name()) == ".sql" { + hasDump = true + break + } + } + if !hasDump { + return 0, nil + } + + discover := m.discoverDBs + if discover == nil { + discover = func(ctx context.Context) ([]DiscoveredDB, error) { + return DiscoverDatabases(ctx, m.logger, m.isDebug()) + } + } + imp := m.importDBDump + if imp == nil { + imp = func(ctx context.Context, db DiscoveredDB, dumpPath string) error { + return ImportDump(ctx, db, dumpPath, m.logger, m.isDebug()) + } + } + + dbs, err := discover(ctx) + if err != nil { + return 0, fmt.Errorf("discovering DB containers for %s: %w", stackName, err) + } + + var imported int + for _, db := range dbs { + if db.StackName != stackName { + continue + } + // The dump for this DB is named "-.sql" (see DumpOne). + dumpPath := filepath.Join(dumpDir, fmt.Sprintf("%s-%s.sql", stackName, db.DBType)) + if _, statErr := os.Stat(dumpPath); statErr != nil { + continue // no dump for this particular DB engine + } + m.logger.Printf("[INFO] [backup] Restore %s: replaying DB dump into %s (%s)", stackName, db.ContainerName, db.DBType) + if err := imp(ctx, db, dumpPath); err != nil { + return imported, fmt.Errorf("importing %s dump for %s: %w", db.DBType, stackName, err) + } + imported++ + } + + if imported == 0 { + m.logger.Printf("[WARN] [backup] Restore %s: a .sql dump exists but no matching running DB container was found — DB content NOT restored", stackName) + } else { + m.logger.Printf("[INFO] [backup] Restore %s: replayed %d DB dump(s)", stackName, imported) + } + return imported, nil +} + +// reimportDBDumpsCtx is a small helper that runs reimportDBDumps with a bounded context so a stuck DB +// import cannot hang the restore indefinitely. +func (m *Manager) reimportDBDumpsCtx(stackName, nsRoot string) (int, error) { + ctx, cancel := context.WithTimeout(context.Background(), 35*time.Minute) + defer cancel() + return m.reimportDBDumps(ctx, stackName, nsRoot) +} diff --git a/controller/internal/backup/restore_db_test.go b/controller/internal/backup/restore_db_test.go new file mode 100644 index 0000000..569a4ec --- /dev/null +++ b/controller/internal/backup/restore_db_test.go @@ -0,0 +1,125 @@ +package backup + +import ( + "context" + "fmt" + "io" + "log" + "os" + "path/filepath" + "strings" + "testing" +) + +func newReimportTestManager() *Manager { + return &Manager{logger: log.New(io.Discard, "", 0)} +} + +func writeDump(t *testing.T, nsRoot, stack string, dbType DBType) string { + t.Helper() + dir := AppDBDumpPath(nsRoot, stack) + if err := os.MkdirAll(dir, 0o755); err != nil { + t.Fatal(err) + } + p := filepath.Join(dir, fmt.Sprintf("%s-%s.sql", stack, dbType)) + if err := os.WriteFile(p, []byte("-- dump\nDROP TABLE IF EXISTS t;\n"), 0o644); err != nil { + t.Fatal(err) + } + return p +} + +// TestReimportDBDumps_ImportsWhenDumpAndDBPresent asserts F17: when a captured .sql dump exists and a +// matching running DB is discovered, reimportDBDumps replays it. The PRE-FIX restore never called any +// import — this orchestration is the fix. +func TestReimportDBDumps_ImportsWhenDumpAndDBPresent(t *testing.T) { + nsRoot := t.TempDir() + wantPath := writeDump(t, nsRoot, "app", DBTypeMariaDB) + + m := newReimportTestManager() + m.discoverDBs = func(ctx context.Context) ([]DiscoveredDB, error) { + return []DiscoveredDB{ + {StackName: "other", DBType: DBTypePostgres, ContainerName: "other-db"}, + {StackName: "app", DBType: DBTypeMariaDB, ContainerName: "app-db", ContainerID: "cid"}, + }, nil + } + var gotPath string + var gotDB DiscoveredDB + m.importDBDump = func(ctx context.Context, db DiscoveredDB, dumpPath string) error { + gotPath, gotDB = dumpPath, db + return nil + } + + n, err := m.reimportDBDumps(context.Background(), "app", nsRoot) + if err != nil { + t.Fatalf("reimportDBDumps: %v", err) + } + if n != 1 { + t.Fatalf("imported = %d, want 1", n) + } + if gotPath != wantPath { + t.Fatalf("imported path = %q, want %q", gotPath, wantPath) + } + if gotDB.ContainerName != "app-db" { + t.Fatalf("imported into %q, want app-db (must match the stack's own DB)", gotDB.ContainerName) + } +} + +// TestReimportDBDumps_FailureSurfaces asserts an import failure is RETURNED, not swallowed (a failed +// data restore must not read as success). +func TestReimportDBDumps_FailureSurfaces(t *testing.T) { + nsRoot := t.TempDir() + writeDump(t, nsRoot, "app", DBTypeMariaDB) + + m := newReimportTestManager() + m.discoverDBs = func(ctx context.Context) ([]DiscoveredDB, error) { + return []DiscoveredDB{{StackName: "app", DBType: DBTypeMariaDB, ContainerName: "app-db"}}, nil + } + m.importDBDump = func(ctx context.Context, db DiscoveredDB, dumpPath string) error { + return fmt.Errorf("boom") + } + + if _, err := m.reimportDBDumps(context.Background(), "app", nsRoot); err == nil || !strings.Contains(err.Error(), "boom") { + t.Fatalf("expected the import failure to surface, got %v", err) + } +} + +// TestReimportDBDumps_NoDumpNoImport asserts apps with no .sql dump never trigger discovery/import. +func TestReimportDBDumps_NoDumpNoImport(t *testing.T) { + nsRoot := t.TempDir() + m := newReimportTestManager() + discoverCalled := false + m.discoverDBs = func(ctx context.Context) ([]DiscoveredDB, error) { + discoverCalled = true + return nil, nil + } + m.importDBDump = func(ctx context.Context, db DiscoveredDB, dumpPath string) error { + t.Fatal("importDBDump must not be called when there is no dump") + return nil + } + n, err := m.reimportDBDumps(context.Background(), "app", nsRoot) + if err != nil || n != 0 { + t.Fatalf("reimportDBDumps with no dump = (%d, %v), want (0, nil)", n, err) + } + if discoverCalled { + t.Fatalf("discovery should be skipped when there is no .sql dump") + } +} + +// TestReimportDBDumps_DumpButNoMatchingDB asserts that a dump with no matching running DB container is a +// non-fatal skip (logged), returning 0 imported and no error — the app is up, just no DB matched. +func TestReimportDBDumps_DumpButNoMatchingDB(t *testing.T) { + nsRoot := t.TempDir() + writeDump(t, nsRoot, "app", DBTypeMariaDB) + m := newReimportTestManager() + m.discoverDBs = func(ctx context.Context) ([]DiscoveredDB, error) { + return []DiscoveredDB{{StackName: "different", DBType: DBTypeMariaDB}}, nil + } + m.importDBDump = func(ctx context.Context, db DiscoveredDB, dumpPath string) error { + t.Fatal("must not import when no DB matches the stack") + return nil + } + n, err := m.reimportDBDumps(context.Background(), "app", nsRoot) + if err != nil || n != 0 { + t.Fatalf("= (%d, %v), want (0, nil)", n, err) + } +} diff --git a/controller/internal/backup/restore_unit.go b/controller/internal/backup/restore_unit.go index 5a69eb9..4381bef 100644 --- a/controller/internal/backup/restore_unit.go +++ b/controller/internal/backup/restore_unit.go @@ -122,19 +122,33 @@ func (m *Manager) RestoreFromRecoveryUnit(stackName string) error { stackName, len(manifest.ImagePins), len(manifest.SecretEnvVars)-len(missing), len(manifest.SecretEnvVars), len(manifest.DataKeyEnvVars)) // Stop, restore named-volume data, then recreate the definition + redeploy with the recovered env. + // F17: surface a data-restore failure instead of swallowing it (we still bring the app back up). + var dataErr error if err := m.stackProvider.StopStack(stackName); err != nil { m.logger.Printf("[WARN] [backup] could not stop %s before restore: %v (continuing)", stackName, err) } if err := m.restoreDockerVolumes(stackName, drivePath); err != nil { - m.logger.Printf("[WARN] [backup] volume restore for %s: %v (continuing)", stackName, err) + m.logger.Printf("[ERROR] [backup] volume restore for %s: %v", stackName, err) + dataErr = err } if err := m.stackProvider.RecreateStackFromUnit(stackName, composeDir, fullEnv); err != nil { return fmt.Errorf("recreating %s from unit: %w", stackName, err) } + // F17: the captured .sql dump is the authoritative logical DB state — replay it into the now-running + // DB container AFTER the volume restore, so the dump WINS over any volume-tar copy of the database. + if _, err := m.reimportDBDumpsCtx(stackName, nsRoot); err != nil { + m.logger.Printf("[ERROR] [backup] DB re-import for %s: %v", stackName, err) + if dataErr == nil { + dataErr = err + } + } if err := m.waitForHealthy(stackName, 90*time.Second); err != nil { m.logger.Printf("[WARN] [backup] %s restored but health check failed: %v", stackName, err) } + if dataErr != nil { + return fmt.Errorf("restore of %s from unit completed with data errors: %w", stackName, dataErr) + } m.logger.Printf("[INFO] [backup] Restore-from-unit completed: %s", stackName) return nil }