[mashkovd]

Problem

Failed Argo workflows strand their per-workflow workdir PVCs (and the backing 10Gi Hetzner volumes) permanently. Argo's default volumeClaimGC.strategy: OnWorkflowSuccess retains the volumeClaimTemplate PVC when a workflow fails; ttlStrategy then GCs the Workflow object, leaving the PVC with no owner → it lingers forever. Several failed mctl-agents issue-poll runs left 10Gi orphans (cleaned manually this week).

Fix — two layers

B) Cause fix (controller default). platform-gitops/bootstrap/templates/core-infra/argo-workflows.yaml — add to controller.workflowDefaults.spec:

volumeClaimGC:
  strategy: OnWorkflowCompletion

Deletes the workdir PVC on completion regardless of success/failure. Inherited by every workflow that doesn't override it (none do). The valuable output (proposals) is pushed to git before failure and logs go to Loki, so losing the scratch clone on failure is fine.

C) Backstop janitor. platform-gitops/argo-workflows/config/pvc-janitor.yaml — SA + Role + RoleBinding + hourly CronJob. Deletes a *-workdir PVC only when both: no pod mounts it and its owning Workflow is gone or terminal (Succeeded/Failed/Error). Running/pending (incl. just-submitted, empty-phase) workflows are kept, so it never races a live workflow. Catches strays B might miss (force-deleted/TTL-GC'd workflows) and pre-existing orphans.

Safety notes

• Janitor is namespace-scoped to argo-workflows and only targets *-workdir PVCs — can't touch minio/vault/pg/VM volumes.
• Uses alpine/k8s:1.33.12, not the retired bitnami/kubectl (which silently wedges on ImagePullBackOff — same trap that broke vault-auto-unseal).
• concurrencyPolicy: Forbid + ttlSecondsAfterFinished so janitor jobs don't pile up.

Verify after merge

• kubectl get cm -n argo-workflows workflow-controller-configmap -o yaml | grep -A1 volumeClaimGC → OnWorkflowCompletion
• Next failed workflow's *-workdir PVC is deleted on completion (no orphan).
• kubectl get cronjob -n argo-workflows argo-pvc-janitor exists; first run logs keep/DELETE decisions and completes.

[claude]

Claude finished @mashkovd's task in 1m 59s —— View job

---

Code Review: Auto-GC Orphaned Workflow Workdir PVCs

Review Checklist:

• Gather context and read changed files
• Verify volumeClaimGC configuration correctness
• Analyze pvc-janitor YAML and shell script logic
• Check RBAC permissions and security
• Verify deployment mechanism
• Check for Argo Workflows specifics
• Post findings and summary

---

Detailed Analysis

1. volumeClaimGC Configuration (argo-workflows.yaml)
✅ Correctly placed in controller.workflowDefaults.spec
✅ Strategy OnWorkflowCompletion matches PR spec
✅ Clear comment explaining the problem (default OnWorkflowSuccess leaves failed workflows' PVCs orphaned)
✅ Will inherit to all workflows automatically

2. PVC Janitor Manifest (pvc-janitor.yaml)

RBAC & Security:
✅ ServiceAccount properly scoped to argo-workflows namespace
✅ Role permissions are minimal: only get, list, delete on PVCs; get, list on Pods/Workflows
✅ RoleBinding correctly wired
✅ Image: alpine/k8s:1.33.12 — explicitly called out in PR description as reliable (not retired bitnami/kubectl)
✅ No secrets or sensitive data in YAML

Operational Safety:
✅ concurrencyPolicy: Forbid — prevents concurrent janitor runs
✅ backoffLimit: 1 — minimal retry
✅ activeDeadlineSeconds: 300 — 5-min timeout (reasonable for namespace scan)
✅ ttlSecondsAfterFinished: 600 — cleanup job records after 10min
✅ Resource limits: 200m CPU, 128Mi memory — appropriate
✅ History limits: 1 success, 3 failures (good for debugging)

Janitor Logic:
✅ Builds list of PVCs mounted by active pods upfront (safe snapshot)
✅ Targets only *-workdir PVCs (namespace-scoped, won't touch minio/vault/pg volumes)
✅ Two-condition delete: (1) not in use by pod AND (2) workflow terminal or gone
✅ Preserves running/pending/empty-phase workflows (safe against race conditions)
✅ Uses --wait=false for non-blocking deletes
✅ Error handling with || true appropriate

Shell Script Edge Cases:
✅ jsonpath handles pods with multiple volumes (sort -u dedupes)
✅ Empty phase treated as "keep" (safe for just-submitted workflows)
✅ Workflow name extract (pvc%-workdir) is correct
✅ Case statement wildcard catches unexpected phases safely

3. Deployment Mechanism
✅ Confirmed: argo-workflows-config Application (in core-infra/argo-workflows-config.yaml) watches entire platform-gitops/argo-workflows directory with recurse: true
✅ pvc-janitor.yaml will be auto-deployed to argo-workflows namespace

---

Summary

Verdict: No P1/P2 findings. Good to merge.

Both layers of the fix are well-designed and operationally safe:

• Controller default will auto-GC workdir PVCs on workflow completion (handles future failures)
• Janitor CronJob provides defence-in-depth, catching pre-existing orphans and edge cases
• RBAC is minimal and properly scoped; concurrency controls prevent job pile-up
• Image choice avoids the retired bitnami/kubectl trap
• Janitor logic safely prevents races against active workflows

Post-merge verification (from PR description) is straightforward: confirm configmap setting, run a failed workflow, verify janitor CronJob exists and logs keep/DELETE decisions.