Support multi trigger apps #53
Conversation
rajatjindal
force-pushed
the
support-multi-trigger-app
branch
5 times, most recently
from
ef00656 to
cbcac65
Compare
rajatjindal
requested review from
kate-goldenring,
jsturtevant and
devigned
as code owners
containerd-shim-spin/src/engine.rs
Outdated
| // exit as soon as any of the trigger completes/exits | ||
| let (result, _, rest) = future::select_all(futures_list).await; |
There was a problem hiding this comment.
Why as soon as one completes? Would that mean if the result of one of the others failed we wouldn't know about it? should we be waiting for all triggers finish, or some type of queue to handle the various results?
There was a problem hiding this comment.
Hello James,
this is to keep it in sync with the Spin behavior
There was a problem hiding this comment.
could we add a log to print which trigger failed / terminates?
There was a problem hiding this comment.
👍
time="2024-04-04T02:58:27.951588261Z" level=info msg=" >>> trigger type 'redis' exited"
There was a problem hiding this comment.
@jsturtevant - does that answer your question? (re: exit behavior). thanks.
There was a problem hiding this comment.
this is to keep it in sync with the Spin behavior
Interesting! I guess we should do the same as spin cli, thanks!
There was a problem hiding this comment.
one other interesting difference (please correct me if I am wrong), is that Spin starts a new process for most of these triggers and shim does not.
I am not sure if that makes any difference or if we could do that (start further child process) in shim at all. but i thought it will be good to note that difference here.
rajatjindal
force-pushed
the
support-multi-trigger-app
branch
from
a1dc1a6 to
cf672ff
Compare
rajatjindal
force-pushed
the
support-multi-trigger-app
branch
from
cf672ff to
12aa63f
Compare
|
Hi @kate-goldenring / @Mossaka , I have rebased this PR and it is ready for review again. thank you |
|
Need a rebase |
|
@rajatjindal it looks like the multi trigger app failed to run in the CI (even though the test passed): https://github.com/spinkube/containerd-shim-spin/actions/runs/8619544944/job/23625457710?pr=53#step:8:1234 |
rajatjindal
force-pushed
the
support-multi-trigger-app
branch
3 times, most recently
from
0bcc203 to
ec798c4
Compare
Signed-off-by: Rajat Jindal <rajatjindal83@gmail.com>
rajatjindal
force-pushed
the
support-multi-trigger-app
branch
from
9252b50 to
3b8a4bd
Compare
I looked at the failure. this test multi-trigger app also require redis pod to be running. I think what we saw in that CI failure was a transient error while redis-pod was getting ready. The assertions in integration tests would pass only if the multi-trigger app is running and expected responses are returned from it. I reran the tests as part of rebasing this PR, and tests seems to be working as expected. Please let me know if you think there is something else that might be triggering this behavior, i am happy to investigate more in that case. |
Signed-off-by: Rajat Jindal <rajatjindal83@gmail.com>
Signed-off-by: Rajat Jindal <rajatjindal83@gmail.com>
rajatjindal
force-pushed
the
support-multi-trigger-app
branch
from
3b8a4bd to
9229593
Compare
|
Hi @kate-goldenring @Mossaka , this PR has been rebased now, and seems to be ready for final set of review/merge. Thank you so much for your patience while I got this PR into a stable state. |
This PR adds support for running Spin apps with multiple triggers. I opened the draft PR to be able to collect some initial feedback.
I've also added a testcase to demonstrate running the multi-trigger-app workload and will be working on adding the integration test for the same.