Skip to content

Add basic per-actor external volume flow#405

Open
Michelle Au (msau42) wants to merge 6 commits into
agent-substrate:mainfrom
msau42:mock-mount-dev
Open

Add basic per-actor external volume flow#405
Michelle Au (msau42) wants to merge 6 commits into
agent-substrate:mainfrom
msau42:mock-mount-dev

Conversation

@msau42

Copy link
Copy Markdown
Collaborator

Initial part of #232.

Extends the ActorTemplate API to add support for per-actor external volumes and adds control plane and atelet hooks following the Actor lifecycle.

Callouts to volume operations are abstracted with a Volume interface. Right now, only a mock volume plugin for testing has been implemented, but we will add CSI support next.

  • Tests pass
  • Appropriate changes to documentation are included in the PR - Not going to update documentation until we add CSI support.

Comment thread pkg/api/v1alpha1/actortemplate_types.go Outdated
Capacity resource.Quantity `json:"capacity" protobuf:"bytes,1,opt,name=capacity"`
// storageClassName refers to the StorageClass to create the volume from.
// +required
StorageClassName string `json:"storageClassName" protobuf:"bytes,2,opt,name=storageClassName"`

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

open for discussion: can we reference k8s objects or should we create our own api?

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 don't think there's a cleaner way. In theory we could have an ateapi level api which just acts as an indirect, but I don't see a reason at this moment.

map<string, string> match_labels = 1;
}

