Skip to content

workspace-mail: mail-backup can write (source untouched), rejects empty backups, + never-succeeded CronJob alert - #1120

Open
mdheller wants to merge 4 commits into
feat/storage-retentionfrom
fix/mail-backup-fsgroup-staleness
Open

workspace-mail: mail-backup can write (source untouched), rejects empty backups, + never-succeeded CronJob alert#1120
mdheller wants to merge 4 commits into
feat/storage-retentionfrom
fix/mail-backup-fsgroup-staleness

Conversation

@mdheller

Copy link
Copy Markdown
Member

What this makes durable

Three fixes a cluster lane verified live for the mail-backup CronJob. ArgoCD runs
selfHeal: true, so the live kubectl patches get reverted — this lands them in git.

Stacked on #1114 (feat/storage-retention), not main. #1114 rewrites this exact
command block (retention + tar tzf verify + receipt). Rebuilding the fix beside it on
main would produce a third divergent backup-cronjob.yaml. Stacking makes the change
strictly additive on #1114 instead (see Overlap). Do not merge before #1114.

1. Backup can't write — fixed WITHOUT touching the mail source

mail-backup runs as uid 1000; workspace-mail-backup is a fresh PD (root:root 0755),
so the container gets EACCES creating the tarball.

The obvious fix — a pod-level fsGroup: 1000 — is wrong here. fsGroup applies to
every mounted volume, including the read-only mail source, and rewrites its root
group to 1000 (this is what dirtied /var/mail/vhosts to root:1000 2775 during live
verification). fsGroupChangePolicy: OnRootMismatch does not save you: it re-applies
on the next run after the ownership is restored, so it would fight the parallel lane
restoring /var/mail/vhosts and re-dirty the source every night.

Instead: an init-backup-perms initContainer (root, CAP_CHOWN only) chowns
only the destination mount. The mail source is never mounted into it, so the source
volume's ownership is provably untouched by this job.
Verified in the rendered manifest:
the initContainer's only volumeMount is backup-target.

Also corrected the image comment (from dc7344cd): its "produces a real, listable
.tar.gz" claim could not have run against the real backup PVC (it EACCESes on a fresh
PD). Reworded to "pull probe only", not deleted.

2. Skip lost+found, and reject an empty backup as a FAILURE

  • tar -C /maildata --exclude=./lost+found . — skips ext4's root:root 0700 lost+found,
    so the job never needs group access to the mount root just to traverse it. The read side
    no longer leans on volume ownership; only the destination needs the chown to write.
  • Non-triviality gate (MIN_BACKUP_BYTES, default 1024): the maildir is empty, so the
    verified artifact was 130 bytes — a readable .tar.gz of nothing that storage: three backup jobs that delete nothing, two of which never ran #1114's
    tar tzf verify passes happily. The gate makes a trivially small archive a failure
    (rm it, receipt status=failed, exit 1), closing the "check that cannot fail" here too.

3. PrometheusRule for CronJobs that never succeed

infra/k8s/observability/base/prometheusrule-cronjob-staleness.yaml. Nothing watched for a
CronJob that stopped succeeding, and the canonical rule is silently wrong:
time() - kube_cronjob_status_last_successful_time > N returns zero series for a job
that never succeeded (the metric has no series for it) — so the broken-since-creation jobs
(mail-backup, minio-sync) are exactly the ones it can't see. The shipped expression adds
the unless arm that emits a marker for every scheduled-but-never-succeeded job, anded
with spec_suspend == 0.

Routing — reaches the null sink until #1112 (stated plainly)

The alert keeps its metric namespace (namespace=socioprophet for mail-backup). #1112's
alert-sink AlertmanagerConfig is operator-scoped OnNamespace and matches
namespace=observability only; estate-wide delivery for socioprophet/ depends on
#1112's Alertmanager default-route change (deploy/argocd/observability-services.yaml),
still null until it syncs. So this rule is live in Prometheus immediately but delivered
to no human until #1112 lands.
It is intentionally not relabelled into observability
(that would misattribute a socioprophet CronJob just to borrow #1112's sub-route).
Referencing #1112's routing, not forking it — I did not touch any alert-delivery/** file.

Overlap (merge-tree vs the live lanes)

My 4-file delta intersects the live lanes on exactly one file:

Lane Overlap
#1112 alert-delivery none
#1113 rule-liveness-guard none
#1114 storage-retention infra/k8s/workspace-mail/base/backup-cronjob.yaml only

Resolved by stacking: git diff feat/storage-retention..HEAD is additive (the only
in-place edits are the corrected comment and adding --exclude to #1114's tar line;
its retention/verify/receipt are untouched). Proof:

Proof (local only — Actions are spend-capped, so no CI run here)

  • kubectl kustomize renders clean for all five affected kustomizations
    (workspace-mail base + p0-lab + p1-single-site; observability base + p0-lab).
  • Staleness expr, inverted vs correct — faithful PromQL set-semantics model over
    representative kube-state-metrics series: naive form returns only the once-succeeded stale
    job (blind to mail-backup/minio-sync); shipped form returns mail-backup and
    minio-sync, suppressing suspended + healthy jobs. Corroborates the live-Prometheus lane
    confirmation.
  • Size gateinfra/k8s/workspace-mail/tests/backup-nontrivial.test.sh runs the
    shipped script (extracted from kubectl kustomize, mount paths localized) against
    synthesized fixtures: empty maildir → 215-byte archive → exit 1, archive removed,
    receipt status=failed; populated maildir → 6755-byte archive → exit 0, retained,
    lost+found excluded, mail present. 9/9.

Deviations / notes

Do not merge.

mdheller added 4 commits July 30, 2026 03:25
…sGroup

The mail-backup job runs as uid 1000 but the workspace-mail-backup PVC is a
freshly provisioned PD (root:root 0755), so the backup container gets EACCES
creating the tarball. Fix the write permission with an initContainer that
chowns ONLY the destination mount to 1000:1000 (CAP_CHOWN, root for that one
step).

Deliberately NOT a pod-level fsGroup: fsGroup applies to every mounted volume,
so it would also rewrite the read-only mail SOURCE volume's root group to 1000
-- and OnRootMismatch re-applies after a restore, fighting the lane restoring
/var/mail/vhosts. The initContainer never mounts the source, so the source
ownership is provably untouched.

Also correct the image comment: the pre-change probe verified the PULL only;
its 'produces a real, listable .tar.gz' claim was against a writable scratch
dir, not the real backup PVC (which EACCESes on a fresh PD).
The maildir is currently empty, so the verified backup was 130 bytes -- a
readable, listable .tar.gz of nothing. #1114's tar-tzf verify passes on it,
so a green run proves nothing. Add a byte-precise non-triviality gate
(MIN_BACKUP_BYTES, default 1024): below the floor the job fails and removes
the trivial archive instead of receipting it as a success.

Also archive with 'tar -C /maildata --exclude=./lost+found .' so the job
skips ext4's root:root 0700 lost+found. The write no longer needs group
access to the mount root to READ the source; only the destination needs the
initContainer chown to WRITE.
Nothing watched for a CronJob that stopped succeeding. The canonical
'time() - kube_cronjob_status_last_successful_time > N' is silent for a job
that NEVER succeeded: the metric has no series for it, so the never-run jobs
(mail-backup, minio-sync -- both failing nightly, unalerted) are exactly the
ones it cannot see. Add cronjob-staleness with the second 'unless' arm that
emits a marker for every scheduled-but-never-succeeded job, intersected with
spec_suspend==0.

Delivery: the alert keeps its metric namespace (socioprophet for mail-backup).
#1112's alert-sink AlertmanagerConfig is OnNamespace=observability only, so
until #1112's default-route change syncs this fires into the null sink. Noted
in-file; not relabelled into observability (that would misattribute the job).
…ses real

Runs the SHIPPED script (extracted from kubectl kustomize, mount paths
localized) against a synthesized empty maildir and a populated one. Proves an
empty maildir tars to a sub-1KB archive that fails the gate (exit 1, archive
removed, receipt status=failed) while a real maildir passes (exit 0, retained),
and that lost+found is excluded. Local-only; Actions are spend-capped.
Copilot AI review requested due to automatic review settings July 30, 2026 07:37

Copilot AI left a comment

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.

Pull request overview

Note

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Makes the mail-backup CronJob durable in GitOps by ensuring it can write to its backup PVC without mutating the mail source volume, failing on trivially small “empty” backups, and adding an alert for CronJobs that stop succeeding (including those that never succeeded).

Changes:

  • Add an initContainer that chowns only the backup destination mount so the unprivileged backup container can write without touching source volume ownership.
  • Update backup creation to exclude lost+found and add a minimum-bytes gate that fails and deletes trivially small archives.
  • Add a PrometheusRule to alert on CronJobs that haven’t succeeded recently or have never succeeded, plus a local test script validating the size gate.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.

File Description
infra/k8s/workspace-mail/base/backup-cronjob.yaml Adds initContainer permissions fix and non-trivial backup gate; tweaks tar invocation and comments.
infra/k8s/workspace-mail/tests/backup-nontrivial.test.sh Adds a local-only regression test that renders the manifest and executes the shipped script against fixtures.
infra/k8s/observability/base/prometheusrule-cronjob-staleness.yaml Adds a CronJob staleness alert that handles “never succeeded” CronJobs.
infra/k8s/observability/base/kustomization.yaml Wires the new PrometheusRule into the observability base.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

# (/maildata, /backup) to temp dirs so it can run unprivileged off-cluster; the
# tar invocation, the tar-tzf verify and the wc -c size gate are the real ones.
#
# Local-only (Actions are spend-capped). Requires: kubectl, python3, tar.
Comment on lines +34 to +35
python3 - "$WORK/rendered.yaml" "$WORK/script.sh" "$WORK/env.sh" <<'PY'
import sys, yaml
Comment on lines +41 to +43
with open(env_out, "w") as f:
for e in c["env"]:
f.write('export %s=%s\n' % (e["name"], e.get("value", "")))
Comment on lines +93 to +95
# workspace-mail-backup. A real archive is produced only after the
# init-backup-perms container below chowns the destination, and the size gate in
# the command rejects the trivially small (empty-maildir) case.
@mdheller

Copy link
Copy Markdown
Member Author

Merge-gate disposition (Copilot 3-channel + adversarial pass)

Size gate verified: step 5b sets RC=1 and rms the archive when wc -c < "${OUT}" < MIN_BACKUP_BYTES=1024 (an empty maildir tars to ~130 bytes → fails). The regression test runs the rendered manifest's actual script against empty/populated fixtures (empty → job fails + archive removed; populated → passes, lost+found excluded). The never-succeeded alert correctly uses kube_cronjob_status_last_schedule_time * 0 unless kube_cronjob_status_last_successful_time to see jobs the naive time() - last_successful_time is blind to.

Acknowledged, non-blocking: the test imports yaml (PyYAML) but the requirements comment says python3-only; env.sh values are emitted without shlex.quote; "init-backup-perms below" wording (it's above).

Verdict: MERGE-READY, but stack-gated — base is feat/storage-retention (#1114); do not merge to main before #1114, then retarget. Alert delivery depends on #1112's default-route landing (honestly disclosed in the manifest).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants