[WIP]Allow force deleting an actor from any state. - #788
[WIP]Allow force deleting an actor from any state.#788Sneha-at (Sneha-at) wants to merge 4 commits into
Conversation
81683b1 to
8676288
Compare
…actor. This will bypass the requirement to suspend the workload cleanly before deletion. A new TERMINATING status is added to indicate that we're starting the termination sequence. The termination sequence will call atelet to terminate the workload. Once atelet termination succeeds, then we will proceed with control plane termination sequence. Finally we will move to the existing DELETING status to delete the actor resources.
- Add E2E tests for force deletion of actors. - Add --force flag to kubectl-ate delete command. - Refactor DeleteActor workflow in controlapi to use the new plain-method (ensure-steps) style. - Fix unit tests for the new DeleteActor workflow. TAG=agy CONV=ab017ff1-c93c-4fb4-a5d0-15bf24183e50
8676288 to
c6877ac
Compare
| // already on disk for atelet to ship. | ||
| tTeardown := time.Now() | ||
| s.teardownActor(ctx, actorUID, ra, client) | ||
| s.terminateWorkload(ctx, actorUID) |
There was a problem hiding this comment.
we are not doing error checks here? the method returns err, what if termination fails
There was a problem hiding this comment.
The method does not return any errors, I see TerminateWorkload does return error but there is actually no error reported in it's implementation, I think we can update the return value. I also checked other methods called by TerminateWorkload - teardownActor and that too doesn't seem to report any error
| setSpanActorRefAttributes(ctx, actorRef) | ||
|
|
||
| deleted, err = s.actorWorkflow.DeleteActor(ctx, req.GetActor().GetAtespace(), req.GetActor().GetName()) | ||
| deleted, err = s.actorWorkflow.DeleteActor(ctx, req.GetActor().GetAtespace(), req.GetActor().GetName(), req.GetForce()) |
There was a problem hiding this comment.
Should we squash this to one commit
| if s.actorTemplateLister != nil && actor.GetActorTemplateNamespace() != "" && actor.GetActorTemplateName() != "" { | ||
| tmpl, err := s.actorTemplateLister.ActorTemplates(actor.GetActorTemplateNamespace()).Get(actor.GetActorTemplateName()) | ||
| if err != nil && !k8serrors.IsNotFound(err) { | ||
| return fmt.Errorf("while fetching actor template: %w", err) |
There was a problem hiding this comment.
Do we want to log info, warn in case actor template was not found?
There was a problem hiding this comment.
ActorTemplate is first referred in ensureAteletTerminated where we return err if the template is nil. I suppose that should be alright as we do fail soon?
if actorTemplate == nil {
return status.Errorf(codes.FailedPrecondition, "actor template %s/%s not found for actor %s", actor.GetActorTemplateNamespace(), actor.GetActorTemplateName(), actorRef)
}
| ateapipb.Actor_STATUS_PAUSING, | ||
| ateapipb.Actor_STATUS_PAUSED, | ||
| ateapipb.Actor_STATUS_CRASHED, | ||
| ateapipb.Actor_STATUS_TERMINATING, |
There was a problem hiding this comment.
Not sure if its elsewhere in the code, do we log info on what the last state of the actor was it will help debug complex lifecycle issues if any
| ateapipb.Actor_STATUS_SUSPENDED: | ||
| return nil | ||
| default: | ||
| return status.Errorf(codes.FailedPrecondition, "Actor %s is not in a deletable status (status: %v)", input.ActorRef, state.Actor.GetStatus()) |
There was a problem hiding this comment.
In case on non force scenario should this fail if actor is already in TERMINATING step. It could be a no-op with a log?
There was a problem hiding this comment.
Considering a scenario where the Actor was deleted with force at T0 and then attempted again to be deleted without force at T1 where the gap between T0 and T1 is minimal we can run into a state where the request at T1 finds the Actor in TERMINATING state (as mentioned in the comment). If so I agree that it is a no-op. However, if the Actor is already in DELETING status then the current logic updates it to TERMINATING which is actually taking a step back (For force workflow X status -> TERMINATING -> DELETING). I think we should have a different workflow for DELETING status.
} else {
switch st {
case ateapipb.Actor_STATUS_SUSPENDED:
// allowed
case ateapipb.Actor_STATUS_DELETING:
return actor, nil // return the same actor object we received as the function arg
default:
return nil, status.Errorf(codes.FailedPrecondition, "Actor %s is not in a deletable status (status: %v)", actorRef, st)
}
}
| return nil | ||
| } | ||
|
|
||
| func (s *LoadActorForDeleteStep) RetryBackoff() *wait.Backoff { return nil } |
There was a problem hiding this comment.
Do we need a backoff here, why is this nil.
There was a problem hiding this comment.
Do you mean we should have backoff for DeleteActor and retry on failure?
| func (s *CallAteletTerminateStep) Name() string { return "CallAteletTerminate" } | ||
|
|
||
| func (s *CallAteletTerminateStep) IsComplete(ctx context.Context, input *DeleteInput, state *DeleteState) (bool, error) { | ||
| return state.Actor.GetStatus() == ateapipb.Actor_STATUS_DELETING, nil |
There was a problem hiding this comment.
Is there a code path that needs to be still added here, we are not passing any non nil value of err ever, then we should never return err if it can never have non nil error
There was a problem hiding this comment.
Could you please highlight which function are you referring to? I see we return some custom errors with return status.Errorf(.....)
There was a problem hiding this comment.
Ohh I see the comments are on the older version of the code. While rebasing the changes to latest main I realized there were some significant impact of Workflow lifecycle implementation so I had to re-write parts of the code. The code blocks this comments refers to no longer exists.
| func (s *DetachVolumesForDeleteStep) Name() string { return "DetachVolumesForDelete" } | ||
|
|
||
| func (s *DetachVolumesForDeleteStep) IsComplete(ctx context.Context, input *DeleteInput, state *DeleteState) (bool, error) { | ||
| return state.Actor.GetStatus() == ateapipb.Actor_STATUS_DELETING, nil |
There was a problem hiding this comment.
same isComplete not using err here.
There was a problem hiding this comment.
Same here, the code blocks this comments refers to no longer exists.
|
|
||
| // TODO: can this be done inside releaseWorker()? | ||
| latestActor.LocalSnapshotInfo = nil | ||
| updatedActor, err := s.store.UpdateActor(ctx, latestActor, latestActor.GetMetadata().GetVersion()) |
There was a problem hiding this comment.
so if update actor fails and release worker succeeds. Is release worker op idempotent, in a retry would release worker succeed if its already released?
There was a problem hiding this comment.
If the worker is released already from the actor the ensureWorkerReleased is just a no-op
if actor.GetWorkerAssignment() == nil {
markSkipped(ctx, "worker already released")
return actor, nil
}
| slog.WarnContext(ctx, "Failed to deactivate actor networking during terminate", slog.Any("err", err)) | ||
| } | ||
|
|
||
| ra := s.running[actorUID] |
There was a problem hiding this comment.
Would it be guaranteed as running when the termination is called? what if it got suspended in interim
There was a problem hiding this comment.
The running variable is a bit confusing, it actually tracks the VMs the actorPod is running on and is not related to the state of the Actor.
| f.FailTerminate = nil | ||
| } | ||
|
|
||
| func (f *FakeAteletServer) Terminate(ctx context.Context, req *ateletpb.TerminateRequest) (*ateletpb.TerminateResponse, error) { |
There was a problem hiding this comment.
Should we have some negative tests, where actor worker is unresponsive etc. Whether that is still able to force delete in those hang or failure conditions
| ateapipb.Actor_STATUS_PAUSING, | ||
| ateapipb.Actor_STATUS_PAUSED, | ||
| ateapipb.Actor_STATUS_CRASHED, | ||
| ateapipb.Actor_STATUS_TERMINATING, |
There was a problem hiding this comment.
TERMINATING and DELETING state cannot be reached. in the case as earlier check already returns error
There was a problem hiding this comment.
Earlier check as in before the ensureMarkedTerminating function is called?
| return status.Errorf(codes.FailedPrecondition, "CallAteletTerminate prerequisite not met for Actor: %s (got: %v, want %s)", actorRef, st, ateapipb.Actor_STATUS_TERMINATING) | ||
| } | ||
| if actorTemplate == nil { | ||
| return status.Errorf(codes.FailedPrecondition, "actor template %s/%s not found for actor %s", actor.GetActorTemplateNamespace(), actor.GetActorTemplateName(), actorRef) |
There was a problem hiding this comment.
What would happen if the actor template goes missing? would this clean up the actor, or atleast not lead to stuckness?
|
|
||
| conn, err := w.dialer.DialForWorker(workerPodNs, workerPodName) | ||
| if err != nil { | ||
| if errors.Is(err, ErrWorkerPodNotFound) { |
There was a problem hiding this comment.
Should we be deleting metadata / clean up the actor even when associated worker was not found?
There was a problem hiding this comment.
So ignore atletTermination error when force deleting?
if err := w.ensureAteletTerminated(ctx, actorRef, actor, actorTemplate); err != nil {
if force {
slog.WarnContext(ctx, "ignoring atelet termination failure during force delete", "error", err)
} else {
return nil, err
}
}
if err := w.ensureVolumesDetachedForDelete(ctx, actor, actorTemplate); err != nil {
return nil, err
}
if actor, err = w.ensureWorkerReleased(ctx, actorRef, actor); err != nil {
return nil, err
}
3 participants
Fixes #643
The "Force Delete" feature allows deleting an actor from almost any state (e.g.,
RUNNING,CRASHED,SUSPENDING), bypassing the normal restriction that required the actor to beSUSPENDEDbefore deletion. This is crucial for cleaning up stuck or failed actors.Involved Components
sequenceDiagram autonumber actor Client participant Ateapi as Ateapi (Control Plane) participant Store as Store (DB) participant Atelet as Atelet (Node Agent) participant Ateom as Ateom (Runtime Agent) participant VM_Sandbox as VM/Sandbox (gVisor/CH) Client->>Ateapi: DeleteActor(Actor, force=true) activate Ateapi Ateapi->>Store: GetActor(Actor) Store-->>Ateapi: Actor State Note over Ateapi: MarkTerminatingStep Ateapi->>Ateapi: Verify state (allows almost any state if force=true) Ateapi->>Store: UpdateActor(Status=TERMINATING) alt Actor has Worker Assignment Note over Ateapi: CallAteletTerminateStep Ateapi->>Atelet: Terminate(ActorUID, TargetAteomUID) activate Atelet Atelet->>Ateom: TerminateWorkload(ActorUID) activate Ateom Ateom->>Ateom: Deactivate Networking alt Runtime is gVisor (ateom-gvisor) Ateom->>VM_Sandbox: runsc delete (containers) Note over Ateom, VM_Sandbox: Kills and deletes sandboxed containers else Runtime is MicroVM (ateom-microvm) Ateom->>VM_Sandbox: Shutdown VMM / Kill Process Note over Ateom, VM_Sandbox: Shuts down Cloud Hypervisor & virtiofsd end Ateom->>Ateom: Unmount OCI Overlays Ateom->>Ateom: Cleanup Actor Network Ateom-->>Atelet: TerminateWorkload Response deactivate Ateom Atelet->>Atelet: Unmount External Volumes on Node Atelet->>Atelet: Reset Actor Directories on Node Atelet-->>Ateapi: Terminate Response deactivate Atelet end Note over Ateapi: DetachVolumesForDeleteStep Ateapi->>Ateapi: Detach volumes from actor in control plane Note over Ateapi: ReleaseWorkerStep Ateapi->>Store: Get Worker Ateapi->>Store: UpdateWorker(Assignment=nil) (releases worker) Ateapi->>Store: UpdateActor(WorkerAssignment=nil, LocalSnapshotInfo=nil) Note over Ateapi: MarkDeletingStep Ateapi->>Store: UpdateActor(Status=DELETING, Volumes=DELETING) Note over Ateapi: DeleteVolumesStep Ateapi->>Ateapi: Delete volumes (durable storage) Note over Ateapi: FinalizeDeletedStep Ateapi->>Store: DeleteActor() (removes record) Ateapi-->>Client: Deleted ActorDetailed Step Descriptions (Control Plane)
LoadActorForDeleteStep: Fetches the latest actor state and template from the store.MarkTerminatingStep: Transition the actor status toTERMINATING. IfForceis true, this transition is allowed from any active state.CallAteletTerminateStep: If the actor is currently assigned to a worker node, this step dials theAteleton that node and requests termination.DetachVolumesForDeleteStep: Initiates detachment of any volumes associated with the actor.ReleaseWorkerStep: Clears the assignment on the worker resource in the store, making the worker available for other actors. It also clears the worker assignment on the actor record.MarkDeletingStep: Transitions the actor status toDELETINGand marks its volumes asDELETING.DeleteVolumesStep: Deletes the actual volume resources (e.g., GCS buckets or directories).FinalizeDeletedStep: Permanently removes the actor record from the store.Testing
Ran the E2E tests