Skip to content

feat(helm): Add log-ingestor deployment. #1856

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
junhaoliao merged 5 commits into from
Jan 12, 2026
Merged

feat(helm): Add log-ingestor deployment. #1856

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
606eb6e
feat(helm): Add Log Ingestor deployment.
junhaoliao Jan 8, 2026
ce597ac
chore(helm): Bump chart version to 0.1.2-dev.18
junhaoliao Jan 8, 2026
bb5919f
feat(helm): Increase log-ingestor storage capacity to 10Gi
junhaoliao Jan 11, 2026
608368a
Change chart version to "0.1.2-dev.17"
junhaoliao Jan 12, 2026
bd692d5
Merge branch 'main' into helm-log-ingestor
junhaoliao Jan 12, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion tools/deployment/package-helm/Chart.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
apiVersion: "v2"
name: "clp"
version: "0.1.2-dev.16"
version: "0.1.2-dev.17"
description: "A Helm chart for CLP's (Compressed Log Processor) package deployment"
type: "application"
appVersion: "0.7.1-dev"
Expand Down
9 changes: 9 additions & 0 deletions tools/deployment/package-helm/templates/configmap.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,15 @@ data:
{{- end }}{{/* with .aws_authentication */}}
{{- end }}{{/* if eq .type "fs" */}}
{{- end }}{{/* with .Values.clpConfig.logs_input */}}
{{- with .Values.clpConfig.log_ingestor }}
log_ingestor:
buffer_flush_threshold: {{ .buffer_flush_threshold | int }}
buffer_flush_timeout: {{ .buffer_flush_timeout | int }}
channel_capacity: {{ .channel_capacity | int }}
host: "localhost"
logging_level: {{ .logging_level | quote }}
port: 3002
{{- end }}
package:
query_engine: {{ .Values.clpConfig.package.query_engine | quote }}
storage_engine: {{ .Values.clpConfig.package.storage_engine | quote }}
Expand Down
88 changes: 88 additions & 0 deletions tools/deployment/package-helm/templates/log-ingestor-deployment.yaml
View file
Open in desktop
Copy link
Member

@LinZhihao-723 LinZhihao-723 Jan 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cross-compared with api-server-deployment.yaml. The changes lgtm as:

  • log-ingestor only waits for DB creation; no need to wait for result cache to start.
  • log-ingestor doesn't need storage mount at this stage.
  • log-ingestor doesn't need stream storage.

Original file line number Diff line number Diff line change
@@ -0,0 +1,88 @@
{{- if .Values.clpConfig.log_ingestor }}
apiVersion: "apps/v1"
kind: "Deployment"
metadata:
name: {{ include "clp.fullname" . }}-log-ingestor
labels:
{{- include "clp.labels" . | nindent 4 }}
app.kubernetes.io/component: "log-ingestor"
spec:
replicas: 1
selector:
matchLabels:
{{- include "clp.selectorLabels" . | nindent 6 }}
app.kubernetes.io/component: "log-ingestor"
template:
metadata:
labels:
{{- include "clp.labels" . | nindent 8 }}
app.kubernetes.io/component: "log-ingestor"
spec:
serviceAccountName: {{ include "clp.fullname" . }}-job-watcher
terminationGracePeriodSeconds: 60
securityContext:
runAsUser: {{ .Values.securityContext.firstParty.uid }}
runAsGroup: {{ .Values.securityContext.firstParty.gid }}
fsGroup: {{ .Values.securityContext.firstParty.gid }}
initContainers:
- {{- include "clp.waitFor" (dict
"root" .
"type" "job"
"name" "db-table-creator"
) | nindent 10 }}
containers:
- name: "log-ingestor"
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/log_ingestor"
- name: "RUST_LOG"
value: {{ .Values.clpConfig.log_ingestor.logging_level | quote }}
ports:
- name: "log-ingestor"
containerPort: 3002
volumeMounts:
- name: {{ include "clp.volumeName" (dict
"component_category" "log-ingestor"
"name" "logs"
) | quote }}
mountPath: "/var/log/log_ingestor"
- name: "config"
mountPath: "/etc/clp-config.yaml"
subPath: "clp-config.yaml"
readOnly: true
command: [
"/opt/clp/bin/log-ingestor",
"--config", "/etc/clp-config.yaml",
"--host", "0.0.0.0",
"--port", "3002"
]
readinessProbe:
{{- include "clp.readinessProbeTimings" . | nindent 12 }}
httpGet: &log-ingestor-health-check
path: "/health"
port: "log-ingestor"
livenessProbe:
{{- include "clp.livenessProbeTimings" . | nindent 12 }}
httpGet: *log-ingestor-health-check
Comment on lines +34 to +78
Copy link
Contributor

@coderabbitai coderabbitai bot Jan 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

Consider adding resource requests and limits.

The container specification doesn't define resource requests or limits. While this works for initial deployment, consider adding them to prevent resource contention and ensure predictable performance in production environments.

📊 Example resource configuration

Add a resources section to the container specification:

           livenessProbe:
             {{- include "clp.livenessProbeTimings" . | nindent 12 }}
             httpGet: *log-ingestor-health-check
+          resources:
+            requests:
+              memory: "256Mi"
+              cpu: "100m"
+            limits:
+              memory: "512Mi"
+              cpu: "500m"
       volumes:

Adjust values based on actual resource requirements and testing.

Committable suggestion skipped: line range outside the PR's diff.

🧰 Tools
🪛 YAMLlint (1.37.1)

[error] 41-41: too many spaces inside braces

(braces)

[error] 41-41: 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] 56-56: too many spaces inside braces

(braces)

[error] 59-59: too many spaces inside braces

(braces)

[error] 72-72: too many spaces inside braces

(braces)

[error] 77-77: too many spaces inside braces

(braces)

🤖 Prompt for AI Agents 
In @tools/deployment/package-helm/templates/log-ingestor-deployment.yaml around
lines 34 - 78, The deployment for the "log-ingestor" container lacks resource
requests/limits; add a resources block under the container spec for the
container named "log-ingestor" that defines appropriate requests and limits (cpu
and memory) using Values or sensible defaults (e.g.,
.Values.image.clpPackage.resources or .Values.clpConfig.log_ingestor.resources)
so the manifest includes a resources: { requests: { cpu, memory }, limits: {
cpu, memory } } entry; ensure you update the template references around the
container definition (the block with image: "{{ include "clp.image.ref" . }}"
and command: [ "/opt/clp/bin/log-ingestor", ... ]) and prefer exposing them via
Helm values for easy tuning in values.yaml.

volumes:
- {{- include "clp.pvcVolume" (dict
"root" .
"component_category" "log-ingestor"
"name" "logs"
) | nindent 10 }}
- name: "config"
configMap:
name: {{ include "clp.fullname" . }}-config
{{- end }}
11 changes: 11 additions & 0 deletions tools/deployment/package-helm/templates/log-ingestor-logs-pv.yaml
View file
Open in desktop
Copy link
Member

@LinZhihao-723 LinZhihao-723 Jan 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cross-compared with api-server-logs-pv.yaml. The changes lgtm.
My only concern is the 5Gi capacity might be too small. Current log ingestor generates a log even whenever a new object is ingested, with the full S3 key and metadata tracked. Assuming each log event is 150-byte long on average, this log capacity would only allow 35k files ingested. What's the behavior if the log size exceeds the capacity?

Copy link
Member Author

@junhaoliao junhaoliao Jan 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the behavior if the log size exceeds the capacity?

if i'm not wrong, logs are dropped silently.

Let's raise it to 10Gi to match the compression scheduler log volume. In the long run, we should implement #1760 instead of requesting host volumes specifically for logs.

Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
{{- if .Values.clpConfig.log_ingestor }}
{{- include "clp.createLocalPv" (dict
"root" .
"component_category" "log-ingestor"
"name" "logs"
"nodeRole" "control-plane"
"capacity" "10Gi"
"accessModes" (list "ReadWriteOnce")
"hostPath" (printf "%s/log_ingestor" .Values.clpConfig.logs_directory)
) }}
{{- end }}
9 changes: 9 additions & 0 deletions tools/deployment/package-helm/templates/log-ingestor-logs-pvc.yaml
View file
Open in desktop
Copy link
Member

@LinZhihao-723 LinZhihao-723 Jan 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cross-compared with api-server-logs-pvc.yaml and the changes lgtm.

Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
{{- if .Values.clpConfig.log_ingestor }}
{{- include "clp.createPvc" (dict
"root" .
"component_category" "log-ingestor"
"name" "logs"
"capacity" "10Gi"
"accessModes" (list "ReadWriteOnce")
) }}
{{- end }}
18 changes: 18 additions & 0 deletions tools/deployment/package-helm/templates/log-ingestor-service.yaml
View file
Open in desktop
Copy link
Member

@LinZhihao-723 LinZhihao-723 Jan 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cross-compared with api-server-srervice.yaml and the changes lgtm.

Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
{{- if .Values.clpConfig.log_ingestor }}
apiVersion: "v1"
kind: "Service"
metadata:
name: {{ include "clp.fullname" . }}-log-ingestor
labels:
{{- include "clp.labels" . | nindent 4 }}
app.kubernetes.io/component: "log-ingestor"
spec:
type: "NodePort"
selector:
{{- include "clp.selectorLabels" . | nindent 4 }}
app.kubernetes.io/component: "log-ingestor"
ports:
- port: 3002
targetPort: "log-ingestor"
nodePort: {{ .Values.clpConfig.log_ingestor.port }}
{{- end }}
15 changes: 9 additions & 6 deletions tools/deployment/package-helm/test.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ mkdir -p "$CLP_HOME/var/"{data,log}/{database,queue,redis,results_cache} \
"$CLP_HOME/var/data/"{archives,streams,staged-archives,staged-streams} \
"$CLP_HOME/var/log/"{compression_scheduler,compression_worker,user} \
"$CLP_HOME/var/log/"{query_scheduler,query_worker,reducer} \
"$CLP_HOME/var/log/"{api_server,garbage_collector,mcp_server} \
"$CLP_HOME/var/log/"{api_server,garbage_collector,log_ingestor,mcp_server} \
"$CLP_HOME/var/tmp" \
"$CLP_HOME/samples"

Expand All @@ -85,18 +85,21 @@ cat <<EOF | kind create cluster --name clp-test --config=-
- hostPath: $CLP_HOME
containerPath: $CLP_HOME
extraPortMappings:
- containerPort: 30306
hostPort: 30306
- containerPort: 30000
hostPort: 30000
protocol: TCP
- containerPort: 30017
hostPort: 30017
protocol: TCP
- containerPort: 30000
hostPort: 30000
protocol: TCP
- containerPort: 30301
hostPort: 30301
protocol: TCP
- containerPort: 30302
hostPort: 30302
protocol: TCP
- containerPort: 30306
hostPort: 30306
protocol: TCP
- containerPort: 30800
hostPort: 30800
protocol: TCP
Expand Down
13 changes: 13 additions & 0 deletions tools/deployment/package-helm/values.yaml
View file
Open in desktop
Copy link
Member

@LinZhihao-723 LinZhihao-723 Jan 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cross-compared with the current package config and this lgtm.

Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,19 @@ clpConfig:
# port: 30800
# logging_level: "INFO"

# log-ingestor config. Currently, the config is applicable only if `logs_input.type` is "s3".
log_ingestor: null
Copy link
Member Author

