From 3a84e0fe1ce17f17ad3efb6b80c7ccb0f75db884 Mon Sep 17 00:00:00 2001 From: balaji Date: Thu, 20 Aug 2026 10:02:46 -0700 Subject: [PATCH] fix(container-cache): restore relay connection reuse, replicate hot objects, make the hop observable Consistent-hash routing relays roughly two thirds of requests to a peer pod. That path had two defects that cost latency on every relayed request, and the relay itself was invisible in metrics, so the cost could not be measured. Connection reuse. proxy-common.conf sets `Connection ""` at server scope to enable upstream keepalive, but nginx cancels inheritance of proxy_set_header as soon as a level declares any of its own, and @cc_relay declares three. The relay therefore fell back to the nginx default of `Connection: close`, and the cc_owner upstreams declared no keepalive pool either, so every relayed request opened a new connection and performed a new TLS handshake. Worst for the many small Range requests the hash deliberately spreads across owners. Repeat the header inside @cc_relay and give each upstream a pool. Hot-object replication. The relay ran with proxy_cache off, so an object requested repeatedly through a non-owner relayed for its whole lifetime with no way to stop. Cache on the relay behind proxy_cache_min_uses, keyed on $cc_hash_key so the local copy carries the owner's exact cache identity. Hot objects stop paying the hop; one-off objects still live only on their owner, and the extra copies are bounded by the existing min_free eviction. Set consistentHashRouting.relayCacheMinUses=0 to restore strict single-copy behavior. Observability. Relayed and local requests were indistinguishable in the request counter, the duration histogram and the throughput histogram, and the host label only ever carried the origin. Add a bounded `route` label with three values: local, relayed, peer. The lookup is emitted only when routing is enabled, because the variable is undeclared otherwise and OpenResty raises on reading an undeclared variable; with routing off every request is local, which is accurate. Histogram buckets. These requests are whole model-file transfers, not API calls. The duration histogram topped out at 10s while a large share of observed traffic exceeded it, and histogram_quantile clamps at the last finite bucket, so any reported high quantile was the bucket edge rather than a measurement. Response sizes jumped 100MB to 1GB to 10GB, putting nearly all traffic in one bucket. Both ladders now cover the range these objects occupy, and both are values. Behavior is unchanged when consistentHashRouting is disabled, which remains the default; the disabled render is still byte-identical to today apart from the widened buckets and the constant route label. Tests: extends tests/render-consistent-hash-test.sh with assertions for the keepalive pool and the repeated Connection header, relay caching and its disabled form, the route label in all three states, and that the routing variable is never read when undeclared. Verified the new assertions fail when the corresponding change is reverted. Closes #1037 Co-Authored-By: Balaji Ganesan --- .../container-cache/deploy/files/nginx.conf | 40 ++++++++++++-- .../deploy/files/proxy-common.conf | 53 ++++++++++++++++--- .../helm/container-cache/deploy/values.yaml | 27 ++++++++++ .../tests/render-consistent-hash-test.sh | 45 ++++++++++++++++ 4 files changed, 154 insertions(+), 11 deletions(-) diff --git a/deploy/helm/container-cache/deploy/files/nginx.conf b/deploy/helm/container-cache/deploy/files/nginx.conf index cbe62212a..a58839ded 100644 --- a/deploy/helm/container-cache/deploy/files/nginx.conf +++ b/deploy/helm/container-cache/deploy/files/nginx.conf @@ -70,8 +70,16 @@ proxy_http_request_time_ms_total = prometheus:counter( "proxy_cache_http_requests_time_ms_total", "Sum of time elapsed between reading the first bytes from the client and writing a log entry after the last bytes were sent for each request", {"request_method", "cache_status", "http_status"}) + {{- /* + `route` distinguishes how a request was served under + consistent-hash routing, and is always "local" when it is off: + local served from this pod's own cache or origin fetch + relayed forwarded to the owner pod, so it paid the peer hop + peer served for another pod that relayed it here + Three bounded values, so this does not add unbounded cardinality. + */}} proxy_http_requests_count = prometheus:counter( - "proxy_cache_http_requests_count", "Count of requests with additional labels", {"request_method", "cache_status", "http_status"}) + "proxy_cache_http_requests_count", "Count of requests with additional labels", {"request_method", "cache_status", "http_status", "route"}) proxy_http_upstream_requests_count = prometheus:counter( "proxy_cache_http_upstream_requests_count", "Count of upstream requests with additional labels", {"request_method", "cache_status", "http_status"}) @@ -80,13 +88,28 @@ "proxy_cache_response_body_size_bytes", "Number of bytes sent to client, not counting the response header", {"cache_status", "http_status"}) proxy_cache_throughput = prometheus:histogram( - "proxy_cache_throughput", "Throughput of proxy cache(bytes/msec)", {"cache_status", "http_status"}, { {{ $.Values.metrics.throughputHistogramBuckets | default "25000000, 30000000, 35000000, 40000000, 50000000, 60000000, 80000000, 100000000" }} }) + "proxy_cache_throughput", "Throughput of proxy cache(bytes/msec)", {"cache_status", "http_status", "route"}, { {{ $.Values.metrics.throughputHistogramBuckets | default "25000000, 30000000, 35000000, 40000000, 50000000, 60000000, 80000000, 100000000" }} }) proxy_cache_upstream_throughput = prometheus:histogram( "proxy_cache_upstream_throughput", "Throughput of upstream server(bytes/msec)", {"host", "http_status"}, { {{ $.Values.metrics.throughputHistogramBuckets | default "25000000, 30000000, 35000000, 40000000, 50000000, 60000000, 80000000, 100000000" }} }) - metric_latency = prometheus:histogram("proxy_cache_request_duration_seconds", "HTTP request latency", {"host", "request_method", "cache_status", "http_status"}, {0.005, 0.01, 0.02, 0.03, 0.05, 0.075, 0.1, 0.2, 0.3, 0.4, 0.5, 0.75, 1, 1.5, 2, 3, 4, 5, 10}) - metric_upstream_latency = prometheus:histogram("proxy_cache_upstream_request_duration_seconds", "HTTP request latency", {"host", "request_method", "cache_status", "http_status"}, {0.005, 0.01, 0.02, 0.03, 0.05, 0.075, 0.1, 0.2, 0.3, 0.4, 0.5, 0.75, 1, 1.5, 2, 3, 4, 5, 10}) + {{- /* + Duration buckets must cover a whole object transfer, not an API + call. These are multi-hundred-MB model files, so a request that + takes tens of seconds is normal and says nothing about cache + health. With a 10s top bucket a large share of traffic landed in + +Inf, and histogram_quantile clamps at the last finite bucket, so + any reported high quantile was the bucket edge rather than a + measurement. Buckets past 10s exist to make those quantiles real. + */}} + metric_latency = prometheus:histogram("proxy_cache_request_duration_seconds", "HTTP request latency", {"host", "request_method", "cache_status", "http_status", "route"}, { {{ $.Values.metrics.durationHistogramBuckets | default "0.005, 0.01, 0.02, 0.03, 0.05, 0.075, 0.1, 0.2, 0.3, 0.4, 0.5, 0.75, 1, 1.5, 2, 3, 4, 5, 10, 15, 30, 60, 120, 300, 600" }} }) + metric_upstream_latency = prometheus:histogram("proxy_cache_upstream_request_duration_seconds", "HTTP request latency", {"host", "request_method", "cache_status", "http_status"}, { {{ $.Values.metrics.durationHistogramBuckets | default "0.005, 0.01, 0.02, 0.03, 0.05, 0.075, 0.1, 0.2, 0.3, 0.4, 0.5, 0.75, 1, 1.5, 2, 3, 4, 5, 10, 15, 30, 60, 120, 300, 600" }} }) metric_connections = prometheus:gauge("nginx_http_connections", "Number of HTTP connections", {"state"}) - metric_response_sizes = prometheus:histogram("proxy_cache_response_size_bytes", "Size of HTTP responses", nil,{1000,2000,4000,8000,16000,32000,64000,128000,256000,1048576, 10485760, 104857600, 1000000000, 10000000000, 100000000000}) + {{- /* + Response-size buckets previously jumped 100MB -> 1GB -> 10GB, so + essentially all model traffic fell in a single bucket and the + distribution was unreadable. The added steps give resolution + across the range these objects actually occupy. + */}} + metric_response_sizes = prometheus:histogram("proxy_cache_response_size_bytes", "Size of HTTP responses", nil, { {{ $.Values.metrics.responseSizeHistogramBuckets | default "1000, 2000, 4000, 8000, 16000, 32000, 64000, 128000, 256000, 1048576, 10485760, 104857600, 268435456, 536870912, 805306368, 1073741824, 2147483648, 5368709120, 10737418240, 107374182400" }} }) instance_info = prometheus:gauge("proxy_cache_instance_info", "Information about proxy cache instance", {"chart_version"}) instance_info:set(1, { "{{ $.Chart.Version }}" }) @@ -129,6 +152,13 @@ {{- if gt $n 1 }} server {{ $.Release.Name }}-peer-{{ mod (add $i 1) $n }}.{{ $.Release.Namespace }}.svc.cluster.local:{{ $port }} backup; {{- end }} + # Keep idle connections to the peer. Without this every relayed + # request pays a fresh TCP and TLS handshake, which is worst for the + # many small Range requests the hash spreads across owners. + # @cc_relay must also send `Connection ""` for these to be reused. + keepalive {{ ($.Values.consistentHashRouting).peerKeepaliveConnections | default 32 }}; + keepalive_timeout {{ ($.Values.consistentHashRouting).peerKeepaliveTimeout | default "60s" }}; + keepalive_requests {{ ($.Values.consistentHashRouting).peerKeepaliveRequests | default 1000 }}; } {{- end }} {{- end }} diff --git a/deploy/helm/container-cache/deploy/files/proxy-common.conf b/deploy/helm/container-cache/deploy/files/proxy-common.conf index ef9b73491..aa595adea 100644 --- a/deploy/helm/container-cache/deploy/files/proxy-common.conf +++ b/deploy/helm/container-cache/deploy/files/proxy-common.conf @@ -32,14 +32,32 @@ local upstream_time = tonumber(ngx.var.upstream_response_time) local host = ngx.var.host + -- How this request was served. Peer routing is optional, and when + -- it is off every request is served here, so "local" is accurate + -- and the lookup below is not emitted at all. The routing variable + -- is only declared when routing is enabled, and reading an + -- undeclared variable raises in OpenResty. + local route = "local" +{{- if ($.Values.consistentHashRouting).enabled }} + -- Non-empty owner means the route helper sent this to a peer, so it + -- paid the relay hop. The inbound relay marker means this pod is the + -- owner answering for a peer. + local cc_owner = ngx.var.cc_owner + if cc_owner ~= nil and cc_owner ~= "" then + route = "relayed" + elseif ngx.var.http_x_nvcf_cc_relayed == "1" then + route = "peer" + end +{{- end }} + if request_uri ~= nil and cache_status ~= nil and remote_addr ~= nil and not string.find(request_uri, "manifest") then if request_time ~= nil and request_time > 0 then proxy_http_request_time_ms_total:inc(request_time, {request_method, cache_status, http_status}) - proxy_http_requests_count:inc(1, {request_method, cache_status, http_status}) + proxy_http_requests_count:inc(1, {request_method, cache_status, http_status, route}) - metric_latency:observe(request_time, {host, request_method, cache_status, http_status}) + metric_latency:observe(request_time, {host, request_method, cache_status, http_status, route}) if body_size ~= nil then - proxy_cache_throughput:observe(body_size / request_time, {cache_status, http_status}) + proxy_cache_throughput:observe(body_size / request_time, {cache_status, http_status, route}) if upstream_time ~= nil and upstream_time > 0 then proxy_cache_upstream_throughput:observe(body_size / upstream_time, {host, http_status}) metric_upstream_latency:observe(upstream_time, {host, request_method, cache_status, http_status}) @@ -154,15 +172,38 @@ # every block, so the relay location is not duplicated per block. set $cc_owner ""; location @cc_relay { - # Relay to the owner pod: a pure stream that never writes the local - # cache or temp files; only the owner stores the object. lua-access - # runs on the owner, which sees the original request headers. + # Relay to the owner pod. lua-access runs on the owner, which sees the + # original request headers. + {{- $relayMinUses := int (($.Values.consistentHashRouting).relayCacheMinUses | default 0) }} + {{- if gt $relayMinUses 0 }} + # Objects pulled through this pod repeatedly are replicated locally + # after relayCacheMinUses requests, so hot objects stop paying the + # relay hop while one-off objects still live only on their owner. + # Keyed on $cc_hash_key, which each server block sets to the same + # value it uses for proxy_cache_key, so the local copy has exactly the + # owner's cache identity. Bounded by the same min_free eviction as + # every other zone. + proxy_cache proxy_ngc; + proxy_cache_key $cc_hash_key; + proxy_cache_min_uses {{ $relayMinUses }}; + proxy_cache_valid 200 206 {{ $.Values.cache.valid | default "31d" }}; + {{- else }} + # A pure stream that never writes the local cache; only the owner + # stores the object. Set consistentHashRouting.relayCacheMinUses to + # replicate hot objects locally instead. proxy_cache off; + {{- end }} proxy_http_version 1.1; proxy_pass_request_headers on; proxy_set_header Host $host; proxy_set_header Range $http_range; proxy_set_header X-NVCF-CC-Relayed "1"; + # Declaring any proxy_set_header here cancels inheritance of the + # server-level set, which includes `Connection ""`. Without repeating + # it nginx sends its default `Connection: close` and the upstream + # keepalive pool is never used, so every relay reconnects and + # re-handshakes TLS. + proxy_set_header Connection ""; # The peer serves the same ssl listener with a per-pod internal leaf # cert, so system-CA verification cannot pass. Verification is skipped # on this internal peer hop only; the tier is an internal, diff --git a/deploy/helm/container-cache/deploy/values.yaml b/deploy/helm/container-cache/deploy/values.yaml index 877730801..5671b73ac 100644 --- a/deploy/helm/container-cache/deploy/values.yaml +++ b/deploy/helm/container-cache/deploy/values.yaml @@ -191,11 +191,38 @@ consistentHashRouting: # Owner pods are in-cluster peers, so this stays short: a slow connect means # the owner is down, and failing over fast is correct. relayConnectTimeout: 2s + # Idle connections kept open to each owner pod. Without a pool every relayed + # request pays a fresh TCP and TLS handshake to the peer, which is worst for + # the many small Range requests the hash spreads across owners. Cheap: these + # are in-cluster peers and the pool is per worker process. + peerKeepaliveConnections: 32 + peerKeepaliveTimeout: 60s + peerKeepaliveRequests: 1000 + # Replicate an object locally after this many requests arrive here for an + # object owned by another pod, so hot objects stop paying the relay hop while + # one-off objects still live only on their owner. Storage is bounded by the + # same min_free eviction as every other zone, and the extra copies are by + # definition the objects being requested most. + # + # 0 disables local replication and restores the strict single-copy behavior, + # where every request for a non-owned object relays for the object's whole + # lifetime. + relayCacheMinUses: 3 # Metrics configuration. metrics: cacheMetricsStorageSize: 300m throughputHistogramBuckets: 25000000, 30000000, 35000000, 40000000, 50000000, 60000000, 80000000, 100000000 + # Request-duration buckets. These requests are whole model-file transfers, not + # API calls, so tens of seconds is normal and the top bucket has to cover a + # full object. If the largest finite bucket is below real traffic, everything + # above it lands in +Inf and histogram_quantile clamps to that edge, reporting + # the bucket boundary instead of a latency. + durationHistogramBuckets: 0.005, 0.01, 0.02, 0.03, 0.05, 0.075, 0.1, 0.2, 0.3, 0.4, 0.5, 0.75, 1, 1.5, 2, 3, 4, 5, 10, 15, 30, 60, 120, 300, 600 + # Response-size buckets, with resolution across the range model objects + # actually occupy. A 100MB -> 1GB -> 10GB ladder puts nearly all traffic in + # one bucket and hides the distribution entirely. + responseSizeHistogramBuckets: 1000, 2000, 4000, 8000, 16000, 32000, 64000, 128000, 256000, 1048576, 10485760, 104857600, 268435456, 536870912, 805306368, 1073741824, 2147483648, 5368709120, 10737418240, 107374182400 # Custom annotations and labels for pods. podAnnotations: {} diff --git a/deploy/helm/container-cache/tests/render-consistent-hash-test.sh b/deploy/helm/container-cache/tests/render-consistent-hash-test.sh index 80d5011c0..ae07bfc80 100755 --- a/deploy/helm/container-cache/tests/render-consistent-hash-test.sh +++ b/deploy/helm/container-cache/tests/render-consistent-hash-test.sh @@ -52,4 +52,49 @@ grep -q 'set $cc_hash_key "$request_method|$uri|$arg_versionId|$http_range"' "$T grep -q 'proxy_set_header X-NVCF-CC-Relayed "1"' "$TMP/on.yaml" || fail "relay hop must emit the one-hop marker" grep -q 'ngx.req.get_headers()\["X-NVCF-CC-Relayed"\]' "$TMP/on.yaml" || fail "cc-route.lua must reject an inbound relay marker (serve locally, prevents relay loops)" +echo "7. enabled: the peer hop reuses connections" +# Both halves are required. A keepalive pool with no `Connection ""` is dead +# weight, because nginx then sends its default `Connection: close` and every +# relayed request re-handshakes TLS. The header must be repeated inside +# @cc_relay specifically: declaring any proxy_set_header in a location cancels +# inheritance of the server-level set. +[ "$(count 'keepalive [0-9]+;' "$TMP/on.yaml")" = 3 ] || fail "each cc_owner upstream needs a keepalive pool" +awk '/location @cc_relay/,/^ *}$/' "$TMP/on.yaml" | grep -q 'proxy_set_header Connection ""' \ + || fail '@cc_relay must repeat Connection "" or the keepalive pool is never used' + +echo "8. enabled: hot objects replicate locally after relayCacheMinUses" +awk '/location @cc_relay/,/^ *}$/' "$TMP/on.yaml" | grep -q 'proxy_cache_min_uses 3' \ + || fail "relay must cache locally after the configured use threshold" +awk '/location @cc_relay/,/^ *}$/' "$TMP/on.yaml" | grep -q 'proxy_cache_key \$cc_hash_key' \ + || fail "relay cache key must match the owner's identity, not the default key" + +echo "8b. relayCacheMinUses=0 restores strict single-copy relaying" +helm template t "$CHART_DIR" --set consistentHashRouting.enabled=true --set replicaCount=3 \ + --set consistentHashRouting.relayCacheMinUses=0 > "$TMP/on-nocache.yaml" 2>/dev/null +awk '/location @cc_relay/,/^ *}$/' "$TMP/on-nocache.yaml" | grep -q 'proxy_cache off' \ + || fail "relayCacheMinUses=0 must leave the relay a pure stream" +if awk '/location @cc_relay/,/^ *}$/' "$TMP/on-nocache.yaml" | grep -q 'proxy_cache_min_uses'; then + fail "relayCacheMinUses=0 must not emit proxy_cache_min_uses" +fi + +echo "9. relay cost is observable, and duration buckets outlast a whole transfer" +# Without the route label a relayed request is indistinguishable from a local +# hit, which is what made the relay's latency cost unmeasurable. +grep -q '"cache_status", "http_status", "route"' "$TMP/on.yaml" \ + || fail "request/throughput metrics must carry the route label" +grep -q 'local route = "local"' "$TMP/on.yaml" || fail "route must default to local" +grep -q 'route = "relayed"' "$TMP/on.yaml" || fail "relayed requests must be labelled" +grep -q 'route = "peer"' "$TMP/on.yaml" || fail "requests served for a peer must be labelled" +# The objects here are whole model files, so a 10s ceiling put a large share of +# traffic in +Inf and histogram_quantile then reports the bucket edge, not a +# latency. +grep -q 'proxy_cache_request_duration_seconds' "$TMP/on.yaml" || fail "duration histogram missing" +awk '/proxy_cache_request_duration_seconds/{print; exit}' "$TMP/on.yaml" | grep -q '600' \ + || fail "duration buckets must extend past a full object transfer" + +echo "9b. route label renders even with routing disabled (no undeclared-variable read)" +grep -q 'local route = "local"' "$TMP/off.yaml" || fail "route label must still render when routing is off" +[ "$(count 'ngx.var.cc_owner' "$TMP/off.yaml")" = 0 ] \ + || fail "must not read the routing variable when it is undeclared (OpenResty raises)" + echo "PASS: all consistent-hash routing render assertions hold"