Skip to content

feat(helm): Add garbage collector deployment with retention period configuration. #1817

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 11 commits into from
Dec 20, 2025
Merged

feat(helm): Add garbage collector deployment with retention period configuration. #1817

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
8cbc0ad
feat(helm): Add garbage collector deployment with retention period co…
junhaoliao Dec 19, 2025
268ab46
add the missing charts
junhaoliao Dec 19, 2025
55ab307
Merge branch 'main' into helm-gc
jackluo923 Dec 19, 2025
abb0ea2
chore(helm): Bump chart version to 0.1.2-dev.11
junhaoliao Dec 19, 2025
36542c1
test: Generate sample log file for garbage collector testing
junhaoliao Dec 19, 2025
c3f1f31
add missing archives_output.rentention_period config; Ensure retentio…
junhaoliao Dec 19, 2025
19c3b25
Add temporary volume mount for garbage collector temp file
junhaoliao Dec 19, 2025
f16b822
adjust directory creation for garbage_collector logs
junhaoliao Dec 19, 2025
1c830e7
Merge branch 'main' into helm-gc
junhaoliao Dec 19, 2025
6f2f9b2
fix(helm): Ensure retention_period has a default value in configmap t…
junhaoliao Dec 19, 2025
5fcb562
Merge branch 'main' into helm-gc
junhaoliao Dec 19, 2025
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.10"
version: "0.1.2-dev.11"
description: "A Helm chart for CLP's (Compressed Log Processor) package deployment"
type: "application"
appVersion: "0.7.1-dev"
Expand Down
15 changes: 15 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 @@ -11,6 +11,11 @@ data:
storage:
directory: "/var/data/archives"
type: "fs"
{{- if .Values.clpConfig.archive_output.retention_period }}
retention_period: {{ .Values.clpConfig.archive_output.retention_period | int }}
Comment thread
coderabbitai[bot] marked this conversation as resolved.
{{- else }}
retention_period: null
{{- end }}
target_archive_size: {{ .Values.clpConfig.archive_output.target_archive_size | int }}
target_dictionaries_size: {{ .Values.clpConfig.archive_output.target_dictionaries_size
| int }}
Expand All @@ -26,6 +31,11 @@ data:
compression_worker:
logging_level: {{ .Values.clpConfig.compression_worker.logging_level | quote }}
data_directory: "/var/data"
garbage_collector:
logging_level: {{ .Values.clpConfig.garbage_collector.logging_level | quote }}
sweep_interval:
archive: {{ .Values.clpConfig.garbage_collector.sweep_interval.archive | int }}
search_result: {{ .Values.clpConfig.garbage_collector.sweep_interval.search_result | int }}
Comment on lines +34 to +38

@coderabbitai coderabbitai Bot Dec 19, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Search for sweep_interval in the codebase
echo "=== Searching for sweep_interval in Python files ==="
find . -type f -name "*.py" | xargs grep -n "sweep_interval" 2>/dev/null | head -30

echo -e "\n=== Searching for sweep_interval in YAML files ==="
find . -type f -name "values.yaml" -o -name "*.yaml" | xargs grep -A 3 -B 3 "sweep_interval" 2>/dev/null | head -50

echo -e "\n=== Checking configmap.yaml for context around sweep_interval ==="
find . -path "*/templates/configmap.yaml" | xargs cat 2>/dev/null | head -100

echo -e "\n=== Searching for retention_period to see comparison pattern ==="
find . -type f \( -name "*.yaml" -o -name "*.yml" \) | xargs grep -A 3 -B 3 "retention_period" 2>/dev/null | head -50

Repository: y-scope/clp

Length of output: 12720

Consider adding conditional rendering for sweep_interval consistency.

The sweep_interval fields currently lack the conditional pattern used for retention_period (lines 14–18, 81–85), which allows null values. While defaults exist in values.yaml (archive: 60, search_result: 30), explicitly setting either value to null would convert to 0 via the | int filter, causing the garbage collector to run continuously without delay.

If sweeping should be disableable via null (like retention_period), apply the same conditional rendering pattern. Otherwise, consider documenting that these values require numeric configuration and cannot be disabled.

