Compare built-in and Rust load on FYRE - #17
Conversation
Signed-off-by: lucarlig <luca.carlig@ibm.com>
Signed-off-by: lucarlig <luca.carlig@ibm.com>
Signed-off-by: lucarlig <luca.carlig@ibm.com>
Signed-off-by: lucarlig <luca.carlig@ibm.com>
Signed-off-by: lucarlig <luca.carlig@ibm.com>
Signed-off-by: lucarlig <luca.carlig@ibm.com>
Signed-off-by: lucarlig <luca.carlig@ibm.com>
Signed-off-by: lucarlig <luca.carlig@ibm.com>
Signed-off-by: lucarlig <luca.carlig@ibm.com>
Signed-off-by: lucarlig <luca.carlig@ibm.com>
Signed-off-by: lucarlig <luca.carlig@ibm.com>
Signed-off-by: lucarlig <luca.carlig@ibm.com>
Signed-off-by: lucarlig <luca.carlig@ibm.com>
Signed-off-by: lucarlig <luca.carlig@ibm.com>
Signed-off-by: lucarlig <luca.carlig@ibm.com>
Signed-off-by: lucarlig <luca.carlig@ibm.com>
Signed-off-by: lucarlig <luca.carlig@ibm.com>
Signed-off-by: lucarlig <luca.carlig@ibm.com>
Signed-off-by: lucarlig <luca.carlig@ibm.com>
Signed-off-by: lucarlig <luca.carlig@ibm.com>
Signed-off-by: lucarlig <luca.carlig@ibm.com>
Signed-off-by: lucarlig <luca.carlig@ibm.com>
Signed-off-by: lucarlig <luca.carlig@ibm.com>
Signed-off-by: lucarlig <luca.carlig@ibm.com>
Signed-off-by: lucarlig <luca.carlig@ibm.com>
msureshkumar88
left a comment
There was a problem hiding this comment.
Review summary
Reviewed against the PR's own stated goals (no linked issue exists in this repo). Local-load fixes are minimal and correct; FYRE campaign is proportionate to the stated need. Requesting changes on 3 items before merge.
Blocking
-
benchmarks/fyre/campaign.py:272-278— command injection (CWE-78):inventory["fast_time"]["private_ip"]is concatenated directly into a remote shell command string instead ofshlex.quote()'d, inconsistent with every other remote command in this file. One-line fix. -
Undocumented possible breaking change:
docker/docker-compose.cf-dataplane.yamland-standalone.yamlsilently drop theCONTEXTFORGE_DATA_PLANE_USER_CONFIG_CACHE_EXPIRY_SECONDSoverride (previously commented as needed to prevent stale-config visibility under steady traffic). The new per-scenario Redisglobal_config_writerpath only fixes this for conformance runs with a fresh UUID subject per scenario — any other consumer of these compose files driving them directly with a long-lived stack loses the old protection with no flag/opt-out/changelog note. Please confirm this is intentional and either restore an equivalent short/zero-TTL default for the general path, or call it out explicitly as a breaking change with a migration note. -
Scope creep:
src/runtime/conformance/mod.rs,src/helpers/config.rs/mod.rs, and the newdocker/docker-compose.cf-dataplane-config.yamlimplement an unrelated conformance token-issuance rework (per-scenario locally-minted tokens replacing one shared revocable catalog token) plus a new Redis config-publish feature. Neither implements any of the four problems this PR states (Locust single-process cap, fd limits, worker-exit-code propagation, VM-comparison capability). Recommend splitting into its own PR for a clean review/revert trail, ideally reviewed by a conformance/auth owner.
Should-fix (non-blocking)
benchmarks/fyre/terraform/main.tf— no firewall/security-group resource restricts inbound to the provisioned VMs; dataplane:4445 and fast-time:9080 (host network) are reachable from the public internet, and Fast Time's/healthhas no auth.src/runtime/fyre.rs— the async orchestration paths (helper-saturation/archive/restart inrun_fyre_matrix,terraform_destroy's retry loop) have zero test coverage; only pure/sync config-validation functions are tested. Highest blast-radius untested code in the PR given it's credential-handling + billed-cloud-spend logic.benchmarks/fyre/report.pyanddeploy/monitor.py(producer side) — zero test coverage; the numbers that land in the published comparison report/PNG are unverified.src/runtime/stack/mod.rs—--isolate-cpus/--builtin-memory-limitare only proven at CLI-parse/static-YAML level, not proven end-to-end to populate the env vars on the executed command.src/runtime/fyre.rs:938vssrc/app.rs—validate_run_idduplicated verbatim in both files; this is the path-traversal guard and needs to stay in sync by hand.scripts/locustfile_mcp.py—_TARGET_SEQUENCEround-robin counter forMCP_BASE_URLSis per-process, not shared, so under--workers > 1traffic skews toward low-index replicas — undercuts the exact multi-replica comparison FYRE is built for.scripts/locustfile_mcp.py— master-side "distributed worker failed" log omits the worker/node id, can't attribute which worker failed with multiple workers running.- PR description says "FYRE campaign unit tests — 9 passed"; actual
test_campaign.pyhas 16 tests (verified by running the suite). Please correct the PR body. release: prepare cf-integration 0.5.0commit is buried mid-branch with 12 more commits appending to the same changelog section afterward — risks a premature/incomplete tag givenrelease-plz.tomlhas tag/release automation enabled. Consider moving the version bump to the tip.
Everything else checked out clean: no dead code, no hardcoded secrets, no yaml.load/shell=True patterns, credential handling in fyre.rs verified sound (never in args/logs/manifests/state), fd-limit/cpuset changes empirically verified safe when the new flags aren't used, docs match implementation elsewhere. No linked GitHub issue exists in this repo — recommend filing one for traceability.
Signed-off-by: lucarlig <luca.carlig@ibm.com>
Signed-off-by: lucarlig <luca.carlig@ibm.com>
Signed-off-by: lucarlig <luca.carlig@ibm.com>
|
Addressed the review on the latest branch:
The removed global cache override remains intentional. These Compose files are internal assets owned by this development CLI; direct long-lived Compose consumers are not a compatibility target. The CLI config writer and isolated per-scenario identities are the supported path, so I did not add a compatibility shim. I also kept the conformance token/config work in this PR because it fixes the CLI-owned JWKS/config lifecycle used by these workflows, and the requested scope is to keep the complete dev-tool change in PR #17 rather than split it. |
Signed-off-by: lucarlig <luca.carlig@ibm.com>
Signed-off-by: lucarlig <luca.carlig@ibm.com>
Signed-off-by: lucarlig <luca.carlig@ibm.com>
|
@msureshkumar88 I rechecked each review item against the current branch and pushed the remaining fixes. Blocking items
Other findings
Current validation: 431 Rust tests passed, 24 FYRE tests passed, Clippy is clean with warnings denied, formatting passes, and comparison JSON/CSV/PNG rendering succeeds. |
msureshkumar88
left a comment
There was a problem hiding this comment.
Re-review
Verified the 3 items from the prior review round directly against the code (not the PR comments):
- Command injection (CWE-78) in
campaign.py— fixed.shlex.quote()now wrapsbackend_urlin both the older scaling path and the new rust/builtin comparison paths (lines 301-302, 1045, 1102). Regression test injects"10.0.0.2; touch /tmp/unquoted"asprivate_ipand asserts the quoted form. - Cache-expiry removal disclosed as intentional breaking change — done, PR body documents it.
- Conformance/config scope creep — not split out; reasoned justification given (shared JWKS/token/Redis lifecycle). Judgment call, acceptable.
Also verified: FYRE-private-only bind IPs, per-worker MCP_REPLICA_OFFSET plumbing, worker ID in fail-fast diagnostics, duplicate validate_run_id removed, test count claim accurate, release notes at tip.
Fresh review + adversarial verification
Ran a fresh pass and then adversarially verified every finding against the code before trusting any of it (including my own). Net: no blocking defects survived verification. Two initially-flagged items collapsed because the reviewer (me) stopped grepping at the repo boundary — GATEWAY_MEM_LIMIT is consumed by the upstream mcp-context-forge compose file pulled into the controlplane checkout, not by anything in this repo, and the dataplane cache-expiry change is a semantics inversion (TTL=0 disables the cache for functional tests; FYRE's :60 restores the image default for realistic throughput, not a kept-vs-dropped security control).
No alembic migration touched. No linked issue number in the PR body.
Small non-blocking cleanups worth taking
benchmarks/fyre/campaign.py:1046—protocol_versionis interpolated into the SSH command unquoted, while the equivalent site at:302quotes it (that site was hardened in the last review round for the same class of issue). Value is operator-controlled from committed campaign YAML, so this is a consistency/hardening gap rather than a remotely exploitable injection, but it should match its sibling..env.example:99-101—CF_DATAPLANE_USER_CONFIG_CACHE_EXPIRY_SECONDSdocumentation is now dangling; its only consumer (docker-compose.cf-dataplane*.yaml) was removed by this PR.benchmarks/fyre/report.py:41(rust["rps"] / builtin["rps"]) — theoretically divides by zero if a phase completes with zero requests and zero failures. Current locust timeout/fail-fast config makes this practically unreachable, but guarding it at the source (read_stats:passed = failures == 0 and requests > 0) is cheap and also covers the same pattern atreport.py:229,255.src/runtime/fyre.rs—terraform_destroy's retry/backoff and the helper-saturation restart/archive path have no test coverage. Verified the logic itself is currently correct (no off-by-one), so this is a coverage request, not a defect.ProcessRunnerinjection seam already exists with fakes in-tree, so a#[tokio::test]is cheap to add.
None of these block merge. Approving.
🤖 Generated with Claude Code
The existing load workflow could not produce a fair built-in-dataplane versus external-dataplane comparison. The lanes could use different MCP client behavior, a single Locust process capped load generation, distributed worker failures could be hidden, and the FYRE workflow required manual infrastructure and artifact handling.
This PR releases
cf-integration0.5.0 with one repeatable CLI workflow for local load testing and FYRE comparison campaigns. The default FYRE command provisions the infrastructure, bootstraps it with Ansible, runs the complete comparison, downloads raw evidence, generates reports, and destroys only VMs owned by that run.Default FYRE comparison
cf-integration load fyre runprovisions three Ubuntu 24.04 VMs:FastHttpUser, three distributed workers, zero waitThe eight default measurements are the built-in dataplane and external dataplane at 125, 250, 500, and 1,000 users. Every point uses a 30-second ramp, a 30-second steady warmup, a statistics reset, and a 60-minute measured window.
Both lanes use the same stateless MCP
2026-07-28Locust client and logical workload. Each user performsserver/discoverbefore measurement; measured traffic contains only onetools/callper request, uniformly selected from convert-time, echo, get-stats, get-system-time, schema-success, and verify-protocol. There are no measuredresources/read, prompt, or request fan-out calls. The built-in gateway owns any upstream protocol/session behavior.The target stack is reset between lanes. The built-in lane uses the Python gateway with four workers, PostgreSQL, and Redis. The external lane uses the Rust dataplane, Redis, and a loopback JWKS helper. The built-in fixture image is pinned to public multi-architecture manifest
ghcr.io/contextforge-org/cf-integration-fixture@sha256:5b206e6f863cea9f8cabea6451392428fbe67b16bdf10fefd1f7ac8032d95b52, built fromIBM/mcp-context-forgecommit33e2dd93a53a9cc2c5088b731822dfec4852fa2eon the MCP SDK v2 branch.Commands
The earlier Rust-only capacity search remains available through
vertical-low-memory.yaml. Comparison report labels and allocations come from the selected profile rather than a hard-coded target size.Fail-fast, fairness, and artifacts
The campaign smokes all six tools before every measured point and stops immediately after the first request or distributed-worker error. Worker IDs are included in fail-fast diagnostics, and each worker receives a different replica offset so multi-replica traffic remains balanced.
Telemetry records host and container CPU, per-core pressure, memory, swap/OOM state, scheduling pressure, socket counters, worker health, virtualization steal, and per-replica request rates. Sustained helper saturation invalidates the partial campaign. The CLI grows only the saturated helper, archives the invalidated evidence, and reruns all eight points so the final comparison uses one helper allocation.
Artifacts and recovery state are stored below
$CF_INTEGRATION_DIR/fyre/<run-id>/:results/summary.csvandresults/summary.jsonresults/slack-comparison.png, including external-dataplane RPS divided by built-in-dataplane RPSresults/comparison/<lane>-<users>/manifest.jsonwith exact images, workload, inventory, and allocationsArtifacts are downloaded after every phase and before cleanup. Cleanup retries Terraform destroy three times, preserves recovery state on failure, and refuses to touch VMs outside the run-owned Terraform state. FYRE expiry remains the final cleanup backstop.
Infrastructure and security
Terraform owns FYRE provisioning and isolated state; pinned Ansible bootstraps Docker and Compose on each VM. The CLI preflights product-group CPU, memory, standalone-VM disk, and public-IP quota before provisioning.
Benchmark services bind only to FYRE private addresses or loopback; public interfaces are used only for SSH orchestration. Provider credentials stay in environment variables and are excluded from manifests, state summaries, logs, and command arguments. Inventory-derived backend URLs are shell-quoted in every remote command.
The removed global dataplane cache-expiry override is intentional. These Compose overlays are packaged implementation assets owned by the CLI, which now publishes the required global configuration before dataplane startup and uses isolated scenario identities. Direct long-lived consumers of the internal Compose fragments should use the CLI workflow rather than depending on the removed override.
The conformance token/config changes remain in this release because FYRE and standalone external workflows use the same CLI-owned JWKS, token, and Redis snapshot lifecycle. Keeping that lifecycle in the packaged CLI avoids environment-specific auth setup.
Other load improvements
-w/--workers,-i/--isolate-cpus, and-m/--builtin-memory-limit.Validation
cargo test --all-targets --locked— 431 passedpython3 -m unittest discover -s benchmarks/fyre -p 'test_*.py'— 24 passed and now runs in CIcargo clippy --all-targets --locked -- -D warningscargo fmt --all --check