From 73378a812c8ad38645fb34aedf35c775d850fa03 Mon Sep 17 00:00:00 2001 From: kisfenyo Date: Sun, 5 Jul 2026 11:46:37 +0200 Subject: [PATCH] =?UTF-8?q?fix(backup):=20F3=20=E2=80=94=20wire=20named-vo?= =?UTF-8?q?lume=20dumps=20into=20the=20app-data=20backup=20run?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit DumpAppVolumesSafe had NO production caller: no trigger ever produced volume-dumps/, so named-volume app data (e.g. nextcloud's html volume) was never captured into the recovery unit and the granular restore silently restored nothing for class-B data (drill finding F3). - runVolumeDumps: per-stack loop in runDBDumpsInternal, BEFORE captureAllRecoveryUnits (so manifests enumerate the fresh tars). Gate order is load-bearing: protected-stack and volume-check gates precede DumpAppVolumesSafe (which stops the stack before its own check — unconditional calls would bounce every volume-less app nightly). Disconnected/decommissioned drives skip with the same summary style as the DB loop. - No silent partials: a per-stack failure lands as a FAIL summary entry, flips Success, and fails the run ("some backup steps failed: ..."), without aborting the other stacks. - Zero-DB early return removed: volume-bearing apps without a database still get their class-B dump + unit refresh. - dumpVolumesSafe seam (same style as the F17 discoverDBs/importDBDump seams) so the gating is unit-tested without Docker. Companion red-proof: neutering the volume gate fails TestRunVolumeDumps_GatesPrecedeDump (dump fired for the volume-less stack) and _VolumelessNeverStopped (verified, reverted). Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01PSK5g6qYLknKj8u3QAFEr6 --- controller/internal/backup/backup.go | 104 ++++++++-- .../internal/backup/volume_dumps_test.go | 193 ++++++++++++++++++ 2 files changed, 282 insertions(+), 15 deletions(-) create mode 100644 controller/internal/backup/volume_dumps_test.go diff --git a/controller/internal/backup/backup.go b/controller/internal/backup/backup.go index 4796943..638db26 100644 --- a/controller/internal/backup/backup.go +++ b/controller/internal/backup/backup.go @@ -40,6 +40,10 @@ type Manager struct { discoverDBs func(ctx context.Context) ([]DiscoveredDB, error) importDBDump func(ctx context.Context, db DiscoveredDB, dumpPath string) error + // F3 volume-dump seam — overridable in tests so runVolumeDumps' gating (protected / volume-less / + // disconnected) can be unit-tested without Docker. Nil → the real DumpAppVolumesSafe. + dumpVolumesSafe func(stackName string) error + // migrationRunning, if set, reports whether a data migration is in progress. The scheduled // backup paths skip when it returns true (Change 3 — backup ↔ migration mutual exclusion), so a // nightly dump/Tier-2 can't race a migration copy/cleanup on the same drive. @@ -202,20 +206,14 @@ func (m *Manager) runDBDumpsInternal(ctx context.Context) error { return err } + // F3: no early return on zero DBs — volume-bearing apps without a database still need their + // class-B volume dump + recovery-unit refresh below (the DB loop simply has no iterations). if len(dbs) == 0 { m.logger.Printf("[INFO] [backup] No database containers found") - m.mu.Lock() - m.lastDBDump = &DBDumpStatus{ - LastRun: time.Now(), - Success: true, - Duration: time.Since(start), - } - m.mu.Unlock() - return nil + } else { + m.logger.Printf("[INFO] [backup] Discovered %d database(s): %s", len(dbs), dbNames(dbs)) } - m.logger.Printf("[INFO] [backup] Discovered %d database(s): %s", len(dbs), dbNames(dbs)) - // Dump each DB to its app's drive path var results []DumpResult allOK := true @@ -270,6 +268,13 @@ func (m *Manager) runDBDumpsInternal(ctx context.Context) error { } } + // F3: class-B leg — dump each app's named-volume data (stop → tar → restart). MUST run before + // captureAllRecoveryUnits so the manifests enumerate the fresh tars into VolumeDumps. + dbOK := allOK + volSummary, volDumped, volOK := m.runVolumeDumps() + summary = append(summary, volSummary...) + allOK = dbOK && volOK + duration := time.Since(start) m.mu.Lock() m.lastDBDump = &DBDumpStatus{ @@ -281,22 +286,91 @@ func (m *Manager) runDBDumpsInternal(ctx context.Context) error { m.mu.Unlock() if allOK { - m.logger.Printf("[INFO] [backup] DB dump completed: %d databases, %s total (%s)", - len(results), humanizeBytes(totalSize), duration.Round(time.Millisecond)) + m.logger.Printf("[INFO] [backup] App-data backup completed: %d databases (%s total), %d volume dump(s) (%s)", + len(results), humanizeBytes(totalSize), volDumped, duration.Round(time.Millisecond)) } else { - // Still refresh recovery units below — a partial DB failure shouldn't leave units stale. - m.logger.Printf("[WARN] [backup] some database dumps failed; refreshing recovery units anyway") + // Still refresh recovery units below — a partial failure shouldn't leave units stale. + m.logger.Printf("[WARN] [backup] some backup steps failed (%s); refreshing recovery units anyway", + strings.Join(failedSummaryLines(summary), "; ")) } // Phase 2: refresh each deployed app's self-contained recovery unit (compose + manifest). m.captureAllRecoveryUnits() + // No silent partials: a DB-dump or volume-dump failure fails the whole run. if !allOK { - return fmt.Errorf("some database dumps failed") + return fmt.Errorf("some backup steps failed: %s", strings.Join(failedSummaryLines(summary), "; ")) } return nil } +// failedSummaryLines filters a run summary down to its FAIL entries (for logs/errors). +func failedSummaryLines(summary []string) []string { + var failed []string + for _, s := range summary { + if strings.HasPrefix(s, "FAIL ") { + failed = append(failed, s) + } + } + return failed +} + +// runVolumeDumps exports the Docker named-volume data of every deployed, unprotected stack whose +// drive is writable — the class-B leg of the nightly app-data backup. (F3: DumpAppVolumesSafe +// previously had NO production caller, so volume-dumps/ was never produced and the granular +// restore had nothing to restore for named-volume apps.) Caller must hold the running flag. +// +// Gate ORDER is load-bearing: the volume check precedes DumpAppVolumesSafe, because the Safe +// variant stops the stack before its own volume check — calling it unconditionally would bounce +// every volume-less app on every nightly run. Per-stack isolation mirrors the DB loop: one app's +// failure is recorded and does not abort the others. +func (m *Manager) runVolumeDumps() (summary []string, dumped int, allOK bool) { + allOK = true + if m.stackProvider == nil { + return nil, 0, true + } + dump := m.dumpVolumesSafe + if dump == nil { + dump = m.DumpAppVolumesSafe + } + + for _, stack := range m.stackProvider.ListDeployedStacks() { + // Never stop/dump infra stacks (felhom-controller, traefik, cloudflared). + if m.cfg != nil && m.cfg.IsProtectedStack(stack.Name) { + continue + } + // Volume check FIRST — a volume-less stack must not be stopped at all (see gate-order note). + if len(m.stackProvider.GetDockerVolumes(stack.Name)) == 0 { + if m.isDebug() { + m.logger.Printf("[DEBUG] [backup] %s has no named volumes — volume dump skipped", stack.Name) + } + continue + } + // Same drive-state skip guards as the DB-dump loop. + drivePath := m.GetAppDrivePath(stack.Name) + if m.settings != nil && m.settings.IsDisconnected(drivePath) { + m.logger.Printf("[WARN] [backup] Skipping volume dump for %s — drive disconnected: %s", stack.Name, drivePath) + summary = append(summary, fmt.Sprintf("SKIP %s volumes (drive disconnected)", stack.Name)) + continue + } + if m.settings != nil && m.settings.IsDecommissioned(drivePath) { + m.logger.Printf("[WARN] [backup] Skipping volume dump for %s — drive decommissioned: %s", stack.Name, drivePath) + summary = append(summary, fmt.Sprintf("SKIP %s volumes (drive decommissioned)", stack.Name)) + continue + } + + if err := dump(stack.Name); err != nil { + allOK = false + summary = append(summary, fmt.Sprintf("FAIL %s volumes: %v", stack.Name, err)) + m.logger.Printf("[ERROR] [backup] Volume dump failed for %s: %v", stack.Name, err) + continue + } + dumped++ + summary = append(summary, fmt.Sprintf("OK %s volumes", stack.Name)) + } + return summary, dumped, allOK +} + // DumpAppVolumes exports Docker named volumes to tar files for the given stack. // Tars are written to AppVolumeDumpPath(drivePath, stackName)/. // Uses "docker run alpine tar" (same pattern as appexport). diff --git a/controller/internal/backup/volume_dumps_test.go b/controller/internal/backup/volume_dumps_test.go new file mode 100644 index 0000000..0b5a66a --- /dev/null +++ b/controller/internal/backup/volume_dumps_test.go @@ -0,0 +1,193 @@ +package backup + +import ( + "io" + "log" + "path/filepath" + "strings" + "testing" + + "gitea.dooplex.hu/admin/felhom-controller/internal/config" + "gitea.dooplex.hu/admin/felhom-controller/internal/settings" +) + +// volDumpFakeProvider is a StackDataProvider for the runVolumeDumps gating tests: a configurable +// stack list with per-stack volumes + drive, and a StopStack recorder (the destructive act the +// gates must prevent for volume-less/protected stacks). +type volDumpFakeProvider struct { + stacks []StackSummary + volumes map[string][]string + hdd map[string]string + stopped []string +} + +func (f *volDumpFakeProvider) GetStackComposePath(string) (string, bool) { return "", false } +func (f *volDumpFakeProvider) ListDeployedStacks() []StackSummary { return f.stacks } +func (f *volDumpFakeProvider) GetStackHDDMounts(string) []string { return nil } +func (f *volDumpFakeProvider) GetStackHDDPath(name string) string { return f.hdd[name] } +func (f *volDumpFakeProvider) GetDockerVolumes(name string) []string { return f.volumes[name] } +func (f *volDumpFakeProvider) StopStack(name string) error { + f.stopped = append(f.stopped, name) + return nil +} +func (f *volDumpFakeProvider) StartStack(string) error { return nil } +func (f *volDumpFakeProvider) RefreshAndIsRunning(string) bool { return true } +func (f *volDumpFakeProvider) GetStackRecoveryInfo(string) (RecoveryInfo, bool) { + return RecoveryInfo{}, false +} +func (f *volDumpFakeProvider) RecoverStackSecrets(string, []string) map[string]string { return nil } +func (f *volDumpFakeProvider) RecreateStackFromUnit(string, string, map[string]string) error { + return nil +} + +// TestRunVolumeDumps_GatesPrecedeDump proves Scenario D/E's gating: the dump is invoked ONLY for +// a volume-bearing, unprotected stack on a writable drive. The negatives are the point — +// volume-less (rallly-like), protected (traefik-like), and disconnected-drive stacks are never +// dumped (and therefore never stopped, since stopping happens inside DumpAppVolumesSafe). +// COMPANION red-proof: removing the volume gate makes the seam fire for "rallly" → this fails. +func TestRunVolumeDumps_GatesPrecedeDump(t *testing.T) { + tmp := t.TempDir() + usbDrive := filepath.Join(tmp, "usb") + badDrive := filepath.Join(tmp, "gone") + + sett, err := settings.Load(filepath.Join(tmp, "settings.json"), log.New(io.Discard, "", 0)) + if err != nil { + t.Fatal(err) + } + if err := sett.AddStoragePath(settings.StoragePath{Path: badDrive, Label: "gone"}); err != nil { + t.Fatal(err) + } + if err := sett.SetDisconnected(badDrive, true, nil); err != nil { + t.Fatal(err) + } + + cfg := &config.Config{} + cfg.Paths.SystemDataPath = filepath.Join(tmp, "sys") + cfg.Stacks.Protected = []string{"traefik"} + + fake := &volDumpFakeProvider{ + stacks: []StackSummary{ + {Name: "nextcloud"}, {Name: "rallly"}, {Name: "traefik"}, {Name: "diskapp"}, + }, + volumes: map[string][]string{ + "nextcloud": {"nextcloud_nextcloud_html"}, + "rallly": nil, // volume-less — must NOT be stopped/dumped + "traefik": {"traefik_data"}, // protected — never considered + "diskapp": {"diskapp_data"}, // volume-bearing but drive disconnected + }, + hdd: map[string]string{"nextcloud": usbDrive, "diskapp": badDrive}, + } + + m := &Manager{cfg: cfg, settings: sett, logger: log.New(io.Discard, "", 0), + systemDataPath: cfg.Paths.SystemDataPath, stackProvider: fake} + var dumpCalls []string + m.dumpVolumesSafe = func(name string) error { + dumpCalls = append(dumpCalls, name) + return nil + } + + summary, dumped, ok := m.runVolumeDumps() + if !ok { + t.Fatalf("run should be ok, summary=%v", summary) + } + if len(dumpCalls) != 1 || dumpCalls[0] != "nextcloud" { + t.Errorf("dump invoked for %v, want exactly [nextcloud] (gates must exclude volume-less/protected/disconnected)", dumpCalls) + } + if dumped != 1 { + t.Errorf("dumped = %d, want 1", dumped) + } + if len(fake.stopped) != 0 { + t.Errorf("StopStack called for %v — the seam bypasses the real dump, so ANY stop means a gate leaked", fake.stopped) + } + // The disconnected drive appears as a SKIP in the summary (same style as the DB loop). + if !containsSummary(summary, "SKIP diskapp volumes (drive disconnected)") { + t.Errorf("summary missing disconnected SKIP entry: %v", summary) + } +} + +// TestRunVolumeDumps_VolumelessNeverStopped drives the REAL DumpAppVolumesSafe path (no seam) with +// only volume-less/protected stacks: the volume gate must keep them from ever being stopped. This +// is the direct Scenario D negative — without the gate, DumpAppVolumesSafe stops the stack BEFORE +// its own volume check, so this test fails with stopped=[rallly]. +func TestRunVolumeDumps_VolumelessNeverStopped(t *testing.T) { + cfg := &config.Config{} + cfg.Paths.SystemDataPath = filepath.Join(t.TempDir(), "sys") + cfg.Stacks.Protected = []string{"traefik"} + + fake := &volDumpFakeProvider{ + stacks: []StackSummary{{Name: "rallly"}, {Name: "traefik"}}, + volumes: map[string][]string{"traefik": {"traefik_data"}}, + } + m := &Manager{cfg: cfg, logger: log.New(io.Discard, "", 0), + systemDataPath: cfg.Paths.SystemDataPath, stackProvider: fake} + // deliberately NO seam: the real DumpAppVolumesSafe would record StopStack on the fake. + + if _, dumped, ok := m.runVolumeDumps(); !ok || dumped != 0 { + t.Fatalf("expected clean zero-dump run, dumped=%d ok=%v", dumped, ok) + } + if len(fake.stopped) != 0 { + t.Errorf("volume-less/protected stacks were stopped: %v", fake.stopped) + } +} + +// TestRunVolumeDumps_FailureSurfaces proves no-silent-partial: a per-stack dump failure lands in +// the summary as a FAIL entry, flips allOK, and does NOT abort the remaining stacks. +func TestRunVolumeDumps_FailureSurfaces(t *testing.T) { + cfg := &config.Config{} + cfg.Paths.SystemDataPath = filepath.Join(t.TempDir(), "sys") + + fake := &volDumpFakeProvider{ + stacks: []StackSummary{{Name: "broken"}, {Name: "healthy"}}, + volumes: map[string][]string{ + "broken": {"broken_data"}, + "healthy": {"healthy_data"}, + }, + } + m := &Manager{cfg: cfg, logger: log.New(io.Discard, "", 0), + systemDataPath: cfg.Paths.SystemDataPath, stackProvider: fake} + m.dumpVolumesSafe = func(name string) error { + if name == "broken" { + return errTest + } + return nil + } + + summary, dumped, ok := m.runVolumeDumps() + if ok { + t.Error("allOK must be false after a dump failure") + } + if dumped != 1 { + t.Errorf("the healthy stack must still be dumped after another's failure (dumped=%d)", dumped) + } + if !containsSummaryPrefix(summary, "FAIL broken volumes:") { + t.Errorf("summary missing FAIL entry: %v", summary) + } + // failedSummaryLines feeds the run's returned error — the FAIL entry must survive the filter. + if failed := failedSummaryLines(summary); len(failed) != 1 { + t.Errorf("failedSummaryLines = %v, want exactly the broken entry", failed) + } +} + +var errTest = &testErr{} + +type testErr struct{} + +func (*testErr) Error() string { return "tar exploded" } + +func containsSummary(summary []string, want string) bool { + for _, s := range summary { + if s == want { + return true + } + } + return false +} + +func containsSummaryPrefix(summary []string, prefix string) bool { + for _, s := range summary { + if strings.HasPrefix(s, prefix) { + return true + } + } + return false +}