fix: skip empty node name in local snapshot info after pause finalization#329
fix: skip empty node name in local snapshot info after pause finalization#329Mesut Oezdil (mesutoezdil) wants to merge 5 commits into
Conversation
|
Mesut Oezdil (@mesutoezdil) - could you please explain how you was able to reproduce the issue, since the proposed fix will not work. The root cause of the issue is somewhere else. |
here are the reproduction steps, i wrote 2 unit tests: T1 // (TestFinalizePausedStep_WorkerGone): worker pod is gone from db before FinalizePausedStep runs. GetWorker returns ErrNotFound. 
see your point about root cause. Even with this fix, resume could pick a wrong node and snapshot restore fails. |
|
Zoe Zhao (@zoez7) is working on a PR introducing the CRASHED state. Could you please wait for her PR to be submitted and after it make a change to move the actor to this stage in case of this condition. |
f14d87d to
907531c
Compare
Without a node name the local snapshot can never be resumed since findFreeWorker would search for a worker on an unknown node forever. Move the actor to CRASHED instead of leaving it stuck in PAUSED. CRASHED is now a terminal state for the workflow, mirroring PAUSED.
d2f4152 to
60ac20b
Compare
| // so the actor can never be resumed (findFreeWorker would search for a | ||
| // worker on an unknown node forever). Crash it instead of leaving it | ||
| // stuck in PAUSED. | ||
| slog.Warn("Node name not found during finalize pause, crashing actor", "actor", input.ActorName) |
There was a problem hiding this comment.
Let's use slog.ErrorCtx instead?
There was a problem hiding this comment.
thx zoe, done
0a5e21d
| SnapshotPrefix: latestActor.InProgressSnapshot, | ||
| } | ||
| if nodeName != "" { | ||
| localInfo.NodeVmsWithLocalSnapshots = []string{nodeName} |
There was a problem hiding this comment.
resume from "SnapshotType_SNAPSHOT_TYPE_LOCAL" supposed to land on machine where files are stored locally, otherwise resume will fail.
we need to find a root cause of the issue that node is empty. This is a bug/exception
There was a problem hiding this comment.
it should be a separete issue i think,i dont solve it in this pr
There was a problem hiding this comment.
+1 Dmitry Berkovich (@dberkov) I think you described a different bug than the one this PR is solving
I think this PR solves a idempotency problem that we don't handle today.
- CallAteletPauseStep succeeded, but we havn't called FinalizePausedStep yet
- ate-apiserver restarts.
- WorkerPod gets deleted.
- Retry PauseActor.
Now we don't know which node the snapshot is on, we can only crash the actor.
I think we should check this PR in as a temporary fix, but we also need to add node name as a field in the actor API, so Pause can be idempotent.
| localInfo := &ateapipb.LocalSnapshotInfo{ | ||
| SnapshotPrefix: latestActor.InProgressSnapshot, | ||
| } | ||
| if nodeName != "" { |
There was a problem hiding this comment.
the nodeName != "" no more relevant, since it is already validated above
There was a problem hiding this comment.
done
3 participants
When the worker pod is gone from the DB during pause finalization, nodeName stays empty.
The code still wrote NodeVmsWithLocalSnapshots: []string{""}, so on the next ResumeActor the scheduler looked for a worker with node name "", found none, and returned "no free workers available" permanently.
Fix: only set NodeVmsWithLocalSnapshots when nodeName is non-empty.