Skip to content

atelet, nodepath: separate per-actor snapshot directories under /var/lib/ate/snapshots - #2746

Merged
Haven Xia (HavenXia) merged 1 commit into
agent-substrate:mainfrom
roycaihw:feat/atelet-separate-snapshots-dir
Oct 21, 2026
Merged

Haven Xia (HavenXia) merged 1 commit into
agent-substrate:mainfrom
roycaihw:feat/atelet-separate-snapshots-dir

Conversation

@roycaihw

@roycaihw Haowei Cai (Roy) (roycaihw) commented Oct 16, 2026 •

Copy link
Copy Markdown
Contributor

Moves each actor's snapshot staging directories (checkpoint-state, restore-state, local-checkpoint) from /var/lib/ate/actors/<uid>/ to /var/lib/ate/snapshots/<uid>/, so a dedicated disk mounted at /var/lib/ate/snapshots keeps heavy guest disk writes from slowing checkpoint and restore.

  • Adds nodepath.SnapshotsDir and ateletpath.ActorSnapshotPath, and moves the three staging directories under it.
  • removeActorDirs removes both trees on Terminate.
  • atelet and objectstore-plugin mount /var/lib/ate with mountPropagation: HostToContainer, as worker pods already do, so they see a disk mounted there after they start.
  • With that propagation atelet also sees external volumes a failed Run or Restore left published, so Run and Restore leave the volume mount points in place when they reset the actor's dirs and publish again over them (NodePublishVolume is idempotent). Checkpoint and Terminate still unpublish first and then remove them.
  • The ActorDirs comment notes that checkpoint_dir and restore_dir may be on a different filesystem from root_dir.
  • docs/node-storage.md documents the mount point.
  • The shared snapshot cache from atelet: serve shared snapshot restores from the node-local file cache #2796 stays under /var/lib/ate on the boot disk. With a dedicated snapshot disk its hard links into restore-state cross mounts, so gVisor restores from a shared snapshot fall back to copying the cached files (no second download); micro-VM restores already copy. The doc notes this, and the cache's comment and cross-mount warning now point at SnapshotsDir.
  • Removes copyLocalCheckpoint and its helpers, which had no caller outside tests.

Breaking change

atelet looks for local pause snapshots only under the new path, so an actor paused by the previous release whose snapshot was not uploaded yet cannot resume after an in-place upgrade. Before upgrading atelet, resume or suspend paused actors; kubectl ate revert actor recovers one that was missed. The old staging directories of such an actor stay under /var/lib/ate/actors/<uid> until it is terminated. Custom manifests that mount a disk under /var/lib/ate after atelet starts also need mountPropagation: HostToContainer on the /var/lib/ate mounts of atelet and objectstore-plugin.

How I tested it

  • go test -race ./internal/nodepath/... ./cmd/atelet/... ./cmd/ateworker-microvm/... ./internal/objectstoreplugin/..., including a local snapshot lifecycle with SnapshotsDir on another mount.

  • On a GKE c3-highmem-192-metal node with a dedicated disk at /var/lib/ate/snapshots, on an earlier revision of this PR (later changes are covered by unit tests): a mount made under it after the pods started was visible in atelet, objectstore-plugin, and a worker; an actor went through suspend, resume, pause, and resume with its snapshot directories on the dedicated disk; deleting a running actor removed both trees.

  • Same node and build, 32 and 64 concurrent SWE-bench actors, with snapshots on the default boot disk (100 GB, 3,600 IOPS, 290 MiB/s) vs a dedicated Hyperdisk Balanced disk (12,000 IOPS, 1,200 MiB/s) at /var/lib/ate/snapshots. The objectstore-plugin ran with --upload-memory-budget=16Gi; at the 512Mi default, queued uploads dominated suspend latency (70 s / 134 s on the dedicated disk).

    p95, 32 / 64 actors Boot disk Dedicated disk
    Suspend 46.6 s / 127.6 s 9.6 s / 18.2 s
    Local snapshot write (worker_checkpoint) 44 s / over 60 s 0.97 s / 0.97 s
  • With 8 actors writing fsync'd 64 MiB chunks to their durable dirs as fast as they could (boot disk 97% busy), probe suspend and resume p95 stayed in line with the quiet baseline (ratios 0.69 and 0.98; one 12-minute run).

  • Tests pass

  • Appropriate changes to documentation are included in the PR

@roycaihw
Haowei Cai (Roy) (roycaihw) force-pushed the feat/atelet-separate-snapshots-dir branch from 9c97ba7 to a5de00d Compare October 16, 2026 07:11
@roycaihw

Copy link
Copy Markdown
Contributor Author

cc Benjamin Elder (@BenTheElder) PTAL

@roycaihw
Haowei Cai (Roy) (roycaihw) force-pushed the feat/atelet-separate-snapshots-dir branch from 840d103 to a06520a Compare October 16, 2026 19:04
@roycaihw

Copy link
Copy Markdown
Contributor Author

cc Walter Fender (@cheftako) PTAL as well

@roycaihw
Haowei Cai (Roy) (roycaihw) force-pushed the feat/atelet-separate-snapshots-dir branch 2 times, most recently from 7f3e209 to d744030 Compare October 17, 2026 03:41
@BenTheElder

Copy link
Copy Markdown
Collaborator

we have in agents.md:

  • Comments: Keep them brief and to the point. Comment the final state of the code, not the path taken to it — a problem that only existed partway through writing the change is noise to the next reader, as is a pointer to a scratch or planning file that isn't in the repository.

Can you try instructing the agent to rework this PR and the PR description? It's super verbose for what I think should be a relatively small change.

Comment thread docs/node-storage.md Outdated
`/var/lib/ate`. This page describes the parts of it that matter for disk
layout, and how to give snapshot I/O its own disk.

## Layout

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure we want to be treating all of this as an API, a lot of this is deep into implementation details that users should not be depending on. It's one thing to say "this is where we store things" and "snapshots are under this subfolder you might want to mount seperately", but

a) you really shouldn't be depending on the contents
b) sprawling the docs like this increases the odds they're out of date with the code

The intent of this PR is to split out snapshot storage, let's not turn it into committing to other disk details.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. The doc is now your trimmed version: it documents only the snapshot mount point and says the rest of /var/lib/ate is an implementation detail.

