-
Notifications
You must be signed in to change notification settings - Fork 228
Add basic per-actor external volume flow #405
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
28d766f
49471cf
883ac3c
f53df1d
698da05
f0ddb9e
9a619ba
7a527fb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,149 @@ | ||
| // Copyright 2026 Google LLC | ||
| // | ||
| // Licensed under the Apache License, Version 2.0 (the "License"); | ||
| // you may not use this file except in compliance with the License. | ||
| // You may obtain a copy of the License at | ||
| // | ||
| // http://www.apache.org/licenses/LICENSE-2.0 | ||
| // | ||
| // Unless required by applicable law or agreed to in writing, software | ||
| // distributed under the License is distributed on an "AS IS" BASIS, | ||
| // WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| // See the License for the specific language governing permissions and | ||
| // limitations under the License. | ||
|
|
||
| package controlapi | ||
|
|
||
| import ( | ||
| "context" | ||
| "errors" | ||
| "fmt" | ||
| "log/slog" | ||
|
|
||
| "github.com/agent-substrate/substrate/cmd/ateapi/internal/store" | ||
| "github.com/agent-substrate/substrate/internal/volume" | ||
| atev1alpha1 "github.com/agent-substrate/substrate/pkg/api/v1alpha1" | ||
| "github.com/agent-substrate/substrate/pkg/proto/ateapipb" | ||
| "google.golang.org/grpc/codes" | ||
| "google.golang.org/grpc/status" | ||
| ) | ||
|
|
||
| var ( | ||
| globalVolumePlugin = volume.NewMockVolumePlugin() | ||
| ) | ||
|
|
||
| // TODO: Replace with actual volume plugin search | ||
| func getVolumePlugin() volume.VolumePluginControlPlane { | ||
| return globalVolumePlugin | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is what actually was I was missing when I reviewed pkg/api/v1alpha1/actortemplate_types.go. How the storage plugins will be registered in the substrate instance?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The CSI integration will include plugin discovery. We're still POC-ing CSI support right now but the current thought is to add a new CRD to register the CSI endpoints to use for substrate. |
||
| } | ||
|
|
||
| // TODO: we should persist creation first so that we can handle background cleanup. | ||
| // this probably requires us to add a PROVISIONING actor state. | ||
|
|
||
| // createActorVolumes provisions external volumes specified in the actor template. | ||
| // It returns the list of created external volumes, or an error if any creation fails. | ||
| // If any volume creation fails, it cleans up any volumes created in this call on a best-effort basis. | ||
| func (s *Service) createActorVolumes(ctx context.Context, ref *ateapipb.ObjectRef, template *atev1alpha1.ActorTemplate) ([]*ateapipb.ExternalVolume, error) { | ||
| var volumes []*ateapipb.ExternalVolume | ||
| for _, vol := range template.Spec.Volumes { | ||
| if vol.ExternalVolumeTemplate != nil { | ||
| // Use a unique name for the volume to ensure idempotency | ||
| uniqueVolName := actorVolumeID(ref, vol.Name) | ||
| storageVolumeID, err := getVolumePlugin().CreateVolume(ctx, uniqueVolName, vol.ExternalVolumeTemplate.Capacity.String(), vol.ExternalVolumeTemplate.StorageClassName) | ||
| if err != nil { | ||
| // TODO: need better system - best effort cleanup of already created volumes | ||
| _ = s.deleteActorVolumes(ctx, ref, volumes) | ||
| return nil, status.Errorf(codes.Internal, "failed to create volume %q: %v", vol.Name, err) | ||
| } | ||
| volumes = append(volumes, &ateapipb.ExternalVolume{ | ||
| ActorVolumeId: uniqueVolName, | ||
| StorageVolumeId: storageVolumeID, | ||
| VolumeType: "mock", // TODO fix when we support multiple plugins | ||
| Status: ateapipb.ExternalVolume_CREATED, | ||
| }) | ||
| } | ||
| } | ||
| return volumes, nil | ||
| } | ||
|
|
||
| // deleteActorVolumes deletes all external volumes in the list. | ||
| func (s *Service) deleteActorVolumes(ctx context.Context, ref *ateapipb.ObjectRef, volumes []*ateapipb.ExternalVolume) error { | ||
| var errs []error | ||
| for _, vol := range volumes { | ||
| if err := getVolumePlugin().DeleteVolume(ctx, vol.GetStorageVolumeId()); err != nil { | ||
| slog.ErrorContext(ctx, "failed to delete volume", | ||
| slog.String("atespace", ref.GetAtespace()), | ||
| slog.String("actor_id", ref.GetName()), | ||
| slog.String("volume_id", vol.GetStorageVolumeId()), | ||
| slog.Any("error", err)) | ||
| errs = append(errs, fmt.Errorf("failed to delete volume %q: %w", vol.GetStorageVolumeId(), err)) | ||
| } | ||
| } | ||
| return errors.Join(errs...) | ||
| } | ||
|
|
||
| // getMountedActorVolumes filters the actor's volumes and returns only those that are declared and mounted in the ActorTemplate. | ||
| func getMountedActorVolumes(ctx context.Context, ref *ateapipb.ObjectRef, volumes []*ateapipb.ExternalVolume, template *atev1alpha1.ActorTemplate) []*ateapipb.ExternalVolume { | ||
| var mounted []*ateapipb.ExternalVolume | ||
| for _, vol := range volumes { | ||
| // Find the corresponding volume in the ActorTemplate to check if it's mounted | ||
| var matchedTemplateVol *atev1alpha1.Volume | ||
| for _, tVol := range template.Spec.Volumes { | ||
| expectedID := actorVolumeID(ref, tVol.Name) | ||
| if vol.GetActorVolumeId() == expectedID { | ||
| matchedTemplateVol = &tVol | ||
| break | ||
| } | ||
| } | ||
|
|
||
| if matchedTemplateVol == nil { | ||
| slog.WarnContext(ctx, "Volume not found in template, skipping", slog.String("volume_id", vol.GetStorageVolumeId())) | ||
| continue | ||
| } | ||
|
|
||
| if !isVolumeMounted(matchedTemplateVol.Name, template) { | ||
| slog.InfoContext(ctx, "Volume not mounted in template, skipping", slog.String("volume_id", vol.GetStorageVolumeId())) | ||
| continue | ||
| } | ||
| mounted = append(mounted, vol) | ||
| } | ||
| return mounted | ||
| } | ||
|
|
||
| func actorVolumeID(ref *ateapipb.ObjectRef, volumeName string) string { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I plan to address this as a folowup (I already have a todo) as it will require adding a "CREATING" status to Actor and persisting that before calling CreateVolume so that the Actor will have a UID by the time we create the volumes. |
||
| // TODO consider if this should be actor UUID | ||
| return fmt.Sprintf("%s-%s-%s", ref.GetAtespace(), ref.GetName(), volumeName) | ||
| } | ||
|
|
||
| // detachActorVolumes detaches all mounted external volumes for an actor from its worker node. | ||
| func detachActorVolumes(ctx context.Context, st store.Interface, actor *ateapipb.Actor, template *atev1alpha1.ActorTemplate, action string) error { | ||
| if actor.GetAteomPodNamespace() == "" { | ||
| slog.WarnContext(ctx, fmt.Sprintf("Actor has no assigned worker pod during %s, skipping detach volumes", action), slog.String("actor_id", actor.GetMetadata().GetName())) | ||
| return nil | ||
| } | ||
|
|
||
| worker, err := st.GetWorker(ctx, actor.GetAteomPodNamespace(), actor.GetWorkerPoolName(), actor.GetAteomPodName()) | ||
| if err != nil { | ||
| if errors.Is(err, store.ErrNotFound) { | ||
| slog.WarnContext(ctx, fmt.Sprintf("Worker not found in store during %s, skipping detach volumes", action), slog.String("actor_id", actor.GetMetadata().GetName())) | ||
| return nil | ||
| } | ||
| return fmt.Errorf("failed to get worker: %w", err) | ||
| } | ||
|
|
||
| node := worker.GetNodeName() | ||
| if node == "" { | ||
| slog.WarnContext(ctx, fmt.Sprintf("Worker has no assigned node name during %s, skipping detach volumes", action), slog.String("actor_id", actor.GetMetadata().GetName())) | ||
| return nil | ||
| } | ||
|
|
||
| ref := &ateapipb.ObjectRef{Atespace: actor.GetMetadata().GetAtespace(), Name: actor.GetMetadata().GetName()} | ||
| for _, vol := range getMountedActorVolumes(ctx, ref, actor.GetActorVolumes(), template) { | ||
| slog.InfoContext(ctx, "Detaching volume from node", slog.String("volume_id", vol.GetStorageVolumeId()), slog.String("node", node)) | ||
| err := getVolumePlugin().DetachVolume(ctx, vol.GetStorageVolumeId(), node) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to detach volume %q from node %q: %w", vol.GetStorageVolumeId(), node, err) | ||
| } | ||
| } | ||
| return nil | ||
| } | ||
| Original file line number | Diff line number | Diff line change | ||
|---|---|---|---|---|
|
|
@@ -260,6 +260,49 @@ func (s *AssignWorkerStep) RetryBackoff() *wait.Backoff { | |||
| } | ||||
| } | ||||
|
|
||||
| type AttachVolumesStep struct { | ||||
| store store.Interface | ||||
| } | ||||
|
|
||||
| func (s *AttachVolumesStep) Name() string { return "AttachVolumes" } | ||||
|
|
||||
| func (s *AttachVolumesStep) IsComplete(ctx context.Context, input *ResumeInput, state *ResumeState) (bool, error) { | ||||
| // TODO replace with a proper check on the volumes. | ||||
| return state.Actor.GetStatus() == ateapipb.Actor_STATUS_RUNNING, nil | ||||
| } | ||||
|
|
||||
| func (s *AttachVolumesStep) CheckPrerequisite(ctx context.Context, input *ResumeInput, state *ResumeState) error { | ||||
| return nil | ||||
| } | ||||
|
|
||||
| func (s *AttachVolumesStep) Execute(ctx context.Context, input *ResumeInput, state *ResumeState) error { | ||||
| if state.Actor.GetAteomPodNamespace() == "" { | ||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. could you please take a look to
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I took a look, right now it just updates DB Actor state. I don't see anything to add here wrt volumes at the moment. Like I mentioned in #405 (comment), I think we are missing some workflow to cleanup the worker/node when a PAUSED or CRASHED actor gets deleted. |
||||
| return fmt.Errorf("actor has no assigned worker pod") | ||||
| } | ||||
|
|
||||
| worker, err := s.store.GetWorker(ctx, state.Actor.GetAteomPodNamespace(), state.Actor.GetWorkerPoolName(), state.Actor.GetAteomPodName()) | ||||
| if err != nil { | ||||
| return fmt.Errorf("failed to get worker for volume attachment: %w", err) | ||||
| } | ||||
|
|
||||
| node := worker.GetNodeName() | ||||
| if node == "" { | ||||
| return fmt.Errorf("assigned worker has no node name") | ||||
| } | ||||
|
|
||||
| ref := &ateapipb.ObjectRef{Atespace: state.Actor.GetMetadata().GetAtespace(), Name: state.Actor.GetMetadata().GetName()} | ||||
| for _, vol := range getMountedActorVolumes(ctx, ref, state.Actor.GetActorVolumes(), state.ActorTemplate) { | ||||
| slog.InfoContext(ctx, "Attaching volume to node", slog.String("volume_id", vol.GetStorageVolumeId()), slog.String("node", node)) | ||||
| err := getVolumePlugin().AttachVolume(ctx, vol.GetStorageVolumeId(), node) | ||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what does attachment supposed to do? Attach volume to node VM? Is this code supposed to be executed on the atelet?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This depends on the actual volume driver implementation but it's essentially "anything you need to do to make the volume accessible on that node". For block storage, that would be attaching the device to the node. For file storage, that could be configuring network policies to allow the node access to the volume. This is run from the CP (ateapi)
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The disadvantage to run this code in control plane, it makes resume operation slower. By running this code in atelet, this code can be executed in parallel with code that downloads images from GCS or download OCI image from artifcat registry.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The main issue is security. In the case of block storage, we don't want to give nodes permissions to attach any volume, and in the case of filers, we don't want to give the node permissions to set ACLs for itself. In the future, we can explore optimizations to make this a non-blocking operation. |
||||
| if err != nil { | ||||
| return fmt.Errorf("failed to attach volume %q to node %q: %w", vol.GetStorageVolumeId(), node, err) | ||||
| } | ||||
| } | ||||
| return nil | ||||
| } | ||||
|
|
||||
| func (s *AttachVolumesStep) RetryBackoff() *wait.Backoff { return nil } | ||||
|
|
||||
| func (s *AssignWorkerStep) findFreeWorker( | ||||
| workers []*ateapipb.Worker, | ||||
| templateClass atev1alpha1.SandboxClass, | ||||
|
|
@@ -353,7 +396,7 @@ func (s *CallAteletRestoreStep) Execute(ctx context.Context, input *ResumeInput, | |||
| } | ||||
| client := ateletpb.NewAteomHerderClient(ateletConn) | ||||
|
|
||||
| workloadSpec, err := workloadSpecFromActorTemplateWithEnv(ctx, s.kubeClient, s.secretCache, state.ActorTemplate) | ||||
| workloadSpec, err := workloadSpecFromActorTemplateWithEnv(ctx, s.kubeClient, s.secretCache, state.ActorTemplate, state.Actor) | ||||
| if err != nil { | ||||
| return err | ||||
| } | ||||
|
|
||||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current implementation does not handle error in deleteActorVolume.
Might be we need introduce "FAILED" state. So the delete volume will be cleaned in this state.
We dont have this problem today, since during create we just create record in suspended state.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I am thinking of 2 main changes to CreateActor: