Repository navigation
feat[auto-upgrade]: hub endpoints, host agent, My Trust controls and upgrade email - #1657
garciadias wants to merge 10 commits into
Conversation
Signed-off-by: R. Garcia-Dias <rafaelagd@gmail.com>
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Signed-off-by: R. Garcia-Dias <rafaelagd@gmail.com>
…ime hub toggle Signed-off-by: R. Garcia-Dias <rafaelagd@gmail.com>
Signed-off-by: R. Garcia-Dias <rafaelagd@gmail.com>
Signed-off-by: R. Garcia-Dias <rafaelagd@gmail.com> # Conflicts: # Makefile # flip-api/src/flip_api/fl_services/services/fl_scheduler_service.py # trust/AGENTS.md
The root lockfile lost its `[options]` block, so `uv lock --check` re-resolved it against `exclude-newer = "3 days"` in pyproject.toml and failed the Dependency Cooldown Check. Regenerated with uv 0.11.16, the version CI pins; only the four `[options]` lines return, no dependency changes. Signed-off-by: R. Garcia-Dias <rafaelagd@gmail.com>
The agent decided purely from the hub's policy and the local git state, so it would recreate every container on a site that was already broken -- and then report SUCCEEDED on the exit code of `make` alone, which only says the deployment applied, not that the site came back. Pre-flight: refuse to start unless trust-api's /health is reachable, status is ok, no tasks are dead and hub_key_match holds, reported to the hub as a new SITE_UNHEALTHY blocker. It sits after the cheap SKIPs, so a disabled or already-current trust never reports a blocker it was not going to act on. Post-flight: poll /health until it serves the target release healthily, with a 10 minute budget, and report FAILED with the health body otherwise. The budget is generous because XNAT's schema upgrade dominates it and FAILED parks the release until an operator clears it. Signed-off-by: R. Garcia-Dias <rafaelagd@gmail.com>
… finishes An unattended upgrade runs in a window chosen because nobody is watching, so its only trace was the Trust panel. Send the trust's Trust Admins and the platform admin address one email when the lease is claimed and one when it resolves, reusing the existing SES sender and Terraform-managed templates. Sending is best-effort and happens after the commit: the upgrade record is the fact and must never be rolled back by a mail failure. BLOCKED is deliberately not emailed, since it repeats every hourly tick. Also fixes a contract break from faa92b7: the hub matched the human-readable `detail` against the known blocker codes, so SITE_UNHEALTHY stopped reaching the portal card. The code now travels in its own `blocker` field, with the old spelling still accepted for agents that have not upgraded themselves yet. Signed-off-by: R. Garcia-Dias <rafaelagd@gmail.com>
terraform plan -chdir=deploy/providers/AWSDiff of 2 changes.+ module.ses.aws_ses_template.flip_trust_upgrade_finished will be created
+ module.ses.aws_ses_template.flip_trust_upgrade_started will be createdPlan: 2 to add, 0 to change, 0 to destroy.
|
garciadias
changed the title
atriaybagur
left a comment
atriaybagur
left a comment
There was a problem hiding this comment.
Thanks — this is a well-shaped feature: the lease/lock protocol is the right idea, the trust-key routes reuse authenticate_trust, the new permission is trust-scoped and seeded correctly, the tag regex keeps hub-chosen values out of git/make argv, and I confirmed with a marker-filled kit that the upgrade's echoed command lines carry no kit secrets. The PR body's "look hardest at" list was useful; answers are inline (lease: the hub already allows renewal).
Requesting changes for four things that stop the feature working, each reproduced locally rather than read (how, in each comment):
- Two Alembic heads once develop is merged — flip-api will not boot (
alembic headson develop + this revision). - A fresh install never upgrades: the
--no-checkoutclone has no agent script and reads as dirty forever. - A failed final report leaves the lease held and the release retried;
kit_identityraises after the claim. - The scheduler's lock-and-look (and
record_status) read stale ORM state, so the idle-check race remains.
Then the stored-but-hidden blockers, the card's blank-window / 422 / poll-reset issues, the missing clear-failure control, and the docs claims that do not match the code. Seven mutations survive the current suite (listed on the test file). The rest are small tidy-ups.
Also ran a security pass over the new surface (authz on the six routes, argv construction, TLS, SES template escaping, v-html, secrets in the captured log tail): nothing to report. Worth constraining UpgradeStatusInput.release to the release-tag regex as hardening, since it is persisted, displayed and emailed as-is.
Verification: flip-api unit tests (121), the script suites (42, one macOS-only failure noted inline), the my-trust vitest spec (27), ruff and mypy all green on the branch; probes and mutation runs were against a detached worktree at c34ccaa.
|
|
||
| # revision identifiers, used by Alembic. | ||
| revision: str = "a91d3c5e7b20" # pragma: allowlist secret | ||
| down_revision: str | None = "161d07d3d134" # pragma: allowlist secret |
There was a problem hiding this comment.
Blocking: two Alembic heads after the merge. develop's head is now 9d4b7a1c5e20 (#1601, trust key rotation, merged 30 Oct), which also revises 161d07d3d134. The merge has no text conflict, and this PR's CI last ran on 29 Oct, so nothing flagged it. Reproduced with alembic heads over develop's versions dir plus this file: 9d4b7a1c5e20 (head) and a91d3c5e7b20 (head). flip-api's entrypoint runs alembic upgrade head fail-fast, so the hub would not boot.
Merge develop and set down_revision = "9d4b7a1c5e20".
| print(f"creating the agent's own checkout at {CHECKOUT}") | ||
| if not dry_run: | ||
| CHECKOUT.parent.mkdir(parents=True, exist_ok=True) | ||
| run(["git", "clone", "--no-checkout", repo_url, str(CHECKOUT)], dry_run) |
There was a problem hiding this comment.
Blocking: a fresh install can never upgrade. --no-checkout leaves an empty working tree and nothing afterwards checks a tag out: the only git checkout is inside the agent's upgrade(), which is never reached. Two effects, both reproduced on a throwaway git clone --no-checkout of this repo:
<CHECKOUT>/scripts/trust_upgrade_agent.pydoes not exist, so the systemd oneshot fails every hour (the hub toggle is already on by then).- Even started by hand,
git status --porcelain --untracked-files=noprints every tracked file asD(2757 lines here), socheckout_cleanis False and every tick reports BLOCKED / LOCAL_MODIFICATIONS.
Check out a tag here at install time (the operator checkout's tag, or the hub's current release). The idempotency test only asserts the clone command was run, which is why the suite does not see this.
| [Service] | ||
| Type=oneshot | ||
| Environment=HOME=/root | ||
| ExecStart=/usr/bin/env uv run --no-config {agent} tick --kit-file {kit_file} --checkout {checkout} |
There was a problem hiding this comment.
The unit sets no PATH, so /usr/bin/env uv resolves against systemd's default (/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin). That works on the Azure VM, where azure.yml installs uv into /usr/local/bin, but an on-prem host that followed the astral installer has it in ~/.local/bin and the timer exits 127 while sudo -E make … keeps working for the operator. Read, not executed on Linux.
Smallest fix: write shutil.which("uv") as an absolute path into ExecStart at install time and refuse to install when it is not found; the same PATH has to find docker for the make run.
|
|
||
| def migrate_kit(kit_file: Path, dry_run: bool) -> Path: | ||
| """Move the kit under /etc/flip/trust and symlink the old path. Idempotent; returns the new path.""" | ||
| if kit_file.is_symlink() and kit_file.resolve().parent == ETC_KIT_DIR: |
There was a problem hiding this comment.
kit_file.resolve() resolves symlinks in the whole path but ETC_KIT_DIR is compared unresolved, so on macOS (/var → /private/var) test_running_it_twice_is_harmless fails with "already exists but … is not its symlink". CI is Linux so it passes there; I hit it locally. ETC_KIT_DIR.resolve() on this line fixes both.
| report = {"state": "SUCCEEDED" if ok else "FAILED", "release": decision.release} | ||
| if not ok: | ||
| report["detail"] = detail | ||
| hub_call(kit, "POST", "/trust/upgrade-status", report) |
There was a problem hiding this comment.
Blocking: a failed final report loses the outcome and breaks "never retried". This call (and the BLOCKED one above) is outside any try. I ran tick() with hub_call returning 200 for the policy and the claim and raising HTTPError 502 here: unhandled traceback, exit 1 (which the docstring defines as "the upgrade ran and failed"), the lease stays held for two hours, and FAILED is never recorded. After the lease expires the next tick re-claims and re-runs the same release. The systemd TimeoutStartSec=2h kill has the same shape: SIGTERM mid-compose up, no report.
Suggest: wrap claim-to-report in try/finally that always posts FAILED (retry the POST a few times before giving up), and keep TimeoutStartSec below LEASE_TTL - VERIFY_TIMEOUT_S so the hub hears about a kill.
| An upgrade recreates database-backed services and runs XNAT's in-place schema upgrade. Rolling that back | ||
| automatically can do more damage than stopping, so the agent does not try. Instead it: | ||
|
|
||
| - stops, and reports **Failed** with the tail of the log, shown on My Trust (see the figure above); |
There was a problem hiding this comment.
No failed figure is on the page; assets/flip/trust-auto-upgrade/failed.png exists and is unreferenced.
| Turning it on or off | ||
| ******************** | ||
|
|
||
| - **Defaults.** A new installation has automatic upgrades **on**; the installer states this and says it can be turned |
There was a problem hiding this comment.
Only true once setup-auto-upgrade has been run and answered yes; up-onprem-trust just prints a hint. "setup-auto-upgrade defaults to on; a Trust where it was never run, and every Trust that predates the feature, is off" matches the code. Also worth one line: the agent checks site health (/health ok, no dead tasks, hub_key_match) before anything else, and the flowchart above checks window → job → checkout while decide() goes unsupported → checkout → health → window → job.
| - **Who can change it.** The Trust Admin of that Trust, and FLIP admins for support. | ||
| - **Changing it during an upgrade.** An upgrade already running finishes. The new setting applies from the next | ||
| release. | ||
| - **Not covered.** Snowflake-hosted Trusts are not upgraded automatically; they report that their mode is |
There was a problem hiding this comment.
There is no Snowflake anywhere in the repo. The code refuses kits whose DOCKER_HOST/DOCKER_CONTEXT is ssh:// or tcp:// at install (exit 2), so nothing is reported: "remotely driven Trusts (including the EC2 trust) are refused at install; on-prem hosts only" — which also duplicates the earlier bullet.
| @$(MAKE) onboard-onprem-trust KIT=$(KIT) ONBOARD_ARGS=--gate | ||
| $(MAKE) DEBUG=$(DEBUG) -C trust up-trust KIT=$(KIT) HUB=$(or $(HUB),prod) | ||
| @[ -n "$(AUTO_UPGRADE)" ] || [ -e /etc/systemd/system/flip-trust-upgrade.timer ] || \ | ||
| echo "ℹ️ Automatic upgrades are not configured: run sudo -E make setup-auto-upgrade KIT=$(KIT) HUB=$(or $(HUB),prod) (or pass AUTO_UPGRADE=yes|no)" |
There was a problem hiding this comment.
AUTO_UPGRADE= here only silences this hint; nothing installs or disables the agent. Either forward it to setup-auto-upgrade or drop the parenthesis.
| - **Decided** — a table of the projects you (or the hub, before your Trust had a Trust Admin) approved or declined, each row marked green or red with who decided and when. Click a row for its description and a link to its query. | ||
| - **Trust detail** — to the right, your Trust as the Connection Status page shows it: whether it is online, and the health and version of each of its services. | ||
|
|
||
| - **Automatic upgrades** — under the trust detail, the switch that lets your Trust upgrade itself to the hub's release when no training is running on it, with an optional maintenance window. See :ref:`admin-trust-auto-upgrade`. |
There was a problem hiding this comment.
The blank line above this bullet splits the list in two in Sphinx; remove it.
garciadias
changed the title
garciadias
changed the title
|
@garciadias assigning this back to you. There are no commits since 29 Oct ( Test coverage also needs to improve before this merges:
Please re-request review once the threads have replies and the fixes are pushed. |
Rebase the migration onto develop's head so there is one head again. Run the service's uv by absolute path: systemd's PATH has no ~/.local/bin, where the astral installer puts it, so /usr/bin/env uv exits 127. Install fails fast when uv is missing rather than leaving a unit that cannot start. Clone with a release tag checked out. A --no-checkout clone leaves an empty working tree, which loses the agent script and makes every tick report LOCAL_MODIFICATIONS. Cap TimeoutStartSec at 1h50m, the hub's 2h lease minus the 10m verify budget, so a timeout kill still leaves time to report FAILED; a kill the hub never hears about holds the lease until it self-expires. Terminal reports are retried for the same reason. Surface every reportable blocker, not just LOCAL_MODIFICATIONS: the other three were recorded and never shown. Carry the agent's detail alongside the code so an admin reading "not healthy" sees which check failed, and keep the UI keyed off the code so an unknown one is dropped rather than rendered raw. Refresh the scheduler's locked re-read with populate_existing, without which it returns the already-loaded attributes and misses a lease committed in between. Make Local.health required: an optional pre-flight is a representable "health check never ran".
|
Pushed f1774da. Walking the threads: Migration head — retargeted
Blockers stored but never shown — fixed; the view returns whatever blocker is current rather than only Compatibility fallback for an older agent — dropped. The agent ships for the first time in this PR, so there is no older one to be compatible with. Scheduler's locked re-read — good catch, it was not actually re-reading:
Tests: 2627 flip-api, 140 script-side, 29 UI unit. |
Description
Automatic upgrades for trusts: a trust can now follow the hub's release on its own, inside a window its own operators choose, without anyone running
upgrade-trustby hand.An agent on the trust host ticks hourly, asks the hub whether it should upgrade, and if everything lines up runs the existing mode-specific upgrade verb. The hub owns the toggle, the target release and the idle decision; the agent owns execution and reports back. Nothing contacts GitHub — a trust that polled releases directly could jump ahead of the hub, and every site would need outbound github.com.
Four slices, in one PR because they are a single contract: the agent calls the endpoints the UI renders, and the email reads the state the lease writes. Splitting them would put the two halves of a wire protocol in separate reviews, which is exactly how the blocker-field mismatch fixed in this branch happened in the first place.
What's here
Hub (
trusts_services/trust_auto_upgrade.py,services/auto_upgrade.py)GET /trust/upgrade-policyandPOST /trust/upgrade-status, authenticated with the existingTRUST_API_KEYinTRUST_API_KEY_HEADER— dedicated endpoints rather than riding the heartbeat, so the agent does not go blind exactly when trust-api is down.Trustcolumns (toggle, maintenance window + timezone, lease holder and expiry, last attempt) and migrationa91d3c5e7b20. Existing trusts migrate to off; nobody's live site changes behaviour because this merged.state=STARTINGclaims the trust, the scheduler assigns it no new jobs while held, the hub rejects a claim if a job landed in the meantime, and the lease self-expires so a dead agent cannot wedge a site.Host agent (
scripts/trust_upgrade_agent.py,scripts/setup_auto_upgrade.py)/healthis polled until it reports the target version, bounded.make setup-auto-upgrade KIT=<slot>is a separate interactive step, deliberately:onboard_onprem_trust.pyruns on everyup-onprem-trust, and aninput()there would hang scripted and CI installs.My Trust panel (
TrustAutoUpgradeCard.vue)Email (
private_services/upgrade_notifications.py)AWS_SES_ADMIN_EMAIL_ADDRESSonSTARTING,SUCCEEDEDandFAILED, over the SES path the hub already has. Not onBLOCKED— a blocked trust re-blocks every hour, and mailing that would train people to ignore the channel.Docs —
sys-admin/admin-trust-auto-upgradecovers the three modes, what each tick checks, the claim that stops a job arriving mid-upgrade, and failure handling. Six screenshots; a Cypress docs spec records the card's states.Please look hardest at
Not in this PR
Kubernetes (#1644) is deliberately held back: its CronJob cannot judge whether an upgrade succeeded until the Helm exit code is unambiguous, which is #1645. Versioned deployment artifacts are #1646.
Deployment note
SES must be out of the sandbox in the deployment account and region with the sender identity verified. While sandboxed it drops mail to unverified addresses, and because sending is best-effort by design that failure is invisible.
Linked Issues
Closes #1641, closes #1642, closes #1643, closes #1709. Part of #1640.
Checklist
2330 flip-api unit tests, 168 agent tests, mypy clean, ruff check and format clean,
terraform fmtclean.my-trust.spec.tspasses. The full vitest run has 71 failures in the auth and training-metrics specs; the same 71 fail on an unmodifieddevelop.Type of Change
make -C docs/ docs.