message ExternalVolume {

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

discuss: should ExternalVolume be its own standalone object rather than embedded fields into the Actor?

Right now ExternalVolume is tied to an Actor but we are also thinking about the scenario where you want to share a volume across multiple actors.

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 vote 👍

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.

We all know how tricky volumes can be in k8s, with RWO, RWX, RO, mounts on the same node being virtually RWX but not ACTUALLY RWX, etc. If we make this a first-class resource, do we need to track the same sorts of things (e.g. NFS can be RWX, blockdev can't)?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Yes, we will need to add the notion of access modes and enforce that shared volumes can only be RWX or ROX.

}
err = s.persistence.CreateActor(ctx, actor)
if err != nil {
// Cleanup created volumes if DB write fails

@msau42 Michelle Au (msau42) Jul 8, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Discuss: In general, how should we approach errors when an operation only partially succeeds? Should we rollback all the partially completed operations? What happens if the rollback fails?

Or we leave the system in the partial state and require users to cleanup by calling Delete? Even then, if deletion fails and Create is called again, we could still have partially leftover resources from a previous invocation.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

My current thoughts:

If there is a pair of "Create" and "Delete" operations, then we can leave the partial state and clean it up on the Delete. This does imply we will have to add some "Creating" and possibly "Deleting" state to Actor before we start creating resources for it. I already have marked that as a todo.

For operations like "Resume", there isn't a corresponding delete operation if the resume failed. It's possible resume could be called on a different node. That means we could have partial state left on the first node. In those cases we can try to rollback the state as much as possible but that would still be a best effort operation. This has a few implications:

  • We probably need to have some process on the node that monitors oprhaned resources and tries to clean them up and/or alert
  • We need to make sure these orphaned resources don't get reused if a new actor with the same name gets created. This potentially implies that we should have some sort of actor uuid and be using that in our actor directories.

@msau42 Michelle Au (msau42) changed the title Add basic external volume flow Add basic per-actor external volume flow Jul 8, 2026
@msau42
Michelle Au (msau42) force-pushed the mock-mount-dev branch 2 times, most recently from c2286ea to 96fa815 Compare July 8, 2026 22:50
Comment thread cmd/ateapi/internal/controlapi/volumes.go Outdated

@thockin Tim Hockin (thockin) left a comment

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 didn't get too far into the reading of this yet, but wanted to send a couple questions before EOD

Comment thread pkg/api/v1alpha1/actortemplate_types.go Outdated
Capacity resource.Quantity `json:"capacity" protobuf:"bytes,1,opt,name=capacity"`
// storageClassName refers to the StorageClass to create the volume from.
// +required
StorageClassName string `json:"storageClassName" protobuf:"bytes,2,opt,name=storageClassName"`

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 don't think there's a cleaner way. In theory we could have an ateapi level api which just acts as an indirect, but I don't see a reason at this moment.

Comment thread internal/volume/plugin.go

// VolumePlugin abstracts storage operations.
type VolumePlugin interface {
CreateVolume(ctx context.Context, name string, capacity string, storageClass string) (volumeID string, err 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.

Do you see this as "kubernetes PVC is the plugin", or "CSI is the plugin", or "NFS is the plugin" ?

What I really want to understand is what credentials are granted to the API server to create and destroy volumes, and what additional layers are in between. E.g. if I need to create a million k8s PVCs and PVs, that's trouble, but if I need to give ateapi binaries the ability to delete EBS volumes in the same context as k8s, that's also trouble.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

My thought is that substrate will directly call CSI grpc APIs (so ateapi needs to be authorized to communicate with CSI drivers). We will not be creating PVC objects, although we will need to have a substrate API reference to the volume IDs (which I have added in this PR).

Comment thread cmd/atelet/oci.go
Type: "bind",
Source: ateompath.DurableDirVolumeMountPoint(atespace, actorID, vm.GetName()),
Source: srcPath,
Options: []string{"bind", "rw"},

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

double check this. durabledir didn't set any options.

Comment thread pkg/api/v1alpha1/actortemplate_types.go Outdated

@thockin Tim Hockin (thockin) left a comment

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 read this. It seems broadly correct. I am still worried about permissions, but not enough to block this.

I swear I had another comment, and it was important, but I forgot what it was. Oh well, Friday. :)

Comment thread pkg/api/v1alpha1/actortemplate_types.go Outdated
// for each actor. The volume only lives as long as the actor and is deleted
// when the actor is deleted.
// +optional
ExternalVolumeTemplate *ExternalVolumeTemplate `json:"externalVolumeTemplate,omitempty" protobuf:"bytes,3,opt,name=externalVolumeTemplate"`

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.

Do we need the word "Template" in here? All of these are templates

@msau42 Michelle Au (msau42) Jul 21, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Open to naming, I am trying to distinguish between volumes that will trigger dynamic provisioning, and volumes that won't. So for shared volumes, for instance, I am thinking we could have something like:

volumes:
  externalVolumeTemplate:
  - name: my-own-volume
    storageClassName: class1
  externalVolume:
  - name: shared-volume
    volumeName: someExistingVolume (in the same atespace) # This volume is already created and populated with data through other means

Comment thread cmd/ateapi/internal/controlapi/workflow_pause.go
Comment thread cmd/ateapi/internal/controlapi/workflow_suspend.go
Comment thread pkg/api/v1alpha1/actortemplate_types.go Outdated
// for each actor. The volume only lives as long as the actor and is deleted
// when the actor is deleted.
// +optional
ExternalVolumeTemplate *ExternalVolumeTemplate `json:"externalVolumeTemplate,omitempty" protobuf:"bytes,3,opt,name=externalVolumeTemplate"`

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.

What exactly does External mean here, External to substrate? If we are just wrapping a StorageClass anyway should we name it PersistentVolumeSource to be closer to the source material?

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.

It's not REALLY a PV in k8s, it's just reusing k8s storage classes

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Open to naming :)

But agree with Tim that this doesn't have quite the same semantics as PVs in k8s (and even the "persistent" part in k8s is misleading sometimes). At least with the per-actor volumes, the volume is definitely not persistent in the traditional sense (it only lives as long as the actor).

By "External", I'm trying to distinguish with the sandbox-backed filesystems, like rootfs and durabledir. The volumes in this case are backed by storage systems external to substrate (ie nfs, remote block, pfs, distributed fs, etc).

map<string, string> match_labels = 1;
}

message ExternalVolume {

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 vote 👍

Comment thread internal/volume/plugin.go Outdated
This adds a new externalVolumeTemplate where users specify
the capacity, storage driver  and storage properties they would
like via StorageClass.

When an actor is created, we will also provision a volume for the
actor according to the template. The volume is deleted when the
actor is deleted.
Adds external volumes to the Actor object and integrates
control plane Actor workflows with the corresponding
volume operations:

CreateActor -> CreateVolume
DeleteActor -> DeleteVolume
ResumeActor -> AttachVolume
Pause/Suspend Actor -> DetachVolume
Volume operations are integrated into the actor workflows:

- Run/Restore: MountVolume
- Checkpoint: UnmountVolume

Volumes are mounted under the actor directory and passed as
bind mounts to OCI.
This allows for different storage backends to be supported.
Right now it only contains the mock volume plugin, which is
only intended for testing purposes right now.
It makes the file counter path configurable so we can test it
against different volume types. And adds an optional file verification step to
test the presence of a file on an existing volume.

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.

[AI First Pass Experiment]

return mounted
}

func actorVolumeID(ref *ateapipb.ObjectRef, volumeName string) string {

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.

🟡 🤖 The hyphen join is ambiguous: atespace, actor name, and volume name may all contain hyphens, so distinct actors can produce the same ID (actor a in atespace x-y vs actor y-a in atespace x). This string is the idempotency name passed to CreateVolume, and CSI dedupes by name — colliding actors would silently share one storage volume. The existing TODO about actor UUID would fix it; an unambiguous encoding of the three parts would too.

}

// Delete associated volumes (TODO: best effort?)
s.deleteActorVolumes(ctx, req.GetActor(), actor.GetActorVolumes())

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.

🟡 🤖 Two issues with volume deletion here. Volumes are never detached first — for an actor deleted while running or paused, CSI DeleteVolume on an attached volume fails. And since failures are only logged before the record is deleted below, the volume leaks with no record pointing at it. Consider detaching first (reuse detachActorVolumes) and deleting the record only after volume deletion succeeds, or persisting a deletion intent for background retry.

@msau42 Michelle Au (msau42) Jul 23, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

This is an interesting question. While the state machine here (#119) does allow delete from SUSPENDED, PAUSED and CRASHED states, I believe for pause/crash, there's probably still some cleanup that has to happen from atelet, and that workflow is currently missing today. Dmitry Berkovich (@dberkov) do we plan to add this workflow?

// for each actor. The volume only lives as long as the actor and is deleted
// when the actor is deleted.
// +optional
ExternalVolumeTemplate *ExternalVolumeTemplate `json:"externalVolumeTemplate,omitempty"`

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.

🟡 🤖 externalVolumeTemplate needs the same microvm gate DurableDir has (the CEL rule below only checks durableDir). Nothing in the microvm path handles VOLUME_TYPE_EXTERNAL: atelet writes a bind mount whose source is a host path that doesn't exist inside the guest (only the bundle rootfs is shared over virtio-fs), so container creation fails at runtime instead of admission.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants