diff --git a/internal/app/names.go b/internal/app/names.go index 07d22d6..e9506fd 100644 --- a/internal/app/names.go +++ b/internal/app/names.go @@ -553,3 +553,11 @@ func (n Names) BackupRunLock(service string) string { func (n Names) BackupVerifyScript(service string) string { return path.Join(n.AppDir(), "backup", "verify-"+service+".sh") } + +// BackupPushScript is the host-side base backup the scheduled backup unit +// runs. It lives beside the verify script for the same reason: it drives +// docker from the host, and it is where the unit's retry loop is written down +// rather than in a unit file that cannot bound one. +func (n Names) BackupPushScript(service string) string { + return path.Join(n.AppDir(), "backup", "backup-"+service+".sh") +} diff --git a/internal/engine/backup_schedule.go b/internal/engine/backup_schedule.go index 8e2b7cb..89869c7 100644 --- a/internal/engine/backup_schedule.go +++ b/internal/engine/backup_schedule.go @@ -19,9 +19,10 @@ import ( // Two timers per protected service, taken straight from the policy rather than // invented: // -// - the backup schedule takes a base backup and then applies retention, in -// that order, so the repository is never briefly below the number of -// generations the policy promises; +// - the backup schedule takes a base backup — retrying it a bounded number +// of times, because one refused upload part aborts the whole push — and +// then applies retention, in that order, so the repository is never +// briefly below the number of generations the policy promises; // - the drill schedule verifies the archived WAL forms an unbroken // chain, which is the check a green backup does not imply. // @@ -100,8 +101,11 @@ func (e *Engine) SyncBackupSchedules(ctx context.Context) error { commands []string }{ {"backup", projection.Policy.Schedule, []string{ - walgExec(container, "backup-push", app.PgDataPath), - // Retention after the new generation exists, never before. + "/bin/sh " + n.BackupPushScript(service), + // Retention after the new generation exists, never before. A + // separate ExecStart, so a push that fails every attempt + // stops the unit here and never expires a generation to make + // room for one that does not exist. prune, }}, {"verify", projection.Policy.Drill.Schedule, []string{ @@ -140,11 +144,19 @@ func (e *Engine) SyncBackupSchedules(ctx context.Context) error { // straight through would be marked successful by systemd over a repository // with holes in it, for as long as nobody looked. The interactive command // judges the same report in Go; this is the unattended half of it. + // + // The backup unit runs a script for a different reason: a single + // ExecStart has no way to try again, and a base backup is the one + // scheduled job where giving up on the first fault loses a whole night. for _, service := range backedUpServiceNames(e.Spec) { if err := e.writeServiceFile(ctx, n.BackupVerifyScript(service), []byte(backupVerifyScript(n.ServiceContainer(service)))); err != nil { return fmt.Errorf("cannot install the archive verification for %s: %w", service, err) } + if err := e.writeServiceFile(ctx, n.BackupPushScript(service), + []byte(backupPushScript(n.ServiceContainer(service)))); err != nil { + return fmt.Errorf("cannot install the scheduled base backup for %s: %w", service, err) + } } wantedNames := map[string]bool{} @@ -223,6 +235,66 @@ func backupVerifyScript(container string) string { }, "\n") } +// backupPushRetryWaits is the pause, in seconds, before each further attempt +// at the scheduled base backup; its length is the number of retries. +// +// A base backup streams gigabytes through many multipart uploads, and the +// whole push aborts on the first part the destination refuses — wal-g treats +// a lost part as fatal, and its S3 client only retries the errors the SDK +// classes as transient, which a bare HTTP 400 from an edge proxy is not. Seen +// on a production host against an S3-compatible destination: three of six +// nights lost to one part, with the same configuration succeeding on the +// others. Two more attempts, a minute and then five apart, cover a fault +// that lasts seconds without hiding one that lasts the night. +var backupPushRetryWaits = []int{60, 300} + +// backupPushScript is the scheduled base backup. +// +// It is a script rather than a unit directive because systemd's Restart= +// re-runs every ExecStart, so a retry of the push would also re-run the +// retention that follows it, and the start rate limiter bounds restarts by +// time, not count — a push slower than the limiter's window retries forever. +// A loop in POSIX sh is bounded by construction and exits with the last +// attempt's status, so the unit still fails, visibly, when every attempt +// does. It runs under the unit's flock for its whole span, retries included, +// so an interactive `ob backup` keeps waiting rather than interleaving. +func backupPushScript(container string) string { + return backupPushScriptWith(walgExec(container, "backup-push", app.PgDataPath), backupPushRetryWaits) +} + +func backupPushScriptWith(command string, waits []int) string { + attempts := len(waits) + 1 + schedule := make([]string, 0, len(waits)) + for _, wait := range waits { + schedule = append(schedule, fmt.Sprint(wait)) + } + // Bounded by the attempt count, not by running out of waits: one wait per + // retry is what makes `$1` defined every time the loop reaches `shift`. + return strings.Join([]string{ + "#!/bin/sh", + "# Written by Onebox. Edits are overwritten on the next apply.", + "set -u", + fmt.Sprintf("attempts=%d", attempts), + "attempt=0", + "set -- " + strings.Join(schedule, " "), + "while :; do", + " attempt=$((attempt + 1))", + " " + command, + " status=$?", + ` [ "$status" -eq 0 ] && exit 0`, + ` if [ "$attempt" -ge "$attempts" ]; then`, + ` echo "onebox: base backup failed on attempt $attempt of $attempts (exit $status); giving up" >&2`, + ` exit "$status"`, + " fi", + ` wait=$1`, + " shift", + ` echo "onebox: base backup attempt $attempt of $attempts failed (exit $status); retrying in ${wait}s" >&2`, + ` sleep "$wait"`, + "done", + "", + }, "\n") +} + func walgExec(container string, args ...string) string { parts := []string{"/usr/bin/docker", "exec", "-u", "postgres", container, app.WalgBinary} return strings.Join(append(parts, args...), " ") diff --git a/internal/engine/schedule_test.go b/internal/engine/schedule_test.go index 57914d2..176bae2 100644 --- a/internal/engine/schedule_test.go +++ b/internal/engine/schedule_test.go @@ -5,6 +5,7 @@ import ( "context" "encoding/json" "errors" + "fmt" "os" "os/exec" "path/filepath" @@ -965,6 +966,161 @@ func TestBackupServiceUnitRecordsEnvironmentOwnership(t *testing.T) { } } +// The scheduled base backup has to survive one refused upload part. +// +// wal-g aborts the whole push when the destination refuses a single multipart +// part, and its S3 client does not retry a bare HTTP 400, so a unit with one +// ExecStart lost three nights in six on a production host while the same +// configuration succeeded on the others. The loop is bounded, and when every +// attempt fails the unit still fails with the last attempt's status — a retry +// that hid that would be a backup that had silently stopped. +func TestTheScheduledBaseBackupRetriesBeforeFailingTheUnit(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("the scheduled backup is a POSIX shell script") + } + dir := t.TempDir() + counter := filepath.Join(dir, "attempts") + for _, tc := range []struct { + name string + succeedFrom int // the attempt that succeeds; 0 means none does + wantExit int + wantRuns int + wantStderr []string + }{ + {"first attempt succeeds", 1, 0, 1, nil}, + {"second attempt succeeds", 2, 0, 2, []string{"attempt 1 of 3 failed (exit 7); retrying in 0s"}}, + {"every attempt fails", 0, 7, 3, []string{ + "attempt 1 of 3 failed (exit 7); retrying in 0s", + "attempt 2 of 3 failed (exit 7); retrying in 0s", + "failed on attempt 3 of 3 (exit 7); giving up", + }}, + } { + t.Run(tc.name, func(t *testing.T) { + if err := os.WriteFile(counter, nil, 0o600); err != nil { + t.Fatal(err) + } + // The stand-in for `docker exec ... backup-push`: a program of its + // own, as the real one is. Each run appends a line, and it succeeds + // once the line count reaches the attempt meant to succeed. + push := filepath.Join(dir, "push.sh") + if err := os.WriteFile(push, []byte(fmt.Sprintf( + "echo run >> %s\n[ %d -gt 0 ] && [ \"$(wc -l < %s | tr -d ' ')\" -ge %d ] || exit 7\n", + counter, tc.succeedFrom, counter, tc.succeedFrom)), 0o600); err != nil { + t.Fatal(err) + } + script := filepath.Join(dir, "backup.sh") + if err := os.WriteFile(script, []byte(backupPushScriptWith("sh "+push, []int{0, 0})), 0o600); err != nil { + t.Fatal(err) + } + var stderr bytes.Buffer + cmd := exec.CommandContext(context.Background(), "sh", script) + cmd.Stderr = &stderr + err := cmd.Run() + exit := 0 + var exitErr *exec.ExitError + if errors.As(err, &exitErr) { + exit = exitErr.ExitCode() + } else if err != nil { + t.Fatal(err) + } + if exit != tc.wantExit { + t.Errorf("exit %d, want %d:\n%s", exit, tc.wantExit, stderr.String()) + } + runs, err := os.ReadFile(counter) + if err != nil { + t.Fatal(err) + } + if got := strings.Count(string(runs), "run\n"); got != tc.wantRuns { + t.Errorf("the push ran %d times, want %d:\n%s", got, tc.wantRuns, stderr.String()) + } + for _, want := range tc.wantStderr { + if !strings.Contains(stderr.String(), want) { + t.Errorf("stderr does not say %q:\n%s", want, stderr.String()) + } + } + }) + } + + // The installed script: the real push, the real waits, and shell that does + // not bail out of the loop at the first failed attempt. + script := backupPushScript("shop-database-1") + for _, want := range []string{ + "/usr/bin/docker exec -u postgres shop-database-1 " + app.WalgBinary + " backup-push " + app.PgDataPath, + "attempts=3", + "set -- 60 300", + `exit "$status"`, + } { + if !strings.Contains(script, want) { + t.Errorf("the scheduled base backup does not contain %q:\n%s", want, script) + } + } + if strings.Contains(script, "set -e") { + t.Errorf("set -e exits at the first failed attempt before the loop can retry:\n%s", script) + } + check := exec.CommandContext(context.Background(), "sh", "-n") + check.Stdin = strings.NewReader(script) + if output, err := check.CombinedOutput(); err != nil { + t.Fatalf("the scheduled base backup is not valid POSIX shell: %v: %s\n%s", err, output, script) + } +} + +// The unit retries the push and only the push. +// +// Retention stays a separate ExecStart on purpose: systemd stops a oneshot at +// the first ExecStart that fails, so a push that failed every attempt never +// reaches `delete retain`. A retry wired into the unit with Restart= would +// instead re-run every ExecStart, retention included, on each attempt. +func TestTheScheduledBackupUnitRetriesThePushAndNotTheRetention(t *testing.T) { + f := &transport.Fake{Dynamic: func(cmd string) (transport.Result, bool) { + if strings.HasSuffix(cmd, "echo ok") { + return transport.Result{Stdout: "ok\n"}, true + } + return transport.Result{}, false + }} + resolved := protectedPostgresResolved(t, "7513211627332151223") + e := New(resolved, nil, f, Options{Out: &bytes.Buffer{}, Sleep: noSleep, Environment: "production"}) + if err := e.SyncBackupSchedules(context.Background()); err != nil { + t.Fatal(err) + } + n := resolved.Spec.NamesFor("production") + var unit, script string + for _, body := range f.Inputs { + switch { + case strings.Contains(body, "Description=Onebox backup backup for postgres"): + unit = body + case strings.HasPrefix(body, "#!/bin/sh") && strings.Contains(body, "backup-push"): + script = body + } + } + if unit == "" { + t.Fatalf("no backup unit was written:\n%s", strings.Join(f.Commands, "\n")) + } + if script == "" || !strings.Contains(strings.Join(f.Commands, "\n"), n.BackupPushScript("postgres")) { + t.Fatalf("the retrying base backup was not installed at %s:\n%s", n.BackupPushScript("postgres"), strings.Join(f.Commands, "\n")) + } + lock := n.BackupRunLock("postgres") + push := "ExecStart=/usr/bin/flock -w 3600 " + lock + " /bin/sh " + n.BackupPushScript("postgres") + prune := "ExecStart=/usr/bin/flock -w 3600 " + lock + " /usr/bin/docker exec -u postgres " + + n.ServiceContainer("postgres") + " " + app.WalgBinary + " delete retain FULL" + for _, want := range []string{push, prune} { + if !strings.Contains(unit, want) { + t.Errorf("the backup unit does not contain %q:\n%s", want, unit) + } + } + if strings.Index(unit, push) > strings.Index(unit, prune) { + t.Errorf("retention runs before the push:\n%s", unit) + } + if strings.Contains(unit, "backup-push") { + t.Errorf("the unit runs the push outside the retrying script:\n%s", unit) + } + if strings.Contains(unit, "Restart=") { + t.Errorf("a systemd restart would re-run retention with every attempt:\n%s", unit) + } + if !strings.Contains(script, "/usr/bin/docker exec -u postgres "+n.ServiceContainer("postgres")+" "+app.WalgBinary+" backup-push "+app.PgDataPath) { + t.Errorf("the installed script does not push this service's cluster:\n%s", script) + } +} + func TestScheduledJobRunnersRecordRunStateForTheNotifier(t *testing.T) { names := app.Names{App: "sample", BasePath: "/var/lib/onebox"} for _, tc := range []struct { diff --git a/site/src/content/docs/guides/back-up-a-database.mdx b/site/src/content/docs/guides/back-up-a-database.mdx index 6b313c4..34243d2 100644 --- a/site/src/content/docs/guides/back-up-a-database.mdx +++ b/site/src/content/docs/guides/back-up-a-database.mdx @@ -67,6 +67,14 @@ be telling you the database is protected at the moment it is not. The restart is a real restart. Enabling is a maintenance action, not a configuration change. +The scheduled base backup does not give up on its first fault. A push streams +gigabytes through many multipart uploads and aborts on the first part the +destination refuses, so the unit tries up to three times, a minute and then +five minutes apart, under the same lock an interactive `ob backup` waits on. +Retention runs only after a push that succeeded. A night on which every attempt +fails is still a failed unit: `systemctl status onebox-backup-database-backup` +says so, and the journal has each attempt's reason. + ## The credential file The encrypted file your target names needs three entries — the two you named,