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

Component filtering #197

Merged
merged 1 commit into from
Oct 15, 2024
Merged

Conversation

kate-goldenring
Copy link
Collaborator

@kate-goldenring kate-goldenring commented Sep 30, 2024

Spin CLI added support for spin up --component-id "foo" --component-id "bar" to run a subset of components of a Spin app in fermyon/spin#2826.

This brings that functionality to the shim assuming that the components to support are supplied in SPIN_COMPONENTS_TO_RETAIN environment variable of a Pod as a comma separated list (SPIN_COMPONENTS_TO_RETAIN="foo,bar").

In the Spin Operator, we can add a field to the SpinApp CR spec called components that lists which components of an app to retain in a pod.

This is on top of @rylev's changes #189. The additive changes are in 1006722

@kate-goldenring kate-goldenring marked this pull request as ready for review October 10, 2024 22:40
Copy link
Contributor

@rylev rylev left a comment

Choose a reason for hiding this comment

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

Looks fine to me - I'm curious if there's anyway to expose some of this logic around component retention in Spin so that the spinkube doesn't need to copy the logic.

@kate-goldenring
Copy link
Collaborator Author

Looks fine to me - I'm curious if there's anyway to expose some of this logic around component retention in Spin so that the spinkube doesn't need to copy the logic.

@rylev i agree that this is entirely duplicated code. I brought up this topic in the Spin PR but we punted not knowing the best place for it: fermyon/spin#2826 (comment).

That being said, I'll look at moving it to a crate

@kate-goldenring
Copy link
Collaborator Author

@Mossaka @devigned @jsturtevant @radu-matei would love your thoughts on this.

@devigned
Copy link
Contributor

I don't have a strong opinion about this particular change. It makes sense and is minimally invasive.

The point about duplicating code is probably going to continue without a better option. I can't recall the context of the conversation, but at some point recently I believe @Mossaka and I were discussing if many of these changes could be avoided by exposing the spin.toml in the SpinApp resource as a structure in the CRD. For this example, a user may be able to specify a SpinApp custom resource with the spin.toml that only contained those limited components.

I think extending the SpinApp with the spin.toml structures might be useful for other cases. Thoughts?

Copy link
Collaborator

@Mossaka Mossaka left a comment

Choose a reason for hiding this comment

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

LGTM.

One concern that i have is that this seems like a lot of sharing code between here and fermyon/spin#2826 and this smell tells me that there is an opportunity to extract them to a library and share between these two projects.

@kate-goldenring
Copy link
Collaborator Author

@Mossaka, @rylev brought up the same point that i addressed here: #197 (comment)

Copy link
Contributor

@jsturtevant jsturtevant left a comment

Choose a reason for hiding this comment

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

LGTM

@kate-goldenring
Copy link
Collaborator Author

kate-goldenring commented Oct 15, 2024

For this example, a user may be able to specify a SpinApp custom resource with the spin.toml that only contained those limited components.

@devigned, I always thought changing/duplicating spin.tomls to be a worse developer experience but that is an interesting perspective that a set of components could have several spin.tomls. The counter is that the app manifest is bundled in the OCI artifact with the components, so the advantage of having configuration outside of the spin.toml is being able to use the same oci artifact in different ways. runtime-config.toml is another way to do that too

@devigned
Copy link
Contributor

devigned commented Oct 15, 2024

@devigned, I always thought changing/duplicating spin.tomls to be a worse developer experience but that is an interesting perspective that a set of components could have several spin.tomls. The counter is that the app manifest is bundled in the OCI artifact with the components, so the advantage of having configuration outside of the spin.toml is being able to use the same oci artifact in different ways. runtime-config.toml is another way to do that too

I felt the same way about duplication of the spin.toml initially. What made me rethink my position is that more and more I think we are going to see components distributed over OCI with only the component(s) as the payload of the OCI artifact. So, would we really be duplicating the spin.toml or defining it for the first time for an existing component? For example, someone packages up a wasi:http component in an OCI artifact which they ran using Wasmtime. Now they would like to use that as a SpinApp. How would they do that?

  1. They could pull down the artifact, unpack it, add a spin.toml, then repackage and deploy.
  2. They could add a reference to OCI artifact in a spin.toml, then push that toml only artifact to OCI which reference another OCI artifact containing the wasi:http component.
  3. They could use the OCI artifact in an inline spin.toml within the SpinApp which pointed to the OCI artifact containing the wasi:http component.

I think these are some interesting cases which deserve some consideration.

@Mossaka
Copy link
Collaborator

Mossaka commented Oct 15, 2024

Happy to merge this PR and create an issue for the points that @devigned raised.

I like idea 2 and 3 that @devigned mentioned

@kate-goldenring
Copy link
Collaborator Author

@devigned, I always thought changing/duplicating spin.tomls to be a worse developer experience but that is an interesting perspective that a set of components could have several spin.tomls. The counter is that the app manifest is bundled in the OCI artifact with the components, so the advantage of having configuration outside of the spin.toml is being able to use the same oci artifact in different ways. runtime-config.toml is another way to do that too

I felt the same way about duplication of the spin.toml initially. What made me rethink my position is that more and more I think we are going to see components distributed over OCI with only the component(s) as the payload of the OCI artifact. So, would we really be duplicating the spin.toml or defining it for the first time for an existing component? For example, someone packages up a wasi:http component in an OCI artifact which they ran using Wasmtime. Now they would like to use that as a SpinApp. How would they do that?

  1. They could pull down the artifact, unpack it, add a spin.toml, then repackage and deploy.
  2. They could add a reference to OCI artifact in a spin.toml, then push that toml only artifact to OCI which reference another OCI artifact containing the wasi:http component.
  3. They could use the OCI artifact in an inline spin.toml within the SpinApp which pointed to the OCI artifact containing the wasi:http component.

I think these are some interesting cases which deserve some consideration.

@devigned have you looked at the component dependencies work @fibonacci1729 has brought into Spin? It enables referencing upstream components in your spin.toml. In this way, nothing special would need to be added to the SpinApp because it is in the spin.toml.

@Mossaka for continuing this discussion in an issue, I feel like Spin may be the best place for that since we are talking about OCI artifact contents

@Mossaka
Copy link
Collaborator

Mossaka commented Oct 15, 2024

Merging it rn.

@Mossaka Mossaka merged commit 71c8351 into spinframework:main Oct 15, 2024
8 checks passed
@Mossaka
Copy link
Collaborator

Mossaka commented Oct 15, 2024

oh oops, I forgot to mention, can you add this change to the CHANGELOG, starting with Unreleased section? 🙏

@kate-goldenring
Copy link
Collaborator Author

oh oops, I forgot to mention, can you add this change to the CHANGELOG, starting with Unreleased section? 🙏

Is this something we will do for all PRs? I am not sure I understand making that a manual process rather than generating a CHANGELOG on release or having someone do it all at once. Is this a best practice you've seen in other projects? I can see how it is nice to be able to see what the changes are on main easily.

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.

6 participants