Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update to latest Spin version with Factors #189

Merged
merged 10 commits into from
Oct 9, 2024
Merged

Conversation

rylev
Copy link
Contributor

@rylev rylev commented Sep 17, 2024

Copy link
Collaborator

@kate-goldenring kate-goldenring left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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());
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@rylev rylev Sep 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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: fermyon/spin#2852

In the meantime, do we have any sort of identifier we can use to globally identified the running application?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By globally do you mean unique to the cluster too? Container name may be a good option

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

@kate-goldenring kate-goldenring Sep 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rylev what are your thoughts on using HOSTNAME?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

containerd-shim-spin/src/engine.rs Outdated Show resolved Hide resolved
@Mossaka
Copy link
Member

Mossaka commented Sep 20, 2024

Need a rebase

@rylev rylev changed the base branch from refactoring to main September 20, 2024 10:09
conformance-tests/src/main.rs Show resolved Hide resolved
containerd-shim-spin/src/engine.rs Outdated Show resolved Hide resolved
rust-toolchain.toml Outdated Show resolved Hide resolved
@rylev rylev force-pushed the factors branch 3 times, most recently from b9d5025 to 15e6a32 Compare September 23, 2024 12:11
@kate-goldenring
Copy link
Collaborator

@rylev can we mark this as "Ready for Review"?

@rylev rylev marked this pull request as ready for review September 24, 2024 11:06
@rylev
Copy link
Contributor Author

rylev commented Sep 24, 2024

@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.

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>
Signed-off-by: Ryan Levick <ryan.levick@fermyon.com>
@kate-goldenring
Copy link
Collaborator

@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

@kate-goldenring
Copy link
Collaborator

@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: spinkube/spin-trigger-mqtt#40.

This should be the fix spinkube/spin-trigger-mqtt#41

Signed-off-by: Ryan Levick <ryan.levick@fermyon.com>
Copy link
Collaborator

@kate-goldenring kate-goldenring left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉

@Mossaka Mossaka merged commit 10e5292 into spinkube:main Oct 9, 2024
8 checks passed
@rylev rylev deleted the factors branch October 11, 2024 08:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants