Fix behavior of --wait when used together with --agent - #78
Conversation
53375b3 to
15a2bcf
Compare
4f59925 to
f691a92
Compare
f691a92 to
dc0c5f8
Compare
gusostow
left a comment
gusostow
left a comment
There was a problem hiding this comment.
Congrats on the Rust! This seems like it could take a little more work to get right. But this should be good enough to start testing with even if it's not released.
| // Wait for work to be assigned or shutdown signal | ||
| tokio::select! { | ||
| _ = procs.wait_for_shutdown() => return, | ||
| _ = tokio::time::sleep(tokio::time::Duration::from_millis(100)) => {}, |
There was a problem hiding this comment.
I think this should use the procs notification mechanism instead of polling
| // Ready to shut down now. | ||
| procs.set_shutdown(shutdown::State::Done); | ||
| // Work has been assigned! Set shutdown to Idling to prevent accepting more work | ||
| procs.set_shutdown(shutdown::State::Idling); |
There was a problem hiding this comment.
Do we need to worry about race condition here where proc start request comes in before we start idling?
626f32b to
7a6596a
Compare
7a6596a to
3014e38
Compare
6d93738 to
23d520c
Compare
There was a problem hiding this comment.
Congrats on the Rust! This seems like it could take a little more work to get right.
Thanks! Yeah the PR was still work in progress, but I appreciated the early feedback since it was your last day before holidays.
But this should be good enough to start testing with even if it's not released.
I'd actually like to get this released so that it's easier for me to test within containers running on EC2 instances. I'll ask Oleh to review.
Do we need to worry about race condition here where proc start request comes in before we start idling?
I've actually decided to not limit the Procstar agent to accept a single process when using the --wait mode. This seems more in line with Alex original design and also, for what concerns the Apsis on ECS project, we will enforce a single run on each Procstar through a unique group_id (or connection_id). Therefore there's probably no need to make --wait that strict; in case, we could also do that change later on.
I've also added a configurable timeout that will make the agent to shutdown in case no processes are assigned within that time window.
3 participants
Currently, using
--waitwith--agentdoes not work, i.e. a Procstar agent would connect to Apsis but:ShutdownState.doneand so it would not be available for use)After this change, when using
--waitand--agentthe Procstar agent:ShutdownState.idlingso not to accept any more runsI've tested this with a local Apsis and Procstar.