// so manifest staging I/O stays on the snapshot filesystem when
// /var/lib/ate/snapshots is mounted on a dedicated block device.
func SnapshotScratchDir() string {
return filepath.Join(nodepath.SnapshotsDir, "scratch")

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 nit 🟢 – The scratch dir only holds one small manifest file per transfer, so moving it onto the snapshot disk doesn't isolate any meaningful I/O. It also puts a non-actor entry next to the actor UID dirs, which is why the SnapshotsDir comment now has to warn that scans must skip names. Can we leave it at /var/lib/ate/snapshot-scratch as a plain var?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, it's back at /var/lib/ate/snapshot-scratch as a plain var.

Comment thread cmd/atelet/main.go Outdated
func (s *Atelet) copyLocalCheckpoint(ctx context.Context, actorDir, snapshotName, srcDir, dstDir string, files []string) error {
root, err := os.OpenRoot(actorDir)
// srcDir into dstDir. Both must be inside snapshotRootDir, which confines every access.
func (s *Atelet) copyLocalCheckpoint(ctx context.Context, snapshotRootDir, snapshotName, srcDir, dstDir string, files []string) error {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 nit 🟢 – Not new to this PR, just noticing: copyLocalCheckpoint (and linkFile) has no caller outside tests, since Restore now uses a local snapshot in place. Can we drop it instead of updating it here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done: removed copyLocalCheckpoint, linkFile, and copyRootFile (only they used it), with their tests.

Comment thread README.md Outdated
* [Enabling man-in-the-middle (MITM) interception for Actor Egress policy](docs/egress-trust-bundle.md): Egress policies such as header injection depend on MITM interception of Actor traffic. This guide explains how an Actor should be configured to enable interception.
* [Request Parking](docs/request-parking.md): How the router parks requests through transient worker-pool saturation.
* [Upgrade Runbook](docs/upgrade.md): Upgrade a running install to a newer supported release, and roll back if needed.
* [Node Storage](docs/node-storage.md): What atelet keeps under `/var/lib/ate`, and how to give snapshot I/O its own disk.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 nit 🟢 – Matches a trimmed doc that only covers the snapshot disk.

Suggested change
* [Node Storage](docs/node-storage.md): What atelet keeps under `/var/lib/ate`, and how to give snapshot I/O its own disk.
* [Node Storage](docs/node-storage.md): How to give snapshot I/O its own disk.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Comment on lines +55 to +58
// SnapshotScratchDir holds short-lived directories for snapshot manifests
// moved through the object-store plugin. It lives under nodepath.SnapshotsDir
// so manifest staging I/O stays on the snapshot filesystem when
// /var/lib/ate/snapshots is mounted on a dedicated block device.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 nit 🟢 – Shorter, the mount details belong on SnapshotsDir. Moot if the scratch dir moves back to snapshot-scratch.

Suggested change
// SnapshotScratchDir holds short-lived directories for snapshot manifests
// moved through the object-store plugin. It lives under nodepath.SnapshotsDir
// so manifest staging I/O stays on the snapshot filesystem when
// /var/lib/ate/snapshots is mounted on a dedicated block device.
// SnapshotScratchDir holds short-lived directories for snapshot manifests
// moved through the object-store plugin

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moot now, the scratch dir and its comment are unchanged from main.

Comment on lines +70 to +79
// ActorSnapshotPath is the per-actor directory under nodepath.SnapshotsDir
// holding control-plane checkpoint/restore and local pause snapshot state,
// separated from ActorPath so snapshot I/O can be isolated onto its own
// filesystem/device away from guest writable directories.
//
// checkpoint-state, restore-state and local-checkpoint stay together under
// it, on one filesystem: a pause renames checkpoint-state into
// local-checkpoint, and a micro-VM checkpoint renames its restore source's
// memory image into checkpoint-state. Nothing renames or hard-links between
// this tree and ActorPath.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 nit 🟢 – Shorter, keeping only the constraint the code can't show.

Suggested change
// ActorSnapshotPath is the per-actor directory under nodepath.SnapshotsDir
// holding control-plane checkpoint/restore and local pause snapshot state,
// separated from ActorPath so snapshot I/O can be isolated onto its own
// filesystem/device away from guest writable directories.
//
// checkpoint-state, restore-state and local-checkpoint stay together under
// it, on one filesystem: a pause renames checkpoint-state into
// local-checkpoint, and a micro-VM checkpoint renames its restore source's
// memory image into checkpoint-state. Nothing renames or hard-links between
// this tree and ActorPath.
// ActorSnapshotPath is the actor's snapshot staging directory, which may be on
// a different filesystem from ActorPath. Checkpoints rename files between its
// subdirectories, so they must stay on one filesystem.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Comment thread cmd/atelet/main_test.go Outdated
Comment on lines +981 to +983
// TestResetActorDirsCreatesBothTrees checks that resetActorDirs creates the
// snapshot staging dirs under ActorSnapshotPath and the guest dirs under
// ActorPath.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 nit 🟢 – The test name says this, so we can drop the comment.

Suggested change
// TestResetActorDirsCreatesBothTrees checks that resetActorDirs creates the
// snapshot staging dirs under ActorSnapshotPath and the guest dirs under
// ActorPath.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Comment thread cmd/atelet/main_test.go Outdated
Comment on lines +1014 to +1016
// TestRemoveActorDirsKeepsActorDirWhenSnapshotTreeRemains checks that when
// the snapshot tree cannot be removed, the actor dir and the sandbox record
// in it survive for the retried Terminate that needs them.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 nit 🟢 – The test name and its error messages say this, so we can drop the comment.

Suggested change
// TestRemoveActorDirsKeepsActorDirWhenSnapshotTreeRemains checks that when
// the snapshot tree cannot be removed, the actor dir and the sandbox record
// in it survive for the retried Terminate that needs them.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Comment thread docs/node-storage.md Outdated
Comment on lines +1 to +40
# Node storage

atelet, the object-store plugin and the workers share one host directory,
`/var/lib/ate`. This page describes the parts of it that matter for disk
layout, and how to give snapshot I/O its own disk.

## Layout

| Path | Holds |
|---|---|
| `/var/lib/ate/actors/<actor-uid>` | Each actor's guest-facing state (OCI bundles and rootfs upper layers, durable-dir, external and system-info volumes), its staged sandbox assets, and atelet's per-actor records |
| `/var/lib/ate/snapshots/<actor-uid>` | Snapshot staging: `checkpoint-state`, `restore-state`, and local pause snapshots under `local-checkpoint` |
| `/var/lib/ate/snapshots/scratch` | Short-lived copies of snapshot manifests |
| `/var/lib/ate/sandbox-asset-cache` | atelet's node-local cache of sandbox runtime binaries, hard-linked into each actor's directory |
| `/var/lib/ate/image-cache` | atelet's node-local cache of OCI image layers, which worker pods mount as overlay lower layers |

## Dedicated snapshot storage

By default all of this shares the node's boot disk, so heavy guest disk writes
can slow checkpoint and restore. To isolate snapshot staging, mount a
dedicated disk at `/var/lib/ate/snapshots`:

- **Mount it there, not at `/var/lib/ate/actors`.** `/var/lib/ate/actors`
must stay on the same mount as `/var/lib/ate/sandbox-asset-cache`, because
hard links cannot cross mounts; atelet checks this at startup and exits if
it does not hold. To move guest I/O off the boot disk as well, mount all of
`/var/lib/ate` on its own disk first, then the snapshot disk on top of it.
- **Mount, don't symlink.** The object-store plugin opens `/var/lib/ate` as an
`os.Root`, which refuses symlinks that lead out of it, and worker pods see
only `/var/lib/ate`.
- **Mount before actors run on the node.** A mount over a populated directory
hides the pause snapshots and restore sources already there. atelet, the
plugin and the workers mount `/var/lib/ate` with `HostToContainer`
propagation, so all three see the new mount.
- **Size it** for the restore staging of running actors, local pause
snapshots, and checkpoints in flight.

The separation covers snapshot staging, not every byte a snapshot touches: a
checkpoint still reads rootfs upper layers and durable-dir volumes from
`/var/lib/ate/actors`, and a restore writes them back there.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 nit 🟢 – Per the earlier comment, a trimmed version that only documents the snapshot mount point.

Suggested change
# Node storage
atelet, the object-store plugin and the workers share one host directory,
`/var/lib/ate`. This page describes the parts of it that matter for disk
layout, and how to give snapshot I/O its own disk.
## Layout
| Path | Holds |
|---|---|
| `/var/lib/ate/actors/<actor-uid>` | Each actor's guest-facing state (OCI bundles and rootfs upper layers, durable-dir, external and system-info volumes), its staged sandbox assets, and atelet's per-actor records |
| `/var/lib/ate/snapshots/<actor-uid>` | Snapshot staging: `checkpoint-state`, `restore-state`, and local pause snapshots under `local-checkpoint` |
| `/var/lib/ate/snapshots/scratch` | Short-lived copies of snapshot manifests |
| `/var/lib/ate/sandbox-asset-cache` | atelet's node-local cache of sandbox runtime binaries, hard-linked into each actor's directory |
| `/var/lib/ate/image-cache` | atelet's node-local cache of OCI image layers, which worker pods mount as overlay lower layers |
## Dedicated snapshot storage
By default all of this shares the node's boot disk, so heavy guest disk writes
can slow checkpoint and restore. To isolate snapshot staging, mount a
dedicated disk at `/var/lib/ate/snapshots`:
- **Mount it there, not at `/var/lib/ate/actors`.** `/var/lib/ate/actors`
must stay on the same mount as `/var/lib/ate/sandbox-asset-cache`, because
hard links cannot cross mounts; atelet checks this at startup and exits if
it does not hold. To move guest I/O off the boot disk as well, mount all of
`/var/lib/ate` on its own disk first, then the snapshot disk on top of it.
- **Mount, don't symlink.** The object-store plugin opens `/var/lib/ate` as an
`os.Root`, which refuses symlinks that lead out of it, and worker pods see
only `/var/lib/ate`.
- **Mount before actors run on the node.** A mount over a populated directory
hides the pause snapshots and restore sources already there. atelet, the
plugin and the workers mount `/var/lib/ate` with `HostToContainer`
propagation, so all three see the new mount.
- **Size it** for the restore staging of running actors, local pause
snapshots, and checkpoints in flight.
The separation covers snapshot staging, not every byte a snapshot touches: a
checkpoint still reads rootfs upper layers and durable-dir volumes from
`/var/lib/ate/actors`, and a restore writes them back there.
# Node Storage
atelet and the workers keep node-local state under `/var/lib/ate`. Apart from
the snapshot directory below, its layout is an implementation detail, please
don't depend on it.
## Dedicated Snapshot Storage
Snapshot staging lives under `/var/lib/ate/snapshots`. By default this shares
the node's boot disk, so heavy guest disk writes can slow checkpoint and
restore. To avoid that, you can mount a dedicated disk there:
- Mount it before any actors run on the node. A mount over a populated
directory hides the snapshots already there.
- Use a mount instead of a symlink, since worker pods only see `/var/lib/ate`.
- Keep `/var/lib/ate/actors` on the same filesystem as `/var/lib/ate`.
atelet checks this at startup.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Comment thread cmd/atelet/lifecycle_linux_test.go Outdated
Comment on lines +27 to +30
// TestLocalSnapshotGCWithSeparateSnapshotsFilesystem repeats TestLocalSnapshotGC
// with SnapshotsDir on a different filesystem from ActorsDir, as when a
// dedicated disk is mounted at /var/lib/ate/snapshots, so any rename or hard
// link between the two trees fails with EXDEV.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 nit 🟢 – Shorter, test comments can stay minimal.

Suggested change
// TestLocalSnapshotGCWithSeparateSnapshotsFilesystem repeats TestLocalSnapshotGC
// with SnapshotsDir on a different filesystem from ActorsDir, as when a
// dedicated disk is mounted at /var/lib/ate/snapshots, so any rename or hard
// link between the two trees fails with EXDEV.
// TestLocalSnapshotGCWithSeparateSnapshotsFilesystem runs TestLocalSnapshotGC
// with SnapshotsDir on a different filesystem from ActorsDir

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Comment thread cmd/atelet/lifecycle_linux_test.go Outdated
Comment on lines +43 to +46
// useSeparateSnapshotsFilesystem is useTempNodeDirs with SnapshotsDir moved
// to another mount (see otherMount). It fails, rather than skips, unless a
// hard link from the temp dir into that mount fails with EXDEV, so the test
// cannot pass without crossing a mount.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 nit 🟢 – The Fatalf message already says why it fails instead of skipping.

Suggested change
// useSeparateSnapshotsFilesystem is useTempNodeDirs with SnapshotsDir moved
// to another mount (see otherMount). It fails, rather than skips, unless a
// hard link from the temp dir into that mount fails with EXDEV, so the test
// cannot pass without crossing a mount.
// useSeparateSnapshotsFilesystem is useTempNodeDirs with SnapshotsDir on another mount

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@roycaihw
Haowei Cai (Roy) (roycaihw) force-pushed the feat/atelet-separate-snapshots-dir branch from d744030 to c552cff Compare October 17, 2026 16:17
@roycaihw

Copy link
Copy Markdown
Contributor Author

Thanks for the review. I applied your suggestions, moved the scratch dir back, removed copyLocalCheckpoint and its helpers, rebased on main, and shortened the description. PTAL.

This changes where atelet looks for unuploaded local snapshots, so an in-place upgrade needs paused actors resumed or suspended first (details in the description). Could you add the breaking-change label? I can't set labels on this repo.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc Michelle Au (@msau42) for this one ...

# the pod starts, e.g. a snapshots disk or CSI volumes.
- name: ate-base
mountPath: /var/lib/ate
mountPropagation: HostToContainer

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 should-fix 🟡 – atelet now sees the CSI mounts under volumes/, so resetActorDirs gets EBUSY on any volume that is still published. Run and Restore don't unpublish when they fail, so a Restore that fails with Unavailable or DeadlineExceeded after mountExternalVolumes is retried on the same node, hits EBUSY, and crashes the actor. Before this, atelet couldn't see the mount, so the rmdir went through, the kernel detached the old mount, and NodePublish mounted it again. Can we NodeUnpublish the spec's external volumes before resetActorDirs in Run and Restore? NodeUnpublishVolume has to be idempotent per the CSI spec.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we skip over the volume directory on run/restore? I would rather avoid unnecessary mount/unmount calls.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, good catch. I went with Michelle Au (@msau42)'s suggestion: Run and Restore now leave volumes/ alone when they reset the actor's dirs, so a mount left by a failed attempt stays published and the retry's NodePublish is a no-op (it has to be idempotent per the CSI spec). Checkpoint and Terminate still unpublish first and then remove the dirs. Added a test that the Run/Restore reset leaves a populated volume dir in place and the volume can be published again. PTAL.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 nit 🟢 – Same EBUSY, other path: Checkpoint and Terminate still os.Remove here, and DeleteActor deletes the volumes even when Terminate failed. The kind hostpath driver deletes a published volume with only a warning, and then its NodeUnpublishVolume returns NotFound without unmounting, which unmountExternalVolumes treats as unmounted, so the retried delete gets EBUSY every time. NFS, PD and Filestore always unmount, so this can be a follow-up. We should probably not delete the volumes until Terminate succeeds.

https://github.com/kubernetes-csi/csi-driver-host-path/blob/c7383ef90bae7803195ede035a7d85ab3c7cffc9/pkg/hostpath/nodeserver.go#L226-L229


At least this is less severe. It's probably OK to follow-up.

@roycaihw Haowei Cai (Roy) (roycaihw) Oct 20, 2026 •

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, thanks for tracing the hostpath driver case. Since it only bites drivers whose NodeUnpublish returns NotFound without unmounting, I'll handle it in a follow-up as you suggest: don't delete the volumes until Terminate has succeeded, so a retried delete doesn't hit EBUSY on a mount the driver never unmounted. Filed #2971 to track it.

@roycaihw
Haowei Cai (Roy) (roycaihw) force-pushed the feat/atelet-separate-snapshots-dir branch from 86cf940 to 56c5038 Compare October 19, 2026 23:06

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These would be good to fix but probably OKayish to not block on. Will circle back soon.

Comment thread cmd/atelet/volumes_test.go Outdated
}
})

t.Run("resetActorDirsKeepingVolumes leaves a still-mounted volume in place", func(t *testing.T) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 nit 🟢 – This calls the helper directly, and the helper never touches volumes/, so the file surviving is a given. If Run or Restore went back to resetActorDirs, no test would fail. Can we test the retry through Run and Restore instead? A case in TestActivationFailureBeforeRegistration with a populated volumes/<name> dir, expecting the activation to get past the reset, would do it.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... I think this is probably a good use of regression coverage, it's a bit fiddly that we have to take care around volumes specifically.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done: moved the coverage into TestActivationFailureBeforeRegistration. Two new cases ("Run retry keeps a published volume" and "Restore retry keeps a published volume") put content in volumes/data and expect the activation to get past the reset and fail later on its own error, leaving the volume content alone. I checked that both fail if Run or Restore go back to resetActorDirs. Removed the helper-level subtest from volumes_test.go.

# the pod starts, e.g. a snapshots disk or CSI volumes.
- name: ate-base
mountPath: /var/lib/ate
mountPropagation: HostToContainer

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 nit 🟢 – Same EBUSY, other path: Checkpoint and Terminate still os.Remove here, and DeleteActor deletes the volumes even when Terminate failed. The kind hostpath driver deletes a published volume with only a warning, and then its NodeUnpublishVolume returns NotFound without unmounting, which unmountExternalVolumes treats as unmounted, so the retried delete gets EBUSY every time. NFS, PD and Filestore always unmount, so this can be a follow-up. We should probably not delete the volumes until Terminate succeeds.

https://github.com/kubernetes-csi/csi-driver-host-path/blob/c7383ef90bae7803195ede035a7d85ab3c7cffc9/pkg/hostpath/nodeserver.go#L226-L229


At least this is less severe. It's probably OK to follow-up.

@roycaihw

Copy link
Copy Markdown
Contributor Author

The e2e-test (envoy, preview) failure in TestIngressAuthentication looks like a flake unrelated to this change. I re-pushed the same commit to re-run CI.

@roycaihw

Copy link
Copy Markdown
Contributor Author

Benjamin Elder (@BenTheElder) Michelle Au (@msau42) PTAL, all comments are addressed.

// SnapshotsDir, and link(2) requires both ends on one mounted filesystem,
// the same mount, not merely the same disk. With SnapshotsDir on its own
// mount, hits are served as copies instead.
SnapshotCacheDir = filepath.Join(nodepath.BasePath, "snapshot-cache")

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 nit 🟢 – Keeping the cache here is fine now that the doc says hits become copies with a dedicated snapshots disk. The --snapshot-cache-dir help text in cmd/atelet/snapshotcache.go still says hits "are served as hard links into the per-actor restore dirs on the same mount", so it should say they fall back to copies when the restore dirs are on another mount.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

deferring this.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will send a follow-up PR once this merges.

@BenTheElder

Copy link
Copy Markdown
Collaborator

e2e failure looks like unrelated flake, I hit rerun earlier but it's still queued (mid day CI slam)

@HavenXia
Haven Xia (HavenXia) added this pull request to the merge queue Oct 20, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to no response for status checks Oct 20, 2026
@zoez7
Zoe Zhao (zoez7) added this pull request to the merge queue Oct 20, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to a conflict with the base branch Oct 21, 2026
@HavenXia

Copy link
Copy Markdown
Member

need rebase

@roycaihw

Copy link
Copy Markdown
Contributor Author

Rebased onto main to resolve the merge conflict in cmd/ateworker-microvm/internal/ch/merge.go. Haven Xia (@HavenXia) could you please re-add this PR to the merge queue once CI finishes?

Each actor's checkpoint-state, restore-state and local-checkpoint
directories move from /var/lib/ate/actors/<uid> to
/var/lib/ate/snapshots/<uid>, so a dedicated disk mounted at
/var/lib/ate/snapshots keeps heavy guest disk writes from slowing
checkpoint and restore. atelet and objectstore-plugin mount /var/lib/ate
with HostToContainer propagation so they see such a disk, and Terminate
removes both trees. With that propagation atelet also sees external
volumes a failed Run or Restore left published, so Run and Restore now
leave the volume mount points in place and publish again over them.
copyLocalCheckpoint and its helpers, which had no caller outside tests,
are removed.
@roycaihw
Haowei Cai (Roy) (roycaihw) force-pushed the feat/atelet-separate-snapshots-dir branch from a7828bc to 35e2f7a Compare October 21, 2026 05:52
@HavenXia
Haven Xia (HavenXia) added this pull request to the merge queue Oct 21, 2026
Merged via the queue into agent-substrate:main with commit 57443d3 Oct 21, 2026
15 checks passed
Haowei Cai (Roy) (roycaihw) added a commit to roycaihw/substrate that referenced this pull request Oct 21, 2026
…refine Unavailable contract

Address follow-up review feedback after rebasing onto main (agent-substrate#2746):

- Atelet.Restore now calls resetActorDirsKeepingVolumes (agent-substrate#2746), which leaves external volume mount points in place across retries, so remove unmountVolumesOnFailure and its unmount tests.
- Update the apierror.Unavailable doc comment to cover an attempt whose state was rolled back so retrying is safe, and document the post-Resume CSI volume window on restoreGuestReadyError.
- Shorten the restoreFailureCounter doc comment, fold the per-snapshot streak assertion into TestRestoreStopsRetryingAfterConsecutiveUnavailableFailures, and drop TestBoundRetryableRestoreError and TestEndsRestoreFailureStreak.
- Use a fresh uuid.NewString() per subtest in TestCleanupFailedRestoreRootfs so running as root never touches a fixed sandbox path or matches a real process.
SamPriyadarshi pushed a commit to SamPriyadarshi/substrate that referenced this pull request Oct 24, 2026
…ent-substrate#3064)

### Summary
Follow-up to @BenTheElder's review comment on agent-substrate#2746
(agent-substrate#2746 (comment)):
update the `--snapshot-cache-dir` flag help text in
`cmd/atelet/snapshotcache.go` to reflect that cache hits fall back to
copies when the per-actor restore directories
(`/var/lib/ate/snapshots/<uid>/restore-state`) are on another mount.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/node area/storage breaking-change PRs with breaking changes kind/feature An enhancement / feature request or implementation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants