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

SKIP 003: Configuring Runtime Options in the Spin Containerd Shim #4

Merged
merged 5 commits into from
Jul 16, 2024

Conversation

kate-goldenring
Copy link
Contributor

No description provided.

@kate-goldenring kate-goldenring marked this pull request as draft June 10, 2024 22:49
containerd-shim-spin-config.md Outdated Show resolved Hide resolved
containerd-shim-spin-config.md Outdated Show resolved Hide resolved
containerd-shim-spin-config.md Outdated Show resolved Hide resolved
containerd-shim-spin-config.md Outdated Show resolved Hide resolved
containerd-shim-spin-config.md Outdated Show resolved Hide resolved
containerd-shim-spin-config.md Outdated Show resolved Hide resolved
containerd-shim-spin-config.md Outdated Show resolved Hide resolved
containerd-shim-spin-config.md Outdated Show resolved Hide resolved
@kate-goldenring
Copy link
Contributor Author

@Mossaka @jsturtevant, I want to add a section on considerations for shared mode in the v3 containerd task api, but i do not fully understand the implications of the changes coming with it. The Runwasi readme makes the following distinction between normal and shared sandbox modes:

There are two modes of operation supported:

"Normal" mode where there is 1 shim process per container or k8s pod.
"Shared" mode where there is a single manager service running all shims in process.

I want to make sure I understand "running all shims in process" correctly. In shared mode, we will have one daemon listener per node that listens for all requests to all Spin apps, right? Is the following a correct summary?

Currently, there is a shim process that runs for the lifetime of each container. This is comparable to a spin up per container. Once runwasi supports the containerd v3 task service, the shim engine can move to supporting a "shared" mode wherein one manager process listens and executes the Spin apps all within the same process. In this scenario where there is one listener for all Spin apps, certain environment variables no longer are relevant to an individual Spin app (such as the Spin listen address).

@Mossaka
Copy link
Member

Mossaka commented Jun 16, 2024

@kate-goldenring following up on the containerd/runwasi#619 that @jsturtevant raised. We were addressing this issue in a lower level as I've reached out to the youki's maintainers. If we are to push the environment variables, from PodSpec, from the shim process to the execute() function, that will provide the flexibility to use these variables either as ones for the Wasm runtime, or for the WASI Context.

@kate-goldenring
Copy link
Contributor Author

@Mossaka I believe I have been suggesting the opposite: that we do not consider container env vars as ones for the instance rather as arguments for the wasm runtime. Instead, I believe configuration variables should be used to set values to the wasm instance

@kate-goldenring
Copy link
Contributor Author

kate-goldenring commented Jun 16, 2024

I think a key thing I am trying to understand is if we do not allow for configuring the wasm runtime/spin engine from container env vars, how will we do app level execution configuration? My other consideration is whether env vars are something Spin users use or whether configuration variables are sufficient

@Mossaka
Copy link
Member

Mossaka commented Jun 16, 2024

I believe I have been suggesting the opposite: that we do not consider container env vars as ones for the instance rather as arguments for the wasm runtime.

