Add wide_schema to standard benchmark allowlist#15
Closed
adriangb wants to merge 123 commits into
Closed
Conversation
Replace bare-metal Python/Bash benchmarking system with: - Pulumi TypeScript infra (GKE Autopilot, Artifact Registry, Workload Identity) - Rust controller (StatefulSet) polling GitHub for benchmark triggers via sqlx/kube-rs - Runner container executing benchmarks on Performance-class spot instances - GitHub Actions workflows for deploy, image builds, CI, and scheduled benchmarks - Remove legacy Python scripts, CSV results, and report tooling Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Run cargo fmt on controller source - Use vars instead of secrets for GCP project ID, WIF provider, SA email - Fix pulumi/auth@v1 -> pulumi/auth-actions@v1 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Upgrade k8s-openapi to 0.24 (matching kube 0.98) - Fix SecretKeySelector.name type change (String, not Option) - Fix clippy: dead code, too_many_arguments, unused imports - Fix WIF attribute condition: adriangb not adriang Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace 9-argument insert_job function with a JobInsert struct for clearer call sites. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Commit Cargo.lock and package-lock.json - Copy Cargo.lock in controller Dockerfile - Use rust:1-bookworm (latest stable) in both Dockerfiles - Remove package-lock.json from .gitignore Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Add JobStatus enum to replace status string literals across db.rs and job_manager.rs - Add convenience methods on GitHubComment (body_text, login, url, etc.) - Extract duplicated mark_comment_seen calls in github_poller — mark once after validation - Extract not_allowed_message helper for duplicate "not whitelisted" replies - Use `let Some(..) else` instead of `.is_none()` + `.unwrap()` patterns - Make pr_number_from_url return Option<i64> instead of 0 sentinel - Extract check_response and request_builder helpers in GitHubClient - Add env_var helper to simplify repetitive EnvVar construction - Move hardcoded K8s constants (ephemeral storage, deadlines, storage class) to Config - Remove phantom "creating" status from get_active_jobs query - Add module-level and function doc comments across all files Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Move bootstrap resources (WIF pool, provider, gha-deployer SA) out of Pulumi into bootstrap_oidc.sh since they're prerequisites for GHA auth. Add scoped artifactregistry.writer on specifically our registry repo via Pulumi instead of project-wide artifactregistry.admin in bootstrap. Also: - Use docker/build-push-action with GHA cache in build-images workflow - Add rust-cache and npm cache to CI and deploy workflows - Improve controller Dockerfile with dependency layer caching - Rewrite README with architecture diagrams and component docs Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Add required requested-token-type to pulumi/auth-actions - Use docker/login-action with access_token instead of gcloud credential helper, which doesn't work with buildx's buildkit daemon Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Cover pure business logic and DB layer: benchmark detection/classification, env var parsing, PR URL extraction, queue formatting, model accessors, and full SQLite CRUD with in-memory databases. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Apply rustfmt formatting fixes across controller test files. Use numeric project number instead of project ID in bootstrap.sh WIF principalSet to prevent future IAM binding mismatches. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The organization token type requires Pulumi Enterprise. Switch all workflows to use personal access tokens via OIDC instead. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Personal tokens require scope: user:{username} to identify the
token owner. References PULUMI_USER GitHub Actions variable.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Pulumi v3 requires explicit 'value' for namespaced config keys (gcp:project, gcp:region) in Pulumi.yaml. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Cannot have both 'default' and 'value' on the same key. Namespaced config keys require 'value', not 'default'. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Use org-qualified stack name and configure gcp:project/gcp:region from GitHub Actions variables instead of hardcoding in Pulumi.yaml - Add stack select --create step before pulumi up - Remove hardcoded config values from Pulumi.yaml Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Namespaced config keys (gcp:project, gcp:region) can't have description-only entries. Config is now set entirely via pulumi CLI in the workflow. Updated bootstrap.sh output with all required GitHub Actions variables and Pulumi OIDC setup instructions. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Add pydantic/datafusion to RepoConfig with same benchmarks as apache/datafusion - Change default WATCHED_REPOS to pydantic/datafusion - Update services K8s env to watch pydantic/datafusion - Set githubToken Pulumi secret from GH_CONTROLLER_TOKEN secret in deploy-services workflow Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Merge build-images.yml and deploy-services.yml into build-and-deploy.yml so deploys always use the exact images just built. The deploy job runs after both build jobs and passes the git SHA as a Pulumi config value. Drop :latest tags and the kubectl rollout restart step. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Pulumi errors with ConcurrentUpdateError when two deploys run simultaneously. Use a concurrency group with cancel-in-progress: false so deploys queue instead of racing. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Add /healthz (liveness) and /readyz (readiness) TCP health endpoints on :8080 - Use CancellationToken for graceful SIGTERM/SIGINT shutdown so in-flight GitHub API calls and DB writes complete before exit - Add periodic cleanup task to prune seen_comments and terminal jobs older than 30 days - Overlap poll scan timestamps by 2 intervals to avoid missing comments on restart - Configure startup/liveness/readiness probes in K8s StatefulSet - Expose port 8080 in Dockerfile Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Add hourly cleanup task: removes seen_comments older than the oldest scan timestamp and benchmark_jobs older than 30 days (any status) - Store last_scan_at with 2-interval overlap to prevent missed comments on restart; seen_comments dedup prevents double-processing - Change default poll interval from 30s to 5s - Add tests for cleanup functions including edge cases Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Add backon-based retry with exponential backoff for all GitHub API
calls (3 attempts, retries on 5xx/429/network errors, respects
Retry-After header)
- Add pagination for fetch_recent_comments following Link rel="next"
headers, capped at 100 pages (10,000 comments)
- Simplify K8s Job names from bench-{id}-{hash} to bench-{id}
- Move mark_comment_seen after job insert for trigger comments so
transient insert failures allow retry on next poll cycle
- Make reconcile_loop return Result so K8s client init failures
propagate and shut down the controller for pod restart
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Runner scripts already post failure details to GitHub via `gh pr comment`. The controller was duplicating this by reading pod logs and posting them, causing double comments on failure. Now reconcile_active only updates DB status, and the read_pod_error function is removed. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
WATCHED_REPOS was set to adriangb/datafusion but for_repo() only recognized apache/datafusion and pydantic/datafusion, causing the poller to skip every poll cycle with "unknown repo". Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Consolidate WATCHED_REPOS, ALLOWED_USERS, and per-repo benchmark allowlists into a single BENCHMARK_CONFIG env var parsed as JSON. This eliminates the dual-source bug where a repo could be in WATCHED_REPOS but missing from RepoConfig::for_repo(). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Move mark_seen after post_comment in all reply paths so failed replies get retried on next poll. Change error format from %e to ?e to show the full anyhow error chain including HTTP status codes. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Also adds smj to README docs (was missing from previous commit). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The cgroup io.stat-based io_read_bytes/io_write_bytes were unreliable proxies for DataFusion spilling since they captured all process I/O. Instead, create a dedicated spill directory per benchmark run, pass it via DATAFUSION_RUNTIME_TEMP_DIRECTORY, and poll directory size every 1s to track peak spill usage directly. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…refs When a user specifies a baseline_ref or changed_ref that lives on the PR branch rather than main, the shallow clone doesn't have that commit. Add fetch_pr_ref() to fetch the PR ref into any clone before checkout, fixing "reference is not a tree" failures. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Replace manual division guard with checked_div to fix clippy::manual_checked_ops - Skip Pulumi preview jobs for fork PRs since GitHub blocks OIDC id-token:write for external forks Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
DataFusion now includes io_uring-based local file I/O (apache/datafusion#21673), but the default container seccomp profile blocks io_uring_setup. Adds a checked-in seccomp profile (services/seccomp/io-uring-allowed.json) that's the standard containerd default allowlist plus three syscalls: io_uring_setup, io_uring_enter, io_uring_register. Deployment: - A Pulumi DaemonSet (services/seccomp.ts) copies the profile from a ConfigMap to /var/lib/kubelet/seccomp/profiles/ on every node - The controller and benchmark-main workflow reference it via seccompProfile.type: Localhost No init containers needed — the DaemonSet keeps the profile present on all nodes continuously. Co-authored-by: Adrian Garcia Badaracco <1755071+adriangb@users.noreply.github.com>
Runner pods no longer receive GITHUB_TOKEN. They post PR comments by
calling the controller's new POST /jobs/{id}/comment endpoint, which the
controller authenticates via a random per-job runner_token and services
with its own GitHub client. This limits the blast radius of a malicious
PR to "post a comment on its own PR while its job is still running"
instead of "exfiltrate a GitHub token and act as its owner".
The main-tracking workflow (trusted apache/datafusion main) keeps
using GITHUB_TOKEN directly via the new PosterMode::Direct path.
Also enforce per-user concurrency: max 5 running and 15 queued. Running
is checked at pickup (get_pending_jobs skips users already at the cap);
queued is checked at ingestion with a friendly rejection reply comment.
And switch benchmark pods' seccomp profile from the custom Localhost
profile (rejected by GKE Autopilot) to Unconfined, which unblocks
io_uring without requiring a node-side profile file. The Autopilot
admission controller accepts Unconfined under baseline PSS.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Two issues surfaced after the previous commit removed GITHUB_TOKEN from
PR-triggered runner pods:
1. runner/git.rs::checkout_pr called `gh pr view` to resolve the PR's
source branch name, which silently requires GITHUB_TOKEN. Move the
lookup to the controller (new GitHubClient::get_pr_head_ref) and pass
the result to the pod as PR_HEAD_REF; runner falls back to `gh` only
when the env var is absent (main-tracking path, where GITHUB_TOKEN is
still set).
2. health::handle_job_comment deserialized the request body into
`CommentReq<'a> { body: &'a str }`, but JSON escape sequences (every
comment body contains `\n`) require an allocated String — serde
returned an error and the handler responded 400, eating every runner
comment attempt. Change `body` to `String`.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Retry every error when the runner posts comments to the controller; brief outages during redeploys or Autopilot preemptions shouldn't fail a whole benchmark run. Bumps controller requests/limits to 1 CPU / 512Mi (Guaranteed QoS) so GKE is less eager to evict it. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…the PR Cut from 1h in 37900d8 made tpch jobs silently hit activeDeadlineSeconds before the runner could post its completion or error comment: the build + baseline + branch + compare workflow doesn't fit for tpch on c4a-highmem-16. Restores the 7200 default (matching the docstring) and posts a PR comment when the controller observes a terminal Failed condition with reason=DeadlineExceeded, so deadline hits stop going silent. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Upstream apache/datafusion#21707 ported `bench.sh run tpch` to a new Criterion-based SQL harness. The new harness reads parquet from a path relative to ${DATAFUSION_DIR}/benchmarks (where data isn't generated in our layout) and writes timings to target/criterion/, which our `bench.sh compare_detail` step doesn't understand. Recent benchmarks were failing because lineitem resolved to an empty external table. The dfbench tpch subcommand still exists upstream, so for the four tpch variants in our allowlist (tpch, tpch10, tpch_mem, tpch_mem10) invoke the prebuilt dfbench binary directly with the same arguments the old run_tpch used and write JSON to where compare_detail expects it. Other benchmarks still go through bench.sh.
The infra-preview and services-preview jobs were missing the GCP authentication and pulumi config steps that the deploy workflows already use, so pulumi preview couldn't authenticate to GCP and the services stack was missing its required githubToken config. Mirror the same setup the deploy jobs use (deploy-infra.yml for infra and build-and-deploy.yml's deploy job for services) so PR previews succeed for same-repo PRs. Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Add instructions to the README for running specific benchmark suites with filters.
wide_schema is a bench.sh standard benchmark (added upstream in apache/datafusion#21970): `bench.sh data/run wide_schema`, backed by a gen_wide_data binary and sql_benchmarks/wide_schema query set. It was missing from the standard allowlist, so `run benchmark wide_schema` fell through to the wildcard criterion path and ran `cargo bench --bench wide_schema`, which has no such target and failed. Adding it to the standard list routes it to the bench.sh runner. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
wide_schemais abench.shstandard benchmark added upstream in apache/datafusion#21970 (bench.sh data/run wide_schema, backed by agen_wide_databinary and asql_benchmarks/wide_schemaquery set — 1024-col × 256-file wide dataset vs an 8-col narrow baseline).It was missing from the standard allowlist, so
run benchmark wide_schema(e.g. on apache/datafusion#22829) fell through to the wildcard criterion path and rancargo bench --bench wide_schema— which has no such target — and failed with cargo's "available bench targets" dump.Changes
wide_schemato thestandardlist forapache/datafusionand theadriangb/datafusiontest fork inservices/controller.ts, routing it to thebench.shstandard runner.🤖 Generated with Claude Code