@junhaoliao junhaoliao Jan 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should explicitly set log_ingestor: null when it is so in values.yaml; otherwise, the Pydantic model parser would fallback to default values which leaves the log-ingestor configured. When clpConfig.package.storage is configured as "clp" (CLP-Text which doesn't support the log-ingestor), the config is deemed invalid and most Helm chart deployments won't start.

@coderabbitai create an issue to track

Copy link
Contributor

@coderabbitai coderabbitai bot Jan 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Create a GitHub issue to track the log_ingestor configuration problem

gh issue create \
  --title "Fix log_ingestor configuration fallback to defaults when set to null in Helm values.yaml" \
  --body "## Problem

When \`log_ingestor: null\` is set in \`values.yaml\`, the Pydantic model parser falls back to default values, which leaves the log-ingestor configured even though it should be disabled.

## Impact

When \`clpConfig.package.storage\` is configured as \`\"clp\"\` (CLP-Text, which doesn't support the log-ingestor), the configuration is deemed invalid and most Helm chart deployments won't start.

## Solution

The configuration parser should explicitly handle \`null\` values for \`log_ingestor\` to prevent fallback to default values, ensuring the feature remains disabled when set to null.

## References

- PR: https://github.com/y-scope/clp/pull/1856
- Comment: https://github.com/y-scope/clp/pull/1856#discussion_r2703116768
- Reported by: `@junhaoliao`

## Files Affected

- \`tools/deployment/package-helm/values.yaml\`
- Configuration parser/validator (likely Python Pydantic models)" \
  --assignee junhaoliao

Length of output: 42

@junhaoliao, I've created issue #1881 to track the log_ingestor configuration problem: #1881

The issue describes how the Pydantic parser falls back to default values when log_ingestor: null, causing deployment failures with CLP-Text storage engine. It's been assigned to you for tracking.

🧠 Learnings used 
Learnt from: junhaoliao
Repo: y-scope/clp PR: 1152
File: components/clp-package-utils/clp_package_utils/scripts/start_clp.py:613-613
Timestamp: 2025-08-08T06:59:42.436Z
Learning: In components/clp-package-utils/clp_package_utils/scripts/start_clp.py, generic_start_scheduler sets CLP_LOGGING_LEVEL using clp_config.query_scheduler.logging_level for both schedulers; compression scheduler should use its own logging level. Tracking via an issue created from PR `#1152` discussion.

Copy link
Member Author

@junhaoliao junhaoliao Jan 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#1882 sets log_ingestor: null in the clp_config.yaml ConfigMap to address #1881. However, the Rust utils cannot handle null values for this key and will fail with a parsing error.

As a result, if the API server is enabled while log_ingestor is disabled by setting log_ingestor: null, the API server will fail to start.

More importantly, this configuration ("enable the API server but disable log_ingestor by setting log_ingestor: null") is currently the default behavior in our Helm values.yaml. This means the v0.8.0 chart deploys an API server that continuously crashes and restarts.

I think we need to address two issues:

  • Update the Rust utils to correctly handle a nullable log_ingestor.
  • Enable log_ingestor by default in Helm values.yaml, so it aligns with the default clp_config.yaml (and clp_config.template.json.yaml).

@coderabbitai create those new two issues as GH issues

Copy link
Contributor

@coderabbitai coderabbitai bot Jan 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash

# Create first issue: Update Rust utils to handle nullable log_ingestor
gh issue create \
  --title "Update Rust utils to handle nullable log_ingestor configuration" \
  --body "## Problem

The Rust utils cannot handle null values for the \`log_ingestor\` key in \`clp_config.yaml\` and fail with a parsing error.

## Impact

When the API server is enabled while \`log_ingestor\` is disabled by setting \`log_ingestor: null\`, the API server fails to start. This is particularly problematic because the current default configuration in Helm \`values.yaml\` has \`log_ingestor: null\`, causing the v0.8.0 chart to deploy an API server that continuously crashes and restarts.

## Solution

Update the Rust configuration parser to correctly handle nullable \`log_ingestor\` values, treating \`null\` as an indication that the log-ingestor feature should be disabled.

## References

- PR: https://github.com/y-scope/clp/pull/1856
- PR: https://github.com/y-scope/clp/pull/1882
- Related issue: `#1881`
- Comment: https://github.com/y-scope/clp/pull/1856#discussion_r2703116768
- Reported by: `@junhaoliao`

## Files Affected

- Rust configuration parser (likely in components utilizing clp_config.yaml)" \
  --assignee junhaoliao

# Create second issue: Enable log_ingestor by default in Helm values.yaml
gh issue create \
  --title "Enable log_ingestor by default in Helm values.yaml" \
  --body "## Problem

The current default configuration in Helm \`values.yaml\` has \`log_ingestor: null\`, which disables the log-ingestor. This is inconsistent with the default \`clp_config.yaml\` and \`clp_config.template.json.yaml\` configurations, and causes the API server to crash and restart continuously in v0.8.0 due to parsing issues.

## Impact

The v0.8.0 Helm chart deploys an API server with the default configuration that continuously crashes and restarts, resulting in a broken deployment out of the box.

## Solution

Enable \`log_ingestor\` by default in Helm \`values.yaml\` with appropriate default values to align with the default \`clp_config.yaml\` and \`clp_config.template.json.yaml\` configurations.

## References

- PR: https://github.com/y-scope/clp/pull/1856
- PR: https://github.com/y-scope/clp/pull/1882
- Related issue: `#1881`
- Comment: https://github.com/y-scope/clp/pull/1856#discussion_r2703116768
- Reported by: `@junhaoliao`

## Files Affected

- \`tools/deployment/package-helm/values.yaml\`" \
  --assignee junhaoliao

echo "Both issues created successfully"

Length of output: 118

@junhaoliao, I've created the two GitHub issues to track these problems:

  1. Issue #1911: Update Rust utils to handle nullable log_ingestor configuration
    Update Rust utils to handle nullable log_ingestor configuration #1911

  2. Issue #1912: Enable log_ingestor by default in Helm values.yaml
    Enable log_ingestor by default in Helm values.yaml #1912

Both issues have been assigned to you and include references to PR #1856, PR #1882, and issue #1881.

🐰✍️

✏️ Learnings added 
Learnt from: junhaoliao
Repo: y-scope/clp PR: 1856
File: tools/deployment/package-helm/values.yaml:94-94
Timestamp: 2026-01-26T22:19:03.543Z
Learning: In the Helm chart's `values.yaml`, the default configuration has `log_ingestor: null`, which causes the v0.8.0 chart to deploy an API server that continuously crashes and restarts due to Rust parsing issues.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

🧠 Learnings used 
Learnt from: junhaoliao
Repo: y-scope/clp PR: 1152
File: components/clp-package-utils/clp_package_utils/scripts/start_clp.py:613-613
Timestamp: 2025-08-08T06:59:42.436Z
Learning: In components/clp-package-utils/clp_package_utils/scripts/start_clp.py, generic_start_scheduler sets CLP_LOGGING_LEVEL using clp_config.query_scheduler.logging_level for both schedulers; compression scheduler should use its own logging level. Tracking via an issue created from PR `#1152` discussion.

# port: 30302
# # The timeout (in seconds) after which the log buffer is flushed for compression if no new
# # input arrives.
# buffer_flush_timeout: 300
# # The log buffer size (in bytes) that triggers a flush for compression.
# buffer_flush_threshold: 268435456 # 256 MiB
# # The capacity of the internal channel used for communication between an ingestion job and the
# # log buffer.
# channel_capacity: 10
# logging_level: "INFO"

Comment on lines +93 to +105
Copy link
Contributor

@coderabbitai coderabbitai bot Jan 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Clarify port requirement and provide validation.

The service template (log-ingestor-service.yaml:17) requires .Values.clpConfig.log_ingestor.port to be set, but the commented example in values.yaml suggests it's optional. When users enable log_ingestor without specifying the port field, the deployment will fail.

Consider one of the following approaches:

  1. Make the port field mandatory in documentation and add a validation in the service template
  2. Provide a default port value if not specified
  3. Update the example to show port as a required field (uncomment it in the example)
🛠️ Suggested validation in service template

Add to the beginning of log-ingestor-service.yaml:

 {{- if .Values.clpConfig.log_ingestor }}
+{{- if not .Values.clpConfig.log_ingestor.port }}
+{{- fail "clpConfig.log_ingestor.port is required when log_ingestor is enabled" }}
+{{- end }}
 apiVersion: "v1"

Or provide a default value in the service template:

-      nodePort: {{ .Values.clpConfig.log_ingestor.port }}
+      nodePort: {{ .Values.clpConfig.log_ingestor.port | default 30302 }}

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents 
In @tools/deployment/package-helm/values.yaml around lines 93 - 105, The
values.yaml example makes log_ingestor.port look optional but the service
template (log-ingestor-service.yaml) requires
.Values.clpConfig.log_ingestor.port; fix this by either (a) providing a default
port in values.yaml (uncomment and set port: 30302) and documenting it, and also
update log-ingestor-service.yaml to use a default fallback for
.Values.clpConfig.log_ingestor.port if missing, or (b) enforce validation by
adding a required check in log-ingestor-service.yaml that errors when
.Values.clpConfig.log_ingestor is set but .port is nil; reference the values key
log_ingestor and the template reference .Values.clpConfig.log_ingestor.port when
applying the change.

# Where archives should be output to
archive_output:
storage:
Expand Down
Loading