database:
auto_commit: false
compress: true
Expand Down Expand Up @@ -68,6 +78,11 @@ data:
db_name: {{ .Values.clpConfig.results_cache.db_name | quote }}
host: {{ include "clp.fullname" . }}-results-cache
port: 27017
{{- if .Values.clpConfig.results_cache.retention_period }}
retention_period: {{ .Values.clpConfig.results_cache.retention_period | int }}
Comment thread
coderabbitai[bot] marked this conversation as resolved.
{{- else }}
retention_period: null
{{- end }}
stream_collection_name: {{ .Values.clpConfig.results_cache.stream_collection_name | quote }}
stream_output:
storage:
Expand Down
109 changes: 109 additions & 0 deletions tools/deployment/package-helm/templates/garbage-collector-deployment.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,109 @@
{{- if or .Values.clpConfig.archive_output.retention_period .Values.clpConfig.results_cache.retention_period }}
apiVersion: "apps/v1"
kind: "Deployment"
metadata:
name: {{ include "clp.fullname" . }}-garbage-collector
labels:
{{- include "clp.labels" . | nindent 4 }}
app.kubernetes.io/component: "garbage-collector"
spec:
replicas: 1
selector:
matchLabels:
{{- include "clp.selectorLabels" . | nindent 6 }}
app.kubernetes.io/component: "garbage-collector"
template:
metadata:
labels:
{{- include "clp.labels" . | nindent 8 }}
app.kubernetes.io/component: "garbage-collector"
spec:
serviceAccountName: {{ include "clp.fullname" . }}-job-watcher
terminationGracePeriodSeconds: 10
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 }}
- {{- include "clp.waitFor" (dict
"root" .
"type" "job"
"name" "results-cache-indices-creator"
) | nindent 10 }}
containers:
- name: "garbage-collector"
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_HOME"
value: "/opt/clp"
- name: "CLP_LOGGING_LEVEL"
value: {{ .Values.clpConfig.garbage_collector.logging_level | quote }}
- name: "CLP_LOGS_DIR"
value: "/var/log/garbage_collector"
- name: "PYTHONPATH"
value: "/opt/clp/lib/python3/site-packages"
volumeMounts:
- name: "config"
mountPath: "/etc/clp-config.yaml"
subPath: "clp-config.yaml"
readOnly: true
- name: "tmp"
mountPath: "/var/log"
Comment on lines +66 to +67

@junhaoliao junhaoliao Dec 19, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

the archive_garbage_collector.py, called by garbage collector, writes to

clp/components/job-orchestration/job_orchestration/garbage_collector/archive_garbage_collector.py

Line 205 in c7c2ffc

recovery_file = clp_config.logs_directory / f"{ARCHIVE_GARBAGE_COLLECTOR_NAME}.tmp"

which:

  1. Doesn't respect the

    clp/components/job-orchestration/job_orchestration/garbage_collector/garbage_collector.py

    Line 39 in c7c2ffc

    logs_directory = Path(os.getenv("CLP_LOGS_DIR"))
    ; otherwise, it would write to /var/log/garbage_collector/archive-garbage-collector.tmp and we don't add to add this volume
  2. Has violated our latest convention of writing temporary files to temporary directories
  3. Doesn't really need to be mapped onto the host.

@coderabbitai create an issue to track the request of letting archive_garbage_collector.py write to some other directory than /var/log. The docker compose orchestration should also be updated according to that

@coderabbitai coderabbitai Bot Dec 19, 2025

Copy link
Copy Markdown
Contributor

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
# Examine the archive_garbage_collector.py file to understand the issue
cat components/job-orchestration/job_orchestration/garbage_collector/archive_garbage_collector.py | head -220 | tail -20

# Also check the garbage_collector.py to see the log directory configuration
cat components/job-orchestration/job_orchestration/garbage_collector/garbage_collector.py | head -50

Length of output: 2576

🏁 Script executed:

#!/bin/bash
# Check for docker-compose files related to garbage collector
fd -t f "docker-compose" tools/deployment/package/ -x echo "File: {}" \; -x grep -l "garbage.collector" {} \;

