Update to latest Spin version with Factors#189
Conversation
kate-goldenring
left a comment
kate-goldenring
left a comment
There was a problem hiding this comment.
This LGTM. This shows how the factor work greatly reduces the complexity of building Spin runtimes! I will be curious how e2e tests fair once this is marked Ready.
containerd-shim-spin/src/engine.rs
Outdated
| }; | ||
| let mut futures_list = Vec::with_capacity(trigger_types.len()); | ||
| for trigger_type in trigger_types.iter() { | ||
| let app = spin_app::App::new("TODO", app.clone()); |
There was a problem hiding this comment.
How is app creation avoided in the Spin CLI? By directly invoking the triggers via commandline? How is app ID used? Should we just set the ID to be the locked app name for now?
There was a problem hiding this comment.
This seems to be something we might be able to change in upstream as that app id is rarely actually used. Currently, in the Spin code base it's only being used in the Redis trigger for telemetry purposes. It's used by some other runtimes simply as a unique identifier.
I've opened an issue for this: spinframework/spin#2852
In the meantime, do we have any sort of identifier we can use to globally identified the running application?
There was a problem hiding this comment.
By globally do you mean unique to the cluster too? Container name may be a good option
There was a problem hiding this comment.
The uniqueness isn't necessary (as far as I can tell) for everything to work properly - it is just needed for proper diagnostics as this string might be exposed to the user in telemetry. Therefore, I think we want something that makes sense to users if they see it.
I'm not sure what the best thing to use is - if you say container name works well that's fine by me. How do I access that information?
There was a problem hiding this comment.
We should be able to use the HOSTNAME env var to get the container name. This is not a unique container id; rather just the container name (ie busybox). Furthermore, if the container is using hostNetwork, the hostname becomes the node hostname, but in production, that is rarely done. For now HOSTNAME sounds like the best bet
There was a problem hiding this comment.
Did a quick check locally, printing the value of HOSTNAME from the shim and looks like it is the fully unique container name: "container name: simple-spinapp-59c4d9668-gz8dr". If you just run locally with ctr run it will use the node/machine hostname which is fine as that is not the normal execution path.
There was a problem hiding this comment.
@rylev what are your thoughts on using HOSTNAME?
There was a problem hiding this comment.
I don't have a better idea. I suppose this isn't hard to change in the future and since nothing is really relying on the id being a particular format (it's just to help humans), we don't really even need to worry about backwards compatibility.
Mossaka
force-pushed
the
refactoring
branch
from
7086cb3 to
79d54ba
Compare
|
Need a rebase |
rylev
force-pushed
the
factors
branch
3 times, most recently
from
b9d5025 to
15e6a32
Compare
|
@rylev can we mark this as "Ready for Review"? |
rylev
requested review from
devigned,
jsturtevant and
radu-matei
as code owners
|
@kate-goldenring I've marked this PR as ready to review, but we shouldn't merge until we've figured out the answer to this thread. |
rylev
force-pushed
the
factors
branch
2 times, most recently
from
b0f49a9 to
92d3461
Compare
Signed-off-by: Ryan Levick <ryan.levick@fermyon.com>
Signed-off-by: Ryan Levick <ryan.levick@fermyon.com>
Signed-off-by: Ryan Levick <ryan.levick@fermyon.com>
Signed-off-by: Ryan Levick <ryan.levick@fermyon.com>
Signed-off-by: Ryan Levick <ryan.levick@fermyon.com>
Signed-off-by: Ryan Levick <ryan.levick@fermyon.com>
Signed-off-by: Ryan Levick <ryan.levick@fermyon.com>
Signed-off-by: Ryan Levick <ryan.levick@fermyon.com>
|
@rylev I am not sure why the MQTT test is failing. I've tested it locally and it passes. I am rerunning the tests to see if it is a race case with how the test is written |
|
@rylev I there was an issue with spin.toml parsing from the latest changes to the MQTT trigger that is causing mqtt tests to fail: spinframework/spin-trigger-mqtt#40. This should be the fix spinframework/spin-trigger-mqtt#41 |
Signed-off-by: Ryan Levick <ryan.levick@fermyon.com>
3 participants
This is ready for review, but it isn't quite ready to land as other PRs need to land first: