Backup rework - #1249
Draft
mxsrc wants to merge 11 commits into
Draft
Conversation
mxsrc
marked this pull request as draft
August 17, 2026 10:44
mxsrc
force-pushed
the
backup-rework
branch
2 times, most recently
from
August 17, 2026 16:38
3ea68c8 to
b50ae6c
Compare
Backup configuration lives on the cluster as an untyped `dict`, and every
consumer re-derives its shape with `.get(key, default)`. That is why a backup
today cannot be interpreted without the cluster that wrote it: nothing records
what the dict contained, and nothing validates it.
Split the concept in two so that "a backup never carries credentials" is a
property of the type rather than of the code that populates it:
BackupLocation everything needed to find and interpret a backup's objects.
Cannot represent a secret. Frozen, extra="forbid", compares
by value so chain-homogeneity checks are a plain `==`.
BackupConfig a location plus the credentials and node-local tuning needed
to act on it. Never leaves the control plane.
No sentinel values. Each field is mandatory, a boolean with a meaningful
default, or Optional where absence is a real state -- `endpoint=None` means AWS
default resolution, `credentials=None` means the instance IAM role,
`s3_thread_pool_size=None` means the data plane's own default. Nothing has to be
compared against "" or 0 to discover whether it was configured.
Two shapes worth calling out:
* `region` is mandatory. A backup whose region was never recorded is not
restorable from anywhere else, which is the problem being fixed. It is
absent from every existing config today -- the data plane hardcodes
us-east-1 under local_testing and otherwise lets the SDK resolve it from the
environment -- so a legacy non-local_testing config now fails validation
with an actionable message. That is deliberate.
* Credentials are an S3Credentials pair, not two fields, so "access key set,
secret missing" is unrepresentable instead of something a validator catches.
`local_testing` bundled four separate decisions into one flag (plain HTTP, no
TLS verification, path-style addressing, hardcoded region). The before-validator
unpacks it into the properties it actually stood for, and maps the rest of the
legacy dict shape, so no FDB migration is needed and tests/perf/backup_config.json
keeps working.
Constraints live in named Annotated aliases rather than per-field Field(...)
calls, following simplyblock_web/api/v2/util.py. They sit in simplyblock_core
because core controllers and both API versions consume them.
Additive: nothing reads these models yet. `Cluster.backup_config` stays a dict
because BaseModel cannot nest pydantic types; `Cluster.get_backup_config()`
validates on read and raises PreconditionError, which is what callers already
guard on -- a raw ValidationError would escape them.
SecondaryTarget is (str, Enum) rather than StrEnum because tox pins
basepython = python3.9.
Replaces the v2 `BackupConfigParams` DTO and every `backup_config.get(key,
default)` call with the typed model. The defaults were scattered across three
files and disagreed with each other; `create_s3_bdev` in particular derived a
bucket name (`simplyblock-backup-{cluster_id}`) when the key was missing, which
is how a cluster could end up writing to a bucket nobody had configured.
Adds GET/PUT /clusters/{id}/backup-config. Backup configuration was settable
only at cluster-create time, so there was no way to correct or complete it --
and no way at all to record a region on a cluster created before regions were
mandatory. PUT is a full replacement rather than a patch because the fields
interact: an endpoint implies an addressing style and TLS expectations, and
merging half a config into an existing one produces combinations nobody chose.
`set_backup_config` goes through atomic_update, since monitors mutate cluster
status concurrently and a full write would clobber them.
`_s3_client` now honours region, verify_tls and use_path_style, none of which it
could previously express, and omits credentials entirely when none are
configured so boto3's default provider chain (instance IAM role) applies. It
previously passed `None` for both keys unconditionally, which is a different
thing from not passing them.
`to_storage_dict` converts only the two values that a python-mode `model_dump`
leaves non-JSON-serializable (the Url and the enum). It deliberately does not
use `mode="json"`: that renders SecretStr as `**********` and would silently
destroy the credentials on write. Keeping the wrappers means write_to_db's
existing unwrap-at-the-last-moment pass still produces plaintext while every log
line in between stays masked.
The data plane still takes the old parameter shape, so create_s3_bdev maps back
to it. Two mappings are lossy and are marked as such until phase 2 replaces the
RPC: `local_testing` is not a mode but the only condition under which the data
plane honours an endpoint override at all, so it now tracks "an endpoint was
configured"; region, verify_tls and use_path_style have nowhere to go.
`switch_backup_source` is adapted rather than fixed -- it is removed later in
this series.
Three changes that together make a Backup record say where its own data is. Backup.location A BackupLocation, resolved once per chain by the caller and passed into _create_single_backup rather than read from the cluster per backup, so every backup in one chain is guaranteed to share it. get_location() validates on read and raises PreconditionError, matching Cluster.get_backup_config. Backup.s3_metadata is deleted It was written in two places and read in none: export_backups rebuilt its own dict from the model fields instead. It was a partial duplicate of fields already on the same record, in the same database, so it survived exactly as well as the cluster did -- while the docstring and the (also deleted, never referenced) BACKUP_S3_METADATA_BUCKET constant claimed it went to S3. The real S3 manifest lands in the next commit; leaving this behind would only keep a second, staler copy of the same facts. s3_id allocation _next_s3_id was max-plus-one over the local cluster's Backup records. It raced, so two concurrent backups could get the same id; it recycled the id of a deleted backup whose objects may still exist, since nothing reclaims them (bdev_lvol_s3_delete does not exist on the data plane); and after an import it counted foreign backups it had no business counting. Replaced with a monotonic FDB sequence, reusing the _VUID_SEQ_KEY pattern already in db_controller, which was introduced for this exact class of problem. Unlike vuid the space is bounded: the data plane packs s3_id into 30 bits and masks rather than validates, so BACKUP_MAX_S3_ID is now explicit and exhaustion raises instead of silently aliasing onto another backup's keys. export/import now carry location and encrypted. Import previously dropped `encrypted` entirely, so an imported encrypted backup restored with use_crypto=False -- a plaintext volume over ciphertext, silently. Entries missing either field are rejected in the pre-check loop, so a stale export file fails whole rather than half-importing. _auto_backup_lvol resolves node, cluster and location before taking the snapshot. It used to snapshot first and discover afterwards that the cluster was unusable, leaving an orphaned auto_* snapshot behind on every scheduler tick. backup_snapshot's `if not snapshot.lvol` guard is removed: it cannot fire for any snapshot read from the database, because SnapShot.write_to_db builds a SnapShotMini whose from_snapshot calls LVolMini().from_lvol unconditionally (snapshot.py:87), so a snapshot without an lvol cannot be persisted in the first place. Dropping it also collapses a duplicated storage-node lookup, where the first fetch swallowed the KeyError that the second then reported. Tests: TestImportBackups and TestBackupSnapshot are converted off the stubbed-DB pattern onto real FoundationDB, per tests/AGENTS.md. Converting them is what surfaced the dead guard above -- the old test only reached it by mocking the database away. Also, TestCreateS3Bdev asserted only pytest.raises(Exception), which passes on any error including an AttributeError from a wrong argument type; tightened to the specific exceptions, and test_exception_handled now raises RPCException rather than a bare Exception, which the code under test never caught, so that test had been passing for the wrong reason.
The data plane writes only opaque objects keyed {s3_id}/{mid}/{extent}. Nothing
in them records which volume they came from, how they are encoded, or which
other backups they depend on -- all of which lived exclusively in the
originating cluster's FoundationDB. That is precisely what a disaster recovery
does not have.
A manifest now goes into the same bucket, at manifests/{backup_id}.json. The
prefix is a leading non-numeric segment, so it cannot collide with the data
plane's decimal keyspace. It carries the location, the complete chain oldest
first (so one read is enough to plan a restore, rather than walking manifest by
manifest), the volume's shape, source provenance, and the object layout. It
carries no credentials: it says where the objects are and how to read them,
never how to authenticate. The reader supplies that.
Publication order is deliberate. The manifest is written BEFORE the backup is
marked COMPLETED, so that status implies "identifiable from the bucket alone",
and a manifest failure fails the backup. Data in a bucket with no manifest is
data nobody can attribute to a volume later.
A merge rewrites the chain, so it republishes the surviving manifest and deletes
the merged-away one -- a bucket that still advertised a merged backup would send
a restore after keys the data plane has unmapped. Because the S3 merge is
already done and not reversible at that point, a manifest failure there retries
rather than failing the task; the finalisation is idempotent.
export_backups now emits manifests instead of a third, narrower format of its
own. That format is how `encrypted` came to be omitted: two shapes for the same
concept, and only one of them was maintained. A hand-carried file and a bucket
read are now interchangeable, which is what lets import accept either.
New: discover_backups / import_from_bucket, and POST /backups/discover. Given a
bucket and credentials for it, these answer "what is in here" and "register it"
with no reference to any cluster, live or dead. That is the disaster-recovery
entry point the feature was missing.
Two deliberate non-deletions:
* delete_backups does not remove manifests. bdev_lvol_s3_delete does not exist
on the data plane, so the objects outlive the call; the manifest is the only
thing that can still identify them, and dropping it would turn a reclaimable
orphan set into anonymous bucket weight.
* list_all and _parse refuse an unreadable or unknown-version manifest rather
than skipping it. Silently omitting a backup from a recovery listing is how
an operator concludes their data is gone.
The S3 client moves to the new module, so there is one place that knows how to
talk to a bucket rather than a copy in the controller.
Tests: manifest schema handling is unit-tested; assembly and the export -> wipe
the database -> import round-trip run against real FoundationDB, with boto3
mocked at the client boundary as an external service. TestImportBackups is
deleted from test_backup.py rather than ported -- the new file covers the same
ground against the manifest format with real fixtures, and maintaining two
copies is what let the old format drift.
…rap it
An encrypted volume's backup is ciphertext in a bucket whose key lives in the
originating cluster's KMS. Under Vault that key is reachable only from a cluster
that can still reach the same Vault; under the FoundationDB-backed LocalKMS it
is in the originating cluster's own database, so once that cluster is gone the
backup is undecryptable by anyone, including its owner. None of this was
recorded anywhere -- the dependency was implicit, and only discovered during a
recovery.
A backup now carries an `encryption` document:
descriptor which KMS held the key, which Vault and mounts, and at what
path. Never key material. Makes the dependency visible, and
supports a restore by a cluster that can still reach that KMS.
wrapped_key the key pair itself, encrypted under a key derived from an
operator-held passphrase via Argon2id, present only when the
cluster has key_wrapping_secret configured.
Key wrapping is the deliberate, narrow exception to "a manifest never carries
secrets": it carries key material, but only as ciphertext under a secret that is
in neither the manifest, the bucket, nor the database. Security rests entirely
on that passphrase, hence Argon2id at OWASP parameters with a per-backup salt --
a shared salt would let one cracked passphrase precompute against every backup.
It is opt-in per cluster, and works identically for Vault and LocalKMS, since
both KMS backends already hand the control plane a plaintext key pair.
Restore resolves the key before creating the volume, preferring the wrapped key
(needs only the passphrase) over the descriptor (needs the KMS to still exist),
and raises PreconditionError when neither is reachable. Previously the volume
was created first and the key looked up after -- and for an imported backup
`encrypted` was always False, so it silently produced a plaintext volume over
ciphertext. The error names the path, the cluster and the KMS, because an
operator mid-recovery needs to know what is missing.
`dr_capable` surfaces the gap where it can be acted on: at backup time as a
warning, and in `backup list` as a column, rather than at recovery time.
`cryptography` becomes an explicit dependency. It was already present
transitively, but this code imports it directly.
Backup.encrypted stays authoritative over the copy inside the encryption
document, and build_manifest overlays it so the two cannot disagree in a
manifest. Two places recording one fact can drift, and drift here decides
whether a restore decrypts.
The key pair is JSON-encoded inside the wrapped blob rather than joined on a
separator, so no key content can affect whether it round-trips.
Every rule here already existed implicitly, enforced by whatever failed first --
usually the data plane, usually mid-operation, sometimes not until someone tried
the restore the backup was taken for. They are now checked at the earliest layer
that can check them, before any side effect.
At backup creation, before the chain lock, before any KMS key, before any task:
* The cluster has a valid backup configuration.
* That configuration can hold backups at all. snapshot_backups=False selects
the secondary-tiering key layout {tiering_id}/{lpgi}; a backup written there
is unreadable by a restore, which addresses {s3_id}/{mid}/{extent}.
* The chain is no longer than BACKUP_MAX_CHAIN_LENGTH. Beyond that the data
plane copies the decoded array into a fixed 40-element stack buffer
(vbdev_lvol_rpc.c) and smashes the storage node's stack. Refusing here is
the only guard until those buffers are sized properly.
* Every existing ancestor shares this backup's location, and agrees on whether
it is encrypted. A restore reads clusters from the whole chain in one
operation against one bucket with one key; nothing in the stack could
express a chain split across buckets or half encrypted. This is what
silently broke when a cluster's bucket was reconfigured mid-chain.
* A configured key-wrapping secret can actually wrap. A cluster that asked for
wrapping must not silently get backups without it -- that is the difference
between a recoverable backup and one that dies with its cluster.
At restore, before the volume is created: the same chain coherence and length
checks. A doomed restore now leaves no half-built volume.
At import, before the first record is written: each manifest's chain must be
satisfiable, either within the batch or by records already present, and must not
span buckets. An import that lands a delta whose ancestors are missing produces
something that looks restorable in `backup list` and fails only when tried --
typically during the recovery it was meant to serve. The chain is recorded in
each manifest precisely so this is answerable up front.
Tests assert the absence of side effects, not just the error: no Backup record,
no task, no chain lock, no volume. TestRestoreBackup is converted off the
stubbed-DB pattern onto real FoundationDB, per tests/AGENTS.md; add_lvol_ha and
the task runner stay mocked, since they sit above the database and drive RPC.
An S3 device holds exactly one bucket with one set of credentials, so reading
another cluster's bucket means attaching another device. The lvstore already
supports that -- its transfer devices are a list -- so a restore now attaches a
device for the backup's own recorded location, reads through it, and drops it
again. This is what the recorded location was for.
The device's whole lifecycle belongs to the task runner, not to the restore
request. Two reasons, either sufficient:
* The node is not known when the restore is requested. target_node_id may be
None, in which case add_lvol_ha chooses the node; there is nothing to attach
a device to until it has.
* A node restart mid-restore takes the device with it. _run_restore already
re-issues after STATUS_SUSPENDED, so the runner is the only component that
can put the device back -- and it already owned teardown, so creation
belongs with it.
The device name is derived from the backup id, so a retry re-derives the same
name rather than leaking a device per attempt, and creation is re-run on every
attempt instead of once.
Cleanup happens on every terminal path, including the two that abandon the
restore because the volume was deleted underneath it, and the timeout /
retry-ceiling path in _terminate_task. It is best-effort and logged rather than
raised: a cleanup failure must not turn a completed restore into a failed one.
A leaked device is worth noticing though, since a non-empty transfer_devs list
blocks the lvstore from being destroyed (vbdev_lvol.c:502).
Credentials for a foreign bucket travel in the task's parameters, because the
runner needs them on every attempt. They are scrubbed when the restore reaches a
terminal state -- a task record is retained for weeks afterwards, and another
cluster's S3 keys have no business outliving the restore that needed them.
foreign_bucket_config returns one value and raises on error, rather than the
tuple-plus-flag it started as. A None result means what it says: there is no
foreign bucket, so there is nothing to describe. The device name is no longer
threaded through as an empty-string sentinel; the runner derives it, which it
can do because it knows the node.
Refusing a foreign bucket with no credentials is deliberate: the cluster's own
static keys say nothing about someone else's bucket, and falling back to them
fails deep in the data plane with nothing pointing at the cause. A cluster with
no static credentials at all is a different matter -- there the nodes' instance
role is the only answer, and it is allowed through.
bdev_lvol_s3_recovery gains an optional s3_bdev, and bdev_s3_delete is added to
the RPC client. The data-plane side of that parameter lands in the commit that
makes it required; until then the data plane ignores it and picks the first
attached S3 device, which is exactly the ambiguity being removed.
The mechanism this served never worked. The S3 bdev picks its bucket as
bucket_names[idx] where idx comes from bit 63 of the packed offset
(bdev_s3_impl.cpp:611), and every s3_pack_offset call site in lib/lvol passes
msb_flag=false. So idx is permanently 0, only the first registered bucket is
ever addressed, and bdev_s3_add_bucket_name -- which appends rather than
replaces -- was adding state nothing reads. Switching the source did nothing at
all.
Two guards enforced that fiction and are removed with it:
* Backup creation was blocked cluster-wide while the source pointed
elsewhere. There is no cluster-wide source any more.
* Restore refused a backup from another cluster unless the whole cluster had
first been re-pointed at that cluster's bucket. Restore now attaches a
device for the backup's own recorded location, so there is nothing to
re-point and nothing to refuse.
Also gone: Cluster.backup_source, get_backup_sources, switch_backup_source,
is_local_backup_source, `sbctl backup source-list` / `source-switch`, the v2
/source-switch and /sources endpoints, and _s3_bucket_exists, which existed
only to pre-flight the switch. cli.py is regenerated from the reference rather
than edited.
source_cluster_id stays, demoted to provenance: an operator reading a bucket
wants to know where its contents came from. A guard test asserts nothing
outside the backup stack reads it, and that backup_controller never resolves a
cluster, a backup configuration or a KMS connection through it -- that
resolution is what made a backup unrestorable once its originating cluster was
gone, and it would be an easy thing to reintroduce by reflex.
A device now takes its bucket at creation and keeps it. That is what a bucket
actually is here -- it comes with its own credentials, endpoint and region, so a
device serving several would need several clients. Reading a second bucket is
done by creating a second device, which the lvstore already supports: its
transfer devices are a list.
What this replaces was a mutable vector indexed by bit 63 of the I/O offset.
Nothing ever set that bit -- every s3_pack_offset call site in lib/lvol passes
msb_flag=false -- so only entry 0 was ever addressed, and the index read past
the end whenever the vector held just the one bucket the control plane
registered. bdev_s3_add_bucket_name is deleted along with it; a device can no
longer exist in a bucket-less state, so there is no window in which one is
attached to an lvstore with nothing to read.
Aws::InitAPI / ShutdownAPI are reference-counted. They were called per device,
so deleting a second S3 device shut the SDK down underneath the first -- which
a restore attaching a device for a foreign bucket does routinely.
Endpoint, region, TLS verification and addressing style are separate parameters.
They were bundled into a `local_testing` flag that forced HTTP, disabled
certificate verification and hardcoded us-east-1, and an endpoint was honoured
only when it was set. That made every non-AWS S3-compatible store a
testing-only configuration. verify_tls defaults to true, set explicitly because
the surrounding memset would otherwise default it to "do not verify".
With no key configured the SDK's default provider chain is used, so the
instance role works as the header has always claimed. Passing an empty
AWSCredentials, as this did, is not the same thing: the SDK takes it as a valid
anonymous identity and never consults the chain.
Two fixes in add_directory_name, which shares this code:
* The bdev lookup was dereferenced before its NULL check, so an unknown name
segfaulted the storage node. The same path leaked its context.
* It read and wrote bucket_names rather than directory_names, so every call
after the first corrupted the bucket list instead of registering a
directory.
The filesystem target's directory selection has the same unbounded index as the
bucket selection did; bounded rather than reworked, since giving it one
directory per device is a separate change.
NOT COMPILED. aws-sdk-cpp is deliberately absent from this monorepo (see
CLAUDE.md), there is no system AWS SDK, and SPDK's submodules are empty, so
nothing here can be built or linked in this tree. The changes are structurally
checked and the Python side is fully tested, but the C++ needs a real build
against the ultra workspace before it can be trusted.
The transfer RPCs now take a required s3_bdev, and spdk_find_s3_bdev resolves it
by name instead of returning the first entry with is_s3 set. An lvolstore can
carry several S3 devices -- its own backup bucket, plus one a restore attached
for another cluster's bucket -- so "the first" is ambiguous exactly during the
operation that needs it to be right.
Required rather than optional, deliberately. The only available default is the
old first-match behaviour, which is the ambiguity being removed, and the caller
always knows the name. spdk_json_decode_object is the non-relaxed form, so an
omitted parameter is rejected rather than guessed, and a caller from before this
change fails loudly instead of writing to an arbitrary bucket. A named device
that is absent now returns -ENODEV and says which name it looked for, rather
than -EINVAL with nothing to go on.
Two memory-safety fixes in the same handlers:
* snapshot_chain and s3_ids_chain were fixed 40-element stack arrays fed by a
decoder bounded at RPC_MAX_LVOL_VBDEV (255). A 41-link chain overran the
stack of the process serving live volumes, reachable from an ordinary tiered
retention schedule. Both are now sized to the decoder's own bound.
* s3_id is validated against S3_ID_BITS in all three handlers. The offset
packing masks it to 30 bits without checking, so a larger value silently
aliased onto another backup's object keys -- one backup overwriting
another's data.
The ordering contract is now stated where callers read it. snapshot_names and
s3_ids must be NEWEST first, because prepare_s3_clusters is first-writer-wins
(blobstore.c:15515). rpc_client.py documented the opposite and passed
reversed(chain), so it was correct by accident; the SPDK binding said only
"Ordered list", which is not wrong but not usable either. Both now say which end
comes first and why, and scripts/rpc.py says it in its --help.
Also in scripts/rpc.py: bdev_lvol_s3_recovery passed offset= to a binding that
has no such parameter, so invoking it from the SPDK CLI raised TypeError before
reaching the target. The argument is dropped along with the call.
Verified by compiling. include/spdk/config.h is normally generated by
./configure, which needs the DPDK submodule this subtree does not wire up;
generating a stub (the file is gitignored) makes gcc -fsyntax-only work, and
lvol.c, vbdev_lvol_rpc.c and vbdev_lvol.c all compile clean with -Wall. That
covers syntax and types, not linking or behaviour. S3_ID_BITS was checked to
expand to 30 through vbdev_lvol.h rather than silently vanishing.
The CLI could not reach a bucket except through a cluster's own configuration,
which is exactly what a disaster recovery does not have. Three additions:
sbctl backup discover --bucket … --region … lists what a bucket contains,
reading its manifests. Takes no
cluster at all.
sbctl backup import --bucket … registers those backups, as an
alternative to --from-file.
sbctl backup restore … --access-key-id … restores from a bucket that is
--key-wrapping-passphrase not this cluster's own.
`backup import` grows --from-file and requires exactly one of that or --bucket;
the old positional metadata_file is gone, because "a file" is no longer the only
place manifests come from. The discover listing surfaces per backup whether it
is recoverable without the originating cluster's KMS, since that is the question
an operator is actually asking.
cli.py is regenerated with `tox run -e generate`, never edited.
Also cleans up bdev_s3_create's signature, which had carried sentinel defaults
over from the shape it replaced. `0` for the two CPU masks and the thread-pool
size, and `""` for endpoint and region, all meant "not specified" -- and the data
plane already reads zero that way (bdev_s3_impl.cpp:1204, 1222, 1239), so an
explicit 0 behaved as absent while reading as a deliberate choice. Those are now
Optional and omitted from the payload when None. Nothing else is special-cased:
a caller that passes an empty string meant to, and the data plane treats it as
absent.
region, secondary_target, with_compression and snapshot_backups lose their
defaults entirely. A device cannot function without a region, and the other
three are decisions the caller has already made -- defaulting them invites the
wrong one silently.
_compute_s3_cpu_masks returns None rather than 0 where the node does not say,
since a zero mask selects no CPUs and only ever meant "unset".
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
1 participant
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.
No description provided.