diff --git a/.github/actions/arc-job-report/README.md b/.github/actions/arc-job-report/README.md index 258494a..2871135 100644 --- a/.github/actions/arc-job-report/README.md +++ b/.github/actions/arc-job-report/README.md @@ -2,13 +2,14 @@ Keeps **one comment per PR** that shows how each job of a workflow run used its self-hosted ARC runner, which jobs would fit a different runner size, and how the -run's wall time changes from commit to commit. +run's wall time changes from commit to commit. Each reported workflow gets its own +section in that comment, and a run only updates its own workflow's section. -Collapsed, the comment is a single line: +Collapsed, each section is a single line: -> ▶ ✅ **4m 33s** (▲ 18s vs previous run) · 🏃 ARC runners · [run #6228](#) · 315e3c4 · 💡 2 runner changes +> ▶ Unit tests · ✅ **4m 33s** (▲ 18s vs previous run) · 🏃 ARC runners · [run #6228](#) · 315e3c4 · 💡 2 runner changes -Expanded, it has: +Expanded, a section has: - **Jobs:** each job's duration, with the job name linking to its pod on the [ARC Job Resources dashboard](https://montaapp.grafana.net/d/arc-job-resources). @@ -23,10 +24,12 @@ Expanded, it has: - **Run history:** one row per run (commit, ✅/❌, wall time, change vs the previous run, slowest job), so speed changes can be traced to commits. The history is stored as JSON in a hidden HTML comment and re-rendered on every - run, keeping the last 50 runs. + run, keeping the last 50 runs, split evenly between the sections. The comment is edited in place on every run. Only a bot-authored comment is updated, so a comment someone posted by running the script locally is left alone. +A comment from before sections existed is adopted as the reporting workflow's +section, history included. ## Usage @@ -62,6 +65,11 @@ jobs: No checkout is needed: the action ships its own scripts. Jobs that did not run on an ARC runner (`arc-*` / `self-hosted-*`) are skipped. +To report several workflows, list them all in `workflows:`; they share the PR's one +comment. If they run on `push` rather than `pull_request` (as in monta-app/server), +drop the `event == 'pull_request'` condition: the report finds the PR from the +commit, and a run whose commit has no PR is skipped without failing. + | Input | Required | Description | |---|---|---| | `github-token` | yes | Reads the run's jobs and writes the comment. Needs `actions: read` and `pull-requests: write`. | diff --git a/.github/actions/arc-job-report/arc_job_report.py b/.github/actions/arc-job-report/arc_job_report.py index a997441..62fd8da 100644 --- a/.github/actions/arc-job-report/arc_job_report.py +++ b/.github/actions/arc-job-report/arc_job_report.py @@ -6,8 +6,8 @@ Needs `gh` authenticated (GH_TOKEN) and GRAFANA_TOKEN, or `gcx` for local runs; see job_resources.py. -The PR gets one comment: the latest run's per-job table, plus a history row per run so -speed changes can be traced to commits. Only a bot-authored comment is updated, so run it +The PR gets one comment with a section per workflow: the workflow's latest run's per-job +table, plus a history row per run so speed changes can be traced to commits. Only a bot-authored comment is updated, so run it locally with --dry-run, which prints the comment instead of posting it. """ import json @@ -20,6 +20,7 @@ MARKER = "" HISTORY = re.compile(r"", re.S) +SECTION = re.compile(r"\n(.*?)(?=\n\n{s}" for w, s in sorted(sections.items()))]) + + def main(): args = [a for a in sys.argv[1:] if not a.startswith("--")] if not args: @@ -181,13 +188,14 @@ def main(): run = json.loads(sh("gh", "api", f"repos/{repo}/actions/runs/{run_id}")) prs = run["pull_requests"] or json.loads(sh("gh", "api", f"repos/{repo}/commits/{run['head_sha']}/pulls")) if not prs: - sys.exit(f"no pull request found for run {run_id}") + print(f"no pull request found for run {run_id}, nothing to report") + return pr = prs[0]["number"] results = analyse_run(args[0]) comments = json.loads(sh("gh", "api", "--paginate", "--slurp", f"repos/{repo}/issues/{pr}/comments")) existing = next((c for page in reversed(comments) for c in reversed(page) if MARKER in c["body"] and c["user"]["type"] == "Bot"), None) - body = render(run, results, existing and existing["body"]) + body = update(existing and existing["body"], run["name"], run, results) if "--dry-run" in sys.argv: print(body) elif existing: diff --git a/.github/actions/arc-job-report/test_arc_job_report.py b/.github/actions/arc-job-report/test_arc_job_report.py index 18ee38c..ae2c1e3 100644 --- a/.github/actions/arc-job-report/test_arc_job_report.py +++ b/.github/actions/arc-job-report/test_arc_job_report.py @@ -4,12 +4,12 @@ sys.path.insert(0, str(Path(__file__).parent)) -from arc_job_report import render # noqa: E402 +from arc_job_report import MARKER, render, update # noqa: E402 from job_resources import history, suggest # noqa: E402 -def run(number, attempt=1, ended="2026-09-28T11:07:46Z", conclusion="success"): - return {"run_number": number, "run_attempt": attempt, "html_url": f"https://example/runs/{number}", +def run(number, attempt=1, ended="2026-09-28T11:07:46Z", conclusion="success", workflow="Unit tests"): + return {"name": workflow, "run_number": number, "run_attempt": attempt, "html_url": f"https://example/runs/{number}", "head_sha": f"{number}abcdef", "conclusion": conclusion, "run_started_at": "2026-09-28T11:00:00Z", "updated_at": ended} @@ -37,7 +37,7 @@ def test_history_keeps_earlier_runs_sorted_replaces_a_reported_run_and_shows_the rows = history_rows(body) self.assertEqual([r.split("]")[0] for r in rows], ["| [#10", "| [#12"]) self.assertIn("| ✅ | 8m 00s | ▲ 14s | Compile (1m 20s) |", rows[1]) - self.assertIn("✅ 8m 00s (▲ 14s vs previous run)", body) + self.assertIn("Unit tests · ✅ 8m 00s (▲ 14s vs previous run)", body) def test_merges_shards_into_one_row_with_a_link_per_shard_and_the_slowest_in_bold(self): body = render(run(1), [job("Tests - shard 1", 110), job("Tests - shard 0", 130), job("Tests - shard 2", 90)], None) @@ -109,6 +109,36 @@ def test_omits_the_suggestion_table_when_every_runner_fits(self): self.assertNotIn("💡", body) +class UpdateTest(unittest.TestCase): + def test_keeps_one_section_per_workflow_and_replaces_only_the_reporting_one(self): + body = update(None, "Unit tests", run(5), [job("Compile", 100)]) + body = update(body, "Integration tests", run(9, workflow="Integration tests"), [job("Suite", 200)]) + body = update(body, "Unit tests", run(6), [job("Compile", 80)]) + + self.assertTrue(body.startswith(MARKER + "\n\n\n
")) + self.assertEqual(update(body, "Unit tests", run(6), [job("Compile", 80)]), body) + self.assertEqual(body.count("")[1].split("| Run |")[0]) + self.assertEqual([r.split("]")[0] for r in history_rows(body)], ["| [#9", "| [#5", "| [#6"]) + + def test_adopts_a_comment_from_before_sections_as_the_reporting_workflow(self): + legacy = MARKER + "\n" + render(run(5), [job("Compile", 100)], None) + body = update(legacy, "Unit tests", run(6), [job("Compile", 80)]) + + self.assertEqual(body.count(""), 1) + self.assertEqual([r.split("]")[0] for r in history_rows(body)], ["| [#5", "| [#6"]) + + def test_splits_the_run_history_between_the_workflows(self): + body = None + for n in range(1, 40): + body = update(body, "Unit tests", run(n), [job("Compile", 100)]) + body = update(body, "Integration tests", run(n, workflow="Integration tests"), [job("Suite", 100)]) + + self.assertEqual(len(history_rows(body)), 50) + + def sized(peak_mem_gib=3.0, cpu_p50=1.0, cpu_peak=2.0, runs=40, oom=0, runner=POD): return suggest(runner, "arm64", peak_mem_gib, cpu_p50, cpu_peak, runs, oom) diff --git a/CLAUDE.md b/CLAUDE.md index 6791a55..8267c6b 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -2,14 +2,15 @@ ## Last Documentation Update - **Date**: 2026-10-13 -- **Latest SHA**: 79f05d3 (check for newer commits) -- **Changes**: `arc-job-report` merges ` (shard N)` / `(shard-N)` shards into one row in both tables +- **Latest SHA**: 821e53e (check for newer commits) +- **Changes**: `arc-job-report` keeps one comment per PR with a section per workflow ## Recent Changes (2026-10-13) 1. **`argocd-resource-name` on the v2 deploy workflows**: `deploy-generic-v2.yml` and `deploy-kotlin-v2.yml` accept `argocd-resource-name` and pass it through `component-deploy-v2.yml` to `argocd-wait-sync`'s existing `resource-name` (kind stays `Deployment`). The wait then requires the expected revision, `Synced`, and that Deployment healthy, and ignores the app's aggregate health. Without it, any Degraded resource in the app fails the deploy on the first poll: service-monta-factory run 38388140776 failed with its Deployment `Healthy` because a CronJob's last Job had failed. Empty (the default) keeps tracking the whole app. 1. **`arc-job-report` reads `mimir-internal` at 15 s**: ARC's cAdvisor and kube-state-metrics series stopped arriving in `grafanacloud-prom` around 2026-10-01, so every job came back `no metrics` and no runner was ever suggested. `job_resources.py` queries `mimir-internal` (uid `ffzoywkuhm5tsd`) at a 15 s step with `rate[1m]`; the old 60 s / `rate[2m]` reported 2-minute averages and hid CPU bursts (monorepo Lint p90 peak 1.8 → 3.3 cores). 1. **`arc-job-report` sizes runners from the job's last 7 days**: one run is not safe to size on, because cache hits change a job's peak memory a lot (monorepo Typecheck: 1.5 GB typical, 13.1 GB worst). The history comes from the `arc:job_memory_working_set_bytes:max1m` and `arc:job_cpu_usage_cores:rate1m` recording rules in monta-app/terraform, which join each runner pod to its GitHub job (`repository`, `job_name`, `workflow_run_id`) once a minute. Doing that join at read time over 7 days OOM-killed the mimir-internal query-frontends, so the report must only read the recorded series. Runners have a memory limit but only a CPU request, so memory is a hard floor (1.3 × the 7-day peak; an OOM kill always moves up a size) and CPU is a labelled trade-off: ⬆️ *CPU-bound, faster* when the median is ≥ 85% of the runner's vCPU; a smaller size must cover the median and is *fits* or *trial: may be slower* depending on the peak. Smaller sizes need 5 runs in the window; there is no backfill, so they start a week after the rules are applied. The old 2-minute cut-off is gone, since the history covers short jobs. The local path uses `gcx api` against the same datasource-proxy URL as CI, because `gcx metrics query` silently widens the step to about 100 points. 1. **`arc-job-report` groups `(shard N)` and `(shard-N)` shards**: shards were only recognised as ` - shard N` (service-ocpi; server uses `unit-tests (shard-N)`), and `name()` strips a `(…)` suffix before that check, so monorepo-typescript's `Test (shard N)` jobs collapsed into one jobs-table row together with the separate `Test` gate job (linking only the slowest shard), and got one suggestion row each ("💡 11 runner changes", 7 of them identical Test rows). Both formats now group into a `Test · shards 1 2 …` row in both tables; in the suggestion table shards are grouped per change and reason, showing the highest memory and CPU and the fewest runs, and the 💡 count is the number of rows. Jobs that aren't shards are grouped by their full name, so two jobs differing only in a `(…)` suffix keep separate rows. +1. **`arc-job-report` keeps one comment per PR with a section per workflow**: monta-app/server runs its ARC jobs in several workflows (`Unit tests`, `Integration tests`, `Static checks`, `Smoke tests`), and with a single section each workflow's run would have overwritten the others' table and mixed their run histories. Each section starts with ``; a run re-renders only its own, and the 50-run history is split evenly between sections to stay under GitHub's comment limit. A comment from before sections is adopted as the reporting workflow's section, history included. A run whose commit has no PR now prints a message and exits 0 instead of failing, since push-triggered workflows on a branch without a PR are normal (monorepo-typescript had a red run from it). ## Recent Changes (2026-10-07) 1. **Terraform plans are stored beside the state, not as Actions artifacts**: `terraform-stack.yml` writes the saved plan to `s3:///tfplans//.tfplan`, read from the initialised backend in `.terraform/terraform.tfstate`, and the apply downloads and then deletes it. A saved plan is a zip of the full prior state (`tfstate`, `tfstate-prev`) plus every variable value, `sensitive` or not, and Actions artifacts are downloadable by anyone with read access to the repository — the whole org. Verified locally: a `sensitive` data-source value was absent from the text plan and present in `terraform show -json tfplan`. The shared `tfplans/` prefix is what lets one bucket lifecycle rule expire unapplied plans and the noncurrent versions left by the post-apply delete (S3 lifecycle filters only match a literal key prefix); the cost is that roles scoped to their own state prefix need `tfplans//*` added. Stacks must use the `s3` backend; anything else fails with `Unsupported backend`. `plan-retention-days` and the `Slug the stack path` step are gone, and callers no longer need `actions: write`. The runner needs the AWS CLI. Callers pinned to an older SHA keep uploading artifacts until they bump, and a PR planned under the old pin finds no plan after the bump, so it needs a fresh plan or a `recover`.