fix(container-cache): use TCP liveness probe so a busy proxy is never killed#318
Open
balajinvda wants to merge 4 commits into
Open
fix(container-cache): use TCP liveness probe so a busy proxy is never killed#318balajinvda wants to merge 4 commits into
balajinvda wants to merge 4 commits into
Conversation
… killed The nginx-proxy liveness probe was an HTTP GET against /healthz, which is answered by the same worker processes that serve the data plane. Under sustained heavy transfer load the probe times out and kubelet kills a proxy that is merely busy, aborting every in-flight transfer; the resulting client retry herd re-saturates the tier, and reconnecting clients re-pool onto the surviving replicas, skewing load long after the killed pods return. Loosening the HTTP thresholds was previously tried and is not sufficient. Replace liveness with a tcpSocket probe on the same port: the kernel completes the accept while nginx holds the listener, so liveness fails only when the process is actually dead or wedged. Readiness stays on HTTP /healthz (slightly relaxed) so an overloaded replica sheds new traffic without being killed. Chart 0.25.21 -> 0.25.22. Validated with tools/ci/validate-helm-chart.sh (lint + template). Fixes #316 Co-Authored-By: Balaji Ganesan <bganesan@nvidia.com>
📝 WalkthroughWalkthroughThe container-cache Helm chart changes nginx-proxy liveness from HTTP ChangesContainer cache probe and throughput tuning
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Kubelet
participant nginx-proxy
participant healthz as /healthz endpoint
Kubelet->>nginx-proxy: Perform TCP liveness check
nginx-proxy-->>Kubelet: Accept TCP connection
Kubelet->>healthz: Perform HTTP readiness check
healthz-->>Kubelet: Return readiness response
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@deploy/helm/container-cache/deploy/templates/statefulset.yaml`:
- Around line 161-168: Add a rendered Helm probe regression test covering the
default TCP liveness probe, confirming liveness no longer renders httpGet,
validating the readiness /healthz probe, and verifying custom probes: overrides.
Use the repository’s native test runner and keep assertions focused on rendered
StatefulSet output.
- Around line 161-168: Update the TCP probe comments in
deploy/helm/container-cache/deploy/templates/statefulset.yaml lines 161-168 and
deploy/helm/container-cache/deploy/values.yaml lines 60-66 to describe only
listener-level liveness: a successful TCP accept confirms the port is listening,
but does not detect a wedged nginx worker. Remove claims that the probe detects
processes that are dead or wedged while preserving the explanation of why TCP is
preferred over HTTP.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 144699d6-05b5-41b8-8005-fa612794c3f1
📒 Files selected for processing (3)
deploy/helm/container-cache/deploy/Chart.yamldeploy/helm/container-cache/deploy/templates/statefulset.yamldeploy/helm/container-cache/deploy/values.yaml
… storage Herd-scale load testing showed the proxy tier saturating at a small fraction of node network capacity while nginx logged slow thread-pool operations and health endpoints starved: with a high-latency backing store, synchronous temp-file writes in worker context freeze the event loop, and queued aio tasks amplify per-IO latency into blocked workers. Three changes, all tunable via values: - aio_write on: cache-fill writes of proxied upstream data move to the thread pool, so a multi-second write parks a pool thread instead of a worker event loop. - Explicit thread_pool sizing (cache.aioThreads, default 64): the pool absorbs storage latency; the previous implicit 32-thread default queued tasks under concurrent fills. - Per-server connection cap on the model-download server (cache.maxConnsPerServer, default 512, 0 disables): excess requests fail fast with 503, which ranged download clients retry per chunk, instead of parking connections on a storage-bound tier. Validated: helm lint and template rendering for defaults, overrides, and the cap-disabled case. Co-Authored-By: Balaji Ganesan <bganesan@nvidia.com>
…docs Address review on #318: - Add tests/render-probes-test.sh (make test): asserts default liveness renders tcpSocket and no httpGet, readiness stays HTTP /healthz, probes overrides render, resilience directives (aio_write, thread_pool, limit_conn) render, and maxConnsPerServer=0 disables the cap. - Narrow the probe comments: tcpSocket detects only a dead process or closed listener; a busy or wedged-but-listening proxy is deliberately not restarted (restarts cannot fix overload; readiness isolates it). Co-Authored-By: Balaji Ganesan <bganesan@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@deploy/helm/container-cache/tests/render-probes-test.sh`:
- Line 18: Add a test case in the render-probes test alongside the existing
default and zero-value cases that renders the chart with cache.maxConnsPerServer
set to a distinct positive value such as 123, then assert the generated
configuration contains limit_conn perserver 123;. Ensure the assertion verifies
the override is rendered rather than the default.
- Around line 34-39: Update the thread-count assertions in the render-probes
test to require the complete directive or a non-digit boundary after 64 and 128,
preventing values such as 640 or 1280 from matching. Preserve the existing
validation messages and checks for the default and override configurations.
- Around line 20-30: Strengthen the assertions in the probe checks around the
default render so the livenessProbe block verifies tcpSocket with port http, and
the readinessProbe block verifies httpGet with both path /healthz and port http.
Scope each check to its respective probe block rather than matching nearby YAML,
while preserving the existing override assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 2cfb08a6-9aba-4b0b-bf75-9c1fe00b5511
📒 Files selected for processing (4)
deploy/helm/container-cache/Makefiledeploy/helm/container-cache/deploy/templates/statefulset.yamldeploy/helm/container-cache/deploy/values.yamldeploy/helm/container-cache/tests/render-probes-test.sh
🚧 Files skipped from review as they are similar to previous changes (2)
- deploy/helm/container-cache/deploy/templates/statefulset.yaml
- deploy/helm/container-cache/deploy/values.yaml
| helm template t "$CHART_DIR" --set probes.liveness.failureThreshold=99 \ | ||
| --set probes.readiness.timeoutSeconds=9 > "$TMP/probes-override.yaml" 2>/dev/null | ||
| helm template t "$CHART_DIR" --set cache.aioThreads=128 > "$TMP/threads-override.yaml" 2>/dev/null | ||
| helm template t "$CHART_DIR" --set cache.maxConnsPerServer=0 > "$TMP/cap-disabled.yaml" 2>/dev/null |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Test a positive maxConnsPerServer override.
The current cases cover the default and disabling the cap with 0, but not whether a nonzero override is rendered. A chart that hardcodes the default 512 for every positive value would still pass. Add a render such as --set cache.maxConnsPerServer=123 and assert limit_conn perserver 123;. The PR objective requires this setting to remain configurable.
Also applies to: 38-40
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@deploy/helm/container-cache/tests/render-probes-test.sh` at line 18, Add a
test case in the render-probes test alongside the existing default and
zero-value cases that renders the chart with cache.maxConnsPerServer set to a
distinct positive value such as 123, then assert the generated configuration
contains limit_conn perserver 123;. Ensure the assertion verifies the override
is rendered rather than the default.
Comment on lines
+20
to
+30
| echo "1. default render: liveness is tcpSocket, not httpGet" | ||
| grep -A12 "livenessProbe:" "$TMP/default.yaml" > "$TMP/liveness.txt" | ||
| grep -c "tcpSocket:" "$TMP/liveness.txt" >/dev/null || fail "liveness tcpSocket missing" | ||
| if grep -c "httpGet:" "$TMP/liveness.txt" >/dev/null; then fail "liveness still renders httpGet"; fi | ||
|
|
||
| echo "2. default render: readiness stays HTTP /healthz" | ||
| grep -A6 "readinessProbe:" "$TMP/default.yaml" | grep -c "path: /healthz" >/dev/null || fail "readiness /healthz missing" | ||
|
|
||
| echo "3. probes overrides render" | ||
| grep -c "failureThreshold: 99" "$TMP/probes-override.yaml" >/dev/null || fail "liveness override not rendered" | ||
| grep -c "timeoutSeconds: 9" "$TMP/probes-override.yaml" >/dev/null || fail "readiness override not rendered" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the complete probe contract, including probe type and port.
These checks do not prove that liveness uses tcpSocket.port: http, or that readiness uses httpGet.port: http; readiness is only checked for a nearby /healthz string. A wrong port or probe type could therefore pass the regression test. Assert the complete fields within each probe block. This matches the contract in deploy/helm/container-cache/deploy/templates/statefulset.yaml Lines 160-183 and the PR objectives.
Suggested assertion additions
grep -c "tcpSocket:" "$TMP/liveness.txt" >/dev/null || fail "liveness tcpSocket missing"
+grep -Eq 'port: http$' "$TMP/liveness.txt" || fail "liveness port is not http"
if grep -c "httpGet:" "$TMP/liveness.txt" >/dev/null; then fail "liveness still renders httpGet"; fi
-grep -A6 "readinessProbe:" "$TMP/default.yaml" | grep -c "path: /healthz" >/dev/null || fail "readiness /healthz missing"
+grep -A6 "readinessProbe:" "$TMP/default.yaml" | grep -q "httpGet:" || fail "readiness httpGet missing"
+grep -A6 "readinessProbe:" "$TMP/default.yaml" | grep -q "path: /healthz" || fail "readiness /healthz missing"
+grep -A6 "readinessProbe:" "$TMP/default.yaml" | grep -Eq 'port: http$' || fail "readiness port is not http"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| echo "1. default render: liveness is tcpSocket, not httpGet" | |
| grep -A12 "livenessProbe:" "$TMP/default.yaml" > "$TMP/liveness.txt" | |
| grep -c "tcpSocket:" "$TMP/liveness.txt" >/dev/null || fail "liveness tcpSocket missing" | |
| if grep -c "httpGet:" "$TMP/liveness.txt" >/dev/null; then fail "liveness still renders httpGet"; fi | |
| echo "2. default render: readiness stays HTTP /healthz" | |
| grep -A6 "readinessProbe:" "$TMP/default.yaml" | grep -c "path: /healthz" >/dev/null || fail "readiness /healthz missing" | |
| echo "3. probes overrides render" | |
| grep -c "failureThreshold: 99" "$TMP/probes-override.yaml" >/dev/null || fail "liveness override not rendered" | |
| grep -c "timeoutSeconds: 9" "$TMP/probes-override.yaml" >/dev/null || fail "readiness override not rendered" | |
| echo "1. default render: liveness is tcpSocket, not httpGet" | |
| grep -A12 "livenessProbe:" "$TMP/default.yaml" > "$TMP/liveness.txt" | |
| grep -c "tcpSocket:" "$TMP/liveness.txt" >/dev/null || fail "liveness tcpSocket missing" | |
| grep -Eq 'port: http$' "$TMP/liveness.txt" || fail "liveness port is not http" | |
| if grep -c "httpGet:" "$TMP/liveness.txt" >/dev/null; then fail "liveness still renders httpGet"; fi | |
| echo "2. default render: readiness stays HTTP /healthz" | |
| grep -A6 "readinessProbe:" "$TMP/default.yaml" | grep -q "httpGet:" || fail "readiness httpGet missing" | |
| grep -A6 "readinessProbe:" "$TMP/default.yaml" | grep -q "path: /healthz" || fail "readiness /healthz missing" | |
| grep -A6 "readinessProbe:" "$TMP/default.yaml" | grep -Eq 'port: http$' || fail "readiness port is not http" | |
| echo "3. probes overrides render" | |
| grep -c "failureThreshold: 99" "$TMP/probes-override.yaml" >/dev/null || fail "liveness override not rendered" | |
| grep -c "timeoutSeconds: 9" "$TMP/probes-override.yaml" >/dev/null || fail "readiness override not rendered" |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@deploy/helm/container-cache/tests/render-probes-test.sh` around lines 20 -
30, Strengthen the assertions in the probe checks around the default render so
the livenessProbe block verifies tcpSocket with port http, and the
readinessProbe block verifies httpGet with both path /healthz and port http.
Scope each check to its respective probe block rather than matching nearby YAML,
while preserving the existing override assertions.
Comment on lines
+34
to
+39
| grep -Ec "thread_pool default threads=64" "$TMP/default.yaml" >/dev/null || fail "thread_pool missing" | ||
| grep -Ec "limit_conn perserver 512;" "$TMP/default.yaml" >/dev/null || fail "limit_conn missing" | ||
| grep -c 'limit_conn_zone $server_name zone=perserver' "$TMP/default.yaml" >/dev/null || fail "limit_conn_zone missing" | ||
|
|
||
| echo "5. knob overrides: aioThreads resizes pool; maxConnsPerServer=0 disables cap" | ||
| grep -Ec "thread_pool default threads=128" "$TMP/threads-override.yaml" >/dev/null || fail "aioThreads override not rendered" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Require an exact thread count in the regex.
threads=64 also matches threads=640, and threads=128 matches threads=1280. A malformed rendered value could therefore pass these tests. Add a numeric boundary or match the complete directive.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@deploy/helm/container-cache/tests/render-probes-test.sh` around lines 34 -
39, Update the thread-count assertions in the render-probes test to require the
complete directive or a non-digit boundary after 64 and 128, preventing values
such as 640 or 1280 from matching. Preserve the existing validation messages and
checks for the default and override configurations.
The cap applies to the NGC file-serving (xfiles) server block generally, not only model downloads; note it is sized to engage only under herd load. Co-Authored-By: Balaji Ganesan <bganesan@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
deploy/helm/container-cache/_cc-analysis/access-2.jsonl (1)
1-236: 🔒 Security & Privacy | 🔴 Critical | ⚡ Quick winCommitted access-log capture embeds live SSE-C encryption keys and presigned-URL signatures.
Nearly every line's
request_uricontainsssec-key(the actual customer-supplied encryption key for SSE-C), plusSignature/Expires/Key-Pair-Idfor presigned URLs (e.g. lines 1, 4, 26, 113, 226). Static analysis only flagged line 221 as a generic API key, but the same secret-shaped fields recur across the entire file. Committing this permanently into version control exposes these keys/signatures in history even if the specific presigned URLs have since expired, since thessec-keyvalues are tied to the underlying encrypted blobs rather than to the URL expiry window.Recommend scrubbing
ssec-key,Signature, andKey-Pair-Idvalues (or the full query string) before committing operational analysis captures, or excluding raw access logs from the repo entirely in favor of a redacted/aggregated summary.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@deploy/helm/container-cache/_cc-analysis/access-2.jsonl` around lines 1 - 236, The committed access-log capture contains sensitive SSE-C keys and presigned URL credentials in request_uri values. Remove this raw log from version control or replace it with a redacted/aggregated capture, ensuring all ssec-key, Signature, Key-Pair-Id, and related query-string values are scrubbed throughout the file.Source: Linters/SAST tools
🧹 Nitpick comments (4)
deploy/helm/container-cache/_cc-analysis/nvcf-container-config.json (1)
1-27: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReconsider committing raw operational snapshots (ConfigMap dumps + Prometheus scrapes) to version control.
All 8 files in this cohort are point-in-time captures from a live cluster (
kubectl get configmap -o jsonoutput and/metricsscrapes), not Helm chart source. They persist live, mutable cluster metadata (resourceVersion,uid,creationTimestamp), a live CA certificate, and real infra identifiers (hostnames, EBS disk serials, volume UUIDs) permanently in git history, with no lasting value once the underlying resources change. Recommend keeping this kind of debugging/validation evidence in the PR description, a linked issue, or an external artifact store instead of the source tree.
deploy/helm/container-cache/_cc-analysis/nvcf-container-config.json: drop, or move to a non-tracked location; if kept for audit purposes, stripresourceVersion/uid/creationTimestampand the redundantlast-applied-configurationannotation.deploy/helm/container-cache/_cc-analysis/nvcf-nginx-config.json: same treatment.deploy/helm/container-cache/_cc-analysis/nvcf-proxy-config.json: same treatment.deploy/helm/container-cache/_cc-analysis/nvcf-default-config.json: same treatment.deploy/helm/container-cache/_cc-analysis/unbound-cm-postdisable.json: same treatment; additionally redact/remove the embedded CA certificate PEM before persisting.deploy/helm/container-cache/_cc-analysis/nvmesh-metrics-59-19.prom: drop or replace with a short summarized excerpt relevant to the throughput-tuning rationale, instead of the full ~2100-line scrape.deploy/helm/container-cache/_cc-analysis/nvmesh-metrics-65-225-post.prom: same treatment.deploy/helm/container-cache/_cc-analysis/nvmesh-metrics-65-225.prom: same treatment.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@deploy/helm/container-cache/_cc-analysis/nvcf-container-config.json` around lines 1 - 27, Remove the raw operational snapshots from tracked source, including deploy/helm/container-cache/_cc-analysis/nvcf-container-config.json (anchor, lines 1-27), nvcf-nginx-config.json (lines 1-31), nvcf-proxy-config.json (lines 1-31), nvcf-default-config.json (lines 1-27), unbound-cm-postdisable.json (lines 1-79), nvmesh-metrics-59-19.prom (lines 1-2141), nvmesh-metrics-65-225-post.prom (lines 1-1598), and nvmesh-metrics-65-225.prom (lines 1-1598). If audit evidence must remain, move it outside the tracked source or sanitize the ConfigMap metadata, remove the embedded CA certificate from unbound-cm-postdisable.json, and replace each full metrics scrape with a short relevant summary.deploy/helm/container-cache/_cc-analysis/stats-2.json (1)
208-208: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFull-cluster raw stats dump committed for one node.
Same concern as the other
_cc-analysisstats files: this captures unrelated cluster-wide pod stats rather than just the container-cache pods. See consolidated comment.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@deploy/helm/container-cache/_cc-analysis/stats-2.json` at line 208, Remove the committed full-cluster raw stats dump from stats-2.json and replace it with data scoped only to container-cache pods, matching the filtering used by the other _cc-analysis stats files. Ensure the "pods" collection contains no unrelated cluster-wide pod entries.deploy/helm/container-cache/_cc-analysis/stats-0.json (1)
432-432: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFull-cluster raw stats dump committed for one node.
This file captures the entire node's pod list (dozens of unrelated namespaces/pods), not just the container-cache pods relevant to the herd-scale test being documented. See consolidated comment.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@deploy/helm/container-cache/_cc-analysis/stats-0.json` at line 432, Remove the full-cluster pod list from the stats-0.json fixture and retain only pod statistics relevant to the container-cache herd-scale test, excluding unrelated namespaces and workloads.deploy/helm/container-cache/_cc-analysis/stats-1.json (1)
397-397: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFull-cluster raw stats dump committed for one node.
Same concern as the other
_cc-analysisstats files: this captures unrelated cluster-wide pod stats rather than just the container-cache pods. See consolidated comment.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@deploy/helm/container-cache/_cc-analysis/stats-1.json` at line 397, Remove the committed full-cluster raw stats dump from the _cc-analysis stats artifact and regenerate or replace the “pods” data so it contains only container-cache pod statistics. Keep unrelated cluster-wide pod entries out of the resulting stats file, consistent with the other _cc-analysis stats files.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@deploy/helm/container-cache/_cc-analysis/access-0.jsonl`:
- Line 1: Remove both operational snapshot files from the PR and Git history,
rotate or revoke all exposed credentials, and regenerate sanitized snapshots. In
deploy/helm/container-cache/_cc-analysis/access-0.jsonl lines 1-1, remove raw
request URIs and identifying metadata; apply the same redaction policy in
deploy/helm/container-cache/_cc-analysis/access-1.jsonl lines 1-1, including
repeated token-like values, before re-exporting.
---
Outside diff comments:
In `@deploy/helm/container-cache/_cc-analysis/access-2.jsonl`:
- Around line 1-236: The committed access-log capture contains sensitive SSE-C
keys and presigned URL credentials in request_uri values. Remove this raw log
from version control or replace it with a redacted/aggregated capture, ensuring
all ssec-key, Signature, Key-Pair-Id, and related query-string values are
scrubbed throughout the file.
---
Nitpick comments:
In `@deploy/helm/container-cache/_cc-analysis/nvcf-container-config.json`:
- Around line 1-27: Remove the raw operational snapshots from tracked source,
including deploy/helm/container-cache/_cc-analysis/nvcf-container-config.json
(anchor, lines 1-27), nvcf-nginx-config.json (lines 1-31),
nvcf-proxy-config.json (lines 1-31), nvcf-default-config.json (lines 1-27),
unbound-cm-postdisable.json (lines 1-79), nvmesh-metrics-59-19.prom (lines
1-2141), nvmesh-metrics-65-225-post.prom (lines 1-1598), and
nvmesh-metrics-65-225.prom (lines 1-1598). If audit evidence must remain, move
it outside the tracked source or sanitize the ConfigMap metadata, remove the
embedded CA certificate from unbound-cm-postdisable.json, and replace each full
metrics scrape with a short relevant summary.
In `@deploy/helm/container-cache/_cc-analysis/stats-0.json`:
- Line 432: Remove the full-cluster pod list from the stats-0.json fixture and
retain only pod statistics relevant to the container-cache herd-scale test,
excluding unrelated namespaces and workloads.
In `@deploy/helm/container-cache/_cc-analysis/stats-1.json`:
- Line 397: Remove the committed full-cluster raw stats dump from the
_cc-analysis stats artifact and regenerate or replace the “pods” data so it
contains only container-cache pod statistics. Keep unrelated cluster-wide pod
entries out of the resulting stats file, consistent with the other _cc-analysis
stats files.
In `@deploy/helm/container-cache/_cc-analysis/stats-2.json`:
- Line 208: Remove the committed full-cluster raw stats dump from stats-2.json
and replace it with data scoped only to container-cache pods, matching the
filtering used by the other _cc-analysis stats files. Ensure the "pods"
collection contains no unrelated cluster-wide pod entries.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 7fb22ad8-ab69-48aa-af11-87bb74b509e1
📒 Files selected for processing (25)
deploy/helm/container-cache/_cc-analysis/access-0.jsonldeploy/helm/container-cache/_cc-analysis/access-1.jsonldeploy/helm/container-cache/_cc-analysis/access-2.jsonldeploy/helm/container-cache/_cc-analysis/ds1.txtdeploy/helm/container-cache/_cc-analysis/ds2.txtdeploy/helm/container-cache/_cc-analysis/n1-a.txtdeploy/helm/container-cache/_cc-analysis/n1-b.txtdeploy/helm/container-cache/_cc-analysis/n2-a.txtdeploy/helm/container-cache/_cc-analysis/n2-b.txtdeploy/helm/container-cache/_cc-analysis/nvcf-container-config.jsondeploy/helm/container-cache/_cc-analysis/nvcf-default-config.jsondeploy/helm/container-cache/_cc-analysis/nvcf-nginx-config.jsondeploy/helm/container-cache/_cc-analysis/nvcf-proxy-config.jsondeploy/helm/container-cache/_cc-analysis/nvmesh-metrics-19301.promdeploy/helm/container-cache/_cc-analysis/nvmesh-metrics-19302.promdeploy/helm/container-cache/_cc-analysis/nvmesh-metrics-53-26.promdeploy/helm/container-cache/_cc-analysis/nvmesh-metrics-59-19.promdeploy/helm/container-cache/_cc-analysis/nvmesh-metrics-65-225-post.promdeploy/helm/container-cache/_cc-analysis/nvmesh-metrics-65-225.promdeploy/helm/container-cache/_cc-analysis/stats-0.jsondeploy/helm/container-cache/_cc-analysis/stats-1.jsondeploy/helm/container-cache/_cc-analysis/stats-2.jsondeploy/helm/container-cache/_cc-analysis/unbound-cm-postdisable.jsondeploy/helm/container-cache/deploy/files/ngc.confdeploy/helm/container-cache/deploy/values.yaml
🚧 Files skipped from review as they are similar to previous changes (2)
- deploy/helm/container-cache/deploy/files/ngc.conf
- deploy/helm/container-cache/deploy/values.yaml
| @@ -0,0 +1,142 @@ | |||
| {"access_time":"18/Jul/2026:04:48:51 +0000","remote_addr":"10.0.242.89","remote_user":"","request":"GET","status":"200","bytes_sent":"2413","http_user_agent":"python-requests/2.32.3","host":"xfiles.ngc.nvidia.com","proxy_host":"xfiles.ngc.nvidia.com","upstream":"","upstream_status":"","ssl_protocol":"TLSv1.3","http_protocol":"HTTP/1.1","upstream_http_location":"","upstream_cache_status":"HIT","connection":"2136","connection_requests":"1","request_uri":"/org/qc69jvmznzxy/team/llm_nim/modelsv2/inkling-nvfp4/blobs/sha256/51/51a61585d2613510a478bb194d24afe1719a4acb126760748a865c1f98d0edcc/file?ssec-algo=AES256&versionId=5neIUQNCIfcPg7V6DLv9T2ACfWV9Ngqb&ssec-key=CCNGKtxYfbJEwwLhf2jPnA7trwBWs8QI%2F3SqJcJrdVo6gmVtGnIpjVBLYR0QpJfgiNbZhMEf5R2h1i%2FuQz2YKOPi0el%2BliovESWUwgT6CF2DpDnCtYuZMHYhR4mcAVuDdDnwx5FzZDJc3%2BBLEURwXr51wKUxx%2B0A%2BH5qYemPgnEehgPA3ESthxWMr1OqNkENeAlxIik%2FJEzbA9vV1vhFChTTh%2F1Jq5JeCG5AH6DcKloK3MXMQxsqTAGIV%2BaFjF3THCAuVtHKTypSwbk0X06NIOl2ls8yevB%2BS0yvJvXcDY3fX7IPvPHbZncpjl1i6oo5fjDV1xMC4SLzKB6%2BMLAqGzMLBdzPe8RHWLeyKun%2BD4ciBdtRvzwWCHKCbpOyWoyDN8iXQW9ObYrBJJgg3uDGydL1GV0rkhuoMpqx%2BMv3OAGrsXVfxosa2ZeU%2BkWuSO1lQDE83NXudRBoKztr5Fw%2F2vwWtmSf7ClVWjPMxkdiNun5sk6RZEBgTeXoE%2FRmgA2B&kid=bXJrLWU3OGM1M2FhZjE4YzRiNmJiNjlkYmRhZjcxNjA3YWEw&ssec-enabled=true&Expires=1784436521&Signature=WaG5A4QCbZAYycOltR4r53m5DFToHerlhPMo7DaQasMVKYQuCsdeHHNohHh0yzrdhoF8ONL1otOWcFf45EndHK1YFmDdB31-viL2cYfe2xCqd7n-oB4~gRRgdVVe0SArSwfnTCoXzPgDuE3NoE~pLiJMVkRfyizJbqn6UNQSDJZxF-H5HuD5luFNslLQppYu9J6G6XYBrfa6mGNOGdYo1UaPQyn02c-pGdkoHQmnECZX9eZgNmDnBoD3ASm-RD3qQaVEszqzSVxIbE5WabeoOn8-4ySrM-rAaR9ffPHF3mPPM1r-sDsZoU84C3yO3qeEQT922D3MmaJYzdfIcB9rqA__&Key-Pair-Id=KCX06E8E9L60W","etag":"","request_range":"","has_auth_header":"0","upstream_response_time":"","request_time":"6.864","docker_proxy_request_type":"nvcf-proxy-cache","disable_cache":"0","http_trace_id":"","http_request_id":""} | |||
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Purge and redact both operational snapshots.
Both files commit raw signed-download data, including ssec-key and Signature query parameters. Remove the files from the PR and Git history, rotate/revoke any exposed credentials, and regenerate sanitized snapshots.
deploy/helm/container-cache/_cc-analysis/access-0.jsonl#L1-L1: remove all raw request URIs and identifying metadata before re-exporting.deploy/helm/container-cache/_cc-analysis/access-1.jsonl#L1-L1: apply the same redaction policy; Betterleaks flags repeated token-like values later in this file.
As per coding guidelines, service changes must “redact secrets and user data.”
📍 Affects 2 files
deploy/helm/container-cache/_cc-analysis/access-0.jsonl#L1-L1(this comment)deploy/helm/container-cache/_cc-analysis/access-1.jsonl#L1-L1
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@deploy/helm/container-cache/_cc-analysis/access-0.jsonl` at line 1, Remove
both operational snapshot files from the PR and Git history, rotate or revoke
all exposed credentials, and regenerate sanitized snapshots. In
deploy/helm/container-cache/_cc-analysis/access-0.jsonl lines 1-1, remove raw
request URIs and identifying metadata; apply the same redaction policy in
deploy/helm/container-cache/_cc-analysis/access-1.jsonl lines 1-1, including
repeated token-like values, before re-exporting.
Sources: Coding guidelines, Linters/SAST tools
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
2 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.
Why
The nginx-proxy liveness probe is an HTTP GET against
/healthz, served by the same worker processes as the data plane. Under sustained heavy transfer load (many concurrent large downloads filling the cache) the probe times out and kubelet kills a proxy that is merely busy. Each kill aborts every in-flight transfer, the client retry herd re-saturates the tier, and reconnecting clients re-pool onto the surviving replicas - skewing traffic long after the killed pods return Ready. Loosening the HTTP liveness thresholds was tried previously and is not sufficient; herd-scale load testing reproduced the kill with node network and backing storage independently verified healthy.What changed
httpGet /healthz->tcpSocketon the same port. The kernel completes the TCP accept while nginx holds the listener, so liveness fails only when the process is actually dead or wedged - a busy proxy is never killed.httpGet /healthzwith slightly relaxed timeout/threshold (5s/5), so an overloaded replica sheds new traffic (degrades) without being restarted (crashes).probes:values block.aio_write onmoves cache-fill temp-file writes to the thread pool (a multi-second write parks a pool thread, not a worker event loop); explicitthread_poolsizing (cache.aioThreads, default 64) absorbs storage latency instead of queuing it; a per-server connection cap on the model-download server (cache.maxConnsPerServer, default 512,0disables) fails excess requests fast with 503 - which ranged clients retry per chunk - instead of parking them on a storage-bound tier.Customer Release Notes
The container cache proxy is no longer restarted by its liveness probe under sustained heavy download load; overloaded replicas shed traffic instead of aborting in-flight transfers.
Plan Summary
Helm chart change only (probe definitions, chart version). No new resources.
Usage
Not applicable.
Testing
make test(newtests/render-probes-test.sh): rendered-output regression assertions for default probes, probe overrides, resilience directives, and the cap-disable path.aioThreadsoverride, and themaxConnsPerServer=0disable path (including the Helm zero-vs-default pitfall).Notes
Follow-up candidates identified during the same testing, intentionally out of scope here:
proxy_cache_lock_timeoutreduction andproxy_ignore_client_abortevaluation for fill completion on client abort.Issues
Fixes #316
Related Merge Requests/Pull Requests
None
Dependencies
None
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests