Skip to content

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

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

Open
Tarekchehahde wants to merge 1 commit into
base: 19.0
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
1 change: 1 addition & 0 deletions docker/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -217,6 +217,7 @@ The production stack includes automated PostgreSQL backups:
- **Schedule:** Daily at 2am (configurable via `BACKUP_SCHEDULE`)
- **Retention:** 7 daily, 4 weekly, 6 monthly
- **Location:** `backup_data` Docker volume
- **Filestore:** When the `odoo_data` volume is mounted on the backup service, attachments under `/odoo_data/filestore/<database>` are archived daily as `*_filestore_*.tar.gz` alongside the database dump

@kneckinator kneckinator Sep 2, 2026

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.

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.


To restore a backup:

Expand Down
22 changes: 22 additions & 0 deletions docker/backup.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
# Environment variables:
# PGHOST, PGPORT, PGUSER, PGPASSWORD, PGDATABASE (standard PostgreSQL vars)
# BACKUP_DIR (default: /backups)
# FILESTORE_SRC (default: /odoo_data/filestore/$PGDATABASE)
# BACKUP_KEEP_DAYS (default: 7)
# BACKUP_KEEP_WEEKS (default: 4)
# BACKUP_KEEP_MONTHS (default: 6)
Expand Down Expand Up @@ -52,15 +53,33 @@ ln -sf "${BACKUP_FILE}" "${DAILY_DIR}/${PGDATABASE:-openspp}_latest.dump"

echo "[$(date -Iseconds)] Daily backup complete: ${BACKUP_FILE}"

# Filestore backup (attachments, documents) when odoo data volume is mounted
FILESTORE_SRC="${FILESTORE_SRC:-/odoo_data/filestore/${PGDATABASE:-openspp}}"

@kneckinator kneckinator Sep 2, 2026

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.

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.

FILESTORE_BACKUP_FILE="${PGDATABASE:-openspp}_filestore_${TIMESTAMP}.tar.gz"
if [ -d "${FILESTORE_SRC}" ]; then

@kneckinator kneckinator Sep 2, 2026

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.

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.

echo "[$(date -Iseconds)] Starting filestore backup from ${FILESTORE_SRC}..."
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"

@kneckinator kneckinator Sep 2, 2026

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.

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.)

echo "[$(date -Iseconds)] Filestore backup complete: ${FILESTORE_BACKUP_FILE}"
Comment on lines +61 to +63

@gemini-code-assist gemini-code-assist Bot Jul 11, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

high

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

else
echo "[$(date -Iseconds)] Filestore not found at ${FILESTORE_SRC}; skipping filestore backup"
fi

# Weekly backup (Sunday)
if [ "${DAY_OF_WEEK}" = "7" ]; then
cp "${DAILY_DIR}/${BACKUP_FILE}" "${WEEKLY_DIR}/"
if [ -f "${DAILY_DIR}/${FILESTORE_BACKUP_FILE}" ]; then
cp "${DAILY_DIR}/${FILESTORE_BACKUP_FILE}" "${WEEKLY_DIR}/"
fi
echo "[$(date -Iseconds)] Weekly backup saved"
fi

# Monthly backup (1st of month)
if [ "${DAY_OF_MONTH}" = "01" ]; then
cp "${DAILY_DIR}/${BACKUP_FILE}" "${MONTHLY_DIR}/"
if [ -f "${DAILY_DIR}/${FILESTORE_BACKUP_FILE}" ]; then
cp "${DAILY_DIR}/${FILESTORE_BACKUP_FILE}" "${MONTHLY_DIR}/"
fi
echo "[$(date -Iseconds)] Monthly backup saved"
fi

Expand All @@ -69,12 +88,15 @@ echo "[$(date -Iseconds)] Cleaning up old backups..."

# Remove daily backups older than BACKUP_KEEP_DAYS
find "${DAILY_DIR}" -name "*.dump" -type f -mtime +${BACKUP_KEEP_DAYS} -delete 2>/dev/null || true
find "${DAILY_DIR}" -name "*_filestore_*.tar.gz" -type f -mtime +${BACKUP_KEEP_DAYS} -delete 2>/dev/null || true

@kneckinator kneckinator Sep 2, 2026

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.

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.


# Remove weekly backups older than BACKUP_KEEP_WEEKS weeks
find "${WEEKLY_DIR}" -name "*.dump" -type f -mtime +$((BACKUP_KEEP_WEEKS * 7)) -delete 2>/dev/null || true
find "${WEEKLY_DIR}" -name "*_filestore_*.tar.gz" -type f -mtime +$((BACKUP_KEEP_WEEKS * 7)) -delete 2>/dev/null || true

# Remove monthly backups older than BACKUP_KEEP_MONTHS months (approximate: 30 days per month)
find "${MONTHLY_DIR}" -name "*.dump" -type f -mtime +$((BACKUP_KEEP_MONTHS * 30)) -delete 2>/dev/null || true
find "${MONTHLY_DIR}" -name "*_filestore_*.tar.gz" -type f -mtime +$((BACKUP_KEEP_MONTHS * 30)) -delete 2>/dev/null || true

# Report disk usage
echo "[$(date -Iseconds)] Backup sizes:"
Expand Down
1 change: 1 addition & 0 deletions docker/docker-compose.nginx.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -427,6 +427,7 @@ services:
- ./backup.sh:/backup.sh:ro,z
- ./backup-entrypoint.sh:/backup-entrypoint.sh:ro,z
- backup_data:/backups:rw,z
- odoo_data:/odoo_data:ro,z

@kneckinator kneckinator Sep 2, 2026

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.

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.

networks:
- openspp-prod
restart: unless-stopped
Expand Down
1 change: 1 addition & 0 deletions docker/docker-compose.production.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -333,6 +333,7 @@ services:
- ./backup.sh:/backup.sh:ro
- ./backup-entrypoint.sh:/backup-entrypoint.sh:ro
- backup_data:/backups
- odoo_data:/odoo_data:ro

@kneckinator kneckinator Sep 2, 2026

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.

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.

networks:
- openspp-prod
restart: always
Expand Down