Actor crash detection (part 1/3): crash an actor if ateom pod has terminated#493
Actor crash detection (part 1/3): crash an actor if ateom pod has terminated#493Zoe Zhao (zoez7) wants to merge 4 commits into
Conversation
49e0181 to
6c9c30d
Compare
6c9c30d to
665a224
Compare
fc9d1a1 to
9d32e47
Compare
Eitan Yarmush (EItanya)
left a comment
Eitan Yarmush (EItanya)
left a comment
There was a problem hiding this comment.
Just some clarifying questions
| } | ||
| } | ||
| // Deleting the pod, because the actor network may not have been cleaned up from the Ateom Pod's net NS. | ||
| if err := s.kubeClient.CoreV1().Pods(pod.Namespace).Delete(ctx, pod.Name, metav1.DeleteOptions{ |
There was a problem hiding this comment.
Maybe I'm totally misunderstanding something, if the ateom crashed wouldn't that automatically rollout the pod because we're using a Deployment under the hood?
There was a problem hiding this comment.
This is because the ateom container's crash will be detected by kubelet, which will restart the ateom container in the same pod.
You are right that once we delete this pod, the Deployment will automatically create a new one.
There was a problem hiding this comment.
🤦
And the worry is that there will be dangling networking and/or other resources if we don't manually kill the pod?
| if actor.GetAteomPodNamespace() != namespace || actor.GetAteomPodName() != podName { | ||
| // Only crash the actor if it's still assigned to the same worker. | ||
| if actor.GetAteomPodNamespace() == "" || actor.GetAteomPodName() == "" || actor.GetAteomPodUid() == "" { | ||
| slog.WarnContext(ctx, "cannot release actor on worker", slog.String("ateom pod namesace", actor.GetAteomPodNamespace()), slog.String("ateom name", actor.GetAteomPodName()), slog.String("ateom pod uid", actor.GetAteomPodUid())) |
There was a problem hiding this comment.
nit: Could we clarify this warning? My understanding isn't that we can't release the actor, it's that the actor is no longer assigned to this worker.
Maybe something along the lines:
"Actor is no longer assigned to worker, skipping crash."
| slog.WarnContext(ctx, "actor is not in running state") | ||
| return nil | ||
| } | ||
| if actor.GetAteomPodNamespace() != worker.GetWorkerNamespace() || actor.GetAteomPodName() != worker.GetWorkerPod() || actor.GetAteomPodUid() != worker.GetWorkerPodUid() { |
There was a problem hiding this comment.
nit: This is a similar class of warning / error as above, where the actor was suspended and no longer assigned to the worker. We should likely log in both cases or neither.
| return | ||
| case worker.Assignment == nil: | ||
| // If the worker is not assigned to any actor, delete the worker from pool. | ||
| if err := s.persistence.DeleteWorker(ctx, pod.Namespace, pool, pod.Name); err != nil { |
There was a problem hiding this comment.
I don't think this is a new issue, so I don't think we need to put a fix in this PR. I believe there are some race conditions around DeleteWorker. However unlikely, it's possible a different actor gets assigned to this worker after we check the state. So we could potentially be deleting a worker that has an actor assigned. We already check the expected version during updates, but maybe we need to do this for deletes as well?
| slog.InfoContext(ctx, "Syncer: worker ateom terminated; crashing actor and recycling pod", | ||
| slog.String("worker", pod.Namespace+"/"+pod.Name)) | ||
| pool := pod.Labels[workerPodLabel] | ||
| worker, err := s.persistence.GetWorker(ctx, pod.Namespace, pool, pod.Name) |
There was a problem hiding this comment.
To avoid workers being re-used before we have a chance to clean them up, I wonder if it's worth doing something similar to how I'm going to handle graceful termination.
- Mark the worker as 'CRASHED' (I'll use 'DRAINING' for graceful termination).
- In the ateom, be able to detect that it crashed and reject any further suspend / resume attempts.
This mitigates the race conditions around state detection and clean up.
3 participants
Part of #292
This is the first CL for detecting a running Actor has crashed, and mark it as so in Redis. The overall plan is to
This CL implements the logic in Workerpool syncer to detect ateom container terminated. There are 2 more PRs coming which will implement the logic to monitor actor process in ateom-gvisor and ateom-microvm.