[DO NOT MERGE] Negative proof for #803: atomicwriter breaks micro-VM resume of actors with system-info volumes - #1020
Draft
Max Thompson (thompsonmax) wants to merge 10 commits into
Conversation
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.
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.
…truments Reverts the stable-paths materialization (eb041fa) ONLY for atelet: system-info files are again written via the kubelet atomic writer's timestamped-directory symlink swap. Deliberately KEEPS the held-fd probe, the identity-suite Held assertion, and the micro-VM suspend/resume test, and drops the stable-paths unit test so CI reaches the e2e stage. Expected result on the micro-VM e2e job: TestActorIdentity_Microvm_SuspendResume_StablePaths fails — the resume aborts (virtiofsd --migration-mode find-paths with default --migration-on-error=abort cannot re-open the suspend-time timestamped payload paths that regeneration deleted) or the held-fd read errors. The gVisor identity suite's Held assertion may fail for the analogous gofer path reason. This branch exists to document the failure mode for PR agent-substrate#803's review thread; it must never merge.
4 tasks
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.
CI-only demonstration branch for the find-paths discussion on #803 ([Ben's comment]). This is #803's branch plus one commit that restores the kubelet atomic-writer materialization of system-info files while keeping the held-fd probe and the micro-VM suspend/resume e2e added there (and dropping the stable-paths unit test so the pipeline reaches e2e).
Expected: TestActorIdentity_Microvm_SuspendResume_StablePaths fails — the resume aborts because virtiofsd (--migration-mode find-paths, default --migration-on-error=abort) cannot re-open the suspend-time timestamped payload paths that regeneration deleted. The gVisor identity suite's held-fd assertion may fail analogously.
Will be closed and the branch deleted once the failing run is linked from #803.