Repository navigation
infra: deploy script follow-ups from the #14 review - #19
Conversation
AbdikhaniAbdirashid
left a comment
AbdikhaniAbdirashid
left a comment
There was a problem hiding this comment.
Approving. All four line comments addressed, both questions answered in k8s/README.md, and I noticed you also put timeout-minutes: 10 on the two CI jobs in #16 — that was my point on #12. Thanks.
What I checked
- Ran the suite: 29 passed, 0 failed, as you say.
- Test 10 is a real test, not decoration. I removed the "re-run this job" wording from the script and re-ran:
28 passed, 1 failed, failing on exactly that assertion. Worth saying out loud because I found the opposite on #17 — thenc -Gcheck there can never fail, so it always "passes". Yours fails when the behaviour goes away, which is the whole point. - The namespace comment at line 16 is better placed than what I suggested. I'd have put it next to a
kubectlcall; next to theKUBECONFIGexport is where someone actually changing the kubeconfig will see it.
I accept the 180 s answer. One node, one image download, small code layers on top. The first deploy's log settles it either way, and a number we've measured beats one we've argued about.
One thing to sequence — see the line comment on the lock. Nothing wrong with this PR on its own; the problem is what it does to #16 if these merge in the wrong order.
| # The lock is released by itself when the script ends, even if it crashes. | ||
| exec 9>/home/deploy/.relay-deploy.lock | ||
| flock --wait 300 9 || fail "another deploy is still running" | ||
| flock --wait 900 9 || fail "another deploy is still running after 15 minutes: nothing was changed, re-run this job" |
There was a problem hiding this comment.
This makes #16 wrong until #16 is updated, so the merge order now matters.
#16 still has timeout-minutes: 20, with the comment "5 min waiting for the server's lock + 3 min rollout + 3 min rollback". Raising the lock wait to 15 min changes that sum:
| lock | rollout | rollback | total | #16's job timeout | |
|---|---|---|---|---|---|
| before this PR | 5 | 6 | 6 | 17 | 20 — fits |
| after this PR | 15 | 6 | 6 | 27 | 20 — over |
So if this merges and #16 goes in unchanged, a deploy that actually waits on the lock gets killed by GitHub mid-rollback: the cluster is left half-reverted and the log says "timed out" with none of the show_state output. That's the failure I was worried about in my comment on #16, now made more likely rather than less.
You say the 30-minute change happens in #16, and I believe you — it just isn't there yet, and #16 is a draft that gets rebased. Could the 30 go in before this merges, or could this PR carry it? Whichever is easier; I mainly want them not to be able to drift apart.
Smaller, for later rather than now: at three Deployments (worker, phase 2) it's 15 + 9 + 9 = 33, over even the new 30. Your argument that the lock rarely waits — because the pipeline's concurrency group already serialises main — is fair, and I'm not asking you to change the number. But the rule behind both numbers is "180 s × however many Deployments are in k8s/app/", and that rule still isn't written down next to either of them. One sentence in this comment block would mean whoever adds the worker knows there are two numbers to revisit, in two different files.
Step 1.8: every merge to main is deployed, then checked from outside. Rebased onto main now that #12, #14 and #15 are merged. **Merge only after the switch-over below.** ## What changes - **`api.yml` → `pipeline.yml`.** It now deploys too, and the dashboard's jobs will join it later (`needs:` only works inside one workflow file). - **No path filter on `main`.** Every merge is a release: a deploy needs an image tagged with exactly the merged commit, even when only `k8s/app/` or the docs changed. - **New job `deploy-script-test`** on every PR and merge. It runs `infra/server/test-relay-deploy.sh` (relay-deploy's gates and rollback, with fake git and kubectl) and `shellcheck` on the server scripts. The deploy needs it, so a broken deploy script never reaches the server. - **New job `deploy`**, after `api-image` and `deploy-script-test`, only on a merge to main: - **`environment: production`:** the only way to get `DEPLOY_SSH_KEY`, and only on main. - **`permissions: {}`:** no GitHub access at all. - **`timeout-minutes: 30`:** the worst case is the server's lock (15 min, #19) plus a rollout and a rollback. - **Step 1:** writes the key and our server's `known_hosts`, then `ssh … deploy@<vps> "$GITHUB_SHA"` (`StrictHostKeyChecking=yes`, `BatchMode=yes`). The exit code of `relay-deploy` becomes the step's. - **Step 2:** asks `https://api.myrelay.tech/version` until it answers this commit, for up to 36 × 5 s, because the first deploy also waits for a new certificate. - **`timeout-minutes: 10`** on `api-test` and `api-image` (from the #12 review). - **`k8s/hello/` is deleted**, together with its row in `k8s/README.md`. The API's Ingress takes over `api.myrelay.tech`. ## Tested - `actionlint` 1.7.7 (with shellcheck on the `run:` scripts): no findings. - The new job's two steps, run locally: `26 passed, 0 failed`, and `shellcheck` is clean. - The version check, run on its own with `bash -eo pipefail` against a local container: - the right SHA → passes; - an old SHA → keeps asking, then fails; - nothing answering → "no answer" on every try, then fails. - The rebase was rehearsed in a throwaway clone first: the 3 commits replayed onto main without conflicts. ## Already done before merging - #12, #14 and #15 merged. - `deployer.yaml` applied: the deployer can read events, and still can't read Secrets or delete. - `relay-deploy` installed, byte-identical to main. - The image is pullable without credentials (ADR 0004, #18), and the server has already pulled `relay-api:a8274c1`. ## Switch-over (right before merging) 1. Delete the hello app from `default`: the Ingress, Service and Deployment, plus its leftover TLS Secret `api-myrelay-tech-tls`. `api.myrelay.tech` is down for about 1–2 minutes, once. 2. Merge → the pipeline tests, builds, deploys → `/version` answers the commit. ## After the first deploy - **A real rollback:** deploy #15's merge commit `a78934c` by hand. It has no image, so the rollout fails and `relay-deploy` redeploys the last good commit, while a curl loop shows only 200s. - **Zero downtime:** a load loop during a normal deploy (merging #19) should show no failed requests. - Then make `api-test` and `deploy-script-test` required checks.
Follow-up to #14, from Abdikhani's review of it.
infra/server/relay-deploy.shconcurrencygroup already runs main's deploys one after the other. The lock protects against an admin deploying by hand at the same moment. The deploy job's timeout goes from 20 to 30 minutes in infra: deploy every merge to main and check that it's live #16.relayfrom the kubeconfig's context, set bysetup-deploy-user.sh.return 0(line 81):deploy_commitnow ends with an explicitreturn 0, so "0 on success" holds by construction.infra/server/test-relay-deploy.sh29 passed, 0 failed, andshellcheckis clean.k8s/README.md--prune: the old object stays and keeps serving. A rename = the new name in the PR, plus deleting the old object right after it merges.After merging: install the script again and post the
diffcheck here.