feat(docker): backup OpenSPP filestore (#66)

[Tarekchehahde]

Summary

• Extend docker/backup.sh to tar the Odoo filestore (/odoo_data/filestore/<db>) after each database dump.
• Mount odoo_data read-only on the backup service in production compose files.
• Apply the same retention policy to *_filestore_*.tar.gz archives.

Test plan

• Run backup container with odoo_data mounted; confirm *_filestore_*.tar.gz appears in /backups/daily
• Confirm backup still works when filestore path is absent (logs skip message)

Fixes #66

Made with Cursor

[gemini-code-assist]

Code Review

This pull request introduces automated filestore backups for Odoo by mounting the odoo_data volume to the backup service and updating the backup script to archive attachments daily, weekly, and monthly, alongside cleaning up expired archives. The reviewer pointed out a critical issue where running the tar command directly under set -e could cause the entire backup script to abort if files are modified during archiving. They suggested wrapping the tar command in an if statement to handle failures gracefully and clean up partial archives.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

[gemini-code-assist]

Running tar directly under set -e can cause the entire backup script to abort if any files are modified or deleted while the archiving process is running (which is common for active filestores). If tar exits with a non-zero status, the script will terminate immediately, preventing subsequent steps like weekly/monthly copies and old backup cleanup from executing.

Wrapping the tar command in an if statement safely handles potential non-zero exit codes without triggering set -e. Additionally, if the archiving fails, we should clean up any partial/corrupt tarball to prevent it from being treated as a valid backup or copied to weekly/monthly directories.

Suggested change

	tar -czf "${DAILY_DIR}/${FILESTORE_BACKUP_FILE}" -C "$(dirname "${FILESTORE_SRC}")" "$(basename "${FILESTORE_SRC}")"
	ln -sf "${FILESTORE_BACKUP_FILE}" "${DAILY_DIR}/${PGDATABASE:-openspp}_filestore_latest.tar.gz"
	echo "[$(date -Iseconds)] Filestore backup complete: ${FILESTORE_BACKUP_FILE}"
	if tar -czf "${DAILY_DIR}/${FILESTORE_BACKUP_FILE}" -C "$(dirname "${FILESTORE_SRC}")" "$(basename "${FILESTORE_SRC}")"; then
	ln -sf "${FILESTORE_BACKUP_FILE}" "${DAILY_DIR}/${PGDATABASE:-openspp}_filestore_latest.tar.gz"
	echo "[$(date -Iseconds)] Filestore backup complete: ${FILESTORE_BACKUP_FILE}"
	else
	echo "[$(date -Iseconds)] Error: Filestore backup failed. Cleaning up partial archive..." >&2
	rm -f "${DAILY_DIR}/${FILESTORE_BACKUP_FILE}"
	fi

[kneckinator]

Thanks for picking this up, and apologies it sat unattended for so long — that's a maintainer failure, not yours.

The change is the right shape: small, correctly ordered, and it closes a real gap. I've left seven inline comments; two are blocking (the tar/set -e interaction, and retention/opt-in), and neither is hard to fix. I verified the failure modes inside the actual backup image rather than reasoning from the diff, so the numbers quoted inline are measured, not estimated.

Smaller notes

1. Trailing slash in FILESTORE_SRC silently changes scope. basename /odoo_data/filestore/ is filestore, so an override with a trailing slash would quietly archive every database's filestore. One-line guard: FILESTORE_SRC="${FILESTORE_SRC%/}".

2. Please comment why the dump comes before the tar. It's the safer of the two orders and someone will eventually "tidy" it. An attachment committed between the dump and the archive leaves a harmless orphan file that Odoo's GC reaps; flip the order and any upload during that window produces a DB row whose file was never captured — the exact FileNotFoundError from #66.

3. The pair still isn't atomic, and the docs should say so. A file unlinked and GC'd inside the dump-to-tar window is referenced by the dump but absent from the archive. It's a much narrower hole than the reverse order — it needs both events inside the same window — but the README should describe the pair as crash-consistent rather than point-in-time. A genuinely consistent pair needs a volume snapshot or a brief Odoo stop.

4. Data-protection line for the docs. backup_data now holds beneficiary documents in the clear, not just a DB dump. Same class of exposure as the dump, so not a new risk, but given what OpenSPP stores it's worth a sentence about encryption at rest.

5. Considered and dismissed: I checked whether odoo_data:/odoo_data:ro,z triggers a costly recursive SELinux relabel on a large filestore. The volume is already mounted rw,z by odoo, so the shared label is set and runtimes skip the walk. Not a problem.

Things I checked that are correct as written: TIMESTAMP is computed once, so the dump and archive filenames pair up; _filestore_latest.tar.gz survives retention because the find predicates use -type f; the missing-directory skip path works, including before Odoo has ever started; and the backup container can read the filestore under cap_drop: [ALL].

On the discussion in #66

For the record, since it shaped whether this PR should exist at all: the spp_storage_backend alternative doesn't address the issue, and @atelal is right. I checked rather than taking it on assertion — spp_storage_backend defines a standalone spp.storage.backend model with store()/retrieve()/delete() for module code to call, and does not inherit or override ir.attachment anywhere. The only _inherit = "ir.attachment" in the repo is in spp_attachment_av_scan, for scanning. So pointing a deployment at S3 changes nothing about where Odoo's own attachments land: data_dir = /var/lib/odoo still receives every standard-flow attachment, web asset and report PDF.

The follow-up question about regenerable files was fair for the specific traceback posted — that one is the websocket worker bundle, a rebuildable asset — but @atelal's answer settles it: rebuilding means stopping Odoo and hand-deleting ir_attachment rows in SQL, i.e. an outage either way, and uploaded beneficiary documents cannot be regenerated at all.

Two process notes so this doesn't repeat:

• @atelal said twice that they would open a PR for this and got no reply, and this PR then arrived from someone else three months later. @atelal — would you review this? You have the operational experience with filestore restores, particularly for the restore procedure in comment 3.
• The unresolved product question is worth settling explicitly on #66: nightly full-filestore tars genuinely do not scale, which is why I'm asking for opt-in-by-default here. The docs should state which path an operator is choosing — this is the small/mid on-prem option, and large deployments want external object storage plus volume snapshots.

[kneckinator]

Blocking — a tar failure here aborts everything below it.

The bot flagged this and it holds up under test. I ran the two race cases inside the exact backup image (postgis/postgis:18-3.6-alpine, which ships tar (busybox) 1.37.0):

case	busybox tar exit
file grows while being read	0 (tolerated)
files unlinked during traversal	1 — tar: error exit delayed from previous errors

The unlink case is precisely what Odoo does on a schedule: ir.autovacuum runs _gc_file_store daily and unlinks checklisted files. When that overlaps the 2am backup, this line exits 1 and set -e kills the script before:

• the Sunday weekly DB copy (line 71) — no weekly backup that week
• the 1st-of-month monthly DB copy (line 80) — no monthly backup that month
• all six retention find … -delete passes (lines 90-99) — backups accumulate until backup_data fills, at which point pg_dump starts failing too

It also fails quietly: crond only writes /var/log/backup.log inside the container, and the partial *_filestore_*.tar.gz is left in daily/ looking like a valid archive. (The _latest symlink correctly isn't updated — that part is right.)

A command used as an if condition is exempt from set -e, so this shape fixes it, and writing to .part first means a partial archive is never mistaken for a good one:

if [ "${BACKUP_FILESTORE}" != "true" ]; then
    echo "[$(date -Iseconds)] Filestore backup disabled (BACKUP_FILESTORE=${BACKUP_FILESTORE})"
elif [ ! -d "${FILESTORE_SRC}" ]; then
    echo "[$(date -Iseconds)] Filestore not found at ${FILESTORE_SRC}; skipping filestore backup"
else
    echo "[$(date -Iseconds)] Starting filestore backup from ${FILESTORE_SRC}..."
    # tar exits 1 when Odoo's filestore GC unlinks a file mid-archive. Running it
    # as an `if` condition keeps `set -e` from skipping the retention pass below.
    if tar -czf "${DAILY_DIR}/${FILESTORE_BACKUP_FILE}.part" \
           -C "$(dirname "${FILESTORE_SRC}")" "$(basename "${FILESTORE_SRC}")"; then
        mv "${DAILY_DIR}/${FILESTORE_BACKUP_FILE}.part" "${DAILY_DIR}/${FILESTORE_BACKUP_FILE}"
        ln -sf "${FILESTORE_BACKUP_FILE}" "${DAILY_DIR}/${PGDATABASE:-openspp}_filestore_latest.tar.gz"
        echo "[$(date -Iseconds)] Filestore backup complete: ${FILESTORE_BACKUP_FILE}"
    else
        rm -f "${DAILY_DIR}/${FILESTORE_BACKUP_FILE}.part"
        echo "[$(date -Iseconds)] WARNING: filestore backup failed; database dump kept"
    fi
fi

(BACKUP_FILESTORE is covered in the retention comment below.)

[kneckinator]

Blocking — filestore archives need their own retention knobs, and the feature needs to be opt-in.

These three lines reuse the DB-dump retention policy verbatim for full filestore copies. With -mtime +7 daily, +28 weekly and +180 monthly, that keeps up to ~19 complete copies of the filestore in backup_data — no dedup, no incrementals, no size cap. A 100 GB filestore turns into ~1.9 TB of backup volume. And because the odoo_data mount is added unconditionally, every production deployment inherits that whether it wants it or not.

Two changes, please:

1. Make it opt-in, defaulting off. Add alongside the existing defaults at the top of the script (lines 21-25):

BACKUP_FILESTORE="${BACKUP_FILESTORE:-false}"

Leave the compose mount unconditional — it's harmless when the toggle is off — and gate the work on the variable (see the tar comment above for the block shape).

2. Give the filestore its own retention knobs, defaulting to the DB values. Declared after the BACKUP_KEEP_* lines so the chained defaults resolve:

BACKUP_FILESTORE_KEEP_DAYS="${BACKUP_FILESTORE_KEEP_DAYS:-${BACKUP_KEEP_DAYS}}"
BACKUP_FILESTORE_KEEP_WEEKS="${BACKUP_FILESTORE_KEEP_WEEKS:-${BACKUP_KEEP_WEEKS}}"
BACKUP_FILESTORE_KEEP_MONTHS="${BACKUP_FILESTORE_KEEP_MONTHS:-${BACKUP_KEEP_MONTHS}}"

then use them here:

find "${DAILY_DIR}"   -name "*_filestore_*.tar.gz" -type f -mtime +${BACKUP_FILESTORE_KEEP_DAYS} -delete 2>/dev/null || true
find "${WEEKLY_DIR}"  -name "*_filestore_*.tar.gz" -type f -mtime +$((BACKUP_FILESTORE_KEEP_WEEKS * 7)) -delete 2>/dev/null || true
find "${MONTHLY_DIR}" -name "*_filestore_*.tar.gz" -type f -mtime +$((BACKUP_FILESTORE_KEEP_MONTHS * 30)) -delete 2>/dev/null || true

Out of the box this behaves identically to what you have — the defaults align with the DB knobs — but an operator can now age filestore copies out far faster than dumps without touching their dump policy.

One detail: keep these three find calls outside the BACKUP_FILESTORE guard. Turning the toggle back off should still let existing archives age out rather than stranding them on the volume forever.

[kneckinator]

Restore procedure is missing, and this is the part issue #66 actually turns on.

A DB/filestore mismatch is the whole bug in #66, so an archive nobody knows how to restore doesn't close it. The "To restore a backup" section right below (line 222) still only covers pg_restore. It needs the filestore counterpart: untar into the odoo_data volume, with the ownership Odoo expects, and paired with the dump carrying the same timestamp — the two filenames share TIMESTAMP, which is a nice property worth spelling out for operators.

Two more things while you're in this section:

• Once BACKUP_FILESTORE defaults to false, this bullet's "When the odoo_data volume is mounted on the backup service" is no longer the trigger. Reword to name the toggle, and add a sizing warning — BACKUP_FILESTORE_KEEP_* × filestore size is the number that bites people.
• Pre-existing, but you're editing the section: the restore snippet says openspp-YYYYMMDD-HHMMSS.sql.gz, while the script actually writes openspp_YYYYMMDD_HHMMSS.dump (underscores, pg_dump -Fc custom format). Worth correcting in passing.

[kneckinator]

gzip is close to pure cost on a filestore.

Measured in the backup image at the nginx stack's cpus: "0.5" limit, on 500 MB of incompressible data:

gzip tar took 19s -> 476.9M
plain tar took  0s -> 476.8M

An OpenSPP filestore is dominated by scanned documents and photos, which are already compressed — so that's roughly 38 s/GB of pinned CPU for a rounding error. A 100 GB filestore is about an hour of CPU-bound work every night, on a service capped at half a core.

Real filestores do hold some compressible content (web asset bundles, XML, text attachments), so the saving isn't literally zero, but the ratio is bad enough to be worth a knob. Either default to plain tar -cf, or add a BACKUP_FILESTORE_COMPRESS toggle — if you make it configurable, widen the retention globs to *_filestore_*.tar* so both extensions age out.

[kneckinator]

No lock around the run.

backup.sh has no flock, so if a run outlasts the cron interval the next one starts on top of it. That was survivable when the script only ran pg_dump; once a multi-hour filestore tar is in the mix on a daily schedule, overlapping runs become realistic — and two concurrent tars writing the same .part name would corrupt each other.

flock is in busybox, so this is cheap:

exec 9>"${BACKUP_DIR}/.backup.lock"
flock -n 9 || { echo "[$(date -Iseconds)] Backup already running; skipping"; exit 0; }

Strictly a pre-existing gap that this change amplifies — fine to split into its own PR if you'd rather keep this one tight, but please don't drop it.

[kneckinator]

The new settings aren't reachable from configuration.

FILESTORE_SRC is documented in the script header (line 14) but isn't wired anywhere, so there's no supported way to override it short of editing the compose file. The same will apply to BACKUP_FILESTORE and the BACKUP_FILESTORE_KEEP_* knobs. Please add all five to the backup service environment: block in both compose files, e.g.:

      # Filestore backup (opt-in; see docker/README.md for sizing)
      BACKUP_FILESTORE: ${BACKUP_FILESTORE:-false}
      BACKUP_FILESTORE_KEEP_DAYS: ${BACKUP_FILESTORE_KEEP_DAYS:-${BACKUP_KEEP_DAYS:-7}}
      BACKUP_FILESTORE_KEEP_WEEKS: ${BACKUP_FILESTORE_KEEP_WEEKS:-${BACKUP_KEEP_WEEKS:-4}}
      BACKUP_FILESTORE_KEEP_MONTHS: ${BACKUP_FILESTORE_KEEP_MONTHS:-${BACKUP_KEEP_MONTHS:-6}}

and give them a block in docker/.env.production.example under the existing BACKUPS heading (around line 141), which currently documents only BACKUP_SCHEDULE.

One trap to avoid: backup-entrypoint.sh writes /etc/profile.d/pg_env.sh, which looks like the place to add these, but it isn't. busybox crond execs jobs as children of the daemon, so they inherit the container environment directly and never source that file — it's effectively dead code for the cron path. The compose environment: block is what actually reaches backup.sh.

[kneckinator]

Nothing here has been executed yet, including by CI.

Both test-plan checkboxes are unticked and the note on #66 says the runtime test needs a production stack — so as far as I can tell this code has never run. CI hasn't covered the gap either: the head repo is Tarekchehahde/OpenSPP2, and workflows on fork PRs need maintainer approval, so not even pre-commit has looked at it. That part is on us, not you — I'll get the workflow run approved. mergeable_state is clean and docker/ has drifted by only two unrelated commits since your branch point, so there's nothing to rebase.

Worth exercising this stack specifically rather than the Traefik one, because it's the more constrained of the two: read_only: true, cap_drop: [ALL], and a cpus: "0.5" limit. I did confirm the two things most likely to bite — root plus DAC_OVERRIDE can read the filestore, and tar writing into /backups is unaffected by read_only — but the end-to-end run is still worth doing once by hand, with BACKUP_FILESTORE=true, before this merges.

I'll dismiss my own review to now have a hard block since it has been sitting waiting for a very long time.

[kneckinator]

Correcting my dismissal message: I'll dismiss my own review now to not have a hard block since it has been sitting waiting for a very long time.