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

Spin core refactor #763

Merged
merged 28 commits into from
Sep 29, 2022
Merged

Spin core refactor #763

merged 28 commits into from
Sep 29, 2022

Conversation

lann
Copy link
Collaborator

@lann lann commented Sep 13, 2022

This refactors Spin to use the new spin-core and spin-app crates that we've been developing in an internal prototype.

With a few small exceptions I have attempted to not change the visible behavior of spin up, but with so many updates I'm sure I've missed some things.

There are thousands of lines of changes here, so I've tried to split things up into reasonable commits for those who prefer to review that way.

@lann lann force-pushed the spin-core-refactor branch from 4df420f to 87ee7f5 Compare September 13, 2022 17:51
@lann lann mentioned this pull request Sep 13, 2022
@lann lann force-pushed the spin-core-refactor branch 2 times, most recently from 89d253d to 50b2b39 Compare September 13, 2022 20:15
crates/app/src/lib.rs Outdated Show resolved Hide resolved
crates/app/src/values.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@dicej dicej left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thanks for taking on this huge effort!

@Mossaka
Copy link
Contributor

Mossaka commented Sep 15, 2022

This looks great to me! I especially love the design of DynamocHostComponent that can update host components' data in runtime (whene http request comes in), and the design of LockedComponent.

I do wonder why do you decide to impl allowed_http_hosts as dynamic data in http. I thought this variable is statically decalred in spin.toml manifest file, and thus can be added to HostComponent Data before runtime. No?

In addition, what is the use cases of spin.lock?

@lann
Copy link
Collaborator Author

lann commented Sep 15, 2022

@Mossaka:

I do wonder why do you decide to impl allowed_http_hosts as dynamic data in http. I thought this variable is statically decalred in spin.toml manifest file, and thus can be added to HostComponent Data before runtime. No?

HostComponent Data is currently constructed per-request, not at startup. Preinitialization of some data would be possible but I don't see much benefit for the current set of host components.

In addition, what is the use cases of spin.lock?