Oh yeah, I understood. The work youki is doing / will be doing does not force env vars to be used for Wasm instance nor for Wasm runtime. It's up to the Executor implementor to decide. What it does, however, is to NOT mutate the shim process's env vars (or more precisely providing an option to not mutate the shim process's env vars), addressing the boundary issue @jsturtevant raised.

Sorry I should clarify: it will not affect what you proposed in this SKIP. To the opposite, it will help us set up a clear boundary (in terms of env vars) on "shim process" vs. "Wasm runtime" and "Wasm instance".

@kate-goldenring
Copy link
Contributor Author

@jsturtevant @Mossaka @radu-matei I am marking this as ready to review. The main evolution is that I have swung away from using application variables and instead suggest we forward container envs to components in order to not limit which Kubernetes libraries we support and to provide the most kubernetes native experience

containerd-shim-spin-config.md Outdated Show resolved Hide resolved
containerd-shim-spin-config.md Outdated Show resolved Hide resolved
containerd-shim-spin-config.md Outdated Show resolved Hide resolved
@kate-goldenring
Copy link
Contributor Author

@radu-matei @jsturtevant @Mossaka @calebschoepp I reduced the scope of this SKIP to remove the CRD design updates for config for the Spin Operator. I didn't want to bloat the focus of the SKIP. This SKIP is really about how the shim consumes config not how it gets there. I think it should be ready for another round of reviews. You should hopefully find it simplified


| Key | Example Value | Scope |
|---------------------------------|---------------|-----------------------------------------------------------------------------------------------------------------------------------------------------------------|
| DISABLE_PRECOMPILATION | "true" | shim |

Choose a reason for hiding this comment

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

what does shim mean here? Runwasi could also read this and disable it at https://github.com/containerd/runwasi/blob/ac4333ed53f246a0ec963c5ecfdef32a73d73a08/crates/containerd-shim-wasm/src/sys/unix/container/instance.rs#L48-L53, this way each shim doesn't need to handle this logic

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To me, it makes sense that this is piped such that the shim engine returns false to the can_precompile call. I also see how runwasi could make the determination as well.

| Key | Spin CLI | Example Value |
|-----------------------------|-------------------------------------------------------------|-------------------------|
| SPIN_HTTP_LISTEN_ADDR | `spin up --listen` | "0.0.0.0:3000" |
| OTEL_EXPORTER_OTLP_ENDPOINT | `OTEL_EXPORTER_OTLP_ENDPOINT=http://123.4.5.6:4318 spin up` | "http://123.4.5.6:4318" |

Choose a reason for hiding this comment

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

@Mossaka does this need to be prefixed to not interact with the shim's endpoint?

Choose a reason for hiding this comment

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

I'm curious @Mossaka if you had any issues testing the new shim otel stuff with the spin shim. Were there any panics or errors?

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

I don't know the answer. While using standardized environment variables for OTLP collectors seems user-friendly, there's a complication: the shim and the Spin runtime share the same process, hence the same environment variables. This means the same variable will be used by the shim to export its traces. I need to research how to isolate two OTLP endpoints within a single process and determine the best practices for this.

@jsturtevant
Copy link

I like the current approach of using application variables. Although Kubernetes users will expect env's to be mapped, I would expect spin users to be familiar with the "wasm" approach which passes only the required variables to the sandbox instances. This approach ensures that a sandbox (component) doesn't accidently get env's mapped into it when it shouldn't.

Most comments were minor, the only open question I have is #4 (review)

@jsturtevant
Copy link

LGTM

Signed-off-by: Kate Goldenring <kate.goldenring@fermyon.com>
… note on Spin app env vars

Signed-off-by: Kate Goldenring <kate.goldenring@fermyon.com>
Signed-off-by: Kate Goldenring <kate.goldenring@fermyon.com>
…cify default injection of env vars

Signed-off-by: Kate Goldenring <kate.goldenring@fermyon.com>

Add SQS trigger required environment variables

Signed-off-by: Kate Goldenring <kate.goldenring@fermyon.com>
Signed-off-by: Kate Goldenring <kate.goldenring@fermyon.com>

Remove SpinKube CRD modifications from the scope of the SKIP

Signed-off-by: Kate Goldenring <kate.goldenring@fermyon.com>

Add environment variables alternative and expound on wording

Signed-off-by: Kate Goldenring <kate.goldenring@fermyon.com>

Move SKIP to folder

Signed-off-by: Kate Goldenring <kate.goldenring@fermyon.com>
@kate-goldenring kate-goldenring changed the title SKIP 00X: Configuring Runtime Options in the Spin Containerd Shim SKIP 003: Configuring Runtime Options in the Spin Containerd Shim Jul 10, 2024
Copy link
Member

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

- Spin shim execution options (such as disabling pre-compilation)
- Spin shim execution environment variables (such as `RUST LOG`)

## Terminology
Copy link
Member

Choose a reason for hiding this comment

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

please consider moving terminology to the top of the page.

proposals/003-skips/README.md Show resolved Hide resolved
@kate-goldenring
Copy link
Contributor Author

@calebschoepp @radu-matei, do you have any other comments? I believe I have addressed all previous ones.

Copy link

@calebschoepp calebschoepp 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 kate-goldenring merged commit 7e26d0d into spinkube:main Jul 16, 2024
@kate-goldenring kate-goldenring deleted the shim-config branch July 16, 2024 20:14
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.

None yet

5 participants