runtime: make Pingora service worker topology explicit - #73
Conversation
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: trueThanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
seonghobae
left a comment
There was a problem hiding this comment.
Exact-current technical review on 4ee0657697b4e2003eca95e674597381726e641f: re-read the 16-path #70→#73 range, with focused source inspection of the generic/pg-erd Admin Config validation, runtime composition, crate-private Pingora ServerConf builder, both production binary composition changes, exact worker-boundary regressions, ADR 0012, and the bounded-origin harness. Authoritative Pingora 0.9.0 source defines ServerConf::threads as the worker count for each service and defaults it to 1; this candidate makes that topology explicit without deriving it from host CPU count or moving product/security authority into the gateway. The current capacity harness exercises service_threads: 4 and records its topology rather than claiming a process-wide four-thread total.
All normal exact-head execution is terminal GREEN: CI 34467396577 passed load-contract, OCI runtime, locked tests, Clippy, warnings-denied rustdoc, complete owned-production coverage and lock verification; Supply Chain 34467396702 passed dependency audit, candidate builds, SPDX SBOM, both image scans and exact-source binding; capacity 34467396707 passed 1,600 iterations / 3,200 checks with zero HTTP failures. The source-bound artifact records 4 configured workers per service on a 4-CPU, single-node/single-socket hosted runner and p95 3.50637775 ms aggregate, 3.39606430 ms backend, 3.56507600 ms frontend. That is valid multi-worker composition/traffic evidence but not representative NUMA scaling or contention closure for #46.
I found no additional actionable writer-safe source/test/documentation/DDD or authority-boundary defect in this current range. This COMMENT is technical evidence only, not an independent APPROVED governance review, and it does not grant protected merge, release, canary/shadow, cutover, or legacy-removal credit.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
seonghobae
left a comment
There was a problem hiding this comment.
Exact-current technical re-read on 625cae4f156366bc39d6782161a4a5f336d58624: reviewed the 16-path #70→#73 range after the later registered-service/evidence repairs, both production composition roots, Admin Config validation/revalidation, crate-private ServerConf construction, runtime-composition boundary, capacity harness and drift contract. Fresh Pingora 0.9.0 source confirms a service-level threads() value overrides ServerConf::threads and None falls back to the global value, so keeping the proxy on validated global service_threads while fixing Prometheus at Some(1) matches supplier runtime semantics rather than multiplying every registered service. The current regression also fail-closes service-registration/metrics-override drift. No additional actionable gateway-local source/test/documentation/DDD or authority-boundary defect is identified in this exact range.
Current exact CI 34478471643 is terminal GREEN: load-contract, locked compile/test, Clippy, warnings-denied rustdoc, complete owned-production coverage, resolved-lock verification and both least-privilege OCI profiles passed. Exact bounded-origin capacity 34478471632 is also GREEN and source-bound to artifact 10152648179 (sha256:b91a3786daea4786af334fd31b7b0ab19d828d8b6d178685215ebe26a3dcdf34). Supply Chain 34478471699 is still executing after exact checkout, toolchain setup and committed dependency audit passed, so this COMMENT does not transfer predecessor Supply Chain credit or authorize Ready/merge before it terminates. Representative many-core/NUMA contention evidence remains a separate #46 requirement. Technical COMMENT only; not an independent APPROVED review, protected merge, release, canary/shadow, cutover, or legacy-removal credit.
seonghobae
left a comment
There was a problem hiding this comment.
Terminal exact-current follow-up on 625cae4f156366bc39d6782161a4a5f336d58624: the Supply Chain lane that was still executing during review 5167316759 has now terminated GREEN without head movement. CI 34478471643 is GREEN across load-contract, locked compile/test, Clippy, warnings-denied public rustdoc, complete owned-production coverage, resolved-lock verification and both least-privilege OCI profiles. Supply Chain 34478471699 is GREEN through committed dependency audit, both admitted image builds, SPDX SBOM generation, both exact image scans and exact-source binding. Bounded-origin capacity 34478471632 remains GREEN with exact-source artifact 10152648179, digest sha256:b91a3786daea4786af334fd31b7b0ab19d828d8b6d178685215ebe26a3dcdf34.
The exact source range is unchanged since the prior re-read and no review thread exists, so no further writer-safe source/test/documentation repair is justified. This closes the #73 branch-local technical/execution gate and makes it suitable for independent review. It does not close #46 representative many-core/NUMA contention acceptance and is not an independent APPROVED review, protected-merge, release, canary/shadow, cutover, or legacy-removal credit.
Runtime Isolation/Admin Config follow-on for #46, stacked on exact shutdown-correctness candidate #70 (
ebdb2c4091aec206fb30174eee9c6c86160bb047).Problem and authority boundary
Both production composition roots previously inherited Pingora 0.9.0
ServerConf::threads = 1. A many-core/NUMA host therefore did not make the gateway multi-worker merely by exposing CPUs. This PR makes data-plane worker topology explicit without moving product authentication/business logic, Keyverse identity, Wardnet/EgressWeave policy, routing authority, supplier source, or deployment authority into the gateway.Pingora's exact 0.9.0 source defines
ServerConf::threadsas the global worker count each service follows unless it supplies a service-level override.HttpProxyalso consumes that same global value when constructing its sharded graceful-shutdown notifier. The global value therefore needs to stay aligned with the proxy data plane, but scaling the Prometheus listener by the same factor is neither necessary nor desirable.Selected contract
service_threads;1..=256; zero and 257 fail closed;ServerConf::threads, which the proxy follows andHttpProxyuses for shutdown-notifier sharding;ServerConfbuilder is crate-private;Proposed ADR 0012, AGENTS, examples, executable contracts and
API_CONFIG_CONTRACT.mdrecord these semantics.Repair history
Hosted review/execution first repaired a stale direct
GatewayConfigfixture, a public activation-boundary validation escape, Rust formatter deltas, missing exact-ceiling admission coverage, and a missing PingoraPeertrait import. Exactcd24ee84aac9e44c0228b82bdde10f47206fbc3athen passed formatting,cargo test --all-targets --locked --no-fail-fast, and the normal load contract.Exact
4ee0657697b4e2003eca95e674597381726e641fmade bounded-origin capacity runservice_threads: 4and record configured worker count plus CPU/node/socket topology. That head was terminal GREEN across CI34467396577, Supply Chain34467396702, and bounded-origin capacity34467396707; capacity artifact10148210022recorded 1,600 iterations, 3,200/3,200 checks, zero HTTP failures and aggregate/backend/frontend p953.50637775/3.39606430/3.56507600 mson four visible CPUs in one NUMA node/socket.A subsequent #46 evidence audit found that receipt did not explicitly record the number of registered Pingora services. Forward commits
0b8d6d0cebc3dcb1aa679f25501f5928e63f1eacand7e41948373a3ad140c25f4dcaf89f8edef77be0badded registered-service evidence and a Rust drift contract tied to the real production composition roots.That review exposed a deeper performance defect before the new evidence could be promoted: leaving every service on the global worker count would make a high data-plane setting multiply Prometheus workers as well. Pingora 0.9.0 exposes
Service::threads: Option<usize>specifically so a service can override the global count, whileHttpProxysizes shutdown sharding fromServerConf::threads. The selected topology is therefore global/data-plane workers for the proxy plus a fixed one-worker metrics override, rather thanservice_threads × all registered services.Ordinary forward commits
b29d7fb242828df41b47fe7fad83d477b1438e89and831eff48d343b53665654797a97d73618563773fapplymetrics_service.threads = Some(1)in the generic and pg-erd production composition roots.50df413faafff138e76477155766ec7ae2e7524dchanges capacity evidence to record proxy threads, metrics threads, registered service count and derived configured service-worker slots separately; with the current characterization value this is 4 proxy workers + 1 metrics worker = 5 configured service-worker slots, not a total OS process-thread claim.fce1f50c43f7e086735c9603d92e27898c248da6fail-closes the regression contract against service-registration or metrics-override drift in both production binaries.b93f6b1c7e8bd063909e15fc8d70d42bb6d48928,4bc66c3e9c958fc0210186562cbeea0d2883c154, and144234189374afc845f03d8f2c4d60d21c4eac50bring ADR 0012, the configuration contract, and AGENTS into the same data-plane-versus-metrics topology.A final code-current sweep found older rustdoc/error wording in
edge_contractandmigration_adminstill described the field as multiplying every registered service. Ordinary descendants7690d1743bc92996d579280a9963c72381a8a93aand08ad492503399115a74b1b0a1f183e882dcd3950repair those executable-contract descriptions to the actual global/data-plane proxy semantics plus service override.625cae4f156366bc39d6782161a4a5f336d58624also renames the two ceiling-test cases from the misleadingprocess_ceilingphrase todata_plane_ceiling. No validation threshold or runtime behavior changed in those final three commits.No route, timeout, request/response policy, supplier identity, traffic sample count/concurrency, p95 threshold, product-domain policy or security authority was changed by this follow-on.
Current exact head / evidence closure
Current exact head is
625cae4f156366bc39d6782161a4a5f336d58624, a strict ordinary-forward descendant of exact baseebdb2c4091aec206fb30174eee9c6c86160bb047. It has independently reacquired all normal current-head evidence after the final source/test/documentation movement; no predecessor GREEN is transferred.34478471643is terminal GREEN.load-contract,test, andoci-runtimeall passed. The test lane passed exact checkout, Rust 1.98.0 formatting, locked compile/tests, Clippy, warnings-denied public rustdoc, pinned coverage tooling, complete owned-production coverage enforcement, and resolved dependency-lock verification. Both admitted OCI profiles were rebuilt and exercised under declared non-root/read-only least-privilege constraints.34478471699is terminal GREEN through exact checkout, committed dependency audit, both admitted candidate-image builds, SPDX dependency SBOM generation, both exact image scans, exact-source binding, and evidence upload.34478471632is terminal GREEN. Exact-source artifact10152648179, digestsha256:b91a3786daea4786af334fd31b7b0ab19d828d8b6d178685215ebe26a3dcdf34, is bound to this head. The evidence model recordsconfigured_proxy_service_threads,configured_metrics_service_threads,registered_service_count,configured_service_worker_slots, online CPUs and CPU→node/socket mapping. For the current two-service characterization, the configured worker-slot arithmetic is 4 proxy workers plus the one metrics worker; this is deliberately not described as total OS process threads.5167316759and terminal follow-up5167331074re-read the complete current range and found no additional actionable writer-safe source/test/documentation/DDD or authority-boundary defect. They are technical COMMENT evidence only, not independentAPPROVEDgovernance credit.This closes the branch-local implementation/execution gate for independent review, so the PR is Ready rather than Draft.
Remaining performance and promotion boundary
#46 still requires the largest representative Linux/NUMA-capable environment with the exact configured proxy worker count, registered service count/service overrides, CPU/socket/NUMA topology, high parked/reused HTTP/1 keep-alive pressure, repeated shutdown jitter, zero correctness survivors, shutdown wall-clock distribution/tail, CPU/scheduler behavior, and where available off-CPU/futex evidence. No worker/sample/connection reduction is admissible merely to remove contention. The ordinary GitHub-hosted low-core result cannot supply commercial NUMA scaling closure.
No independent
APPROVEDgovernance review is claimed. #70 correctness, #54/#889 derivative removal, #71/#447 whole-header lifetime, #72/#1000 parser admission, H2→H1 Cookie normalization, zero-length framing, #56 approval/governance, immutable release, shadow/canary, rollback, cutover, and legacy removal remain separate dependency-ordered gates. No protected merge, release, canary/shadow, rollback, cutover, or Nginx/OpenResty removal credit is claimed.