feat: add systemInfo volume source with actorIdentity data source - #803
feat: add systemInfo volume source with actorIdentity data source#803Max Thompson (thompsonmax) wants to merge 10 commits into
Conversation
Max Thompson (thompsonmax)
changed the title
8a20a2a to
6555c5f
Compare
caad956 to
4c34cdc
Compare
22da91b to
b5f7dfa
Compare
Taahir Ahmed (ahmedtd)
left a comment
Taahir Ahmed (ahmedtd)
left a comment
There was a problem hiding this comment.
Overall this looks good. My main open question is about the virtiofsd handling on kata.
| } | ||
|
|
||
| // SystemInfoVolumeRootsDir is the directory containing the per-volume root | ||
| // directories of system-info volumes. It is deliberately separate from |
There was a problem hiding this comment.
This seems like a non sequitur --- why would keeping the host-side folders in a different folder have any effect on whether or not they end up in snapshots? It would be fine if we kept all of the root folders, for all of the volumes, regardless of their type, in the same folder together.
There was a problem hiding this comment.
So I think the mechanism of how this keeps the SystemInfo data out of snapshots differs between microVM and gVisor:
For MicroVM, we just tar everything in DurableDirVolumeMountsDir and stores that directly in the snapshot as the included data. So keeping SystemInfo out of that prevents it from being included at pause time.
for gVisor, we declare the volumes registered as durable in mount hints here, so gVisor knows to include them in the runsc checkpointing logic. Since we don't declare the SystemInfo volumes in the same way, they aren't incorporated into the snapshot.
My goal is to keep the SystemInfo data from showing up in snapshots (although it may still leak through process memory, but the SystemInfo volume should be the source of truth), since it represents data specific to the system the actor is running on and will become stale after a SUSPEND + RESUME.
| for _, m := range mounts { | ||
| out = append(out, specs.Mount{ | ||
| Destination: m.GetMountPath(), | ||
| Source: kata.GuestSystemInfoVolumeDir(m.GetVolumeName()), |
There was a problem hiding this comment.
OK, I'm missing something here...
Earlier, we start up one virtiofsd to handle all SystemInfo volume mounts. Here we tell CHV which host folder should back the mounted guest folder.
How does CHV know which virtiofsd to talk to to handle this particular mount. Are we telling it that somewhere? Benjamin Elder (@BenTheElder)
There was a problem hiding this comment.
Per volume seperation is just subdirs in the shares, so we create one virtiofs device per virtiofsd instance / "volume type", and then each mount is a subdir under that. The virtiofs devices have a tag / dedicated socket, that part we do specify to chv, but that just passes through each instance in bulk.
Setting up the subdir bind mounts guest side from each of the virtiofs share happens in overlay_linux.go, we do that through the kata guest agent currently.
There was a problem hiding this comment.
Tracing this out, how this works is:
- stageSystemInfoShare starts the third virtiofsd with its shared dir pointing at the host folder (ateompath.SystemInfoVolumeRootsDir(actorUID)), listening on a per-share vhost-user socket. kata.SystemInfoVirtiofsdSocketPath. The host path is only known to that virtiofsd.
- buildFsConfigs is where we tell cloud hypervisor which virtiofsd is which. The VM config (built here gets one FsConfig per share, and the system-info entry is {Tag: SystemInfoFsTag ("ateSystemInfo"), Socket: ...}. cloud hypervisor creates a virtio-fs device bound to that socket, labeled with that tag. So the VM has up to three virtio-fs devices each wired to its own virtiofsd: kataShared , ateDurable, and ateSystemInfo.
- When we create the sandbox, CreateSandboxForActor tells the kata agent to mount Storage{Source: SystemInfoFsTag, MountPoint: /run/ateom-system-info} here. The guest kernel matches that mount to the device by tag. That links the guest folder and the right virtiofsd.
- By the time we get to systemInfoMounts, each volume is just a subdirectory of the share mounted in step 3. These are ordinary guest-side bind mounts into the container.
There was a problem hiding this comment.
Also noting: The main reason we have two instances currently is because one of them is for read-only data [the container image rootfs], which we can avoid repeatedly querying the host for, so we configure it differently.
We might not need a third instance, I haven't had time to read this PR in depth yet, just commenting on this question. #846 also changes this and needs a fresh review (it was ~completely rewritten yesterday, I haven't caught up yet).
| // +kubebuilder:validation:MinItems=1 | ||
| // +kubebuilder:validation:MaxItems=8 | ||
| // +kubebuilder:validation:XValidation:rule="self.all(x, self.exists_one(y, y.field == x.field))",message="items must not project the same field twice" | ||
| // +kubebuilder:validation:XValidation:rule="self.all(x, self.exists_one(y, y.path == x.path))",message="items must not contain duplicate paths" |
There was a problem hiding this comment.
We are going to want this validation to cover all the data sources, since they are all part of one mounted volume. I don't know if we will be able to do that with the built-in CEL validation --- maybe it can be done with object-level CEL validation.
But anyways, this is going to migrate into the Ate API layer, so I guess we will just run validation in Go code there.
There was a problem hiding this comment.
Yeah good point. I think we can enumerate type pairs once we have 2 data sources, but that will get really ugly fast as we expand the number of sources.
Is there an issue tracking moving this validating into Ate API? I'd prefer to do that sooner rather than later to avoid having to add/maintain CEL hacks to support this validation across multiple data sources.
|
Max Thompson (@thompsonmax) , who are the intended consumers of |
|
Hi haiyanmeng, the intended use case is similar to https://kubernetes.io/docs/concepts/workloads/pods/downward-api/ in k8s, basically actors that want to know their own identity for e.g. DNS self address, self-labeling when writing metrics, logs, or other telemetry, etc. Immediate consumers will be anyone using |
- third_party/atomicwriter: correct the copy-vs-import rationale in the README (upstream is importable; the dependency tree it drags in is why we copy) and trim the justification down. - atelet: TODO(agent-substrate#802) noting rotating data sources (JWTs, certificates) will need system-info files refreshed mid-run; actorMetadata never changes after start, so Run/Restore-time writes suffice for it. - ateompath: document how each sandbox class keeps system-info out of snapshots — the micro-VM checkpoint tars DurableDirVolumeMountsDir wholesale (capture by location), while gVisor captures durable mounts by declaration and never declares system-info mounts. - ateom-microvm: trim the teardown comment.
b5f7dfa to
88344c1
Compare
|
keep in mind: virtiofsd resume runs in |
|
That's not the right phrasing: I'm reasonably sure it's not safe, the timestamping bit ... |
Can you elaborate? Are you saying that the content of the host folder needs to be exactly the same across snapshot/restore? |
|
So on restore, it will find the guest's inodes again by matching the file paths. Any paths in the guest should be present at the same path again on restore, IIRC restore will just hard-fail otherwise. AFAIK the contents of the files don't need to be identical, though that's not something we've been doing currently (we're only modifying things while the guest is live). |
Review feedback on agent-substrate#803 (find-paths safety): every virtiofsd runs with --migration-mode find-paths, which re-binds the guest's FUSE state on restore by re-opening the paths recorded at suspend — and gVisor's gofer re-opens by path the same way. The kubelet atomic writer breaks that contract: it serves files through a symlink into a timestamped payload directory, so every regeneration moves the real paths and deletes the old ones, and a restore of any guest that touched a system-info file would fail to re-bind (reproduced in TestWriteSystemInfoVolume_StableRealPaths, which fails under the old layout). Write plain files via per-file write-to-temp-and-rename instead (writeFileAtomic). Whole-set atomicity is unnecessary: generation only runs while the sandbox is down, so no reader can observe a partial write. Contents may change across a restore (that is the feature); paths never move. Path cleanliness is validated defensively in atelet since the atomic writer's checks are gone with it. Drop the now-unused third_party/atomicwriter package. The probe fixture now opens the identity file at startup and holds the fd across checkpoints, and the identity e2e asserts a post-restore read through that fd yields the restored actor's own id — the guest-handle re-binding scenario that would have caught this.
Got it, thanks Benjamin Elder (@BenTheElder) for the heads up on that. I reworked the approach to not use AtomicWriter and instead use a tmp file + rename to perform the file writes to keep it atomic per-file while maintaining the same file paths on the host once we restore that were present at suspend. I don't think we need the multi-file atomic updates that AtomicWriter provides actually, even with later sources we plan to add , we'll only need to update single files atomically (tokens, certs, trust bundles). Added regression testing in both unit + e2e testing. |
This commit defines a new volume type, SystemInfoVolume, that will serve a similar purpose as Projected volumes in Kubernetes. It will support writing information from multiple sources to automatically-updating files in the Actor's filesystem. For a first pass, I have converted the existing hardcoded Actor ID file to be one of the available information sources in a SystemInfoVolume. Further work will add Actor Identity JWTs and Actor Identity certificates.
Complete the initial actorIdentity data source support: - e2e: declare a systemInfo volume in the identity probe's ActorTemplate, mounted at /run/ate, replacing the removed automatic identity mount so the restore-identity regression gate exercises the new API. - Validate actorIdentity paths at admission: must be a clean relative Unix path (no absolute paths, '..', '.', '//', ':', or control characters), and paths must be unique within a volume. Previously bad paths were only rejected by the atomic writer at Run/Restore time. - Unit tests for the ateapi systemInfo conversion and for atelet's system-info volume population (extracted into writeSystemInfoVolume). - Update the stale micro-VM known-gap comment to reference systemInfo volumes instead of the removed /run/ate identity mount.
Per the API discussion on agent-substrate#802: substrate has no "actor ID" concept -- resource identity is (atespace, name) plus a server-generated UID. Replace the actorIdentity data source with an actorMetadata source that projects each identity field to its own file, downwardAPI-style: systemInfo: dataSources: - actorMetadata: items: - field: name # enum: name | atespace | uid path: actor-name - CRD: ActorMetadataDataSource with a field enum and per-item path; admission validation for unknown fields, duplicate fields, duplicate paths, and non-clean/absolute paths; at most one actorMetadata entry per volume keeps paths unique volume-wide. - atelet proto: ActorMetadataDataSource/ActorMetadataItem with a field enum; ateapi converts CRD items to wire items. - atelet: writeSystemInfoVolume projects name/atespace/uid from the Run/Restore request; unknown fields (newer ateapi) are skipped rather than written empty. - e2e: the identity probe projects and serves all three fields; the suite now also asserts atespace matches and the projected UID equals the control plane's authoritative UID per actor, distinct across actors seeded from the same snapshot. - docs: api-guide section rewritten for actorMetadata. This also frees the "identity" naming for the planned credential data sources (actorIdentityToken, actorIdentityCertificate), which relate to the existing ateapi.ActorIdentity service.
SystemInfo volumes were gVisor-only; per the agent-substrate#802 discussion, micro-VM support lands with Part 1 rather than as a follow-up. The mechanism mirrors the durable-dir share: - ateom proto: containers carry system_info_volume_mounts (volume name + mount path), populated by atelet's buildAteomWorkloadSpec. - ateom-microvm serves ateompath.SystemInfoVolumeRootsDir(actorUID) over a third virtiofsd (cache=auto: atelet rewrites the contents underneath the guest on every restore). The agent mounts the share at sandbox creation, and each declaring container gets a READ-ONLY bind from the share's per-volume subdirectory to its declared mount path. - Restore restarts the share's virtiofsd and rewrites its vhost-user socket in the snapshot's VM config (matched by fs tag). Nothing is restored from the snapshot itself: atelet has already regenerated the files with the resumed actor's values, which is the point of system-info volumes. - Checkpoint deliberately ignores the share: the volume roots live outside the durable-dir tree precisely so the durable tar can never capture generated identity data. - Replace the stale "KNOWN GAP" comment in spec.go: dropping host-path binds in the kata spec shaper is fine because volumes reach micro-VM containers via the shares, not spec.Mounts.
Add a README pinning the upstream source (k8s.io/kubernetes pkg/volume/util, delta verified against kubernetes/kubernetes@52ba9013) and enumerating every class of local modification, plus maintenance rules (mechanical adaptations only in upstream-derived files; behavioral changes go in substrate-owned files) and a re-sync procedure. Mark each copied file with a greppable '// substrate:' header so the patch surface is discoverable without diffing against upstream.
- third_party/atomicwriter: correct the copy-vs-import rationale in the README (upstream is importable; the dependency tree it drags in is why we copy) and trim the justification down. - atelet: TODO(agent-substrate#802) noting rotating data sources (JWTs, certificates) will need system-info files refreshed mid-run; actorMetadata never changes after start, so Run/Restore-time writes suffice for it. - ateompath: document how each sandbox class keeps system-info out of snapshots — the micro-VM checkpoint tars DurableDirVolumeMountsDir wholesale (capture by location), while gVisor captures durable mounts by declaration and never declares system-info mounts. - ateom-microvm: trim the teardown comment.
Review feedback on agent-substrate#803 (find-paths safety): every virtiofsd runs with --migration-mode find-paths, which re-binds the guest's FUSE state on restore by re-opening the paths recorded at suspend — and gVisor's gofer re-opens by path the same way. The kubelet atomic writer breaks that contract: it serves files through a symlink into a timestamped payload directory, so every regeneration moves the real paths and deletes the old ones, and a restore of any guest that touched a system-info file would fail to re-bind (reproduced in TestWriteSystemInfoVolume_StableRealPaths, which fails under the old layout). Write plain files via per-file write-to-temp-and-rename instead (writeFileAtomic). Whole-set atomicity is unnecessary: generation only runs while the sandbox is down, so no reader can observe a partial write. Contents may change across a restore (that is the feature); paths never move. Path cleanliness is validated defensively in atelet since the atomic writer's checks are gone with it. Drop the now-unused third_party/atomicwriter package. The probe fixture now opens the identity file at startup and holds the fd across checkpoints, and the identity e2e asserts a post-restore read through that fd yields the restored actor's own id — the guest-handle re-binding scenario that would have caught this.
0dcb219 to
eb041fa
Compare
Certificates and private keys require cross-file atomic updates if we support writing the private key and certificate to separate files. |
Can we test this? This doesn't match my mental model of how a virtiofs client should work --- it always needs to be prepared for a host-side file delete, and has no mechanism to block it. |
Ah good point. I think for now the atomic per-file update will suffice (I'd like to avoid making this PR too much more complex). When we introduce support for certificates, I believe we can write them to a temp directory and then use the |
It's fine for now, but atomicwriter grew its complexity by handling all the nasty edge cases. It didn't start out that complex. |
Ugh, it sounds like virtiofsd is just very brittle for snapshot/restore: https://gitlab.com/virtio-fs/virtiofsd/-/blob/main/doc/migration.md?ref_type=heads We should probably run it with |
The demo suite exercises micro-VM suspend/resume and the identity suite exercises system-info volumes with a startup-held fd, but nothing combined them — the exact configuration where the find-paths contract bites (virtiofsd migrates with --migration-mode find-paths and its default --migration-on-error=abort hard-fails a resume whose recorded paths are missing). Add a micro-VM probe fixture in its own namespace and a gated identity test that restores an actor from the golden snapshot, asserts identity through the startup-held fd, then suspends and resumes the same actor and asserts again. The pre-suspend /whoami call deliberately seeds the guest's FUSE state (find-paths records all indexed inodes, not just open fds) so the suspend-time snapshot references every projected file. Gated on the microvm CI job's environment, same as the demo suite.
I added another e2e test to confirm that suspend + resume with SystemInfo works with microVM and also created a throwaway draft PR #1020 to run the test with the old implementation to validate it fails in that case.
IIUC, the case we really need to worry about with this is when we need to rotate mounted credentials, which may delete + create files and overlap in time with a suspend + resume. Should we defer setting that option until we need to handle that or is there a case I'm missing where it is helpful for this PR? |
The probe actors live in the ateapi store and outlive the fixture namespace, so a run that dies before its actor is deletable (DeleteActor requires SUSPENDED or CRASHED, e.g. after a failed restore) leaks the record and wedges every rerun on AlreadyExists. Best-effort suspend+delete any leftover before CreateActor, and log — rather than swallow — a failed delete in cleanup.
|
Just a heads up, it seems the E2E tests are not running in the CI for some reason. But I manually ran the new E2E test I added to validate suspend + resume with a SystemInfo volume on microVM. It fails with the old implementation using k8s atomicwriter, which doesn't preserve file paths across suspend + resume, but passes with the new implementation which does preserve these. https://gist.github.com/thompsonmax/7cbafda5b3df91de13456eb295080fbc |
6 participants
Part of #802 (first PR: the actorIdentity data source; does not close the issue).
What changed and why
Adds a systemInfo volume source to ActorTemplate — a read-only volume whose files are generated by atelet on every Run/Restore, analogous to Kubernetes projected volumes. The initial data source, actorIdentity, writes the actor's own name to a configurable relative path:
Because the files are regenerated before the sandbox starts, they carry the resumed actor's own values regardless of what checkpointed state it boots from — the property the old hardcoded /run/ate identity mount provided, now as an explicit, extensible API that future data sources (identity JWTs, certificates — see #802) can slot into.
Behavior change: the automatic /run/ate/actor-id mount is removed; actors must opt in by declaring the volume (the e2e identity probe in this PR is the reference example).
Reviewer notes:
Checklist