One purpose is to provide a more stable interface between spin up and trigger executors, which should simplify the implementation of trigger executor plugins (see also #735).

@lann lann force-pushed the spin-core-refactor branch 3 times, most recently from dc57dd8 to 4053aef Compare September 19, 2022 14:41
@lann lann mentioned this pull request Sep 19, 2022
crates/app/src/lib.rs Show resolved Hide resolved
crates/core/src/host_component.rs Show resolved Hide resolved
crates/core/src/host_component.rs Show resolved Hide resolved
@lann lann force-pushed the spin-core-refactor branch from 4053aef to a35ed5d Compare September 20, 2022 16:46
@lann lann mentioned this pull request Sep 20, 2022
@radu-matei
Copy link
Member

Have switched to this branch as the Spin binary I am using on a daily basis, and I am hitting the error below when trying to run https://github.com/coderoflagos/bartholomew-sample.

Rendering "/content/index.md": Capabilities insufficient (os error 76)

@lann
Copy link
Collaborator Author

lann commented Sep 27, 2022

Have switched to this branch as the Spin binary I am using on a daily basis, and I am hitting the error below when trying to run https://github.com/coderoflagos/bartholomew-sample.

Rendering "/content/index.md": Capabilities insufficient (os error 76)

Bartholomew tries to create a cache file: https://github.com/fermyon/bartholomew/blob/c602220ee4da6b2a22b17f0b04ff8eba1b8adc75/src/content.rs#L133

Looks like Spin currently allows writing new files even without --allow-transitent-write

@lann lann force-pushed the spin-core-refactor branch 2 times, most recently from ff6f5db to c179934 Compare September 27, 2022 17:05
@lann
Copy link
Collaborator Author

lann commented Sep 27, 2022

Bartholomew issue should be fixed in fermyon/bartholomew#135

This will replace spin-trigger in a future commit.

Signed-off-by: Lann Martin <[email protected]>
Signed-off-by: Lann Martin <[email protected]>
It isn't clear why this was included in the first place and it doesn't
appear to be used now.

Signed-off-by: Lann Martin <[email protected]>
Also add AppComponent::require_metadata for interface consistency.

Signed-off-by: Lann Martin <[email protected]>
Also add AppComponent::require_metadata for interface consistency.

Signed-off-by: Lann Martin <[email protected]>
Signed-off-by: Lann Martin <[email protected]>
The new trigger code expects the working directory to be absolute, which
is the case for tempdirs but not necessarily `--temp`. Fix by
`.canonicalize()`ing the work dir.

Signed-off-by: Lann Martin <[email protected]>
@lann lann force-pushed the spin-core-refactor branch from c179934 to c12caa5 Compare September 29, 2022 18:43
Copy link
Member

@radu-matei radu-matei left a comment

Choose a reason for hiding this comment

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

I've been using a build from this branch as my main Spin binary for a while now and everything appears to be in order.

As we start working on various parts of the codebase, I am sure we will start finding small bits that we can improve, but overall, the improvements in this PR are great!

Thank you!

LGTM

@lann lann merged commit dfb3a90 into fermyon:main Sep 29, 2022
@lann lann deleted the spin-core-refactor branch September 29, 2022 20:36
etehtsea added a commit to etehtsea/spin that referenced this pull request Oct 2, 2022
Refs: fermyon#763.

Signed-off-by: Konstantin Shabanov <[email protected]>
etehtsea added a commit to etehtsea/spin that referenced this pull request Oct 2, 2022
Fixes the following error:

$ spin build up --follow-all
<..>
Successfully ran the build command for the Spin components.
Error: Failed to configure Redis trigger

Caused by:
    metadata error: missing required "redis_address"
Error: exit status: 1

Refs: fermyon#763.

Signed-off-by: Konstantin Shabanov <[email protected]>
etehtsea added a commit to etehtsea/spin that referenced this pull request Oct 2, 2022
Set base path in spin-http rather than joining it as part of the route.
Split test configs in spin-testing because the trigger's configuration
is different for http and redis.

Refs: fermyon#763.

Signed-off-by: Konstantin Shabanov <[email protected]>
etehtsea added a commit to etehtsea/spin that referenced this pull request Oct 2, 2022
Refs: fermyon#763.

Signed-off-by: Konstantin Shabanov <[email protected]>
etehtsea added a commit to etehtsea/spin that referenced this pull request Oct 2, 2022
Fixes the following error:

$ spin build up --follow-all
<..>
Successfully ran the build command for the Spin components.
Error: Failed to configure Redis trigger

Caused by:
    metadata error: missing required "redis_address"
Error: exit status: 1

Refs: fermyon#763.

Signed-off-by: Konstantin Shabanov <[email protected]>
etehtsea added a commit to etehtsea/spin that referenced this pull request Oct 2, 2022
Set base path in spin-http rather than joining it as part of the route.
Split test configs in spin-testing because the trigger's configuration
is different for http and redis.

Refs: fermyon#763.

Signed-off-by: Konstantin Shabanov <[email protected]>
etehtsea added a commit to etehtsea/spin that referenced this pull request Oct 3, 2022
Fixes the following error:

$ spin build up --follow-all
<..>
Successfully ran the build command for the Spin components.
Error: Failed to configure Redis trigger

Caused by:
    metadata error: missing required "redis_address"
Error: exit status: 1

Refs: fermyon#763.
Closes fermyon#799.

Signed-off-by: Konstantin Shabanov <[email protected]>
etehtsea added a commit to etehtsea/spin that referenced this pull request Oct 3, 2022
Set base path in spin-http rather than joining it as part of the route.
Split test configs in spin-testing because the trigger's configuration
is different for http and redis.

Refs: fermyon#763.

Signed-off-by: Konstantin Shabanov <[email protected]>
etehtsea added a commit to etehtsea/spin that referenced this pull request Oct 3, 2022
Refs: fermyon#763.

Signed-off-by: Konstantin Shabanov <[email protected]>
etehtsea added a commit to etehtsea/spin that referenced this pull request Oct 3, 2022
Fixes the following error:

$ spin build up --follow-all
<..>
Successfully ran the build command for the Spin components.
Error: Failed to configure Redis trigger

Caused by:
    metadata error: missing required "redis_address"
Error: exit status: 1

Refs: fermyon#763.
Closes fermyon#799.

Signed-off-by: Konstantin Shabanov <[email protected]>
etehtsea added a commit to etehtsea/spin that referenced this pull request Oct 3, 2022
Set base path in spin-http rather than joining it as part of the route.
Split test configs in spin-testing because the trigger's configuration
is different for http and redis.

Refs: fermyon#763.

Signed-off-by: Konstantin Shabanov <[email protected]>
etehtsea added a commit to etehtsea/spin that referenced this pull request Oct 3, 2022
Refs: fermyon#763.

Signed-off-by: Konstantin Shabanov <[email protected]>
etehtsea added a commit to etehtsea/spin that referenced this pull request Oct 3, 2022
Fixes the following error:

$ spin build up --follow-all
<..>
Successfully ran the build command for the Spin components.
Error: Failed to configure Redis trigger

Caused by:
    metadata error: missing required "redis_address"
Error: exit status: 1

Refs: fermyon#763.
Closes fermyon#799.

Signed-off-by: Konstantin Shabanov <[email protected]>
etehtsea added a commit to etehtsea/spin that referenced this pull request Oct 3, 2022
Set base path in spin-http rather than joining it as part of the route.
Split test configs in spin-testing because the trigger's configuration
is different for http and redis.

Refs: fermyon#763.

Signed-off-by: Konstantin Shabanov <[email protected]>
etehtsea added a commit to etehtsea/spin that referenced this pull request Oct 3, 2022
Refs: fermyon#763.

Signed-off-by: Konstantin Shabanov <[email protected]>
etehtsea added a commit to etehtsea/spin that referenced this pull request Oct 3, 2022
Fixes the following error:

$ spin build up --follow-all
<..>
Successfully ran the build command for the Spin components.
Error: Failed to configure Redis trigger

Caused by:
    metadata error: missing required "redis_address"
Error: exit status: 1

Refs: fermyon#763.
Closes fermyon#799.

Signed-off-by: Konstantin Shabanov <[email protected]>
etehtsea added a commit to etehtsea/spin that referenced this pull request Oct 3, 2022
Set base path in spin-http rather than joining it as part of the route.
Split test configs in spin-testing because the trigger's configuration
is different for http and redis.

Refs: fermyon#763.

Signed-off-by: Konstantin Shabanov <[email protected]>
etehtsea added a commit to etehtsea/spin that referenced this pull request Oct 3, 2022
Refs: fermyon#763.

Signed-off-by: Konstantin Shabanov <[email protected]>
etehtsea added a commit to etehtsea/spin that referenced this pull request Oct 3, 2022
Fixes the following error:

$ spin build up --follow-all
<..>
Successfully ran the build command for the Spin components.
Error: Failed to configure Redis trigger

Caused by:
    metadata error: missing required "redis_address"
Error: exit status: 1

Refs: fermyon#763.
Closes fermyon#799.

Signed-off-by: Konstantin Shabanov <[email protected]>
etehtsea added a commit to etehtsea/spin that referenced this pull request Oct 3, 2022
Set base path in spin-http rather than joining it as part of the route.
Split test configs in spin-testing because the trigger's configuration
is different for http and redis.

Refs: fermyon#763.

Signed-off-by: Konstantin Shabanov <[email protected]>
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.

5 participants