Repository navigation
feat(sandboxd): add runtime catalog and live volume support - #379
minatoaquaMK2 wants to merge 72 commits into
Conversation
minatoaquaMK2
changed the title
minatoaquaMK2
changed the title
Startup without a registry used to clear replica fields and queue their checkpoints for deletion. A failed rewrite followed by a rollback to the registry could then delete a checkpoint a peer had adopted, and the procedure depended on operator preconditions nothing could observe. Without a registry nothing can retire a replica, so live writable additions on such a claim now return a permanent 409 instead, and its checkpoint and remote row stay intact. The expected 503 while a replica retires no longer logs at error level.
Without a registry, expiry routed a claim with replica state through dropReplica, which cannot settle a pending intent and deletes a published replica checkpoint that another node may have adopted. An expired ReplicaPending claim therefore never left the reaper. Expiry now drops rows only when a registry exists, so such claims are destroyed and their checkpoints stay on disk. Live writers also treat an unfinished archive intent as stranded state, since nothing retires it without a registry.
A claim that archived at its deadline on a node without a registry deleted its replica checkpoint, which a peer may still own through the registry row. Without a registry, archive now leaves that checkpoint on disk, matching release and expiry, and the docs say so instead of promising automatic cleanup.
Without a registry, archiving a claim cleared its replica field, so after a wake live writers were admitted while the replica's registry row still let a peer adopt the pre-attach image. The claim now keeps that replica through a local archive, so the 409 still applies after wake and the replica scan can retire it once failover is re-enabled.
Keeping replica fields and checkpoints through a registry-less archive closed one admission gap but broke fencing and row cleanup once archive_failover was re-enabled, and left archive intents uncovered. Running without a registry while claims still carry replica state is an operator precondition violation that sandboxd cannot reason about, so archive keeps its upstream behaviour and the docs state the precondition and what each exit does.
|
Hi @CMGS, while working on the writable half of live attach I ran into an interaction with replica failover that we didn't cover in #300 or #370. I'd like your view before I push the next milestone to this PR. Before this PR, a writable volume could only be attached when a claim starts, and What I have locally right now is deliberately conservative. A live RW addition on a pool with I also looked at what happens if A few other options I considered:
A few things I'd like to check with you:
|
|
thanks, this lines up with what we agreed in #370. answers first:
two more calls from me:
on the shape:
i'll leave a review with inline notes, mostly hot-path cost, guards we can drop, and style. |
CMGS
left a comment
CMGS
left a comment
There was a problem hiding this comment.
inline notes below. a few repo-wide ones:
asl ./...(https://github.com/CMGS/asl) shows new layout findings: split method sets involume_catalog.go,volume.go,claims.go,engine/volume_live.goandconfig.go, a secondvarblock inpool.go, a secondconstblock involume.go, and helpers above tests in six test files. it also wantscmp.Oratvolume_catalog.go:91.- tests are about 4.9k of the 7k added lines, with nine new fake engine wrappers.
fakeEnginealready has err and hook fields. a few more hooks there, plus table tests per area, would cut a lot. - small cuts:
resolveCatalogVolumeandresolveVolumeseach have one prod caller now, so inline them.
| return nil | ||
| } | ||
| rows, seq := slices.AppendSeq(make([]*claimRow, 0, len(s.rows)), maps.Values(s.rows)), s.seq | ||
| var refs map[string]claimRefs |
There was a problem hiding this comment.
this walks every row and builds a refs map on each commit. i benched 5000 claims with a checkpoint on each (a replicating pool): 19 → 5089 allocs, 43 KB → 1.9 MB, about 1.0 ms vs 0.5–0.7 ms per commit on main. the rows already carry their refs, so can the lookups read rows + the last acked set when asked, and leave commit as it was?
| written uint64 | ||
| ackRefs map[string]claimRefs | ||
| attemptRefs map[string]claimRefs | ||
| replaceFile func([]byte) error |
There was a problem hiding this comment.
replaceFile here and catalog.replace / catalog.stat are test seams in prod structs. the existing claims tests make writes fail by pointing the path at a missing dir or a fifo. can these tests do the same, so the fields go?
| if err := m.catalog.admissionError(); err != nil { | ||
| return err | ||
| } | ||
| recovery := m.store.recoveryVolumes() |
There was a problem hiding this comment.
this merges every claim's refs under m.mu for each claim with volumes, so admission cost grows with the node's claim count. a lookup by volume name is enough here.
| m.mu.Unlock() | ||
| return nil, err | ||
| } | ||
| catalog := maps.Clone(m.catalog.entries) |
There was a problem hiding this comment.
the catalog map is copy-on-write: adding and removal clone it, and persist swaps it. so take the map under the lock, no clone needed. same in catalogSnapshot.
| func (m *Manager) removeStaleVM(ctx context.Context, name string, rec types.VMRecord) bool { | ||
| logger := log.WithFunc("pool.removeStaleVM") | ||
| m.mu.Lock() | ||
| m.catalog.unknown++ |
There was a problem hiding this comment.
this fences volume admission on the whole node for each stale vm until it is gone. a stuck rm on a vm with no disks then blocks every volume claim on the node. line 198 also keeps the fence for a running vm with zero disks. can the fence come from inspect per volume, and go node-wide only when inspect fails?
| generation: c.generation + 1, | ||
| runtime: true, | ||
| disk: engine.VolumeSpec{Name: name, Path: spec.Path, DirectIO: spec.DirectIO}, | ||
| acl: newVolumeACL(spec.Tenants, volumeOperatorOnly), |
There was a problem hiding this comment.
per my comment above: empty tenants mean the same as a static entry. same at 152.
| return fmt.Errorf("volume %q directio must be on, off, or auto, got %q", volume.Name, volume.DirectIO) | ||
| } | ||
| for _, tenant := range volume.Tenants { | ||
| if !types.NameRe.MatchString(tenant) { |
There was a problem hiding this comment.
per my comment above: warn and skip a bad name in static config, don't fail startup. keep this strict for runtime registration.
|
|
||
| func (m *Manager) expiryDropsRow(sb *types.Sandbox) bool { | ||
| return m.archiveRowed(sb) || sb.HasReplica() && !m.archivesAtDeadline(sb) | ||
| return m.archiveRowed(sb) || m.archives != nil && sb.HasReplica() && !m.archivesAtDeadline(sb) |
There was a problem hiding this comment.
this is the expiry fix. please move it to its own pr with its repro test.
| if err := m.deleteCkLocked(ctx, ckID); err != nil { | ||
| return err | ||
| unlock := m.lockRec(ckID) | ||
| if _, referenced := m.store.checkpointRefs()[ckID]; referenced { |
There was a problem hiding this comment.
this changes checkpoint cleanup for every pool, not only volume claims. same for the clearReplicaIntent and dropReplica changes in replica.go. please ship them with the expiry fix, each with its repro.
| // exclude remaining device mounts, and confirm host detach. Absence, an earlier | ||
| // external unmount, attach-only requests, read-only state, and errors give no | ||
| // proof. It cannot be reused across calls or certify externally managed writers. | ||
| type VolumeDetachResult struct { |
There was a problem hiding this comment.
our comment budget is one line per doc. something like NormalUnmount reports a clean writable unmount by this call. is enough, the rest goes in the commit message. same for the two-line comments at replica.go:231 and server_http.go:25.
|
some more perf numbers. i ran the same benchmarks on main and this head (5385209), interleaved main/pr/pr/main, 3 runs each, 5000 claims on the node. "with ck" means every claim holds a
the admission row hurts most. those 317 µs run under the cause is one thing: how i'd fix it, without changing the design:
please add these benchmarks to the pr and post main vs head numbers when you fix it. claims with no checkpoints don't show the problem, so bench with every claim holding a checkpoint. |
Why
An external controller needs to register existing disks and attach or detach them on a running sandbox. This implements the scope agreed in #370, replacing that PR's broader managed-disk proposal.
Plan and progress
This draft contains all three implementation stages in one PR, with separate commits:
The PR remains draft. Final-revision Linux VM validation details will be recorded before it is marked ready; historical hardware and model-checking results do not qualify this implementation.
Review order
sandboxd/pool/volume_catalog.go, registration routes, and claim recovery references.sandboxd/pool/volume_mutation.go, engine detach/dirty-marker handling, and both SDKs.docs/volume-controller.md, and the Python controller example.Compatibility and boundaries
replicate_seconds: 0. Existing or pending replicas and this claim's recovery checkpoint references must retire first. Initial claim-time writers, retained writer repair/removal and RO live changes remain supported. Disabling archive failover with retained state has explicit transition requirements indocs/deploy.md.data_dir/readers,data_dir/goldensordata_dir/fork-*from cleanup. The registration API and controller guide state this requirement.