fix(job-orchestration): Move garbage collector recovery file to temporary directory (fixes #1823). by junhaoliao · Pull Request #1834 · y-scope/clp

junhaoliao · 2025-12-26T12:16:35Z

Description

This PR moves the garbage collector recovery file from the logs directory to the temporary directory, addressing the issues raised in #1823.

The archive_garbage_collector.py was writing its recovery file to clp_config.logs_directory, which:

Violated the convention of writing temporary files to temporary directories
Caused issues because the garbage-collector service didn't have a writable logs directory mounted

This PR:

Changed the recovery file location from clp_config.logs_directory to clp_config.tmp_directory
Added the *volume_clp_tmp mount to the garbage-collector service in Docker Compose
Fixed the Helm chart garbage-collector deployment to mount the tmp volume at /var/tmp

Checklist

The PR satisfies the contribution guidelines.
This is a breaking change and that has been indicated in the PR title, OR this isn't a
breaking change.
Necessary docs have been updated, OR no docs need to be updated.

Validation performed

Verified that the recovery file path now uses clp_config.tmp_directory
Confirmed that the garbage-collector service has the tmp volume * mounted in Docker Compose
Verified the Helm chart mounts the tmp volume at the correct path /var/tmp

Docker Compose deployment

task && cd build/clp-package && ./sbin/start-clp.sh

2026-01-14T23:21:12.117 INFO [controller] Started CLP.

All services started successfully including garbage-collector with the tmp volume mounted.

Helm chart deployments

Single-node setup

cd tools/deployment/package-helm && ./set-up-test.sh

pod/test-clp-garbage-collector-6bd59567d7-8b54g condition met
...
All jobs completed and services are ready.

Multi-node setup with shared worker nodes

cd tools/deployment/package-helm && ./set-up-multi-shared-test.sh

pod/test-clp-garbage-collector-6bd59567d7-875ws condition met
...
All jobs completed and services are ready.

Multi-node setup with dedicated worker nodes

cd tools/deployment/package-helm && ./set-up-multi-dedicated-test.sh

pod/test-clp-garbage-collector-6bd59567d7-fccqm condition met
...
All jobs completed and services are ready.

Summary by CodeRabbit

Chores
- Garbage collector now stores recovery/temporary files in a dedicated temporary directory (mount path changed to /var/tmp) instead of the logs directory.
- Deployment manifests and compose setup updated to include a shared temporary bind mount for the garbage collector.
- Helm chart version incremented to 0.3.2-dev.4.

To see the specific tasks where the Asana app for GitHub is being used, see below:
- https://app.asana.com/0/0/1212888460154406
- https://app.asana.com/0/0/1212888430394537

…rary directory (fixes y-scope#1823).

… Compose.

coderabbitai · 2025-12-26T12:16:41Z

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

@coderabbitai resume to resume automatic reviews.
@coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

▶️ Resume reviews
🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 73816a9c-51ea-4208-832d-f1f3b3a3f504

📥 Commits

Reviewing files that changed from the base of the PR and between a6bcbb8 and 4e469ca.

📒 Files selected for processing (2)

tools/deployment/package-helm/Chart.yaml
tools/deployment/package/docker-compose-all.yaml

Walkthrough

The archive garbage collector’s recovery file path is changed from the configured logs directory to the configured temporary directory. Deployment artifacts (Helm template and Docker Compose) are updated so the garbage-collector container mounts the shared tmp volume at the new path; chart version is bumped.

Changes

Garbage Collector: tmp recovery file + deployment mounts

Layer / File(s)	Summary
Core Logic `components/job-orchestration/job_orchestration/garbage_collector/archive_garbage_collector.py`	Recovery file is created in `clp_config.tmp_directory` instead of `clp_config.logs_directory`, filename unchanged (`{ARCHIVE_GARBAGE_COLLECTOR_NAME}.tmp`).
Helm Deployment Wiring `tools/deployment/package-helm/templates/garbage-collector-deployment.yaml`	`tmp` volume mountPath in the garbage-collector container changed from `/var/log` to `/var/tmp`.
Compose Orchestration Wiring `tools/deployment/package/docker-compose-all.yaml`	`garbage-collector` service volumes updated to include the shared temporary bind mount (`*volume_clp_tmp`) in addition to existing mounts.
Chart Metadata `tools/deployment/package-helm/Chart.yaml`	Helm chart `version` incremented from `0.3.2-dev.3` to `0.3.2-dev.4`.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately describes the main change: moving the garbage collector recovery file from the logs directory to the temporary directory, which is the primary modification across all affected files.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

# Conflicts: # tools/deployment/package-helm/Chart.yaml # tools/deployment/package-helm/templates/garbage-collector-deployment.yaml

junhaoliao · 2026-01-14T22:32:40Z

Since "using tmpfs instead of binding mount" is a different requirement from changing the GC file list directory to use clpConfig.tmp_directory, i believe that change should be submitted in a separate PR.

hoophalab

LGTM.

It looks that the garbage collector could leave orphaned archives in storage if the pod crashes and k8s recycles its emptyDir volume. Not saying we should fix this right now -- we can just ask coderabbit to create an issue

junhaoliao · 2026-05-07T19:21:08Z

LGTM.

It looks that the garbage collector could leave orphaned archives in storage if the pod crashes and k8s recycles its emptyDir volume. Not saying we should fix this right now -- we can just ask coderabbit to create an issue

@coderabbitai create an issue to track

coderabbitai · 2026-05-07T19:21:41Z

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
gh issue create \
  --repo y-scope/clp \
  --title "fix(job-orchestration): Handle garbage collector recovery file loss when k8s emptyDir volume is recycled" \
  --body "## Problem

When the garbage-collector pod crashes and Kubernetes recycles its \`emptyDir\` volume (mounted at \`/var/tmp\`), the recovery file written by \`archive_garbage_collector.py\` is lost. Without this recovery file, the garbage collector loses track of archives that were already scheduled for deletion, potentially leaving orphaned archives in storage that are never cleaned up.

This issue was identified during the review of `#1834`, which moved the recovery file from \`clp_config.logs_directory\` to \`clp_config.tmp_directory\`.

## Steps to Reproduce

1. Deploy CLP on Kubernetes.
2. Start a garbage collection job.
3. Crash the garbage-collector pod mid-run.
4. Allow Kubernetes to recycle the \`emptyDir\` volume.
5. Observe that the recovery file is gone; previously scheduled archives may not be cleaned up.

## Expected Behaviour

The garbage collector should be resilient to pod restarts and volume recycling — orphaned archives should still be identified and cleaned up correctly.

## Possible Solutions

- Persist the recovery file to a durable volume (e.g., a PersistentVolumeClaim) rather than an \`emptyDir\` volume.
- Redesign the garbage collection logic to be idempotent and not rely on a recovery file that may be lost across restarts.

## References

- PR: https://github.com/y-scope/clp/pull/1834
- Comment: https://github.com/y-scope/clp/pull/1834#issuecomment-2575063900

Raised by `@junhaoliao`." \
  --assignee junhaoliao

Length of output: 43

@junhaoliao: Done! I've created issue #2260 — "fix(job-orchestration): Handle garbage collector recovery file loss when k8s emptyDir volume is recycled" — to track this. It's assigned to you and includes a description of the problem, steps to reproduce, and possible solutions (persisting to a durable volume or redesigning GC logic to be idempotent).

…rary directory (fixes y-scope#1823). (y-scope#1834) Co-authored-by: hoophalab <200652805+hoophalab@users.noreply.github.com>

junhaoliao added 2 commits December 26, 2025 04:32

fix(job-orchestration): Move garbage collector recovery file to tempo…

e65a8b4

…rary directory (fixes y-scope#1823).

fix(clp-package)!: Use tmpfs for temporary directory mounts in Docker…

958a655

… Compose.

junhaoliao mentioned this pull request Dec 26, 2025

WIP - feat(deployment): Add k8s Helm chart for CLP package deployment. #1603

Closed

16 tasks

chore(helm): bump chart version to 0.1.2-dev.18

ce02789

junhaoliao commented Jan 7, 2026

View reviewed changes

Comment thread tools/deployment/package-helm/Chart.yaml Outdated

junhaoliao and others added 2 commits January 7, 2026 15:22

bump chart version

8e68200

Merge branch 'main' into docker-compose-tmp-tmpfs

8dd3902

# Conflicts: # tools/deployment/package-helm/Chart.yaml # tools/deployment/package-helm/templates/garbage-collector-deployment.yaml

revert tmpfs change

2c0aa5f

junhaoliao changed the title ~~fix(clp-package)!: Use tmpfs for temporary directory mounts in Docker Compose.~~ fix(job-orchestration): Move garbage collector recovery file to temporary directory (fixes #1823). Jan 14, 2026

junhaoliao requested a review from kirkrodrigues January 14, 2026 23:31

junhaoliao marked this pull request as ready for review January 14, 2026 23:31

junhaoliao requested a review from a team as a code owner January 14, 2026 23:31

Merge branch 'main' into docker-compose-tmp-tmpfs

ba3cf5d

junhaoliao modified the milestones: Backlog, February 2026 Jan 19, 2026

Merge branch 'main' into docker-compose-tmp-tmpfs

df52e4f

junhaoliao modified the milestones: February 2026, March 2026 Mar 3, 2026

junhaoliao modified the milestones: March 2026, Mid-April 2026 Apr 1, 2026

junhaoliao modified the milestones: Mid-April 2026, Mid-May 2026 Apr 24, 2026

Merge branch 'main' into docker-compose-tmp-tmpfs

511276e

junhaoliao mentioned this pull request May 4, 2026

WIP - change /var/tmp dir to tmpfs #1833

Closed

3 tasks

junhaoliao requested review from hoophalab and removed request for kirkrodrigues May 4, 2026 23:23

hoophalab previously approved these changes May 7, 2026

View reviewed changes

Merge branch 'main' into docker-compose-tmp-tmpfs

a6bcbb8

Merge branch 'main' into docker-compose-tmp-tmpfs

975d601

coderabbitai Bot mentioned this pull request May 7, 2026

fix(job-orchestration): Handle garbage collector recovery file loss when k8s emptyDir volume is recycled #2260

Open

chore(helm): Bump chart version to 0.3.2-dev.4.

4e469ca

junhaoliao dismissed hoophalab’s stale review via 4e469ca May 7, 2026 19:22

junhaoliao requested a review from hoophalab May 7, 2026 19:22

hoophalab approved these changes May 7, 2026

View reviewed changes

junhaoliao merged commit aeb077c into y-scope:main May 7, 2026
26 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(job-orchestration): Move garbage collector recovery file to temporary directory (fixes #1823).#1834

fix(job-orchestration): Move garbage collector recovery file to temporary directory (fixes #1823).#1834
junhaoliao merged 12 commits into
y-scope:mainfrom
junhaoliao:docker-compose-tmp-tmpfs

junhaoliao commented Dec 26, 2025 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented Dec 26, 2025 •

edited

Loading

Reviews paused

❌ Failed checks (1 warning)

Uh oh!

Uh oh!

junhaoliao commented Jan 14, 2026

Uh oh!

hoophalab left a comment

Uh oh!

junhaoliao commented May 7, 2026

Uh oh!

coderabbitai Bot commented May 7, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

junhaoliao commented Dec 26, 2025 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

Validation performed

Docker Compose deployment

Helm chart deployments

Single-node setup

Multi-node setup with shared worker nodes

Multi-node setup with dedicated worker nodes

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Dec 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews paused

Walkthrough

Changes

Estimated code review effort

❌ Failed checks (1 warning)

Uh oh!

Uh oh!

junhaoliao commented Jan 14, 2026

Uh oh!

hoophalab left a comment

Choose a reason for hiding this comment

Uh oh!

junhaoliao commented May 7, 2026

Uh oh!

coderabbitai Bot commented May 7, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

junhaoliao commented Dec 26, 2025 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Dec 26, 2025 •

edited

Loading