Repository navigation
feat: schema v1 and migrations before every rollout - #27
Conversation
…ostgres image)
AbdikhaniAbdirashid
left a comment
AbdikhaniAbdirashid
left a comment
There was a problem hiding this comment.
All three points answered, and one of them better than I asked for. Approving.
The one I want to single out
I wrote that SET LOCAL lock_timeout was the one line in the migration that couldn't be tested, because an empty database has no locks to wait for. You went and found a way to test it anyway: a session holding a lock, a migration with our header trying to ALTER through it, 55P03 canceling statement due to lock timeout after 5 s, and then the two checks that matter — the column was not added, and the migration was not recorded as applied, so the next deploy just retries.
That is the positive control on the one line I had written off as unverifiable. It also settles the dbmate question properly rather than by reading the README: the behaviour was observed on 2.37.0, and the note about several statements sent together forming an implicit transaction means the header can't silently be a no-op even if that default ever changes.
The other two
updated_at dropped. Option 3, which was my preference too. And you caught a nuance I had not thought about: editing migration 001 is fine because it has never run in production, and ADR 0006's "never edit a merged migration" is about migrations that have. That distinction is worth having in the ADR itself at some point — it's the difference between a rule and a superstition.
next_attempt_at. Written down, and placed: CASE WHEN status = 'pending' THEN next_attempt_at END in the deliveries repository's read query, as an input to the Phase 3 plan. One place, named, and the comment is in the migration where someone reading the column will see it. That is exactly what I was asking for — the column and the API field are allowed to differ, as long as the mapping isn't folk knowledge.
The digest, checked
api/scripts/postgres-image.sh is the tidier of the two options. I ran its logic against the manifest now that #22 is merged:
$ sed -n 's/^ *image: *\(postgres:[^ ]*\).*/\1/p' k8s/data/postgres.yaml | head -1
postgres:18.6@sha256:979bccfde35445db618c65107d27d08c6c21339d8dc3ad4c830c6633947be0e4
Matches the script's own regex, so the guard would not fire on a correct manifest — which is the half of a validation that is easy to forget to check. And the failure path having a clear message rather than an empty variable is what stops it from degrading into docker run "" a year from now.
Also confirmed: zero updated_at column definitions left in migration 001.
Unchanged from my first review
Everything I agreed with still stands, so I won't repeat it — the migrate:down exception, text + CHECK over enums, deriving the attempt number instead of storing it, the no-delete-on-jobs Role, and commits without k8s/migrate/ skipping the step so a rollback to Phase 1 still works.
Approving. Please post the output of step 3 of the apply order, the by-hand deploy that runs migration 001 — and if the lock-timeout test is cheap to re-run on the server after that, I'd rather see it there once than trust that it behaves the same way against the real database.
|
Live on 1 Nov, in the order from the description. The output you asked for: 1. The deployer Role first, checked as the deployer (
2. The new 3. The deploy by hand of The Job's pod ran as uid 1000, non-root, with a read-only root filesystem. 4. The database (read-only queries):
From outside, 5. The negative tests (a Job with an image that doesn't exist; the legacy rollback) come Next: #26 rebased onto |
|
Step 5, the negative tests, both passed (1 Nov, UTC): 1. A migration that can't start doesn't hang the deploy. We created a Job from 2. The new
So #27's go-live list is complete. Both tests leave nothing behind: the Jobs are removed after an |
….6) (#26) ## What Step 2.6 of Phase 2: the API connects to Postgres, and the tests run against a real one. **Stacked on #27** (base `feat/schema-migrations`; #27 replaces #25): the integration tests need migration 001. Before #27 merges, this PR's base is switched to `main`; then it gets one rebase. ## Why it works this way - **The API waits for the database before it listens** (at most 60 s, by the clock). Readiness never checks the database: one database blip would take every pod out at once (the point from the review of #15). A pod with a wrong `DATABASE_URL` never listens, so its startupProbe fails, the rollout fails and `relay-deploy` rolls back. - **Waiting can't fix a wrong login**, so `28P01` (wrong password), `28000` (login not allowed) and `3D000` (no such database) stop it at once. Everything else (refused, timeout, `57P03` "starting up") is retried every 2 s. Each try is limited to the time that is left (at most 5 s). A try that runs over is abandoned, not cancelled: postgres.js 3.4.9's `cancel()` drops the promise of its own cancel request, so a failed cancel would go uncaught. - **A database connection error answers `503 service_unavailable` with `Retry-After: 5`**, in our usual error shape. Every other database error stays a 500, including `57014` (a query stopped by `statement_timeout`): slow SQL is our bug, not an outage, and must not look like one. - **postgres.js 3.4.9**, pinned (4.0.0 came out on 29 Oct). Pool 5 per pod, idle 30 s, `connect_timeout` 5 s, `statement_timeout` 5 s, `application_name` `relay-api`. - **Signal handlers are set up before the wait**, not after `listen`: bun is process 1 in the container, and Linux ignores SIGTERM for process 1 unless it has a handler. Shutdown is `app.close()`, then `sql.end({ timeout: 5 })`. ## Changes - **API:** `config.ts` (`DATABASE_URL` required, `DB_POOL_MAX` 1–20, default 5), `db.ts` (new), `server.ts`, `error-handler.ts`, the comment in `routes/health.ts`; unit tests with a fake database and a fake clock. - **Tests against a real Postgres:** `api/scripts/test-db.sh` with `db:up`, `test:db`, `db:down` and `test:unit`. A throwaway container on `127.0.0.1:55432`, with production's image (the digest is read from `k8s/data/postgres.yaml`) and the same first-start script, so the tests log in as `relay`. `db:down` removes only the container the script made (it carries a label). It takes production's image from `api/scripts/postgres-image.sh` (#27), the same script the CI smoke test uses. `api/test/integration/`: the connection tests and the schema tests. - **CI:** `api-test` runs `db:up`, then `test:db`. shellcheck also checks `api/scripts/`. - **Kubernetes:** `k8s/app/deployment.yaml`: `DATABASE_URL` from Secret `relay-db` (key `url`); a startupProbe (45 × 2 s = 90 s, more than the API's own 60 s wait); liveness without the 15 s delay; `terminationGracePeriodSeconds: 30` written out, with what has to fit inside it. - **Docs:** `docs/api.md` (readiness never checks the database; the new 503), `docs/CONTEXT.md`, the error table in ADR 0003. ## One change from the reviewed plan CI starts the test Postgres with `docker run`, the same way as the existing image smoke test, instead of a `services:` container. Services start **before** checkout, so they can't mount `k8s/data/initdb/`, and the first-start script would not run the way production runs it. ## How it was checked - **59 tests pass against the real Postgres, logged in as `relay`.** They include: - `relay` isn't a superuser; - our settings arrive on every connection; - a wrong password stops at the first answer with `28P01` (no retry is logged); - `pg_sleep(6)` is cancelled at 5 s with `57014`; - a connection ended with `pg_terminate_backend` comes back (a pool of exactly one connection); - one valid row per table, two NULL idempotency keys don't collide, the limits themselves (20 event types, a 255-character key) are accepted, and deleting an owner cascades; - 14 rule-breaking rows, each refused with the right SQLSTATE (`23514`, `23505`, `23503`, `23502`). - **Negative checks:** - without `DATABASE_URL`, the integration tests fail (exit 1) and never skip; - without `DATABASE_URL`, the API stops at once (exit 1, `→ at DATABASE_URL`). - **On the cluster**, a server-side dry run of the Deployment is accepted with no Pod Security warnings. The same file with a misspelled probe field is refused (`--validate=strict`). - **Other checks:** - `tsc --strict` is clean, and so is shellcheck; - Snyk Code: 0 findings in our code; - `bun audit`: 0 vulnerabilities. ## Changed after the review - **No cancel on a timed-out try** (above). A unit test checks that a late error after the timeout is still handled. - **503 codes listed by name:** - added `57P02` (Postgres crashed), `EPIPE`, `EHOSTUNREACH` and `ENETUNREACH`; - "every `08…`" was replaced by the five connection codes by name; - `08P01` (protocol violation, a driver bug) and `08007` ("did the commit happen? unknown") now stay a 500: "try again" could save a write twice. - **The clock:** `waitForDatabase` uses `performance.now()`, which never jumps when the system clock is set. - **`db:up` always starts fresh.** It removes its own old container first, so an older image, first-start script or port binding can't be reused. - **No stopwatch checks in the integration tests.** They would be flaky on a slow runner; the SQLSTATE and "no retry was logged" prove the same things. - **The `terminationGracePeriodSeconds` comment is corrected:** the 5 s preStop runs inside the 30 s, before SIGTERM. - **From the review of #22:** the comment on `RELAY_DB_PASSWORD` in `k8s/data/postgres.yaml` now says it stays in the pod's env on purpose, because runbook 6b reads it again. It's only a comment, so Postgres doesn't need to be applied again. ## Before it can go live It needs #22 applied (done on 1 Nov: the Secret `relay-db` and Postgres) and #27's migration 001. Without the Secret, the new pods can't start: the rollout fails and rolls back (safe, but red). The order: 1. #22 merged and applied (done); 2. #27 merged (its deploy runs migration 001); 3. this PR rebased and merged. **Not in this PR:** - `docs/CONTEXT.md`'s "Where we are": updated at the end of Phase 2. - The API doesn't read any table yet: Phase 3 gives the routes the pool with the first repository.
## What Closes Phase 2 in `docs/CONTEXT.md`'s "Where we are": what is live, and the tests that proved it. ## Phase 2, as merged and applied - **#21:** guards on namespace `relay` (Pod Security `restricted`, ClusterIP-only Services). - **#22:** Postgres 18.6 on a disk that is kept on delete, applied by hand, with the `relay` login. - **#27** (replaces #25): migration 001, and the migrate Job before every rollout (ADR 0006). - **#26:** the API waits for the database before it listens; Postgres in the tests. ## The live evidence, posted on the PRs - **#22:** a login from another pod as `relay`, a wrong password refused, 5432 closed from outside, and the deletion drill: the data survived deleting the pod, the StatefulSet and the claim. - **#27:** the Role checked both ways, the installed script identical to git, and migration 001 applied by its Job. Two negative tests: - a Job that can't start fails within 150 s; - a rollback to Phase 1's release under load: 123,904 requests, 0 failed. - **#26 (live 2 Nov):** both pods log `✅ database answered` before they listen; `/health/ready` answers 200 from outside. Docs only.
What
Step 2.4 of Phase 2: every deploy runs the commit's database migrations first, before any pod
changes.
api/db/migrations/…_init.sql,api/package.json+bun.lock,api/Dockerfiledeleted_atfor the soft delete and named indexes;downraises an error on purpose). dbmate 2.37.0, pinned, inside the API image, run asbun run --bun db:migrate(dbmate … migrate).github/workflows/pipeline.ymlapi-imagebuilds the image, smoke-tests the migrations inside it (uid 1000, read-only root, 128 MiB, against a throwaway Postgres made by our first-start script), and only then pushes, on maink8s/migrate/job.yamlgenerateName,backoffLimit: 0, deadline 150 s, pod grace 10 s, TTL 1 h, Pod Securityrestrictedinfra/server/relay-deploy.shrun_migrations: create the Job, then ask for its verdict every 2 s, at most 90 times (≥ 180 s).Complete→ continue;Failed/ no verdict / can't read → print why, return 1, and the existing rollback takes over. Commits withoutk8s/migrate/skip this, so a rollback to Phase 1 still worksk8s/platform/deployer.yamljobs(get, list, watch, create: no update, no delete) andpods/log(get)infra/server/test-relay-deploy.shsleep, and a 30 s watchdog so a hanging script fails the test instead of CIpipeline.ymldocs/decisions/0006-migrations-expand-contract.mdk8s/README.md,docs/CONTEXT.mdk8s/migrate/;deleted_atand the migrations row in CONTEXTTested before the PR
db:migratecreates the 6 tables, owned byrelay, and a second run does nothing;dbmate rollbackis refused and the history stays;test-relay-deploy.sh: 49 passed, 0 failed. A Job that stays Pending creates exactly 1 Job,reads its status 90 times, then gives up with "may still be running".
rules change. shellcheck, actionlint and
bun auditare clean.Changed after the review of #25
No
updated_atcolumns (onendpointsanddeliveries). Nothing kept them current, sowithout a trigger they would quietly have meant
created_atwhenever anUPDATEforgot them.Nothing reads them, and they aren't in
docs/api.md. If something needs one later, a migrationadds the column and its trigger together, which is an additive change and safe under ADR 0006.
Editing migration 001 itself is fine here: it has never run in production, only in throwaway
databases. ADR 0006's "never edit a merged migration" is about migrations that have.
next_attempt_atwritten down: never NULL in the table (while a row is leased, it holdsthe lease's expiry), but
nullin the API unless the status ispending. That mapping lives inone place, the deliveries repository's read query in Phase 3
(
CASE WHEN status = 'pending' THEN next_attempt_at END). The comment is in the migration, andit's an input to the Phase 3 plan.
lock_timeouttested with dbmate 2.37.0, against a real lock:ALTERit;55P03 canceling statement due to lock timeout;simply retries.
dbmate runs each migration in a transaction unless it's marked
transaction:false. And evenwithout that, Postgres runs several statements sent together as one implicit transaction, so
SET LOCALcan't silently do nothing. The result is noted in the migration's header.One source for the Postgres image:
api/scripts/postgres-image.shprints thedigest-pinned image from
k8s/data/postgres.yaml. The smoke test uses it instead of ahardcoded digest, and so will
test-db.shin feat: connect the API to Postgres, with Postgres in the tests (step 2.6) #26. A manifest without a digest makes the stepfail with a clear message (tested). shellcheck now also checks
api/scripts/.Checked again:
relay, and a second run finds nothing to do;relay, 0updated_atcolumns.After merging (an admin, in this order; output posted here)
The merge itself deploys with the old installed script, which doesn't migrate yet. That's
harmless: nothing in
k8s/app/changes. Then:k8s/platform/deployer.yamlfirst, and check as the deployer:create jobsandget pods/log→ yes;delete jobs,patch jobs,get secrets→ no. (The other way round,the new script couldn't create its Job, and the rollback would fail the same way.)
relay-deploy(install -o root -g root -m 755) anddiffit against git.sudo -u deploy env SSH_ORIGINAL_COMMAND=<sha> relay-deploy).This runs migration 001:
migrating with job.batch/migrate-…, thenApplying: …_init.sql.schema_migrationsand the tables asrelay. One hour later, the old Jobs are gone.Failed(
DeadlineExceeded), and the legacy rollback test (Phase 1's commita78934c…, full SHA)still rolls back with 0 failed requests.
For the reviewer
pods/loglets the deployer read every pod's log inrelay. Acceptable, given that nothinglogs a secret?