rds test 22 better setup - #831
Conversation
WalkthroughUpdates a test fixture: switches the workflow to deploy a manifest in namespace app-22, validates DB secrets, stops an RDS instance, polls for RDS stopped status and MySQL connection errors in pod logs, and cleans up resources. The manifest adds namespace app-22 and removes commented monitoring resources. Changes
Sequence Diagram(s)sequenceDiagram
participant TR as Test Runner
participant K8s as Kubernetes API
participant RDS as AWS RDS
participant Pod as App Pod (logs)
TR->>K8s: Verify secret db-secrets-for-medium in namespace app-22
TR->>RDS: Check RDS instance status
alt RDS running
TR->>RDS: Stop instance
TR->>RDS: Poll status until stopped/stopping (≤60s)
end
TR->>K8s: Apply manifest.yaml to app-22
TR->>Pod: Poll logs for MySQL connection error (≤60s)
Pod-->>TR: Emit connection error detected
TR->>K8s: Delete applied manifest (cleanup)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~18 minutes ✨ 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: 3
🔭 Outside diff range comments (1)
tests/llm/fixtures/test_ask_holmes/22_high_latency_dbi_down/manifest.yaml (1)
43-49: Inline shell script in PodSpec violates “ALWAYS use Secrets for scripts” guideline.Replace the inline loop with a script mounted from a Secret or avoid a shell script entirely.
Apply this minimal change to call a mounted script instead of embedding it inline:
- name: curl-sidecar image: curlimages/curl - args: - - /bin/sh - - -c - - while true; do curl -s http://localhost:8000; sleep 60; done + command: ["/bin/sh", "/scripts/curl-loop.sh"]Additionally, add a volume mount and the Secret-backed volume elsewhere in this manifest:
# add under the curl-sidecar container volumeMounts: - name: curl-loop-script mountPath: /scripts readOnly: true # add at the pod spec level volumes: - name: curl-loop-script secret: secretName: curl-loop-script defaultMode: 0555And create a Secret (outside this manifest or as a separate doc) containing the script:
apiVersion: v1 kind: Secret metadata: name: curl-loop-script namespace: app-22 type: Opaque stringData: curl-loop.sh: | #!/bin/sh while true; do curl -sS http://localhost:8000 || true; sleep 60; done
🧹 Nitpick comments (6)
tests/llm/fixtures/test_ask_holmes/22_high_latency_dbi_down/manifest.yaml (1)
16-21: Add minimal resource requests/limits to keep test footprint low.Tests should specify small resources to avoid cluster contention.
containers: - name: fastapi-app image: us-central1-docker.pkg.dev/genuine-flight-317411/devel/rds-demo:v1 + resources: + requests: + cpu: 25m + memory: 64Mi + limits: + cpu: 100m + memory: 128Mi ports: - containerPort: 8000 - containerPort: 8001 - name: curl-sidecar image: curlimages/curl + resources: + requests: + cpu: 5m + memory: 32Mi + limits: + cpu: 25m + memory: 64MiAlso applies to: 43-45
tests/llm/fixtures/test_ask_holmes/22_high_latency_dbi_down/test_case.yaml (5)
7-10: Small grammar tweak in description.Fix “creating a the secret” → “creating the secret.”
- This requires first creating a the secret 'db-secrets-for-medium' in namespace app-22 w/ credentials for the RDS database. + This requires first creating the secret 'db-secrets-for-medium' in namespace app-22 w/ credentials for the RDS database.
18-29: Avoid decoding secret values during validation; check base64 length instead.Reduces exposure risk while still validating non-empty fields.
- kubectl get secret db-secrets-for-medium -n app-22 -o json \ - | jq -e ' - [.data.username, .data.password, .data.host, .data.database] - | map(select(. != null) | @base64d | select(length > 0)) - | length == 4 - ' >/dev/null \ + kubectl get secret db-secrets-for-medium -n app-22 -o json \ + | jq -e ' + [.data.username, .data.password, .data.host, .data.database] + | map(select(. != null and length > 0)) + | length == 4 + ' >/dev/null \
55-76: Make log match more robust.Match a broader, stable substring and limit log span to reduce noise. Also handle multiple restarts.
- if kubectl logs "$pod" -n app-22 2>/dev/null | grep -q "Can't connect to MySQL server on 'promotions-db-for-medium.cp8rwothwarq.us-east-2.rds.amazonaws.com"; then + if kubectl logs "$pod" -n app-22 --since=5m --max-log-requests=5 2>/dev/null | grep -q "Can't connect to MySQL server on"; then echo "Found MySQL connection error in logs" found=true break fi
33-35: Nit: -n flag is redundant when the manifest already specifies namespace.Harmless to keep; safe to drop for clarity.
- kubectl apply -f ./manifest.yaml -n app-22 + kubectl apply -f ./manifest.yaml
78-79: Cleanup step looks good; optional: delete namespace to guarantee isolation.Leaving it commented is fine if namespaces are reused by humans; consider enabling in CI.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/llm/fixtures/test_ask_holmes/22_high_latency_dbi_down/manifest.yaml(2 hunks)tests/llm/fixtures/test_ask_holmes/22_high_latency_dbi_down/test_case.yaml(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
tests/**
📄 CodeRabbit Inference Engine (CLAUDE.md)
Tests must match source structure under tests/
Files:
tests/llm/fixtures/test_ask_holmes/22_high_latency_dbi_down/manifest.yamltests/llm/fixtures/test_ask_holmes/22_high_latency_dbi_down/test_case.yaml
**/*.yaml
📄 CodeRabbit Inference Engine (CLAUDE.md)
ALWAYS use Secrets for scripts, not inline manifests or ConfigMaps (prevents code visibility with kubectl describe)
Files:
tests/llm/fixtures/test_ask_holmes/22_high_latency_dbi_down/manifest.yamltests/llm/fixtures/test_ask_holmes/22_high_latency_dbi_down/test_case.yaml
tests/**/*.yaml
📄 CodeRabbit Inference Engine (CLAUDE.md)
tests/**/*.yaml: Never use names that hint at the problem or expected behavior in resource names (e.g., avoid 'broken-pod', 'test-project-that-does-not-exist', 'crashloop-app'). Use neutral names that don't give away what the LLM should discover
Each test must use a dedicated namespace app- to prevent conflicts
All pod names must be unique across tests
Resource naming should be neutral, not hint at the problem
Use minimal resource footprints (e.g., reduce memory/CPU for Loki in tests)
Files:
tests/llm/fixtures/test_ask_holmes/22_high_latency_dbi_down/manifest.yamltests/llm/fixtures/test_ask_holmes/22_high_latency_dbi_down/test_case.yaml
⏰ 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 (2)
tests/llm/fixtures/test_ask_holmes/22_high_latency_dbi_down/manifest.yaml (2)
5-5: Namespace selection aligns with test isolation guideline (app-22).Good use of a dedicated namespace for this test.
54-54: Service namespace aligns with test isolation guideline (app-22).Consistent with the Deployment’s namespace.
| @@ -2,6 +2,7 @@ apiVersion: apps/v1 | |||
| kind: Deployment | |||
| metadata: | |||
| name: customer-orders-for-medium | |||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Consider namespacing resource names with the test ID to ensure cross-test uniqueness.
To satisfy “All pod names must be unique across tests,” consider suffixing with “-22” (e.g., customer-orders-for-medium-22).
Run this to check if names are reused elsewhere:
Also applies to: 53-53
🏁 Script executed:
#!/bin/bash
# Verify uniqueness of names across tests
rg -n $'name: *(customer-orders-for-medium|customer-orders-service-for-medium)\b' tests -A 2Length of output: 92
🏁 Script executed:
#!/bin/bash
# Re-verify usage of pod names across all test manifests
rg -F -n "name: customer-orders-for-medium" -g "*.yaml" tests
rg -F -n "name: customer-orders-service-for-medium" -g "*.yaml" testsLength of output: 366
Namespace resource names with the test ID for uniqueness
Although no other tests currently use these names, our convention requires every pod and service name to include the test ID. Please suffix both entries in this manifest with “-22”:
Files to update:
- tests/llm/fixtures/test_ask_holmes/22_high_latency_dbi_down/manifest.yaml
Proposed diff:
@@ tests/llm/fixtures/test_ask_holmes/22_high_latency_dbi_down/manifest.yaml
- name: customer-orders-for-medium
+ name: customer-orders-for-medium-22
@@ tests/llm/fixtures/test_ask_holmes/22_high_latency_dbi_down/manifest.yaml
- name: customer-orders-service-for-medium
+ name: customer-orders-service-for-medium-22📝 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.
| name: customer-orders-for-medium | |
| @@ tests/llm/fixtures/test_ask_holmes/22_high_latency_dbi_down/manifest.yaml | |
| - name: customer-orders-for-medium | |
| + name: customer-orders-for-medium-22 | |
| @@ tests/llm/fixtures/test_ask_holmes/22_high_latency_dbi_down/manifest.yaml | |
| - name: customer-orders-service-for-medium | |
| + name: customer-orders-service-for-medium-22 |
🤖 Prompt for AI Agents
tests/llm/fixtures/test_ask_holmes/22_high_latency_dbi_down/manifest.yaml around
line 4: the resource names must include the test ID for uniqueness; update the
manifest so any pod and service name values in this file (including the current
name value "customer-orders-for-medium") are suffixed with "-22" (e.g.
"customer-orders-for-medium-22") so both pod and service entries use the test ID
suffix.
| # Stop RDS instance if not already stopped | ||
| [ "$(aws rds describe-db-instances --db-instance-identifier promotions-db-for-medium --query "DBInstances[0].DBInstanceStatus" --output text)" != "stopped" ] && aws rds stop-db-instance --db-instance-identifier promotions-db-for-medium || echo "RDS instance is already stopped." | ||
| kubectl apply -f ./slow-rds-query-for-medium.yaml | ||
| sleep 60 | ||
|
|
||
| # Apply deployment |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Pin AWS region to avoid reliance on ambient configuration.
Add --region us-east-2 to both describe and stop calls.
- [ "$(aws rds describe-db-instances --db-instance-identifier promotions-db-for-medium --query "DBInstances[0].DBInstanceStatus" --output text)" != "stopped" ] && aws rds stop-db-instance --db-instance-identifier promotions-db-for-medium || echo "RDS instance is already stopped."
+ [ "$(aws rds describe-db-instances --region us-east-2 --db-instance-identifier promotions-db-for-medium --query "DBInstances[0].DBInstanceStatus" --output text)" != "stopped" ] \
+ && aws rds stop-db-instance --region us-east-2 --db-instance-identifier promotions-db-for-medium \
+ || echo "RDS instance is already stopped."🤖 Prompt for AI Agents
In tests/llm/fixtures/test_ask_holmes/22_high_latency_dbi_down/test_case.yaml
around lines 30 to 33, the AWS CLI calls rely on ambient AWS region
configuration; update both aws rds describe-db-instances and aws rds
stop-db-instance invocations to include the flag --region us-east-2 so they
explicitly target the us-east-2 region.
| echo "Verifying RDS instance status..." | ||
| timeout=60 | ||
| elapsed=0 | ||
| while [ $elapsed -lt $timeout ]; do | ||
| status=$(aws rds describe-db-instances --db-instance-identifier promotions-db-for-medium --query "DBInstances[0].DBInstanceStatus" --output text) | ||
| if [ "$status" = "stopped" ] || [ "$status" = "stopping" ]; then | ||
| echo "RDS instance is $status" | ||
| break | ||
| fi | ||
| sleep 2 | ||
| elapsed=$((elapsed + 2)) | ||
| done | ||
|
|
||
| if [ "$status" != "stopped" ] && [ "$status" != "stopping" ]; then | ||
| echo "ERROR: RDS instance status is $status, expected stopped or stopping" | ||
| exit 1 | ||
| fi | ||
|
|
||
| # Wait for MySQL connection error in logs instead of fixed sleep |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Also pin region in polling loop.
Ensures consistent behavior regardless of env defaults.
- status=$(aws rds describe-db-instances --db-instance-identifier promotions-db-for-medium --query "DBInstances[0].DBInstanceStatus" --output text)
+ status=$(aws rds describe-db-instances --region us-east-2 --db-instance-identifier promotions-db-for-medium --query "DBInstances[0].DBInstanceStatus" --output text)📝 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.
| # Verify RDS instance is stopped or stopping | |
| echo "Verifying RDS instance status..." | |
| timeout=60 | |
| elapsed=0 | |
| while [ $elapsed -lt $timeout ]; do | |
| status=$(aws rds describe-db-instances --db-instance-identifier promotions-db-for-medium --query "DBInstances[0].DBInstanceStatus" --output text) | |
| if [ "$status" = "stopped" ] || [ "$status" = "stopping" ]; then | |
| echo "RDS instance is $status" | |
| break | |
| fi | |
| sleep 2 | |
| elapsed=$((elapsed + 2)) | |
| done | |
| if [ "$status" != "stopped" ] && [ "$status" != "stopping" ]; then | |
| echo "ERROR: RDS instance status is $status, expected stopped or stopping" | |
| exit 1 | |
| fi | |
| # Verify RDS instance is stopped or stopping | |
| echo "Verifying RDS instance status..." | |
| timeout=60 | |
| elapsed=0 | |
| while [ $elapsed -lt $timeout ]; do | |
| status=$(aws rds describe-db-instances --region us-east-2 --db-instance-identifier promotions-db-for-medium --query "DBInstances[0].DBInstanceStatus" --output text) | |
| if [ "$status" = "stopped" ] || [ "$status" = "stopping" ]; then | |
| echo "RDS instance is $status" | |
| break | |
| fi | |
| sleep 2 | |
| elapsed=$((elapsed + 2)) | |
| done | |
| if [ "$status" != "stopped" ] && [ "$status" != "stopping" ]; then | |
| echo "ERROR: RDS instance status is $status, expected stopped or stopping" | |
| exit 1 | |
| fi |
🤖 Prompt for AI Agents
In tests/llm/fixtures/test_ask_holmes/22_high_latency_dbi_down/test_case.yaml
around lines 36 to 54, the AWS CLI describe-db-instances call in the polling
loop does not specify a region, which can yield inconsistent behavior; update
the command to explicitly pin the region (either by adding --region <REGION>
using the same region variable used elsewhere, e.g. --region "$AWS_REGION", or a
hardcoded region constant used by the test suite) so every invocation of aws rds
describe-db-instances uses the intended region.
2 participants
No description provided.