From d1e1c226d9a63432c652d373c039a3ae4430f3a2 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 7 Aug 2026 00:03:04 -0500 Subject: Guard deploys and config changes against leftover state on retry - Extract deploy.Run's snapshot -> apply -> restart -> health-check -> rollback-on-failure sequence into deploy.GuardedApply, shared with cmd/skiff's applyConfigChange, which duplicated it verbatim. - Clean up the worktree and snapshot a deploy creates when it fails before cutover (e.g. a failing release-phase migration), so retrying the same sha no longer needs manual cleanup. - Make checkout and snapshotting idempotent for a same-sha retry after a cutover that later got rolled back on a failed health check, where the release dir and snapshot are deliberately left behind by design. - Document the actual cause of a jail not surviving a host reboot: FreeBSD ships no /etc/jail.conf, so boot-time `service jail start` finds nothing to start regardless of jail.conf.d - only name-targeted starts read jail.conf.d directly, which is why manual starts worked. --- README.md | 14 +++++ USER_GUIDE.md | 24 ++++++++- cmd/skiff/main.go | 41 ++++----------- internal/deploy/deploy.go | 125 +++++++++++++++++++++++++++++++++++--------- internal/zfsutil/zfsutil.go | 8 +++ 5 files changed, 155 insertions(+), 57 deletions(-) diff --git a/README.md b/README.md index 632fbb0..ba51aba 100644 --- a/README.md +++ b/README.md @@ -51,6 +51,20 @@ clone` requires the parent dataset to already exist. # mkdir -p /skiff/repos ``` +### Jail config + +FreeBSD doesn't ship an `/etc/jail.conf`, and at boot `service jail start` +(no jail named) only creates jails it finds in that one file - it never +looks at `jail.conf.d/` on its own. Point it there once: + +``` +# echo '.include "/etc/jail.conf.d/*.conf";' > /etc/jail.conf +``` + +Without this, `skiff new-app` and manual `service jail start ` still +work fine - both read `jail.conf.d/.conf` directly - so the gap stays +hidden until the host reboots and no app jail comes back up. + ### Nginx Add once to `/usr/local/etc/nginx/nginx.conf`, inside the `http` block: diff --git a/USER_GUIDE.md b/USER_GUIDE.md index 04ec92e..fa8449c 100644 --- a/USER_GUIDE.md +++ b/USER_GUIDE.md @@ -283,7 +283,26 @@ bar is garbled without a real tty. # mkdir -p /skiff/repos ``` -#### 3. Nginx +#### 3. Jail config + +FreeBSD doesn't ship an `/etc/jail.conf`, and at boot `service jail +start` (invoked with no jail named) only creates jails it finds in that +one file - it never looks inside `jail.conf.d/` on its own. Point it +there once: + +``` +# echo '.include "/etc/jail.conf.d/*.conf";' > /etc/jail.conf +``` + +Skip this and everything still *appears* to work: `skiff new-app` starts +the new jail immediately, and a manual `service jail start ` after +a reboot works too - both read `jail.conf.d/.conf` directly, +bypassing `/etc/jail.conf` entirely. Only the boot-time, no-name form of +`service jail start` actually depends on the include line, so the gap +stays invisible until the host reboots and no app jail comes back up on +its own - see the [Troubleshooting](#troubleshooting) entry below. + +#### 4. Nginx Add once to `/usr/local/etc/nginx/nginx.conf`, inside the `http` block: @@ -300,7 +319,7 @@ include /usr/local/etc/nginx/skiff.d/*.conf; Skiff only ever writes files under `skiff.d/` and reloads - it never touches `nginx.conf` itself. -#### 4. Install skiff +#### 5. Install skiff Cross-compile from any machine with Go, then copy the binary over: @@ -405,6 +424,7 @@ Carried over from the project's own README, plus what testing surfaced: |---|---|---| | `zfs clone ...: parent does not exist` on `new-app` | `zroot/skiff/apps` was created as a directory, not a dataset | `zfs create -o mountpoint=/skiff/apps zroot/skiff/apps` | | `Cannot 'start' jail. Set jail_enable to YES...` | `jail_enable` unset | `sysrc jail_enable=YES` | +| App jails don't come back up after a host reboot, even with `jail_enable=YES` and manual `service jail start ` working fine | `/etc/jail.conf` missing or doesn't `.include` `jail.conf.d/*.conf` - boot-time `service jail start` only creates jails it finds in `/etc/jail.conf` itself | `echo '.include "/etc/jail.conf.d/*.conf";' > /etc/jail.conf` (see [Jail config](#3-jail-config)) | | Deploy always health-check-fails with `connection refused`, even though the app builds fine | Jail's IP was never aliased onto a host interface | `ifconfig alias /` (see [Provisioning](#provisioning-a-new-app)) | | `git worktree add ...: missing but already registered worktree` on redeploy | Stale worktree entry after a rollback deleted a release dir (older skiff binaries only) | `git -C /skiff/repos/.git worktree prune` | | `bsdinstall jail` spins at 100% CPU forever | Interactive `dialog` waiting on a terminal that isn't there | Kill it, re-run with `nonInteractive=YES` and `BSDINSTALL_DISTSITE` set (see setup section) | diff --git a/cmd/skiff/main.go b/cmd/skiff/main.go index 2fe5907..1c48681 100644 --- a/cmd/skiff/main.go +++ b/cmd/skiff/main.go @@ -14,7 +14,6 @@ import ( "skiff/internal/appmeta" "skiff/internal/deploy" "skiff/internal/envfile" - "skiff/internal/health" "skiff/internal/jailmgr" "skiff/internal/layout" "skiff/internal/sh" @@ -266,9 +265,10 @@ func cmdConfigList(args []string) error { } // applyConfigChange loads app's current env file, applies mutate to it, and -// carries the same snapshot -> restart -> health-check -> rollback-on- -// failure sequence deploy.Run uses for code deploys - a bad config change -// is exactly as much of an outage as a bad deploy, guarded the same way. +// saves it under deploy.GuardedApply - the same snapshot -> restart -> +// health-check -> rollback-on-failure sequence deploy.Run uses for code +// deploys, since a bad config change is exactly as much of an outage as a +// bad deploy. func applyConfigChange(app string, mutate func(map[string]string) error) error { cfg := layout.Default() if _, err := os.Stat(cfg.MetaPath(app)); err != nil { @@ -285,36 +285,15 @@ func applyConfigChange(app string, mutate func(map[string]string) error) error { ctx := context.Background() out := os.Stdout - dataset := cfg.AppDataset(app) tag := fmt.Sprintf("precfg-%d", time.Now().Unix()) - fmt.Fprintln(out, "==> snapshotting current state") - if err := zfsutil.Snapshot(ctx, out, dataset, tag); err != nil { - return fmt.Errorf("snapshot: %w", err) - } - - if err := envfile.Save(cfg.EnvPath(app), kv); err != nil { - return fmt.Errorf("save config: %w", err) - } - - if err := jailmgr.Restart(ctx, out, app); err != nil { - return fmt.Errorf("restart: %w", err) - } - - meta, err := appmeta.Read(cfg.MetaPath(app)) - if err != nil { - return fmt.Errorf("read app metadata: %w", err) - } - - if err := health.WaitHealthy(ctx, meta); err != nil { - fmt.Fprintf(out, "==> health check failed (%v), rolling back to %s\n", err, tag) - if rbErr := zfsutil.Rollback(ctx, out, dataset, tag); rbErr != nil { - return fmt.Errorf("health check failed AND rollback failed: %v / %w", err, rbErr) - } - if rbErr := jailmgr.Restart(ctx, out, app); rbErr != nil { - return fmt.Errorf("health check failed, rolled back, but restart failed: %v / %w", err, rbErr) + if err := deploy.GuardedApply(ctx, out, cfg, app, tag, "config change", func() error { + if err := envfile.Save(cfg.EnvPath(app), kv); err != nil { + return fmt.Errorf("save config: %w", err) } - return fmt.Errorf("config change failed health check and was rolled back: %w", err) + return nil + }); err != nil { + return err } fmt.Fprintln(out, "==> config updated") diff --git a/internal/deploy/deploy.go b/internal/deploy/deploy.go index 5430b7f..751c976 100644 --- a/internal/deploy/deploy.go +++ b/internal/deploy/deploy.go @@ -22,8 +22,6 @@ import ( ) func Run(ctx context.Context, out io.Writer, cfg layout.Config, app, sha string) error { - dataset := cfg.AppDataset(app) - snapshotTag := "pre-" + sha releaseDir := cfg.ReleaseDir(app, sha) fmt.Fprintf(out, "==> checking out %s\n", sha) @@ -33,10 +31,37 @@ func Run(ctx context.Context, out io.Writer, cfg layout.Config, app, sha string) if err := sh.Run(ctx, out, "", "git", "-C", cfg.RepoPath(app), "worktree", "prune"); err != nil { fmt.Fprintf(out, "==> warning: worktree prune failed: %v\n", err) } + if _, err := os.Stat(releaseDir); err == nil { + // A retry of this exact sha whose previous attempt got far enough + // to cut over, then failed its health check: the dataset rollback + // reverts `current` but - by design - leaves this release dir (and + // its worktree registration) on disk, since it existed at + // snapshot time. Clear it so `add` below doesn't collide. + fmt.Fprintln(out, "==> removing leftover checkout from a previous attempt") + if err := sh.Run(ctx, out, "", "git", "-C", cfg.RepoPath(app), "worktree", "remove", "--force", releaseDir); err != nil { + fmt.Fprintf(out, "==> warning: worktree remove failed (%v), removing directory directly\n", err) + if err := os.RemoveAll(releaseDir); err != nil { + return fmt.Errorf("remove leftover checkout: %w", err) + } + } + } if err := sh.Run(ctx, out, "", "git", "-C", cfg.RepoPath(app), "worktree", "add", releaseDir, sha); err != nil { return fmt.Errorf("checkout: %w", err) } + cutover := false + defer func() { + if cutover { + return + } + // Nothing ever went live under this checkout - leaving it behind + // blocks a retry of the same sha with "already exists" on the next + // `worktree add`. + if err := sh.Run(ctx, out, "", "git", "-C", cfg.RepoPath(app), "worktree", "remove", "--force", releaseDir); err != nil { + fmt.Fprintf(out, "==> warning: failed to clean up worktree %s: %v\n", releaseDir, err) + } + }() + fmt.Fprintln(out, "==> building") if err := sh.Run(ctx, out, releaseDir, "make", "-B"); err != nil { return fmt.Errorf("build: %w", err) @@ -45,27 +70,85 @@ func Run(ctx context.Context, out io.Writer, cfg layout.Config, app, sha string) return fmt.Errorf("build: `make` did not produce ./app: %w", err) } - fmt.Fprintln(out, "==> snapshotting current state") - if err := zfsutil.Snapshot(ctx, out, dataset, snapshotTag); err != nil { - return fmt.Errorf("snapshot: %w", err) + if err := GuardedApply(ctx, out, cfg, app, "pre-"+sha, "deploy", func() error { + hasRelease, err := release.Declared(ctx, releaseDir) + if err != nil { + return fmt.Errorf("check for release target: %w", err) + } + if hasRelease { + if err := release.Run(ctx, out, app, cfg.ReleaseDirInJail(sha)); err != nil { + return fmt.Errorf("release phase failed, deploy aborted before cutover: %w", err) + } + } else { + fmt.Fprintln(out, "==> no release target declared, skipping") + } + + fmt.Fprintln(out, "==> cutting over") + if err := swapCurrent(cfg.CurrentRelease(app), cfg.ReleaseDirInJail(sha)); err != nil { + return fmt.Errorf("cutover: %w", err) + } + cutover = true + return nil + }); err != nil { + return err + } + + fmt.Fprintln(out, "==> healthy, updating routing") + meta, err := appmeta.Read(cfg.MetaPath(app)) + if err != nil { + return fmt.Errorf("read app metadata: %w", err) + } + if err := nginx.EnsureRoute(ctx, out, cfg, app, meta.IP, meta.Port, meta.ServerName); err != nil { + return fmt.Errorf("nginx: %w", err) } - hasRelease, err := release.Declared(ctx, releaseDir) + fmt.Fprintf(out, "==> deployed %s at %s\n", app, sha) + return nil +} + +// GuardedApply snapshots app's dataset under tag, runs apply, restarts the +// jail, and waits for it to report healthy - rolling the dataset back to +// the snapshot (and restarting again) if it doesn't. Shared by code +// deploys and config changes: either one is exactly as much of an outage +// as the other if it goes wrong, so both are guarded the same way. label +// is used only in the final error message (e.g. "deploy", "config change"). +func GuardedApply(ctx context.Context, out io.Writer, cfg layout.Config, app, tag, label string, apply func() error) error { + dataset := cfg.AppDataset(app) + + existing, err := zfsutil.Snapshots(ctx, dataset) if err != nil { - return fmt.Errorf("check for release target: %w", err) + return fmt.Errorf("list snapshots: %w", err) } - if hasRelease { - if err := release.Run(ctx, out, app, cfg.ReleaseDirInJail(sha)); err != nil { - return fmt.Errorf("release phase failed, deploy aborted before cutover: %w", err) + for _, s := range existing { + if s != tag { + continue + } + // A previous attempt under this same tag (e.g. a retried deploy of + // the same sha, rolled back to this exact snapshot) left it + // behind - retaking it now just recaptures the state a rollback + // already reverted the dataset to. + fmt.Fprintf(out, "==> replacing stale snapshot %s from a previous attempt\n", tag) + if err := zfsutil.DestroySnapshot(ctx, out, dataset, tag); err != nil { + return fmt.Errorf("clean up stale snapshot: %w", err) } - } else { - fmt.Fprintln(out, "==> no release target declared, skipping") + break } - fmt.Fprintln(out, "==> cutting over") - if err := swapCurrent(cfg.CurrentRelease(app), cfg.ReleaseDirInJail(sha)); err != nil { - return fmt.Errorf("cutover: %w", err) + fmt.Fprintln(out, "==> snapshotting current state") + if err := zfsutil.Snapshot(ctx, out, dataset, tag); err != nil { + return fmt.Errorf("snapshot: %w", err) } + + if err := apply(); err != nil { + // Nothing has been restarted yet, so this snapshot was never needed + // as a rollback target - left behind, it collides with the same + // tag on the next retry. + if destroyErr := zfsutil.DestroySnapshot(ctx, out, dataset, tag); destroyErr != nil { + fmt.Fprintf(out, "==> warning: failed to clean up snapshot %s: %v\n", tag, destroyErr) + } + return err + } + if err := jailmgr.Restart(ctx, out, app); err != nil { return fmt.Errorf("restart: %w", err) } @@ -76,22 +159,16 @@ func Run(ctx context.Context, out io.Writer, cfg layout.Config, app, sha string) } if err := health.WaitHealthy(ctx, meta); err != nil { - fmt.Fprintf(out, "==> health check failed (%v), rolling back to pre-%s\n", err, sha) - if rbErr := zfsutil.Rollback(ctx, out, dataset, snapshotTag); rbErr != nil { + fmt.Fprintf(out, "==> health check failed (%v), rolling back to %s\n", err, tag) + if rbErr := zfsutil.Rollback(ctx, out, dataset, tag); rbErr != nil { return fmt.Errorf("health check failed AND rollback failed: %v / %w", err, rbErr) } if rbErr := jailmgr.Restart(ctx, out, app); rbErr != nil { return fmt.Errorf("health check failed, rolled back, but restart failed: %v / %w", err, rbErr) } - return fmt.Errorf("deploy failed health check and was rolled back: %w", err) - } - - fmt.Fprintln(out, "==> healthy, updating routing") - if err := nginx.EnsureRoute(ctx, out, cfg, app, meta.IP, meta.Port, meta.ServerName); err != nil { - return fmt.Errorf("nginx: %w", err) + return fmt.Errorf("%s failed health check and was rolled back: %w", label, err) } - fmt.Fprintf(out, "==> deployed %s at %s\n", app, sha) return nil } diff --git a/internal/zfsutil/zfsutil.go b/internal/zfsutil/zfsutil.go index 8f1b0e2..918289c 100644 --- a/internal/zfsutil/zfsutil.go +++ b/internal/zfsutil/zfsutil.go @@ -33,6 +33,14 @@ func Rollback(ctx context.Context, out io.Writer, dataset, tag string) error { return sh.Run(ctx, out, "", "zfs", "rollback", "-r", snapRef(dataset, tag)) } +// DestroySnapshot removes a snapshot that was never needed as a rollback +// target - e.g. one taken right before an apply step that failed before +// anything actually changed. Left behind, it collides with the same tag on +// the next retry. +func DestroySnapshot(ctx context.Context, out io.Writer, dataset, tag string) error { + return sh.Run(ctx, out, "", "zfs", "destroy", snapRef(dataset, tag)) +} + // Snapshots lists snapshot names (without the dataset@ prefix), newest last. func Snapshots(ctx context.Context, dataset string) ([]string, error) { raw, err := sh.Output(ctx, "", "zfs", "list", "-H", "-t", "snapshot", "-o", "name", "-s", "creation", "-r", dataset) -- cgit v1.3