Add CheckPrerequisite to WorkflowStep to validate state machine edges#374
Add CheckPrerequisite to WorkflowStep to validate state machine edges#374Zoe Zhao (zoez7) wants to merge 2 commits into
Conversation
Zoe Zhao (zoez7)
force-pushed
the
check-prerequisite
branch
4 times, most recently
from
9e8acdf to
3921ac8
Compare
| } | ||
| func (s *AssignWorkerStep) CheckPrerequisite(ctx context.Context, input *ResumeInput, state *ResumeState) error { | ||
| // The resume edge exists from SUSPENDED and PAUSED. | ||
| // RESUMING is allowed for retrying this step. |
There was a problem hiding this comment.
This is because of line 168 below.
There was a problem hiding this comment.
could you please clarify what is special at line 168? Is it for cases it was an error in resuming process and we are OK to try resuming one more time?
Same case might happening with Pausing and the CheckPrerequisite for CallAteletPauseStep does not accept pausing state.
| func (s *FinalizeSuspendedStep) Name() string { return "FinalizeSuspended" } | ||
| func (s *FinalizeSuspendedStep) IsComplete(ctx context.Context, input *SuspendInput, state *SuspendState) (bool, error) { | ||
| // The workflow is completely done ONLY if the status is SUSPENDED *and* we've successfully freed the worker. | ||
| return state.Actor.GetStatus() == ateapipb.Actor_STATUS_SUSPENDED && state.Actor.GetAteomPodNamespace() == "", nil |
There was a problem hiding this comment.
I removed this check, because we actually update the Actor's status and worker assignment in the same UpdateActor request in Execute() below, so we only need to check if status==suspended.
Zoe Zhao (zoez7)
force-pushed
the
check-prerequisite
branch
from
3921ac8 to
ec3b70c
Compare
Zoe Zhao (zoez7)
force-pushed
the
check-prerequisite
branch
from
ec3b70c to
4104a7d
Compare
github-project-automation
Bot
moved this to Todo
in Substrate Milestones (go/substrate-milestones)
| return nil | ||
| } | ||
| func (s *CallAteletPauseStep) Execute(ctx context.Context, input *PauseInput, state *PauseState) error { | ||
| if state.Actor.GetAteomPodNamespace() == "" || state.Actor.GetAteomPodName() == "" { |
There was a problem hiding this comment.
this is a pre-requisite too, however function in current implementation does not support transition to crashActor.
Might be rename the "CheckPrerequisite" to different name? might be allow transition to crash? or might be from the beginning we were not supposed to be in this state, that actor does not have pod or namespace?
| // seeded actor statuses, covering both the rejected and the idempotent-success | ||
| // paths. The atelet dialer is nil, so any step that unexpectedly reaches it | ||
| // panics. | ||
| func TestPauseActorWorkflow(t *testing.T) { |
There was a problem hiding this comment.
TestPauseSteps_CheckPrerequisite validates Prerequisite that matches name of the test.
In this case, the name is generic for entire Step, however it validates CheckPrerequisite transition only.
3 participants
Fixes #369
Add a CheckPrerequisite method to the WorkflowStep interface, called by RunWorkflow after IsComplete returns false and before Execute, so that each workflow validates its actor state-machine edge up front while retried (reentrant) workflows still fast-forward past completed steps.