fix(ansible): move pgBackRest spool-path off the 10GB root volume - #2312
Open
hunleyd wants to merge 1 commit into
Open
Conversation
This comment has been minimized.
This comment has been minimized.
Collaborator
|
Looks good. Will need to make sure it migrates correctly on existing instances. I think we need to move the directory if it exists. It depends on how/when pg_backrest is run and what is in the directory. |
hunleyd
marked this pull request as ready for review
August 22, 2026 18:17
hunleyd
enabled auto-merge
August 22, 2026 20:20
hunleyd
disabled auto-merge
August 25, 2026 17:19
This comment has been minimized.
This comment has been minimized.
hunleyd
enabled auto-merge
August 25, 2026 23:36
Contributor
Author
|
@claude review |
hunleyd
disabled auto-merge
August 27, 2026 13:47
Contributor
Author
|
@claude review |
Contributor
Author
|
:robot_face: Migration for already-provisioned instances is handled in a companion PR, not this one: supabase-admin-agent#120's |
PostgreSQL Extension Dependency Analysis: PR #2312
SummaryNo extensions had dependencies with MAJOR version updates. Full Analysis ResultsPostgreSQL 15 Extension DependenciesPostgreSQL 17 Extension DependenciesOrioleDB 17 Extension Dependencies |
PostgreSQL Package Dependency Analysis: PR #2312
SummaryNo packages had MAJOR version updates. Full Analysis ResultsPostgreSQL 15 Dependency ChangesExtracting PostgreSQL 15 dependencies...
Runtime Closure Size
Raw Dependency ClosurePostgreSQL 17 Dependency ChangesExtracting PostgreSQL 17 dependencies...
Runtime Closure Size
Raw Dependency Closure |
hunleyd
enabled auto-merge
August 27, 2026 14:44
Contributor
Author
|
@claude review |
hunleyd
disabled auto-merge
September 1, 2026 12:19
hunleyd
enabled auto-merge
September 1, 2026 14:14
pgBackRest's spool-path was never set explicitly, so it defaulted to /var/spool/pgbackrest on the AMI's 10GB root volume instead of /data (the dedicated EBS volume PGDATA lives on). Sets spool-path = /data/pgbackrest_spool in the ansible-managed pgbackrest.conf, and updates the AppArmor profile to allow writes there. The AMI-bake-time directory-creation task added here does not create this directory on any live instance's real /data -- that volume is always a separately-provisioned EBS volume attached at real instance launch, never part of the AMI image at any Packer stage. So the config value and the AppArmor grant here are correct and needed groundwork, but on their own they do not make archive-async safe to enable anywhere in the fleet. The real fix is a runtime mechanism, tracked at INDATA-1153.
hunleyd
force-pushed
the
hunleyd/indata-1038-pgbackrest-archive-async-spool-path-defaults-onto-the-10gb-root-volume
branch
from
September 3, 2026 21:50
09d40b7 to
2086ad5
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
pgBackRest's
spool-pathwas never set explicitly, so it defaulted to/var/spool/pgbackreston the AMI's 10GB root volume instead of/data(the dedicated EBS volume PGDATA lives on). This PR setsspool-path = /data/pgbackrest_spoolin the ansible-managed pgbackrest.conf, and updates the AppArmor profile to allow writes there.Correction from an earlier version of this body: the AMI-bake-time directory-creation task added here does not create this directory on any live instance's real
/data. This holds for every instance, not just the resize-compute/hibernation-resume/pg-major-upgrade cases originally disclosed under Known gap./datais always a separately-provisioned EBS volume attached at real instance launch. It is never part of the AMI image at any Packer stage. So the config value and the AppArmor grant in this PR are correct and needed groundwork, but on their own they do not makearchive-asyncsafe to enable anywhere in the fleet. The real fix is a runtime mechanism, tracked at INDATA-1153 (reopened, see Known gap below for why).(bot-generated information collapsed below)
Details
spool-path = /data/pgbackrest_spoolin the ansible-managed[global]pgbackrest.conf, and updates the directory-creation task to create that path instead of/var/spool/pgbackrest.sbpostgresAppArmor profile (postgres_shellandpgbackrest_shellsub-profiles) to grant write access to/data/pgbackrest_spoolinstead of the old/var/spool/pgbackrest. The profile runs in enforce mode, so without this change the config change alone would just trade "spool fills the root volume" for "every archive-push/archive-get write gets denied."/data/pgbackrest_spool/ rw,) plus the recursive/data/pgbackrest_spool/** rw,grant. The bare-dir rule has no counterpart in the old/var/spool/pgbackrest/** rw,line, since**alone doesn't cover the directory inode itself.02770), matching/etc/pgbackrest/conf.d's own mode in the same loop, for the same reason: bothpostgres_shellandpgbackrest_shellwrite here, so files need thepostgresgroup regardless of which sub-profile creates them first.[global]-scope pgBackRest option, set once at provisioning. Nosupabase-admin-agentchanges needed for the config value itself. Split out of feat(sudoers): let supabase-admin-agent invoke pgbackrest reconcile as root #2291 (sudoers grant forpgbackrest reconcile) since the two are unrelated concerns: one is a privilege-escalation grant, this one is a storage-location fix, and neither depends on the other.spool-pathis not reachable yet:archive-async = nin this same file, andsupabase-admin-agent'srepo1_async.confstill ships# archive-async = ycommented out. pgBackRest's own config parser only errors on an unmet option dependency when the value comes from the command line, not a config file, so the unused setting is silently accepted rather than rejected. The benefit only materializes once something turnsarchive-asyncon fleet-wide (INDATA-996), which is gated on the Known gap below closing first.archive-asyncis on, the spool shares disk headroom with PGDATA instead of the root volume: worst case isarchive-get's default 128MiB prefetch queue landing on a data volume that's already tight (a 100MiBtune2fs -rreserve is already root-only). Not a new risk this PR introduces on its own, since the setting is inert untilarchive-asyncships, but worth boundingarchive-get-queue-max/archive-push-queue-maxexplicitly wherever that enablement lands, rather than relying on the default.Why the config and AppArmor changes are still correct, on their own
spool-pathlive on the same filesystem aspg_wal, soarchive-getcan hand a WAL segment to Postgres with a rename instead of a copy./data/pgdata/pg_walis on/data, so this change satisfies that recommendation directly. That's the primary justification, independent of whether the directory-creation task itself does anything useful (see Known gap).spool-pathcontents aren't durable state.archive-pushrechecks each WAL segment against the repo andarchive-getrebuilds its queue on loss. No migration needed for spool contents, whatever eventually creates the directory.Known gap (tracked separately, blocking
archive-asyncfleet-wide, not blocking this PR)/data. Traced through both Packer stages (amazon-*-nix.pkr.hcl,stage2-nix-psql.pkr.hcl): neither declares anami_block_device_mappingsentry for a data volume, and the production data volume is always attached and mounted at/dataat real instance launch, outside this repo, overlaying whatever the bake wrote underneath./dataitself is root:root 0755 at runtime, and the unprivilegedpgbackrestuser can't self-create a directory there (pgBackRest's ownstoragePathCreatePdoes create missing parents, but the DAC check on root-owned/datafails first)./data/pgbackrest_spoolon the instance itself, at the point pgBackRest is about to need it (pgbackrest enable/setup-replica/stanza-upgrade), not at AMI-bake time. That issue was previously closed as superseded bysupabase-admin-agent#120'sEnsureSpoolDir(). Confirmed during this PR's own review thatEnsureSpoolDir()is only called fromreconcile, whose own preflight requiresenable's configs to already exist, so it structurally cannot create the directory beforeenableitself needs it. Reopened with the corrected finding.archive-asyncenablement gate). Without it,archive-push/archive-getfail outright on every instance the momentarchive-asyncturns on, not just a subset.supabase/saltmanages its own copy of thesbpostgresAppArmor profile (salt/state/formula/apparmor/files/sbpostgres_apparmor) and reapplies it on every highstate, fleet-wide, roughly every 10 minutes. That copy still only grants/var/spool/pgbackrest/** rw,; it has no/data/pgbackrest_spoolgrant at all, so it overwrites this PR's AppArmor change on every live instance shortly after boot. A companionsupabase/saltPR needs to add the same two-rule grant beforearchive-asynccan write to/data/pgbackrest_spoolanywhere in the fleet.archive-asyncstays off.nix/packages/supascan'sShallowDirsnow excludes/data/pgbackrest_spool, matching the existing/data/pgdataentry. Both hold transient, high-churn state that would otherwise register as audit-baseline drift on every capture.Considered and not done
/data/pgdata,/var/lib/pgbackrest). Templating this one path alone means converting two plaincopy-sourced files to.j2and adding a variable, for a string used three times in Ansible plus once in Go (which can never consume an Ansible variable regardless). Not worth it for this one path when nothing else in the repo does it either.Testing
ansible-lint ansible/tasks/setup-pgbackrest.ymlpasses cleanlyansible-playbook --syntax-checkpasses cleanly on the full playbook/data/pgbackrest_spoolexists withpgbackrest:postgresownership after provisioning, andarchive-get/archive-pushoperate against it under the updated AppArmor profile. Expected to fail on a real instance per the Known gap above, until INDATA-1153 ships. Useful only to confirm the AMI-bake-time artifact itself (config value present, AppArmor grant present) is correct.Misc
Resolves INDATA-1038
Internal-only entry: supabase/changelog#225