Repository navigation
ROB-1807: add eval for disk space PVC issue - #920
Conversation
WalkthroughAdds a new pytest marker Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant T as Test Runner
participant K as Kubernetes API
participant SS as StatefulSet Controller
participant P as Pod
participant A as DataProcessor
participant V as PVC (100Mi)
T->>K: create Namespace `app-157` and apply manifest (Secret, Service, StatefulSet)
K-->>SS: resource creation
SS->>K: request Pod + PVC
K-->>P: schedule Pod with volume V
P->>A: start container (python /app/app.py)
A->>V: seed historical data + write batch files
A-->>A: disk usage >100Mi → OSError ENOSPC
A->>P: process exits (non-zero)
P-->>K: CrashLoopBackOff observed
T->>K: wait and verify CrashLoopBackOff
T-->>T: assert expected outputs (disk-full, PVC=100Mi, resize recommendation)
T->>K: delete Namespace (cleanup)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
tests/llm/fixtures/test_ask_holmes/157_disk_full_statefulset/manifest.yaml (2)
275-279: Avoid executable permissions on Secret files.0755 is unnecessary; default (0644) is fine. Drop defaultMode to use Kubernetes default.
- name: app-code secret: secretName: data-processor-app - defaultMode: 0755
211-227: Service appears unused; consider removing to simplify the eval.The container exposes no port and the app doesn’t listen on 8080. Headless Service adds noise for the LLM without value.
---- -apiVersion: v1 -kind: Service -metadata: - name: data-processor - namespace: app-157 - labels: - app: data-processor -spec: - clusterIP: None - selector: - app: data-processor - ports: - - name: metrics - port: 8080 - targetPort: 8080tests/llm/fixtures/test_ask_holmes/157_disk_full_statefulset/test_case.yaml (2)
8-12: Make the CrashLoop wait more robust across kubectl/K8s versions.jsonpath waits can be flaky if fields are momentarily absent. Consider a simple poll loop.
before_test: | - kubectl create namespace app-157 || true kubectl apply -f manifest.yaml -n app-157 - kubectl wait --for=jsonpath='{.status.containerStatuses[0].state.waiting.reason}'=CrashLoopBackOff pod -l app=data-processor -n app-157 --timeout=300s + end=$((SECONDS+300)) + while :; do + reason=$(kubectl get pods -l app=data-processor -n app-157 -o jsonpath='{range .items[*]}{.status.containerStatuses[0].state.waiting.reason}{"\n"}{end}' 2>/dev/null || true) + if echo "$reason" | grep -q CrashLoopBackOff; then break; fi + if [ $SECONDS -ge $end ]; then echo "Timed out waiting for CrashLoopBackOff"; exit 1; fi + sleep 3 + done
1-1: Minor prompt polish.Single “?” reads cleaner.
-user_prompt: "What is wrong with data-processor in the app-157 namespace??" +user_prompt: "What is wrong with data-processor in the app-157 namespace?"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
pyproject.toml(1 hunks)tests/llm/fixtures/test_ask_holmes/157_disk_full_statefulset/manifest.yaml(1 hunks)tests/llm/fixtures/test_ask_holmes/157_disk_full_statefulset/test_case.yaml(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
tests/llm/**/test_case.yaml
📄 CodeRabbit inference engine (CLAUDE.md)
Eval test cases may declare runbooks in test_case.yaml using either runbooks: {} or runbooks: {catalog: [...]}; if omitted, defaults are used
Files:
tests/llm/fixtures/test_ask_holmes/157_disk_full_statefulset/test_case.yaml
tests/llm/**/*.{yaml,yml}
📄 CodeRabbit inference engine (CLAUDE.md)
tests/llm/**/*.{yaml,yml}: Each LLM eval test must use a dedicated Kubernetes namespace named app-
For Kubernetes-related eval assets, always use Secrets for scripts; do not embed scripts in inline manifests or ConfigMaps
Files:
tests/llm/fixtures/test_ask_holmes/157_disk_full_statefulset/test_case.yamltests/llm/fixtures/test_ask_holmes/157_disk_full_statefulset/manifest.yaml
tests/llm/**
📄 CodeRabbit inference engine (CLAUDE.md)
Resource and file naming in evals should be neutral and must not hint at the problem (avoid names like broken-pod or crashloop-app)
Files:
tests/llm/fixtures/test_ask_holmes/157_disk_full_statefulset/test_case.yamltests/llm/fixtures/test_ask_holmes/157_disk_full_statefulset/manifest.yaml
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-24T07:21:02.611Z
Learning: Applies to tests/**/*.py : Do not use invalid pytest markers; only use markers/tags declared in pyproject.toml
📚 Learning: 2025-08-24T07:21:02.611Z
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-24T07:21:02.611Z
Learning: Applies to tests/**/*.py : Do not use invalid pytest markers; only use markers/tags declared in pyproject.toml
Applied to files:
pyproject.toml
🪛 Checkov (3.2.334)
tests/llm/fixtures/test_ask_holmes/157_disk_full_statefulset/manifest.yaml
[MEDIUM] 229-286: Containers should not run with allowPrivilegeEscalation
(CKV_K8S_20)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: build (3.11)
- GitHub Check: build (3.10)
- GitHub Check: build (3.12)
- GitHub Check: build (3.10)
- GitHub Check: build (3.12)
- GitHub Check: build (3.11)
- GitHub Check: llm_evals
🔇 Additional comments (1)
pyproject.toml (1)
123-124: Approve storage marker addition
Verified that only declared custom markers (including storage and llm) are used in tests; no undeclared pytest markers detected.
…isk space enforcement instead of artifical log line
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
tests/llm/fixtures/test_ask_holmes/157_disk_full_statefulset/manifest.yaml (2)
223-226: Mount the Secret read-only and tighten file mode.Make /app read-only and avoid executable bits for code files.
volumeMounts: - name: app-code mountPath: /app + readOnly: true @@ - name: app-code secret: secretName: data-processor-app - defaultMode: 0755 + defaultMode: 0444Also applies to: 227-231
200-206: Optional hardening: disallow privilege escalation, drop caps, seccomp, read-only root FS.This addresses CKV_K8S_20 and improves determinism. Note: learnings say security hardening isn’t a priority for evals—treat as optional.
securityContext: fsGroup: 1000 runAsNonRoot: true runAsUser: 1000 + seccompProfile: + type: RuntimeDefault @@ - name: data-processor image: python:3.9-slim command: ["python", "/app/app.py"] + securityContext: + allowPrivilegeEscalation: false + readOnlyRootFilesystem: true + capabilities: + drop: ["ALL"]Also applies to: 215-222
🧹 Nitpick comments (5)
tests/llm/fixtures/test_ask_holmes/157_disk_full_statefulset/manifest.yaml (5)
208-214: Prevent .pyc writes to read-only code mount.Add PYTHONDONTWRITEBYTECODE=1 to avoid pycache attempts when /app is read-only.
env: - name: PYTHONUNBUFFERED value: "1" + - name: PYTHONDONTWRITEBYTECODE + value: "1"
205-214: Surface errors in kube describe.Set terminationMessagePolicy so the traceback shows up in pod status.
- name: data-processor image: python:3.9-slim command: ["python", "/app/app.py"] + terminationMessagePolicy: FallbackToLogsOnError
206-206: Pin image by digest for reproducibility.Consider python:3.9-slim@sha256: to avoid upstream image drift in CI.
236-241: Longhorn coupling—confirm or make storage class generic.If CI clusters don’t guarantee Longhorn as default, omit storageClassName to use the default SC; otherwise keep as-is.
- storageClassName: longhorn
112-117: Reduce LLM bias: let OSError crash without crafted error messages.Remove custom exception prints so the raw "No space left on device" propagates naturally, per prior guidance.
- try: - # Initialize the historical data repository with seed data - print(f"[{datetime.now()}] Loading historical analytics dataset...") - historical_file = f"{self.data_dir}/historical_analytics.dat" - with open(historical_file, 'wb') as f: - months_of_data = 24 # 2 years of historical data - for month in range(months_of_data): - monthly_data_size = 50 * 1024 * 1024 # 50MB per month is realistic - f.write(os.urandom(monthly_data_size)) - if month % 6 == 0 and month > 0: - print(f"[{datetime.now()}] Loaded {month} months of historical data for trend analysis") - print(f"[{datetime.now()}] Historical data repository initialized successfully") - except OSError as e: - print(f"[{datetime.now()}] ERROR: Failed to initialize historical data repository") - print(f"[{datetime.now()}] CRITICAL: Unable to load required historical dataset") - print(f"[{datetime.now()}] {e}") - raise + print(f"[{datetime.now()}] Loading historical analytics dataset...") + historical_file = f"{self.data_dir}/historical_analytics.dat" + with open(historical_file, 'wb') as f: + months_of_data = 24 + for month in range(months_of_data): + monthly_data_size = 50 * 1024 * 1024 + f.write(os.urandom(monthly_data_size)) + if month % 6 == 0 and month > 0: + print(f"[{datetime.now()}] Loaded {month} months of historical data for trend analysis") + print(f"[{datetime.now()}] Historical data repository initialized successfully") @@ - try: - batch_id += 1 + batch_id += 1 print(f"[{datetime.now()}] Processing batch {batch_id}") @@ - except OSError as e: - print(f"[{datetime.now()}] CRITICAL ERROR: Failed to write batch {batch_id}") - print(f"[{datetime.now()}] {e}") - raise - except Exception as e: - print(f"[{datetime.now()}] ERROR: Unexpected error in batch {batch_id}: {e}") - raiseAlso applies to: 152-159
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
tests/llm/fixtures/test_ask_holmes/157_disk_full_statefulset/manifest.yaml(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
tests/llm/**/*.{yaml,yml}
📄 CodeRabbit inference engine (CLAUDE.md)
tests/llm/**/*.{yaml,yml}: Each LLM eval test must use a dedicated Kubernetes namespace named app-
For Kubernetes-related eval assets, always use Secrets for scripts; do not embed scripts in inline manifests or ConfigMaps
Files:
tests/llm/fixtures/test_ask_holmes/157_disk_full_statefulset/manifest.yaml
tests/llm/**
📄 CodeRabbit inference engine (CLAUDE.md)
Resource and file naming in evals should be neutral and must not hint at the problem (avoid names like broken-pod or crashloop-app)
Files:
tests/llm/fixtures/test_ask_holmes/157_disk_full_statefulset/manifest.yaml
🧠 Learnings (1)
📚 Learning: 2025-08-05T06:14:39.523Z
Learnt from: aantn
PR: robusta-dev/holmesgpt#783
File: tests/llm/fixtures/test_ask_holmes/100_historical_logs/payment-api.yaml:49-70
Timestamp: 2025-08-05T06:14:39.523Z
Learning: For evaluation test fixtures in the holmesgpt project, security contexts and security hardening are not priorities. The focus should be on functionality and test reliability rather than adding security configurations to Kubernetes manifests used in evals.
Applied to files:
tests/llm/fixtures/test_ask_holmes/157_disk_full_statefulset/manifest.yaml
🪛 Checkov (3.2.334)
tests/llm/fixtures/test_ask_holmes/157_disk_full_statefulset/manifest.yaml
[MEDIUM] 182-240: Containers should not run with allowPrivilegeEscalation
(CKV_K8S_20)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Pre-commit checks
- GitHub Check: llm_evals
- GitHub Check: Pre-commit checks
🔇 Additional comments (2)
tests/llm/fixtures/test_ask_holmes/157_disk_full_statefulset/manifest.yaml (2)
1-5: Namespace and Secret usage comply with eval guidelines.
- Dedicated namespace app-157 is correct.
- Scripts are provided via a Secret, not inline/ConfigMap. Good.
215-221: Resource requests/limits look reasonable for this workload.Should trigger disk exhaustion before memory/CPU limits. LGTM.
…rn for disk space enforcement instead of artifical log line" This reverts commit e5b964a.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (6)
tests/llm/fixtures/test_ask_holmes/157_disk_full_statefulset/manifest.yaml (6)
247-269: Optional hardening: add seccomp and drop privileges.Checkov flagged CKV_K8S_20. For test fixtures, security isn’t a priority per learnings, but this is low-cost.
Apply:
securityContext: fsGroup: 1000 runAsNonRoot: true runAsUser: 1000 + seccompProfile: + type: RuntimeDefault containers: - name: data-processor image: python:3.9-slim command: ["python", "/app/app.py"] + securityContext: + allowPrivilegeEscalation: false + readOnlyRootFilesystem: true + capabilities: + drop: ["ALL"]
270-274: Mount Secret read-only.Code under /app should not be writable.
Apply:
volumeMounts: - name: app-code mountPath: /app + readOnly: true - name: data-storage mountPath: /data
1-1: Directory name leaks scenario; rename to be neutral.tests/llm/fixtures/test_ask_holmes/157_disk_full_statefulset → e.g., 157_statefulset_app.
145-156: Replace bare except and exit() with Exception and sys.exit(1).Prevents masking BaseException and uses proper termination.
Apply:
- except: + except Exception: pass @@ - print(e) - exit(1) + print(e) + sys.exit(1)
194-203: Use sys.exit(1) instead of exit() in runtime error paths.Aligns with non-interactive best practices; avoids REPL-only helper.
Apply:
- print(e) - exit(1) + print(e) + sys.exit(1) @@ - print(f"[{datetime.now()}] ERROR: I/O error while processing batch {batch_id}: {e}") - exit(1) + print(f"[{datetime.now()}] ERROR: I/O error while processing batch {batch_id}: {e}") + sys.exit(1)
14-20: Import sys and stop using exit().Use sys.exit for non-interactive programs.
Apply:
import json from datetime import datetime + import sys
🧹 Nitpick comments (3)
tests/llm/fixtures/test_ask_holmes/157_disk_full_statefulset/manifest.yaml (3)
103-110: Nit: clarify units to MiB to match PVC.Comment says “100MB” but threshold is 10010241024 (~100MiB) and PVC is 100Mi. Consider updating the comment for consistency.
- """Check if folder size exceeds 100MB and simulate disk full + """Check if folder size exceeds 100MiB and simulate disk full
211-227: Headless Service likely unnecessary.The app doesn’t expose a listener; consider dropping the Service to reduce clutter unless something depends on it.
195-199: Confirm logging strategy won’t bias the eval.Explicit “CRITICAL ERROR … storage capacity exceeded” may steer the model; consider letting the raw OSError surface.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
tests/llm/fixtures/test_ask_holmes/157_disk_full_statefulset/manifest.yaml(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
tests/llm/**/*.{yaml,yml}
📄 CodeRabbit inference engine (CLAUDE.md)
tests/llm/**/*.{yaml,yml}: Each LLM eval test must use a dedicated Kubernetes namespace named app-
For Kubernetes-related eval assets, always use Secrets for scripts; do not embed scripts in inline manifests or ConfigMaps
Files:
tests/llm/fixtures/test_ask_holmes/157_disk_full_statefulset/manifest.yaml
tests/llm/**
📄 CodeRabbit inference engine (CLAUDE.md)
Resource and file naming in evals should be neutral and must not hint at the problem (avoid names like broken-pod or crashloop-app)
Files:
tests/llm/fixtures/test_ask_holmes/157_disk_full_statefulset/manifest.yaml
🧠 Learnings (2)
📚 Learning: 2025-08-05T06:14:39.523Z
Learnt from: aantn
PR: robusta-dev/holmesgpt#783
File: tests/llm/fixtures/test_ask_holmes/100_historical_logs/payment-api.yaml:49-70
Timestamp: 2025-08-05T06:14:39.523Z
Learning: For evaluation test fixtures in the holmesgpt project, security contexts and security hardening are not priorities. The focus should be on functionality and test reliability rather than adding security configurations to Kubernetes manifests used in evals.
Applied to files:
tests/llm/fixtures/test_ask_holmes/157_disk_full_statefulset/manifest.yaml
📚 Learning: 2025-08-24T07:21:02.611Z
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-24T07:21:02.611Z
Learning: Applies to tests/llm/** : Resource and file naming in evals should be neutral and must not hint at the problem (avoid names like broken-pod or crashloop-app)
Applied to files:
tests/llm/fixtures/test_ask_holmes/157_disk_full_statefulset/manifest.yaml
🪛 Checkov (3.2.334)
tests/llm/fixtures/test_ask_holmes/157_disk_full_statefulset/manifest.yaml
[MEDIUM] 229-286: Containers should not run with allowPrivilegeEscalation
(CKV_K8S_20)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: llm_evals
- GitHub Check: Pre-commit checks
- GitHub Check: Pre-commit checks
🔇 Additional comments (4)
tests/llm/fixtures/test_ask_holmes/157_disk_full_statefulset/manifest.yaml (4)
1-5: Namespace naming LGTM.Namespace app-157 complies with app- guideline.
6-13: Using a Secret for the script is correct.Meets “use Secrets for scripts” requirement.
280-286: PVC storage class not specified; summary mentions longhorn.If reproducibility depends on enforcement, set storageClassName. Otherwise your in-app simulation suffices.
Suggested:
spec: accessModes: [ "ReadWriteOnce" ] + storageClassName: longhorn resources: requests: storage: 100Mi
275-279: Fix defaultMode: 0755 is incorrect here.Kubernetes expects decimal mode; use 420 (0644) or drop the field. The file doesn’t need +x when invoked as “python app.py”.
Apply one:
- defaultMode: 0755 + # 420 == 0644 + defaultMode: 420or
- defaultMode: 0755 + # defaultMode not required; keep Secret default (0644)Likely an incorrect or invalid review comment.
2 participants
No description provided.