atelet: live refresh of projected trust bundles for running actors - #1231
atelet: live refresh of projected trust bundles for running actors#1231Max Thompson (thompsonmax) wants to merge 15 commits into
Conversation
A trustBundle projection was only resolved at Run/Restore, so a running actor kept the bundle as of its last start until its next suspend/resume cycle, and rotations relied entirely on overlap windows outlasting every actor's run. Hang live refresh off the ClusterTrustBundle informer atelet already runs: Run/Restore registers the actor's projections once its volumes are written, bundle events rewrite the projected files of every registered actor with the same temp-and-rename discipline (paths never move, readers see old or new contents, never partial), and Checkpoint/ Terminate deregister when the sandbox goes down. This matches kubelet's live-update semantics for clusterTrustBundle projections. Refresh keeps last-good contents on failure: an unreadable, deleted, or unusable bundle leaves every projected file as it was — fail-closed stays an actor-start behavior, a running actor never sees its bundle go missing or empty. Files are rewritten only when the raw backing contents actually changed; the comparison hashes the informer input, not the projected output, because sanitization shuffles anchors. Informer relists and atelet restarts therefore cause no inode churn, which matters because a suspended guest re-binds recorded paths on resume. Registrations survive atelet restarts through a per-actor record kept beside the volume roots and wiped with them by resetActorDirs; recovery re-syncs each recovered projection against the current bundle, applying any rotation missed while atelet was down. The identity e2e now rotates the pool under two RUNNING actors on both sandbox classes and waits for each to observe the new contents live, then keeps the existing rotate+suspend/resume cycle without waiting for that second rotation to propagate — the resume must deliver the rotated contents whether or not the live rewrite won the race with the suspend.
A failed resume in the identity suite left nothing to diagnose with: the probe fixture's cleanup deleted its namespace even when the test failed, taking the worker pods whose logs hold the ateom, guest console, and virtiofsd output; and the cloud-hypervisor client surfaced only the vm.restore status line, discarding the response body that names which device failed to restore. Keep the probe fixture when the test fails, exactly as CreateNamespace already retains failed tests' namespaces, and read the vm.restore response body into the error.
Under the default abort, a snapshot whose serialized FUSE state holds an inode with no findable path can never be restored: the destination virtiofsd rejects the whole device state and cloud-hypervisor fails vm.restore, leaving the actor permanently unresumable. Live trust-bundle refresh makes that state routine — the rewrite replaces the projected file's inode, and a guest that read the old file and suspends before re-reading still holds a dcache reference to the unlinked inode; no open fd is needed. Guest-created unlinked temp files (tmpfile, O_TMPFILE) held across a suspend hit the same wall. With guest-error the restore succeeds and only the stale reference is faulty: access through it returns EIO, and a fresh lookup at the stable path heals it — matching the documented reader contract (the platform rewrites the file, applications re-read it). The flag is a property of the backend process, not of the VM configuration, so existing snapshots restore unchanged. The trade-off is that a share-reconstruction bug now surfaces after resume, as guest EIO and a readyz failure, instead of failing the restore outright. virtiofsd names the inodes it marks faulty in its log, which lands in the worker pod output. Flagged as needing this decision when the vendored AtomicWriter was removed; the identity suite's rotate-under-a-running-actor cycle is the reproducer and stays as the regression gate.
prepareOCIBundles is the AteomHerder method that writes the projections, and the refresher already hangs off the same receiver, so registering there removes the projection return value and the threading through Run's and Restore's call sites. The RPC handlers keep only the deregister-on-error defer, installed before the prepare call so a failed start leaves no registration behind. Also trims the live-refresh wiring comment in main and drops a redundant comment in Terminate.
…ments resolveTrustBundle returned (pem, hash, error), leaving callers to know which string is which; bundle both into a resolvedTrustBundle with named fields instead. The comment sweep shortens the larger refresher comments and updates the ones written when --migration-on-error was still abort: a gratuitous inode replacement no longer fails a later restore, it turns guest references faulty, and the comments now say that.
Check the vm.restore status line with a string prefix, the way AddNetWithFDs beside it already does, and drop an issue reference from an identity-test comment.
The refresher's comments promised that relists retry failed rewrites, but no such path existed: the informer factory had resync 0, and a relist's sync notifications never reach a non-syncing handler, so a projection whose rewrite failed transiently stayed on pre-rotation anchors until the bundle changed again or atelet restarted. Give the ClusterTrustBundle factory a real resync; the periodic replay re-drives exactly the stale projections and the AppliedHash compare makes it a no-op otherwise. Covered by a write-failure isolation-and-retry test. Also deregister in Terminate only after TerminateWorkload confirms the sandbox is down, matching Checkpoint: a Terminate that fails before teardown leaves the actor running, and it must keep refreshing until the retry succeeds. Comment corrections: the projections record IS served read-only into the micro-VM guest's shared tree (it is not mounted into containers), and a record recovered for an actor that never returns is only cleared by a later lifecycle RPC for that actor.
waitForTrust polls up to two minutes through a single long-lived port-forward, but each iteration used the fatal whoami helper, so one dropped connection or transient router error mid-poll failed the whole test. Split out a non-fatal tryWhoami, poll with it, and report the last error at the deadline, matching the suite's other wait helpers. Also label the probe fixture's namespace with ate.dev/e2e: retention on failure only works with the cleanup sweep that reclaims labeled namespaces, which the fixture's hand-written manifest predates.
…rrors The virtiofsd flag comment justified guest-error only via system-info rewrites and unlinked temp files, but the mode is per-daemon and the one share also serves rootfs and durable-dir subtrees, where a genuinely missing path now surfaces as guest EIO instead of failing vm.restore — say so, and update systeminfo.go's claim that projected files are only rewritten while virtiofsd is down. AddNetWithFDs now reads the response body into its error the way RestoreWithNetFDs does; a status line alone says only 500.
The single mutex covering projection writes is what makes Deregister return only after an in-flight refresh finishes, so Checkpoint and Terminate can wipe the actor's directories immediately after. Any finer-grained locking must keep that property.
| // guest-error, not the default abort: the serialized FUSE state can | ||
| // reference inodes with no findable path — a system-info file | ||
| // live-rewritten under the guest (trust-bundle rotation), or a | ||
| // guest-unlinked temp file held open across the suspend — and abort | ||
| // would make such snapshots permanently unrestorable. Under | ||
| // guest-error the stale reference degrades to EIO on access, and a | ||
| // fresh lookup at the stable path heals it. The mode is daemon-wide: | ||
| // a path genuinely missing elsewhere in the share (a rootfs or | ||
| // durable-dir reconstruction gap) also surfaces as guest EIO instead | ||
| // of failing vm.restore; virtiofsd's log names the inodes it could | ||
| // not re-establish. |
There was a problem hiding this comment.
nit: can you refresh your session(e.g. do a developer reload in vscode or claude --resume or ...) and ask your agent to apply the current AGENTS.md rules? I'm pretty sure it should write a terser comment following #1177
(also, do you have any idea if that commit was in the branch when you started? if it's not taking effect we need to iterate)
some of these agents have been defaulting to excessively verbose code comments, we don't really need 11 lines checked in to justify this flag.
There was a problem hiding this comment.
Hmm, seems the AGENTS.md was in fact loaded in my session. It's possible it was pushed too far back in the context window when this huge comment was generated. Or perhaps we need to update AGENTS.md to be a bit stricter with its rules around comments.
I also have this in my CLAUDE.md (but maybe it's the same issue of it got pushed too deep in the ctx window):
## Code comments
Write sparse comments. A comment must say something the code cannot: a
constraint, an invariant, a non-obvious why. Default to 1–3 lines; if a
comment needs a second paragraph, the content probably belongs in a doc
comment, an issue, or the commit message. Never write comments that:
- restate what the adjacent code does
- justify the change to a reviewer ("per the review", "as discussed in the
PR") — that context dies when the PR merges
- explain the same fact in more than one place; pick the site a reader hits
first and cross-reference from the others
- give background essays for hypothetical questions nobody has asked
Match the comment density and tone of the surrounding file. When editing
existing code, do not add comments narrating the edit.
At least for this PR, I refreshed that guidance in my session, which seems to have trimmed down most of the unnecessarily long comments.
There was a problem hiding this comment.
Interesting, thank you.
[Also explicit +1 for the flag]
The virtiofsd flag comment carried the whole design rationale, which belongs to (and already lives in) its commit message; the checked-in comment now states only what the mode must not silently revert to. Shrink a few neighboring refresher comments the same way.
| // regenerations: restore re-binds suspend-time guest state to files by | ||
| // recorded path (virtiofsd find-paths, gVisor's gofer), so a scheme that | ||
| // relocates files on write (kubelet's atomic writer) would leave that state | ||
| // dangling. Per-file write-to-temp-and-rename is atomic enough — a reader |
There was a problem hiding this comment.
Is this still true with the guest-error flag change?
Atomicwriter lets kubelet simplify the refresh logic somewhat --- first Kubelet builds a model of all the content that should be in the projected volume and then atomically applies it.
There was a problem hiding this comment.
Yeah you're right, we don't have this restriction any more. Since we don't need multi-file atomic updates yet though, I'd prefer to keep the simple temp + rename updates to keep the PR simple, but feel free to push back if there's something AtomicWriter gives us that would be helpful now.
Left a TODO to switch to AtomicWriter once we need multi-file atomic updates.
| // rather than in the CRD schema so a configurable backend registry (#932) | ||
| // can widen it without a template API change. | ||
| var supportedTrustBundles = map[string]string{ | ||
| EgressTrustBundleName: "egress-mitm.ate.dev:mitm:primary-bundle", |
There was a problem hiding this comment.
Technically speaking, the way you are supposed to use a signer-linked cluster trust bundle is by using a combination of a selector on the signer name and a label selector. This is something to keep in mind for the future, since it will let us canary changes to a fraction of workloads.
If everyone is just reading one direct ClusterTrustBundle, then there's no way to trial adding a new root for n% of workloads.
| return nil, fmt.Errorf("trust bundle %q is not supported by this deployment (supported: %s)", name, supportedTrustBundleNames()) | ||
| return "", "", fmt.Errorf("trust bundle %q is not supported by this deployment (supported: %s)", name, supportedTrustBundleNames()) | ||
| } | ||
| if lister == nil { |
There was a problem hiding this comment.
Is this something that can actually happen in practice? If not, it would be better to just panic on the nil access.
There was a problem hiding this comment.
Good point, removed the check
| } | ||
|
|
||
| // sanitizeTrustBundle sanitizes raw backing contents into the projected PEM. | ||
| func sanitizeTrustBundle(name, objectName, raw string) ([]byte, error) { |
There was a problem hiding this comment.
I don't think this function adds anything over pemutil.SanitizeCertificateBundle?
There was a problem hiding this comment.
Deleted, collectData calls pemutil.SanitizeCertificateBundle directly now
| // lifecycle: wiped by resetActorDirs, never snapshotted. The micro-VM class | ||
| // serves the whole roots dir into the guest read-only, so nothing sensitive | ||
| // belongs here; the dot cannot collide with a volume root (DNS-label names). | ||
| const trustBundleProjectionsFileName = "trust-bundle-projections.json" |
There was a problem hiding this comment.
Let's not persist anything. We tried something like this in gke-spiffe-node-agent and it was a huge pain in the ass. It's OK if an atelet restart changes file contents.
| // with it the refresher's retry cadence: a failed rewrite gets no further | ||
| // event until the bundle changes again, so the periodic replay re-drives it | ||
| // (a no-op otherwise, per the AppliedHash compare). | ||
| const trustBundleResyncPeriod = 10 * time.Minute |
There was a problem hiding this comment.
Resync should be probably be 0 (disabling resync) or 24 hours. Every resync period, the client will do a complete list of all ClusterTrustBundles --- it could drive significant traffic if we set it to 10 minutes.
It's not an appropriate mechanism to redrive failed actions on our side; it is originally intended to be a guard against bugs in the k8s informer libraries.
If we need retries for actions that can fail on our end, we need to use a workqueue. The podcert signer should have an example.
| const trustBundleResyncPeriod = 10 * time.Minute | ||
|
|
||
| // trustBundleProjection is one projected trustBundle file of one actor. | ||
| type trustBundleProjection struct { |
There was a problem hiding this comment.
This will probably be more work, but I think it will be important for future flexibility here: instead of tracking specific files, the refresh logic should re-interpret the volume and volume mount fields of the actor spec.
Right now, you've got the logic ping-ponging between writeSystemInfoVolume, trustBundleRefresher, and writeSystemInfoFile.
Instead, we should have a systemInfoVolumeRefresher with a write() function that knows how to write the complete contents of a system info. The write() function should be called once at actor startup, and then re-called from an timer or informer hook.
| if p.AppliedHash == h { | ||
| continue | ||
| } | ||
| pemBundle, err := sanitizeTrustBundle(p.Bundle, objectName, raw) |
There was a problem hiding this comment.
Here's a litmus test for if this system is factored correctly --- there should only be one call to sanitizeTrustBundle in the source code.
…lier Model the projection machinery on kubelet's projected volumes: collectData builds a volume's complete contents in one place, write applies them, and every lifecycle point goes through the pair. Run/Restore registers the actor's volume specs and writes fail-closed; ClusterTrustBundle events enqueue bundle names that a workqueue-driven run loop applies keep-last-good, requeueing failed writes with backoff. The informer resync drops to a daily safety net against missed events instead of doubling as the retry mechanism. Registrations now live only in memory. The on-disk projection record and its startup recovery are gone: after an atelet restart, an already-running actor's projected bundles keep their last-written contents until its next Run/Restore, whose registration rewrites them from current cluster state. Files whose desired contents already match are left untouched, so a bundle rotation no longer replaces the inodes of a volume's unchanged files, and re-registration of a resuming actor keeps identical files on their original inodes.
The refactor's doc comments ran long; cut each down to the constraint or invariant it carries.
Direct object names cannot canary a new root to a fraction of workloads; the configurable registry should select by signer name and label selector instead.
4 participants
Part of #932 (PR 2 of 3). PR 1 (#941) added the
trustBundleSystemInfo data source, resolved on the node at Run/Restore. This PR keeps those projections current while the actor runs.Live refresh
A
systemInfoVolumeRefresherin atelet, modeled on kubelet's projected volumes:collectDatais the one place that builds a volume's complete contents from its spec,writeapplies them, and every lifecycle point uses the pair. Run/Restore registers the actor's volumes, which writes them fail-closed before the sandbox boots; ClusterTrustBundle events rewrite them while it runs; Checkpoint, Terminate, and failed starts deregister. No API, proto, or RBAC changes: the wire still carries only{name, path}.virtiofsd:
--migration-on-error=guest-errorCI confirmed the hazard (3 of 3 micro-VM runs): a rotation renames over a file whose inode the guest still references (a dcache reference is enough, no held fd), the actor suspends before re-reading, and find-paths serializes an inode with no findable path. Under the default
abort, the destination virtiofsd rejects the device state at vm.restore: suspend succeeds, every restore fails, and the actor is permanently stuck. Full analysis in the PR comments.With
guest-errorthe restore succeeds and only the stale reference is faulty: EIO on access until a fresh lookup at the stable path heals it. That matches the documented reader contract (the platform rewrites the file, applications re-read it) and also covers guest-createdO_TMPFILEs held across suspend, which already fail restores underabort. The flag is backend-process configuration, so existing snapshots (template goldens included) restore unchanged. Trade-off: a share-reconstruction bug now surfaces as post-resume EIO and a readyz failure instead of a loud restore failure; virtiofsd names the faulty inodes in the worker pod log.E2E
The identity suite covers live refresh on both sandbox classes: rotate the pool under two running actors and wait for both to observe the new bundle live; rotate/suspend/resume without waiting for propagation, so the resume must deliver rotated contents whether the rewrite landed before or after the guest went down (the exact
abortrepro); assert a sibling that never cycled also got the rotation live. The probe fixture now keeps its namespace when a test fails so the worker pod logs survive, and the cloud-hypervisor client includes the vm.restore response body in errors.Not in this PR
Auto-injected egress trust volume (#932 PR 3) and the configurable backend registry (tracked in #932).