Backup rework - #1249
Draft
mxsrc wants to merge 18 commits into
Draft
Conversation
mxsrc
marked this pull request as draft
August 17, 2026 10:44
mxsrc
force-pushed
the
backup-rework
branch
5 times, most recently
from
August 18, 2026 13:07
57d4a06 to
103641e
Compare
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
|
|
||
| assert response.status_code == 200 | ||
| assert response.json() == {'imported': 1} | ||
| (manifests,), kwargs = backup_controller.import_backups.call_args |
4 tasks
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.
Absence is `None`, not a sentinel -- `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. The point is to
replace the pattern where "" and 0 stood for "not configured", not to make an
empty string unrepresentable: `bucket_name=""` still validates, and is a
misconfiguration rather than something the type system defends against.
Two shapes worth calling out:
* `bucket_name` is the only mandatory field. Nothing can invent one -- and
something used to: create_s3_bdev derived `simplyblock-backup-{cluster_id}`
when the key was missing, which is how a cluster could write to a bucket
nobody had configured.
* Credentials are an S3Credentials pair, not two fields, so "access key set,
secret missing" is unrepresentable instead of something a validator catches.
`region` is deliberately NOT mandatory, for the same reason credentials are not:
the AWS SDK resolves it from the environment, the profile or instance metadata,
and every layer below already accepts its absence -- boto3 by its own resolution,
the data plane by `if (region && *region)` in init_client, and its RPC decoder by
marking the field optional. Recording a region is better, since a manifest that
names one can be read from anywhere while one that does not depends on the
reader's environment agreeing; but it is recoverable rather than lost, because
bucket names are globally unique and S3 can be asked where a bucket lives.
Requiring it would have failed every config written before this model for a
property the stack never needed.
`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.
SecondaryTarget is an IntEnum whose members ARE the values the data plane's RPC
takes, so it names the 0 and 1 without needing a translation layer, and existing
stored configs validate unchanged.
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 ValueError -- both for an absent config and for an
invalid one, since ValidationError is a ValueError and callers want one except
clause. It is not a PreconditionError: a stored document failing validation is
not a precondition the caller could have checked.
Replaces 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.
The v2 `BackupConfigParams` request DTO becomes `BackupConfigDTO`, an alias of
the core model. The two shapes are identical today, so a hand-copied duplicate
would only drift -- but the API keeps a name of its own, so the wire format can
diverge later by turning the alias into a real class, without touching a route
signature.
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.
The form written into the untyped `Cluster.backup_config` dict is a plain
`model_dump(exclude_none=True)`. Field serializers on the two values a
python-mode dump leaves non-JSON-serializable -- the Url and the enum -- make
that hold, so there is no hand-written conversion step to keep in sync with the
fields. Deliberately not `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 ValueError, matching Cluster.get_backup_config -- a stored document failing validation is not a precondition the caller could have checked. 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. That rejection is a ValueError: the shape of a supplied entry is a bad request, where "this backup id already exists" is a precondition -- the v2 import endpoint maps the former to 400 explicitly, since only PreconditionError has a global handler. _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 id of the backup this one
is a delta against, 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.
Chains are derived, not stored. backup_manifest.chain_of walks prev_backup_id
across the manifests in the bucket. Storing the chain in each manifest would mean
every merge invalidates the manifest of every descendant of the backup it folded
away: the write cost of a merge becomes the length of the chain, and a partial
failure leaves the bucket advertising object keys the data plane has already
unmapped. With only the immediate link stored, a merge is exactly two objects --
republish the survivor, whose prev_backup_id just moved, and delete the
merged-away one -- and every other manifest stays true without being touched.
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.
Absence is spelled as absence. A chain root has prev_backup_id=None rather than
""; a volume's settings and the object size are Optional and absent together with
the volume or cluster they came from, because 0 is a real answer for a QoS cap
(it means unlimited) and for a priority class. What is always knowable off the
backup record -- ids, timestamps, size, whether it is encrypted -- is mandatory,
with no default to fall back on. The volume settings are recorded even though
restore still uses hardcoded defaults, because a manifest is read years after it
is written and a backup cannot be given a shape retroactively once its volume is
gone.
export_backups and discover_backups return the manifests themselves rather than
dicts; whoever writes them out decides how they are rendered. import_backups
takes manifests too, so "is this a manifest at all" is answered by whoever read
the bytes -- the API by its request body's type, the CLI when it parses the file
-- and reported against the thing the operator actually supplied.
export_backups also stops emitting 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.
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.
POST /backups/import takes a union of two bodies rather than one model with two
optional fields and a validator forbidding both-or-neither: pydantic then rejects
a malformed request itself, the manifests are typed on the way in, and the OpenAPI
schema says "one of these two" instead of "everything optional, good luck".
Status codes: an unreadable bucket is 400, not 502 -- nothing here proxies for
S3, and the bucket named in the request is the only thing that can be wrong from
this side. PreconditionError is no longer caught locally and turned into 409;
app.py maps it to 400 for the whole API, and a second, disagreeing mapping in one
router only made the API inconsistent with itself.
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.
build_manifest's docstring now records where Backup and BackupManifest overlap
without justification, and where the overlap is already wrong:
dataplane.cluster_size is recomputed from the current cluster, so re-exporting an
imported backup restamps it with the importing cluster's page size. Nothing reads
it yet. Collapsing the two documents is left as its own change.
Tests: manifest schema handling and chain derivation are 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.
The import endpoint's union, its 400s and the absence of a local 409 are covered
in the v2 endpoint tests. 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.
An encrypted volume's backup is ciphertext in a bucket whose key lives in a KMS. Nothing recorded which KMS, at what path, or under which key -- the dependency was implicit, and only discovered during a recovery. Worse, for an imported backup `encrypted` was always False, so restoring one produced a plaintext volume over ciphertext, silently. A backup now carries an `encryption` document holding a key descriptor: which backend held the key, which Vault and mounts where that applies, and at what path. Never key material. Restore resolves the key from it before creating the volume, so a restore that cannot decrypt fails leaving nothing behind, and the error names the path, the cluster and the KMS -- an operator mid-recovery needs to know what is missing. The working assumption is that a KMS is recoverable independently of the cluster that used it: a Vault deployment outlives one cluster, and the FoundationDB behind LocalKMS is itself backed up. So recording the dependency is enough, and no key material has to travel with the ciphertext. This is why the descriptor is a document rather than loose fields: a scheme that wraps the keys under an operator-held secret adds a sibling field to Encryption and a branch in _resolve_crypto_key, and touches nothing else -- including no change to what is already written in any bucket, since a reader treats an absent field as absent. Invalid combinations are unrepresentable rather than checked at each use. `kms` is a Literal, so a manifest naming a backend this build does not implement is refused instead of read with the wrong fields meaning something; the Vault mounts are Optional and absent for the local backend rather than ""; and an Encryption validator requires a descriptor exactly when `encrypted` is set, so "encrypted, key location unknown" cannot be constructed at all. 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. An unreachable KMS is a RuntimeError, not a PreconditionError: there is no condition the caller could have checked to avoid it. A backup that records nothing at all about its key stays a PreconditionError, since that is a property of the request's target rather than a failure.
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.
The rules are predicates:
chain_fits(length) the data plane copies the decoded array into a
fixed 40-element stack buffer
(vbdev_lvol_rpc.c), so beyond
BACKUP_MAX_CHAIN_LENGTH it smashes the storage
node's stack. Refusing here is the only guard
until those buffers are sized properly.
location_holds_backups(loc) 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}.
chain_is_coherent(...) 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.
They return booleans, so a rule can answer a question as well as block an
operation -- "can this bucket hold backups" is a thing a caller may want to know
without being refused. require_restorable is the one place that turns a false
answer into a PreconditionError, so the wording an operator sees is written once
rather than at each of the three entry points that enforce the same three rules.
Applied at backup creation before the chain lock, before any KMS key and before
any task; at restore before the volume is created, so a doomed restore leaves no
half-built volume behind; and at import before the first record is written.
Import needs one rule of its own, so it has its own gate. Whether every ancestor
is either in this batch or already in the database is a question only the importer
can ask, and the answer is what stops an import from landing a delta whose
ancestors are missing -- something that looks restorable in `backup list` and
fails only when tried, typically during the recovery it was meant to serve. It
walks prev_backup_id through the batch, continues into the database when a link
lands on a record already there (so existing ancestry counts towards the length
the data plane has to accept), and refuses a cycle rather than looping on it.
Tests assert the absence of side effects, not just the error: no Backup record,
no task, no chain lock, no volume. The predicates are also tested directly, at
their boundaries. 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.
On the exceptions here: refusing a foreign bucket with no credentials is a
PreconditionError in its intended sense -- the caller could have supplied them,
and the message says so. The broad `except Exception` around the device teardown
is deliberate and logged rather than re-raised, per the reason above; it replaces
a `(RPCException, Exception)` tuple that needed a noqa to silence the redundancy
it introduced.
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.
Backup.source_cluster_id goes too, along with BackupDTO.source_cluster_id and
the "Source" column in `backup list`. Every remaining reader was either the
switch itself or a listing built to explain it: the field was written at
creation as a copy of cluster_id, and on import as the manifest's source. Which
means it recorded something real in exactly one case -- an imported backup -- and
what it recorded is already in the manifest that import read.
The cost of removing it is one narrow regression, and it is the same one already
noted on build_manifest: re-exporting an imported backup now stamps `source` with
the importing cluster rather than the originating one, because the record keeps
nothing from the manifest it was imported from. Nothing reads that field, and the
manifests in a bucket -- which is what a recovery actually reads -- are written
once, by the cluster that made the backup, and stay correct. Fixing it properly
means storing the manifest on the record, which is the same change that fixes
dataplane.cluster_size.
The guard test that asserted nothing resolves a cluster through the field goes
with the field. It was defending against reintroducing
get_cluster_by_id(backup.source_cluster_id); with no such attribute, that is now
a NameError rather than a subtle dependency on a dead cluster.
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 process-global and 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. A device now
holds a share of a t_sdk_session, whose constructor calls InitAPI and whose
destructor calls ShutdownAPI, handed out through a weak_ptr so there is at most
one alive at a time.
A shared_ptr rather than a counter because the pairing then holds by
construction rather than by remembering to write the other half. It survives the
paths a counter would leak on: try_shutdown_client refuses while requests are
still pending, ~t_disk runs whether or not that was ever called, and init_client
can throw partway through building its two clients. Keeping the SDKOptions inside
the session also means InitAPI and ShutdownAPI are handed the same object, whose
lifetime brackets both calls exactly -- as two statics they merely happened to.
The member is declared ahead of the clients so it is destroyed after them.
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 BUILT, but syntax and type checked. A full build needs SPDK configured and
simplyblock's aws-sdk-cpp fork, neither of which exists in this monorepo (see
CLAUDE.md). `-fsyntax-only` against the *upstream* SDK does work, and reports no
errors for this translation unit, with the same 102 -Wall -Wextra warnings as
before the change -- all of them pre-existing. The recipe, its two necessary
shims and what it does not prove are recorded in COMPILE_CHECK.md later in this
series. It does not link and does not run, so it says nothing about whether a
transfer completes or whether the SDK ends up initialised the right number of
times at runtime; that still needs a real build in the ultra workspace against
the forked SDK.
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.
Sizing those buffers to 255 also retires the reason the control plane's
BACKUP_MAX_CHAIN_LENGTH was 40, so its comment and chain_fits are corrected here
rather than left justifying a bound that no longer exists. The limit stays at 40,
now as a policy: a restore reads every backup in the chain in one operation, so
chain length multiplies restore time and the objects a recovery has to fetch.
Raising it is safe up to 255 and needs no data-plane change.
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
--secret-access-key … 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. Its file is parsed into manifests here rather than in
the controller, so a malformed export is reported against the file, by name.
The discover listing works from the manifest models the controller hands back. It
reports chain length by walking prev_backup_id over the set rather than reading a
stored chain, and prints "incomplete" for a backup whose ancestor is missing from
the bucket rather than refusing the whole listing -- an incomplete chain is the
finding an operator is looking for, not an error. It names the KMS an encrypted
backup depends on, which is what decides whether a recovery can proceed.
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.
secondary_target, with_compression and snapshot_backups lose their defaults
entirely: they are decisions the caller has already made, and defaulting them
invites the wrong one silently. `region` goes the other way and joins the
optional group, because the data plane leaves its SDK config untouched when it is
absent (`if (region && *region)`) and its RPC decoder marks the field optional --
so it behaves exactly like the credentials, and requiring it here would have been
this layer inventing a constraint the layer below does not have.
The S3 RPC surface is annotated, and the node parameters of the functions that
drive it with it. Between them they close a hole: `node` was implicitly Any, so
`node.rpc_client()` was Any too and every argument to every S3 RPC went
unchecked, however carefully bdev_s3_create itself was typed. With both ends
annotated, mypy now rejects a str where a list is wanted, or an Optional where a
str is -- which is the class of mistake that made region look mandatory in the
first place.
_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".
backup_controller.py had grown to ~1400 lines holding five separable concerns,
and backup_manifest.py sat at simplyblock_core top level despite being backup's
alone. Both are now one package, in dependency order:
manifest the self-describing record written into the bucket, and the only
thing that can interpret a backup's objects once the cluster that
wrote them is gone. Owns the control plane's S3 access.
validation whether a chain can be restored: the predicates, plus the one
function that refuses.
device the S3 devices a node reads and writes through, one bucket each,
and the names they get.
controller creating, restoring, importing, exporting, discovering.
policy retention limits, tiered schedules, and the merges they cause.
Each module may import the ones above it and none below, so the graph is a DAG:
device and controller reach manifest, controller reaches validation, policy
reaches controller. Nothing reaches back.
Pure movement -- no function body is changed. The split was done by slicing the
file at top-level definition boundaries and reassembling, so nothing was retyped
and nothing can have drifted from what was reviewed in the preceding commits.
Two names lose their leading underscore, create_single_backup and
get_latest_backup_for_lvol, because policy legitimately calls them and an
underscore means private to a module rather than to a package.
__init__.py deliberately re-exports nothing. A caller writes
from simplyblock_core.controllers.backup import controller as backup_controller
so the import says which part of the subsystem it depends on -- and the callers
show the split was worth making: cluster_ops and storage_node_ops need only
device, tasks_runner_backup_merge only policy, and the v2 router and the CLI
turn out to use controller and policy as two separate things. Flattening that
back into one namespace through the package would undo the reason for splitting
it.
Every reference is updated, including the patch targets in the tests, which have
to follow each function to the module it now lives in. tests/unit/test_imports.py
imports all five modules, so a circular import between them fails the fast tier.
The manifest's volume shape was carried as untyped values: allowed_hosts as bare dicts, ha_type and fabric as free strings, the Vault base URL as a string next to an endpoint that was already an HttpUrl. They now have the types they always had -- Literals for the two enumerations, HttpUrl for the URL -- and both Volume and KeyDescriptor are built in one validated construction rather than by model_copy(update=), which does not validate and would have let an unrecognised ha_type or an unparsed URL into a document nobody reads until a recovery. allowed_hosts also carried each host's DHCHAP keys and PSK. Nothing reads them: restore passes the NQNs to add_lvol_ha, which mints fresh keys from the target pool, and for a DHCHAP pool discards the list entirely in favour of the pool's own. Publishing them therefore put the volume's NVMe authentication material into the bucket in the clear -- the one thing this document promises not to carry -- and BackupDTO handed the same entries to anyone permitted to list backups, while LVolDTO had always projected that field down to NQNs. The manifest, the Backup record and the DTO now hold NQNs alone. A manifest written with the old shape is read for the NQNs it also holds rather than refused, and the key material is dropped on read, so the next write of that manifest stops republishing it. Backups taken before this still hold the keys on their records and in objects already sitting in a bucket; nothing sweeps those. An NQN is now a type. simplyblock_core.utils.NQN wraps the existing NQN_PATTERN, and simplyblock_web.api.v2.util re-exports it so API models find it beside Unsigned and Size while the manifest imports it from core -- one definition for both layers. Every field holding an NQN is declared with it, which replaces three hand-spelled Annotated forms and one use of the duplicate pattern in simplyblock_web.utils. The volume-create and add-host request bodies were not validated at all before, so the CLI's --allowed-hosts file is the only remaining door: it checks the NQNs it reads before calling add_lvol_ha. That leaves the format enforced at each interface rather than in the controllers, which is the argument for a real NQN value object later; the alias is the cheap stand-in. Finally, BackupLocation no longer accepts "" as a region or endpoint. The untyped dicts written before the model used empty strings for "unset", the model says None, and an empty endpoint would have failed HttpUrl validation outright. The legacy migrator drops them before the local_testing defaults run, so a config stored that way still resolves its region.
A local KMS has no key-encryption key: LocalKMS stores its DEKs as they are, its KEK operations are no-ops, and it ignores the kek_name argument entirely. So a required kek_name on KeyDescriptor made every local backup record a name that describes nothing. It moves in with the other Vault-only fields, required there by a validator because a Vault-held DEK cannot be unwrapped without naming the transit key that wraps it. Also stop _resolve_crypto_key from leaking the ValidationError that the Encryption validator now raises for an encrypted backup with no key recorded -- the case the function is documented to refuse with PreconditionError, and whose explicit branch had become unreachable. The tests kept the pre-typing shapes: a manifest with a flat `encrypted` flag and no `encryption` document, descriptors without the fields the model requires, and an expectation that a location without a region is refused, which this commit deliberately made valid.
A chain reaching back past the batch being imported spans both shapes of the same thing: the manifests being imported and the Backup records already stored. They name their id differently (backup_id against uuid) and hold their location differently (a BackupLocation value against the raw dict it was stored as), so a chain that reached into the database compared a model to a dict, always found them divergent, and then died with AttributeError while formatting the refusal. Walking (id, location) pairs instead: the two facts the rules here need, and the only shape both forms share.
Member
|
A PR on the operator side: simplyblock/simplyblock-operator#441 depends on these changes. Adding a comment so that I get notified when the PR is merged. |
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.
No description provided.