feat(helm): Add API server deployment.#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✅ Passed checks (3 passed)
📜 Recent review detailsConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (4)📓 Common learnings📚 Learning: 2025-12-19T05:03:42.629ZApplied to files:
📚 Learning: 2025-11-03T16:17:40.223ZApplied to files:
📚 Learning: 2025-12-04T03:31:55.239ZApplied to files:
🪛 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] 99-99: too many spaces inside braces (braces) [error] 102-102: 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). (4)
🔇 Additional comments (8)
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? |
![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
📜 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
Plan: 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.
| volumeMounts: | ||
| - name: {{ include "clp.volumeName" (dict | ||
| "component_category" "api-server" | ||
| "name" "logs" | ||
| ) | quote }} | ||
| mountPath: "/var/log/api_server" | ||
| - name: "config" | ||
| mountPath: "/etc/clp-config.yaml" | ||
| subPath: "clp-config.yaml" | ||
| readOnly: true | ||
| - name: {{ include "clp.volumeName" (dict | ||
| "component_category" "shared-data" | ||
| "name" "streams" | ||
| ) | quote }} | ||
| mountPath: "/var/data/streams" |
There was a problem hiding this comment.
Thinking about this again: this volume mount probably won't work on a true cluster (not minikube/kind) right? They need to be PVs on a nfs?
And even if the user specifies s3 in archive output, the pod would probably fail to start because this mount is unconditional?
There was a problem hiding this comment.
good points. those are valid concerns and we do plan to address them:
-
streams ... need to be PVs on a nfs
If everything is running on a single node (i.e., the same host), this isn't an issue because all pods can access the same files via the local filesystem. However, once we scale query workers across multiple nodes, or place query workers on different nodes from the API server (and similarly the Web UI), each node gets its own PV backed by the same hostPath. If that hostPath is not on shared storage, files created by a pod on one node will not be accessible to pods on other nodes. I believe this is what you mean by "won't work". While we recommend S3 in a distributed compute environment, if users insist to use "fs" storage, we plan to ask users to set up distributed storage and create (FUSE) mounts at those host locations, so files can be shared. For example, SeadweedFs can be used. (I plan to submit those user guides to docs(helm): Add Kubernetes Helm user guides and dev guides; Restructure user documentation. #1827 but haven't pushed all of them. will try to get them all polished and pushed tonight. sorry)
-
even if the user specifies s3 in archive output, the pod would probably fail to start because this mount is unconditional
You're right to question this. The plan is to add the conditional template in feat(helm): Add S3 storage support for archives, streams, and log inputs. #1825 . We will review feat(helm): Add S3 storage support for archives, streams, and log inputs. #1825 once feat(helm): Add garbage collector deployment with retention period configuration. #1817 and feat(helm): Add API server deployment. #1818 are merged.
There was a problem hiding this comment.
xD It's probably almost impossible to configure seaweedfs on every node in a serious use case, but we could always ask them to use s3
There was a problem hiding this comment.
The concerns of the ability to deploy on a multi-node cluster are fairly valid. Some changes will be made in #1829 to address such concerns
junhaoliao
changed the title
junhaoliao
requested review from
kirkrodrigues
and removed request for
kirkrodrigues
| app.kubernetes.io/component: "api-server" | ||
| spec: | ||
| serviceAccountName: {{ include "clp.fullname" . }}-job-watcher | ||
| terminationGracePeriodSeconds: 10 |
There was a problem hiding this comment.
If we're going with 10s, maybe we should make the same change in Docker Compose?
There was a problem hiding this comment.
Sorry, that was a typo. Let' use 60 for consistency with Docker Compose
| - {{- include "clp.pvcVolume" (dict | ||
| "root" . | ||
| "component_category" "api-server" | ||
| "name" "logs" | ||
| ) | nindent 10 }} |
There was a problem hiding this comment.
For consistency with other helm files, can we move this below the config mount?
There was a problem hiding this comment.
i suppose you mean listing the (non-templated) config mount as the last item, for consistency with other Helm files, and i made the change accordingly
… to match docker compose
There was a problem hiding this comment.
Actionable comments posted: 3
Fix all issues with AI Agents 🤖
In @tools/deployment/package-helm/templates/api-server-deployment.yaml:
- Around line 89-102: Reorder the volume entries so the config volume comes
before the logs PVC to match chart conventions: move the block defining name:
"config" / configMap: name: {{ include "clp.fullname" . }}-config so it appears
above the clp.pvcVolume call that creates the "logs" volume (the clp.pvcVolume
invocation with component_category "api-server" and name "logs"), leaving the
shared-data "streams" PVC last.
- Around line 38-59: Add configurable resource requests/limits to the api-server
container spec: update the container with a templated resources block that reads
values from values.yaml keys like resources.apiServer.requests and
resources.apiServer.limits so the "api-server" container can receive CPU/memory
requests and limits; expose these keys in values.yaml
(resources.apiServer.requests and resources.apiServer.limits) with sensible
defaults and reference them in the template using the Helm templating helpers
(e.g., using .Values.resources.apiServer.* and include defaulting via default or
required functions) so the scheduler can make placement decisions and avoid
unbounded resource use.
- Around line 42-56: Make RUST_LOG configurable by adding an
api_server.log_level entry in values.yaml (default "INFO") and update the
deployment template to replace the hardcoded RUST_LOG value with a Helm value
lookup that uses api_server.log_level with a fallback to "INFO" when unset;
reference the RUST_LOG env var in the template change and ensure chart
documentation/README notes the new api_server.log_level option.
📜 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
🧰 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:42.629Z
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:42.629Z
Learnt from: junhaoliao
Repo: y-scope/clp PR: 1818
File: tools/deployment/package-helm/templates/configmap.yaml:12-12
Timestamp: 2025-12-19T05:03:42.629Z
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
📚 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/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)
⏰ 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). (4)
- GitHub Check: package-image
- GitHub Check: lint-check (ubuntu-24.04)
- GitHub Check: lint-check (macos-15)
- GitHub Check: check-generated
🔇 Additional comments (8)
tools/deployment/package-helm/templates/api-server-deployment.yaml (8)
1-1: LGTM: Conditional deployment gate.The deployment is properly gated, ensuring the API server resources are only created when explicitly enabled in values.
4-14: LGTM: Metadata and labels follow Helm conventions.The use of helper templates ensures consistent naming and labeling across the chart.
21-26: LGTM: Service account and security context properly configured.The job-watcher service account enables the pod to monitor job completion, and the security context is appropriately templated from values.
27-37: LGTM: Init containers ensure proper startup ordering.The API server correctly waits for database schema and cache indices to be ready before starting.
60-74: Volume mounts are correctly configured.The mounts for logs, config, and streams are properly structured with appropriate paths and options (e.g.,
readOnlyfor config).Note: As discussed in past reviews, the streams volume mount may not work in multi-node clusters without shared storage (e.g., NFS or S3). The team has acknowledged this limitation and plans to address it in issue #1829.
75-80: LGTM: Command configuration is correct.The API server command properly binds to
0.0.0.0(required for Kubernetes networking) and uses the correct container port and config path.
81-88: LGTM: Health probes are well-configured.Both readiness and liveness probes correctly target the
/healthendpoint, and the use of a YAML anchor avoids duplication. Templated timings ensure consistency across the chart.
103-103: LGTM: Conditional block properly closed.The template syntax is correct.
| 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
Resource limits and requests remain unaddressed.
The past review comment about adding resource requests and limits is still valid. Without these, the scheduler cannot make optimal placement decisions, and the pod could consume excessive cluster resources.
Consider exposing resource configuration in values.yaml (e.g., resources.apiServer.requests and resources.apiServer.limits) and templating them here for production readiness.
🧰 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-59, Add configurable resource requests/limits to the api-server
container spec: update the container with a templated resources block that reads
values from values.yaml keys like resources.apiServer.requests and
resources.apiServer.limits so the "api-server" container can receive CPU/memory
requests and limits; expose these keys in values.yaml
(resources.apiServer.requests and resources.apiServer.limits) with sensible
defaults and reference them in the template using the Helm templating helpers
(e.g., using .Values.resources.apiServer.* and include defaulting via default or
required functions) so the scheduler can make placement decisions and avoid
unbounded resource use.
| 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" |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider making RUST_LOG configurable.
The RUST_LOG environment variable is hardcoded to "INFO". For debugging or troubleshooting, operators may need to adjust the log level without modifying the chart.
🔎 Suggested enhancement
In values.yaml, add:
api_server:
log_level: "INFO"Then update the template:
- name: "RUST_LOG"
- value: "INFO"
+ value: {{ .Values.clpConfig.api_server.log_level | quote }}Committable suggestion skipped: line range outside the PR's diff.
🧰 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 42-56, Make RUST_LOG configurable by adding an api_server.log_level entry
in values.yaml (default "INFO") and update the deployment template to replace
the hardcoded RUST_LOG value with a Helm value lookup that uses
api_server.log_level with a fallback to "INFO" when unset; reference the
RUST_LOG env var in the template change and ensure chart documentation/README
notes the new api_server.log_level option.
| volumes: | ||
| - {{- include "clp.pvcVolume" (dict | ||
| "root" . | ||
| "component_category" "api-server" | ||
| "name" "logs" | ||
| ) | nindent 10 }} | ||
| - name: "config" | ||
| configMap: | ||
| name: {{ include "clp.fullname" . }}-config | ||
| - {{- include "clp.pvcVolume" (dict | ||
| "root" . | ||
| "component_category" "shared-data" | ||
| "name" "streams" | ||
| ) | nindent 10 }} |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider reordering volumes for consistency.
A past review suggested moving the logs volume definition below the config volume for consistency with other Helm files in the chart. Current order: logs, config, streams. Suggested order: config, logs, streams.
This is a minor stylistic preference and does not affect functionality.
🧰 Tools
🪛 YAMLlint (1.37.1)
[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)
🤖 Prompt for AI Agents
In @tools/deployment/package-helm/templates/api-server-deployment.yaml around
lines 89-102, Reorder the volume entries so the config volume comes before the
logs PVC to match chart conventions: move the block defining name: "config" /
configMap: name: {{ include "clp.fullname" . }}-config so it appears above the
clp.pvcVolume call that creates the "logs" volume (the clp.pvcVolume invocation
with component_category "api-server" and name "logs"), leaving the shared-data
"streams" PVC last.
… list template usages before static items
4 participants
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.