feat(helm): Add api-server deployment to the chart. #1818
Conversation
WalkthroughConditionally adds an optional API server to the Helm chart: new Changes
Sequence Diagram(s)mermaid note over Helm: Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
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. Comment |
|
@hoophalab / @LinZhihao-723 could you see if there's any missing coverage in the "Validation performed" section and help test it a bit more? |
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
tools/deployment/package-helm/templates/configmap.yaml(1 hunks)tools/deployment/package-helm/test.sh(2 hunks)tools/deployment/package-helm/values.yaml(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-05-06T10:07:04.654Z
Learnt from: kirkrodrigues
Repo: y-scope/clp PR: 881
File: components/core/tools/scripts/lib_install/macos/install-all.sh:11-12
Timestamp: 2025-05-06T10:07:04.654Z
Learning: In CLP installation scripts, temporary directories with downloaded files should not be automatically cleaned up on failure (e.g., with EXIT traps) to preserve artifacts for debugging purposes.
Applied to files:
tools/deployment/package-helm/test.sh
📚 Learning: 2025-12-04T03:31:55.239Z
Learnt from: junhaoliao
Repo: y-scope/clp PR: 1690
File: tools/deployment/package/docker-compose-all.yaml:424-427
Timestamp: 2025-12-04T03:31:55.239Z
Learning: In tools/deployment/package/docker-compose-all.yaml, the query-worker service writes to /var/data/streams (CLP_STREAM_OUTPUT_DIR_HOST mount), so this directory must remain read-write and should not be mounted with :ro flag.
Applied to files:
tools/deployment/package-helm/test.sh
📚 Learning: 2025-11-03T16:17:40.223Z
Learnt from: hoophalab
Repo: y-scope/clp PR: 1535
File: components/clp-rust-utils/src/clp_config/package/config.rs:47-61
Timestamp: 2025-11-03T16:17:40.223Z
Learning: In the y-scope/clp repository, the `ApiServer` struct in `components/clp-rust-utils/src/clp_config/package/config.rs` is a Rust-native configuration type and does not mirror any Python code, unlike other structs in the same file (Config, Database, ResultsCache, Package) which are mirrors of Python definitions.
Applied to files:
tools/deployment/package-helm/templates/configmap.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). (1)
- GitHub Check: package-image
🔇 Additional comments (3)
tools/deployment/package-helm/values.yaml (1)
77-84: LGTM! API server configuration looks well-structured.The configuration block follows existing patterns in the file and provides sensible defaults. The port 30301 correctly aligns with the port mapping added in test.sh.
tools/deployment/package-helm/test.sh (2)
62-62: LGTM! Log directory addition is consistent.The API server log directory follows the same pattern as other service log directories in the script.
91-93: LGTM! Port mapping correctly configured.The port mapping for 30301 is properly structured and aligns with the API server port defined in values.yaml.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Pla 8000 n: Pro
📒 Files selected for processing (6)
tools/deployment/package-helm/templates/api-server-deployment.yaml(1 hunks)tools/deployment/package-helm/templates/api-server-logs-pv.yaml(1 hunks)tools/deployment/package-helm/templates/api-server-logs-pvc.yaml(1 hunks)tools/deployment/package-helm/templates/api-server-service.yaml(1 hunks)tools/deployment/package-helm/templates/configmap.yaml(1 hunks)tools/deployment/package-helm/values.yaml(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: junhaoliao
Repo: y-scope/clp PR: 1818
File: tools/deployment/package-helm/templates/configmap.yaml:12-12
Timestamp: 2025-12-19T05:03:32.320Z
Learning: In the y-scope/clp Helm chart, the `api_server.host` configuration field in the ConfigMap (tools/deployment/package-helm/templates/configmap.yaml) is unused because no other k8s internal services need to reach the API server—it's only accessed from outside the cluster via NodePort.
📚 Learning: 2025-12-19T05:03:32.320Z
Learnt from: junhaoliao
Repo: y-scope/clp PR: 1818
File: tools/deployment/package-helm/templates/configmap.yaml:12-12
Timestamp: 2025-12-19T05:03:32.320Z
Learning: In the y-scope/clp Helm chart, the `api_server.host` configuration field in the ConfigMap (tools/deployment/package-helm/templates/configmap.yaml) is unused because no other k8s internal services need to reach the API server—it's only accessed from outside the cluster via NodePort.
Applied to files:
tools/deployment/package-helm/values.yamltools/deployment/package-helm/templates/api-server-service.yamltools/deployment/package-helm/templates/api-server-logs-pvc.yamltools/deployment/package-helm/templates/api-server-logs-pv.yamltools/deployment/package-helm/templates/api-server-deployment.yamltools/deployment/package-helm/templates/configmap.yaml
📚 Learning: 2025-11-03T16:17:40.223Z
Learnt from: hoophalab
Repo: y-scope/clp PR: 1535
File: components/clp-rust-utils/src/clp_config/package/config.rs:47-61
Timestamp: 2025-11-03T16:17:40.223Z
Learning: In the y-scope/clp repository, the `ApiServer` struct in `components/clp-rust-utils/src/clp_config/package/config.rs` is a Rust-native configuration type and does not mirror any Python code, unlike other structs in the same file (Config, Database, ResultsCache, Package) which are mirrors of Python definitions.
Applied to files:
tools/deployment/package-helm/templates/api-server-service.yamltools/deployment/package-helm/templates/api-server-deployment.yamltools/deployment/package-helm/templates/configmap.yaml
📚 Learning: 2025-11-10T05:19:56.600Z
Learnt from: junhaoliao
Repo: y-scope/clp PR: 1575
File: components/clp-py-utils/clp_py_utils/clp_config.py:602-607
Timestamp: 2025-11-10T05:19:56.600Z
Learning: In the y-scope/clp repository, the `ApiServer` class in `components/clp-py-utils/clp_py_utils/clp_config.py` does not need a `transform_for_container()` method because no other containerized service depends on the API server - it's only accessed from the host, so no docker-network communication is expected.
Applied to files:
tools/deployment/package-helm/templates/configmap.yaml
🪛 YAMLlint (1.37.1)
tools/deployment/package-helm/templates/api-server-service.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
[error] 5-5: too many spaces inside braces
(braces)
[error] 5-5: too many spaces inside braces
(braces)
[error] 7-7: too many spaces inside braces
(braces)
[error] 12-12: too many spaces inside braces
(braces)
[error] 17-17: too many spaces inside braces
(braces)
[error] 17-17: too many spaces inside braces
(braces)
tools/deployment/package-helm/templates/api-server-logs-pvc.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
[error] 8-8: too many spaces inside braces
(braces)
[error] 9-9: too many spaces inside braces
(braces)
tools/deployment/package-helm/templates/api-server-logs-pv.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
[error] 10-10: too many spaces inside braces
(braces)
[error] 11-11: too many spaces inside braces
(braces)
tools/deployment/package-helm/templates/api-server-deployment.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
[error] 5-5: too many spaces inside braces
(braces)
[error] 5-5: too many spaces inside braces
(braces)
[error] 7-7: too many spaces inside braces
(braces)
[error] 13-13: too many spaces inside braces
(braces)
[error] 18-18: too many spaces inside braces
(braces)
[error] 21-21: too many spaces inside braces
(braces)
[error] 21-21: too many spaces inside braces
(braces)
[error] 24-24: too many spaces inside braces
(braces)
[error] 24-24: too many spaces inside braces
(braces)
[error] 25-25: too many spaces inside braces
(braces)
[error] 25-25: too many spaces inside braces
(braces)
[error] 26-26: too many spaces inside braces
(braces)
[error] 26-26: too many spaces inside braces
(braces)
[error] 32-32: too many spaces inside braces
(braces)
[error] 37-37: too many spaces inside braces
(braces)
[error] 46-46: too many spaces inside braces
(braces)
[error] 46-46: too many spaces inside braces
(braces)
[error] 51-51: too many spaces inside braces
(braces)
[error] 51-51: too many spaces inside braces
(braces)
[error] 61-61: too many spaces inside braces
(braces)
[error] 64-64: too many spaces inside braces
(braces)
[error] 70-70: too many spaces inside braces
(braces)
[error] 73-73: too many spaces inside braces
(braces)
[error] 82-82: too many spaces inside braces
(braces)
[error] 87-87: too many spaces inside braces
(braces)
[error] 94-94: too many spaces inside braces
(braces)
[error] 97-97: too many spaces inside braces
(braces)
[error] 97-97: too many spaces inside braces
(braces)
[error] 102-102: too many spaces inside braces
(braces)
⏰ 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: package-image
- GitHub Check: check-generated
- GitHub Check: lint-check (ubuntu-24.04)
🔇 Additional comments (7)
tools/deployment/package-helm/templates/api-server-logs-pvc.yaml (1)
1-9: LGTM! PVC configuration is appropriate.The conditional PVC creation aligns well with other API server resources. The 5Gi capacity is reasonable for log storage, and ReadWriteOnce access mode is appropriate for the single-replica deployment.
Note: The YAMLlint errors reported by static analysis are false positives—this is a Helm template using Go templating syntax, not pure YAML.
tools/deployment/package-helm/templates/configmap.yaml (1)
9-19: LGTM! API server configuration is internally consistent.The conditional rendering correctly templates the API server configuration when enabled. The hardcoded
port: 3001matches the internal container port used throughout the deployment (container port, command-line argument, health probe), maintaining consistency across all manifests.Based on learnings, the
hostfield is unused as no internal Kubernetes services need to communicate with the API server.tools/deployment/package-helm/templates/api-server-logs-pv.yaml (1)
1-11: LGTM! Local PV configuration is appropriate for development deployments.The conditional PV creation correctly uses a hostPath derived from the configured logs directory, with appropriate capacity (5Gi) matching the PVC. The
nodeRole: control-planeselector ensures proper scheduling in single-node test environments.Note: YAMLlint syntax errors are false positives (Helm template syntax).
tools/deployment/package-helm/templates/api-server-service.yaml (1)
1-18: LGTM! Service configuration correctly exposes the API server externally.The NodePort service properly maps the external port (30301 from values) to the internal container port (3001), with correct selector labels targeting the api-server component. This aligns with the architecture where the API server is accessed only from outside the cluster.
Note: YAMLlint errors are false positives (Helm template syntax).
tools/deployment/package-helm/templates/api-server-deployment.yaml (3)
27-37: LGTM! Init containers properly ensure dependencies are ready.The init containers correctly wait for both the database table creator and results cache indices creator jobs to complete before starting the API server, preventing startup failures due to missing database schema or indices.
38-88: LGTM! API server container is well-configured with proper security and observability.The container configuration follows best practices:
- Database credentials properly sourced from Kubernetes secrets
- Security context with non-root user (UID/GID from values)
- Explicit command-line arguments (including
--port 3001) for clarity- Both readiness and liveness probes configured on the
/healthendpoint- Appropriate volume mounts for logs, configuration (read-only), and shared streams
89-102: LGTM! Volume configuration properly supports API server operations.The volumes correctly provide:
- Persistent log storage via PVC
- Configuration access via ConfigMap
- Shared streams data for serving query results
There was a problem hiding this comment.
Validation:
- the following query works
curl -s -X POST http://localhost:30301/query \
-H "Content-Type: application/json" \
-d '{
"query_string": "*",
"write_to_file": true
}'
- write to file works
curl -s -X POST http://localhost:30301/query \
-H "Content-Type: application/json" \
-d '{
"query_string": "*",
"write_to_file": false
}'
- Modifying configuration in values works.
- Logs are correct.
- stdout captures correctly
- extract query results to s3 doesn't work, but I guess it's in an upcoming PR
I feel coderabbit is valid in part, but let's prioritize the release and discuss k8s config after the release.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
tools/deployment/package-helm/templates/api-server-deployment.yaml(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: junhaoliao
Repo: y-scope/clp PR: 1818
File: tools/deployment/package-helm/templates/configmap.yaml:12-12
Timestamp: 2025-12-19T05:03:32.320Z
Learning: In the y-scope/clp Helm chart, the `api_server.host` configuration field in the ConfigMap (tools/deployment/package-helm/templates/configmap.yaml) is unused because no other k8s internal services need to reach the API server—it's only accessed from outside the cluster via NodePort.
Learnt from: hoophalab
Repo: y-scope/clp PR: 1535
File: components/clp-rust-utils/src/clp_config/package/config.rs:47-61
Timestamp: 2025-11-03T16:17:40.223Z
Learning: In the y-scope/clp repository, the `ApiServer` struct in `components/clp-rust-utils/src/clp_config/package/config.rs` is a Rust-native configuration type and does not mirror any Python code, unlike other structs in the same file (Config, Database, ResultsCache, Package) which are mirrors of Python definitions.
📚 Learning: 2025-12-19T05:03:32.320Z
Learnt from: junhaoliao
Repo: y-scope/clp PR: 1818
File: tools/deployment/package-helm/templates/configmap.yaml:12-12
Timestamp: 2025-12-19T05:03:32.320Z
Learning: In the y-scope/clp Helm chart, the `api_server.host` configuration field in the ConfigMap (tools/deployment/package-helm/templates/configmap.yaml) is unused because no other k8s internal services need to reach the API server—it's only accessed from outside the cluster via NodePort.
Applied to files:
tools/deployment/package-helm/templates/api-server-deployment.yaml
📚 Learning: 2025-11-03T16:17:40.223Z
Learnt from: hoophalab
Repo: y-scope/clp PR: 1535
File: components/clp-rust-utils/src/clp_config/package/config.rs:47-61
Timestamp: 2025-11-03T16:17:40.223Z
Learning: In the y-scope/clp repository, the `ApiServer` struct in `components/clp-rust-utils/src/clp_config/package/config.rs` is a Rust-native configuration type and does not mirror any Python code, unlike other structs in the same file (Config, Database, ResultsCache, Package) which are mirrors of Python definitions.
Applied to files:
tools/deployment/package-helm/templates/api-server-deployment.yaml
🪛 YAMLlint (1.37.1)
tools/deployment/package-helm/templates/api-server-deployment.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
[error] 5-5: too many spaces inside braces
(braces)
[error] 5-5: too many spaces inside braces
(braces)
[error] 7-7: too many spaces inside braces
(braces)
[error] 13-13: too many spaces inside braces
(braces)
[error] 18-18: too many spaces inside braces
(braces)
[error] 21-21: too many spaces inside braces
(braces)
[error] 21-21: too many spaces inside braces
(braces)
[error] 24-24: too many spaces inside braces
(braces)
[error] 24-24: too many spaces inside braces
(braces)
[error] 25-25: too many spaces inside braces
(braces)
[error] 25-25: too many spaces inside braces
(braces)
[error] 26-26: too many spaces inside braces
(braces)
[error] 26-26: too many spaces inside braces
(braces)
[error] 32-32: too many spaces inside braces
(braces)
[error] 37-37: too many spaces inside braces
(braces)
[error] 46-46: too many spaces inside braces
(braces)
[error] 46-46: too many spaces inside braces
(braces)
[error] 51-51: too many spaces inside braces
(braces)
[error] 51-51: too many spaces inside braces
(braces)
[error] 61-61: too many spaces inside braces
(braces)
[error] 64-64: too many spaces inside braces
(braces)
[error] 70-70: too many spaces inside braces
(braces)
[error] 73-73: too many spaces inside braces
(braces)
[error] 82-82: too many spaces inside braces
(braces)
[error] 87-87: too many spaces inside braces
(braces)
[error] 94-94: too many spaces inside braces
(braces)
[error] 97-97: too many spaces inside braces
(braces)
[error] 97-97: too many spaces inside braces
(braces)
[error] 102-102: too many spaces inside braces
(braces)
🔇 Additional comments (5)
tools/deployment/package-helm/templates/api-server-deployment.yaml (5)
1-103: Static analysis errors are false positives for Helm templates.The YAMLlint errors flagged (syntax error on line 1, "too many spaces inside braces" throughout) are expected false positives. YAMLlint does not understand Go/Helm template syntax (
{{- ... }}), so it incorrectly flags the templating constructs as YAML syntax violations. These can be safely ignored.
1-14: LGTM!Conditional rendering and metadata structure follow Helm best practices. The selector labels properly match the pod template labels, ensuring correct pod selection.
20-37: LGTM!The init container pattern correctly ensures the API server waits for database table creation and results cache index creation before starting. The security context is properly configured with configurable UID/GID values.
60-80: LGTM!Volume mounts are well-structured: config is mounted read-only, logs directory matches
CLP_LOGS_DIR, and the streams volume enables serving stream files. The command correctly binds to0.0.0.0for container accessibility.
81-102: LGTM!Good use of YAML anchors (
&api-server-health-check/*api-server-health-check) to avoid duplicating the health check configuration between readiness and liveness probes. Volume definitions properly reference the PVC and ConfigMap resources.
| containers: | ||
| - name: "api-server" | ||
| image: "{{ include "clp.image.ref" . }}" | ||
| imagePullPolicy: "{{ .Values.image.clpPackage.pullPolicy }}" | ||
| env: | ||
| - name: "CLP_DB_PASS" | ||
| valueFrom: | ||
| secretKeyRef: | ||
| name: {{ include "clp.fullname" . }}-database | ||
| key: "password" | ||
| - name: "CLP_DB_USER" | ||
| valueFrom: | ||
| secretKeyRef: | ||
| name: {{ include "clp.fullname" . }}-database | ||
| key: "username" | ||
| - name: "CLP_LOGS_DIR" | ||
| value: "/var/log/api_server" | ||
| - name: "RUST_LOG" | ||
| value: "INFO" | ||
| ports: | ||
| - name: "api-server" | ||
| containerPort: 3001 |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding resource requests and limits.
The container lacks resources configuration. Without resource requests, the Kubernetes scheduler cannot make optimal placement decisions. Without limits, a misbehaving pod could consume excessive cluster resources.
🔎 Suggested addition after the `ports` section
ports:
- name: "api-server"
containerPort: 3001
+ resources:
+ requests:
+ cpu: "100m"
+ memory: "128Mi"
+ limits:
+ cpu: "500m"
+ memory: "512Mi"
volumeMounts:Adjust values based on expected workload or expose them via values.yaml for configurability.
📝 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.
| containers: | |
| - name: "api-server" | |
| image: "{{ include "clp.image.ref" . }}" | |
| imagePullPolicy: "{{ .Values.image.clpPackage.pullPolicy }}" | |
| env: | |
| - name: "CLP_DB_PASS" | |
| valueFrom: | |
| secretKeyRef: | |
| name: {{ include "clp.fullname" . }}-database | |
| key: "password" | |
| - name: "CLP_DB_USER" | |
| valueFrom: | |
| secretKeyRef: | |
| name: {{ include "clp.fullname" . }}-database | |
| key: "username" | |
| - name: "CLP_LOGS_DIR" | |
| value: "/var/log/api_server" | |
| - name: "RUST_LOG" | |
| value: "INFO" | |
| ports: | |
| - name: "api-server" | |
| containerPort: 3001 | |
| containers: | |
| - name: "api-server" | |
| image: "{{ include "clp.image.ref" . }}" | |
| imagePullPolicy: "{{ .Values.image.clpPackage.pullPolicy }}" | |
| env: | |
| - name: "CLP_DB_PASS" | |
| valueFrom: | |
| secretKeyRef: | |
| name: {{ include "clp.fullname" . }}-database | |
| key: "password" | |
| - name: "CLP_DB_USER" | |
| valueFrom: | |
| secretKeyRef: | |
| name: {{ include "clp.fullname" . }}-database | |
| key: "username" | |
| - name: "CLP_LOGS_DIR" | |
| value: "/var/log/api_server" | |
| - name: "RUST_LOG" | |
| value: "INFO" | |
| ports: | |
| - name: "api-server" | |
| containerPort: 3001 | |
| resources: | |
| requests: | |
| cpu: "100m" | |
| memory: "128Mi" | |
| limits: | |
| cpu: "500m" | |
| memory: "512Mi" | |
| volumeMounts: |
🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 46-46: too many spaces inside braces
(braces)
[error] 46-46: too many spaces inside braces
(braces)
[error] 51-51: too many spaces inside braces
(braces)
[error] 51-51: too many spaces inside braces
(braces)
🤖 Prompt for AI Agents
In tools/deployment/package-helm/templates/api-server-deployment.yaml around
lines 38 to 59, the api-server container has no resources block; add a resources
section (requests.cpu, requests.memory, limits.cpu, limits.memory) under the
container (e.g., immediately after the ports section) and wire each value to
configurable Helm values (e.g., .Values.resources.apiServer.requests.cpu,
.Values.resources.apiServer.requests.memory,
.Values.resources.apiServer.limits.cpu,
.Values.resources.apiServer.limits.memory) so defaults live in values.yaml and
can be tuned for workload; ensure values.yaml contains sensible defaults and
document them.
Description
Note
This PR is part of the ongoing work for #1309. More PRs will be submitted until the Helm chart is complete and fully functional.
Add the API server deployment to the CLP Helm chart. The API server provides a REST API for querying compressed logs and is a key component for programmatic access to CLP.
New Files
api-server-deployment.yaml- Deployment manifest for the API serverapi-server-logs-pv.yaml- PersistentVolume for API server logsapi-server-logs-pvc.yaml- PersistentVolumeClaim for API server logsapi-server-service.yaml- NodePort service exposing the API server on port 30301Configuration
Added
api_serverconfiguration invalues.yaml:The API server deployment is conditional - it is only created when
api_serveris set (not null).Features
db-table-creatorandresults-cache-indices-creatorjobs to complete before startingChecklist
breaking change.
Validation performed
1. Helm chart deployment
2. Log ingestion
Ingested sample logs at
/tmp/clp/samplesusing the WebUI at http://localhost:30000/ingest .3. API server logs
Observed that logs are generated at
/tmp/clp/var/log/api_server:4. API endpoint validation
Health check endpoint
Submit a search query
$ curl -s -X POST http://localhost:30301/query \ -H "Content-Type: application/json" \ -d '{ "query_string": "*", "dataset": "default", "ignore_case": false, "max_num_results": 10, "write_to_file": false }' {"query_results_uri":"/query_results/3"}Retrieve search results (SSE stream)
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.