# Search for garbage collector volume mounts in docker-compose files
rg -n "garbage.collector" -A 5 -B 2 tools/deployment/package/ --type yaml

Length of output: 6361

🏁 Script executed:

#!/bin/bash
# Create a GitHub issue to track this request
gh issue create \
  --title "Move archive_garbage_collector.py temporary file writing from /var/log to temporary directory" \
  --body "## Background

In PR #1817, we identified that \`archive_garbage_collector.py\` writes temporary files to an inappropriate location.

**Related PR**: #1817
**Related Comment**: https://github.com/y-scope/clp/pull/1817#discussion_r2634344014

## Problems

1. **Doesn't respect log directory configuration**: The \`archive_garbage_collector.py\` writes the recovery file using \`clp_config.logs_directory\` instead of the \`CLP_LOGS_DIR\` environment variable that is used by the logger. This means it would write to a different location than expected (e.g., would write to the directory specified in clp-config.yaml rather than \`/var/log/garbage_collector\`).

2. **Violates convention**: Writing temporary files to \`/var/log\` violates our latest convention of writing temporary files to temporary directories.

3. **Unnecessary host mapping**: The temporary recovery file doesn't need to be mapped onto the host filesystem.

## Affected Code

- [\`archive_garbage_collector.py:205\`](https://github.com/y-scope/clp/blob/c7c2ffcc13473e386223fb3a1b4e1d5d45267f7b/components/job-orchestration/job_orchestration/garbage_collector/archive_garbage_collector.py#L205): Creates recovery file using \`clp_config.logs_directory\`
- [\`garbage_collector.py:39\`](https://github.com/y-scope/clp/blob/c7c2ffcc13473e386223fb3a1b4e1d5d45267f7b/components/job-orchestration/job_orchestration/garbage_collector/garbage_collector.py#L39): Sets up logging using \`CLP_LOGS_DIR\` environment variable

## Required Changes

1. Update \`archive_garbage_collector.py\` to write the recovery file to a proper temporary directory instead of \`/var/log\`.
2. Update Docker Compose orchestration to reflect this change.
3. Update Helm chart orchestration if needed.

## Suggested Solution

Consider using a dedicated temporary directory (e.g., \`/tmp/clp/garbage_collector/\`) or passing a separate temporary directory configuration to the garbage collector.

---
Reported by: @junhaoliao" \
  --assignee junhaoliao \
  --repo y-scope/clp

Length of output: 42

@junhaoliao I've created issue #1823 to track this request: #1823

The issue documents all three problems you identified:

  1. Not respecting the logger's directory configuration (using clp_config.logs_directory instead of CLP_LOGS_DIR)
  2. Violating the convention of writing temporary files to temporary directories
  3. Unnecessarily mapping the recovery file onto the host

The issue includes backlinks to this PR and comment, references the affected code locations, and outlines the required changes for both the Python code and Docker Compose orchestration. I've assigned it to you for tracking.

🧠 Learnings used 
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.

Learnt from: LinZhihao-723
Repo: y-scope/clp PR: 1340
File: components/job-orchestration/job_orchestration/executor/compress/compression_task.py:528-528
Timestamp: 2025-09-28T15:00:22.170Z
Learning: In components/job-orchestration/job_orchestration/executor/compress/compression_task.py, there is a suggestion to refactor from passing logger as a parameter through multiple functions to creating a ClpCompressor class that takes the logger as a class member, with current helper functions becoming private member functions.

Learnt from: junhaoliao
Repo: y-scope/clp PR: 1178
File: components/clp-package-utils/clp_package_utils/controller.py:315-315
Timestamp: 2025-10-17T19:59:25.596Z
Learning: In components/clp-package-utils/clp_package_utils/controller.py, worker log directories (compression_worker, query_worker, reducer) created via `mkdir()` do not need `_chown_paths_if_root()` calls because directories are created with the same owner as the script caller. This differs from infrastructure service directories (database, queue, Redis, results cache) which do require explicit ownership changes.

- name: {{ include "clp.volumeName" (dict
"component_category" "garbage-collector"
"name" "logs"
) | quote }}
mountPath: "/var/log/garbage_collector"
- name: {{ include "clp.volumeName" (dict
"component_category" "shared-data"
"name" "archives"
) | quote }}
mountPath: "/var/data/archives"
- name: {{ include "clp.volumeName" (dict
"component_category" "shared-data"
"name" "streams"
) | quote }}
mountPath: "/var/data/streams"
command: [
"python3", "-u",
"-m", "job_orchestration.garbage_collector.garbage_collector",
"--config", "/etc/clp-config.yaml"
]
volumes:
- {{- include "clp.pvcVolume" (dict
"root" .
"component_category" "garbage-collector"
"name" "logs"
) | nindent 10 }}
- {{- include "clp.pvcVolume" (dict
"root" .
"component_category" "shared-data"
"name" "archives"
) | nindent 10 }}
- {{- include "clp.pvcVolume" (dict
"root" .
"component_category" "shared-data"
"name" "streams"
) | nindent 10 }}
- name: "config"
configMap:
name: {{ include "clp.fullname" . }}-config
- name: "tmp"
emptyDir: {}
{{- end }}
11 changes: 11 additions & 0 deletions tools/deployment/package-helm/templates/garbage-collector-logs-pv.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
{{- if or .Values.clpConfig.archive_output.retention_period .Values.clpConfig.results_cache.retention_period }}
{{- include "clp.createLocalPv" (dict
"root" .
"component_category" "garbage-collector"
"name" "logs"
"nodeRole" "control-plane"
"capacity" "5Gi"
"accessModes" (list "ReadWriteOnce")
"hostPath" (printf "%s/garbage_collector" .Values.clpConfig.logs_directory)
) }}
{{- end }}
9 changes: 9 additions & 0 deletions tools/deployment/package-helm/templates/garbage-collector-logs-pvc.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
{{- if or .Values.clpConfig.archive_output.retention_period .Values.clpConfig.results_cache.retention_period }}
{{- include "clp.createPvc" (dict
"root" .
"component_category" "garbage-collector"
"name" "logs"
"capacity" "5Gi"
"accessModes" (list "ReadWriteOnce")
) }}
{{- end }}
7 changes: 7 additions & 0 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,6 +59,7 @@ mkdir -p "$CLP_HOME/var/"{data,log}/{database,queue,redis,results_cache} \
"$CLP_HOME/var/data/"{archives,streams} \
"$CLP_HOME/var/log/"{compression_scheduler,compression_worker,user} \
"$CLP_HOME/var/log/"{query_scheduler,query_worker,reducer} \
"$CLP_HOME/var/log/garbage_collector" \
"$CLP_HOME/var/tmp" \
"$CLP_HOME/samples"

Expand All @@ -67,6 +68,12 @@ wget -O - https://zenodo.org/records/10516402/files/postgresql.tar.gz?download=1
| tar xz -C "$CLP_HOME/samples" &
SAMPLE_DOWNLOAD_PID=$!

# Generate sample log file for garbage collector testing.
cat <<EOF > /tmp/clp/samples/test-gc.jsonl
{"timestamp": $(date +%s%3N), "level": "INFO", "message": "User login successful"}
{"timestamp": $(date +%s%3N), "level": "ERROR", "message": "Database connection failed"}
EOF

cat <<EOF | kind create cluster --name clp-test --config=-
kind: Cluster
apiVersion: kind.x-k8s.io/v1alpha4
Expand Down
11 changes: 11 additions & 0 deletions tools/deployment/package-helm/values.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,9 @@ clpConfig:
db_name: "clp-query-results"
stream_collection_name: "stream-files"

# Retention period for search results, in minutes. Set to null to disable automatic deletion.
retention_period: 60

compression_worker:
logging_level: "INFO"

Expand Down Expand Up @@ -113,6 +116,14 @@ clpConfig:
# How large each stream file should be before being split into a new stream file
target_uncompressed_size: 134217728 # 128 MB

garbage_collector:
logging_level: "INFO"

# Interval (in minutes) at which garbage collector jobs run
sweep_interval:
archive: 60
search_result: 30

# Location where other data (besides archives) are stored. It will be created if
# it doesn't exist.
# NOTE: This directory must not overlap with any path used in CLP's execution container. An error
Expand Down
Loading