Add prototype for distributing Spin applications using OCI #1014

radu-matei · 2023-01-04T15:51:06Z

See SIP — #1033

Signed-off-by: Radu Matei [email protected]

itowlson

I haven't looked at the code yet but the SIP is really good. The main thing I'd like to understand better is how we see versioning working. Otherwise looking good!

docs/content/sips/008-using-oci-registries.md

kate-goldenring

Just looked at the SIP to start -- thanks for walking through it with an example!

docs/content/sips/008-using-oci-registries.md

itowlson

SIP looks good to me. Thanks!

docs/content/sips/008-using-oci-registries.md

lann

SIP LGTM

kate-goldenring · 2023-01-18T16:53:38Z

SIP LGTM!

radu-matei · 2023-01-18T18:05:58Z

As discussed earlier, #1033 breaks out the SIP into a separate PR so we can iterate on this easier.

I will remove the SIP from this PR in a future commit.

Thanks for the reviews, everyone!

crates/cache/Cargo.toml

crates/cache/src/oci.rs

crates/publish/Cargo.toml

crates/publish/src/lib.rs

vdice

Only real feedback at this prototype stage is to see where we can make it obvious to users that this functionality is experimental and subject to change.

Also, where do we want to track the prototype's status of implementation of the corresponding SIP?

Lastly, could use a rebase to remove the SIP from this PR (now that it is in main).

vdice · 2023-01-23T21:18:51Z

src/commands/oci.rs

+
+/// Commands for working with OCI registries to distribute applications.
+#[derive(Subcommand, Debug)]
+pub enum OciCommands {


Assuming this makes it into a Spin release while still in an experimental phase, should we add text to these commands (perhaps the whole oci suite) mentioning this is and that these commands/flags are subject to change in future releases?

src/commands/oci.rs

crates/loader/src/oci/mod.rs

crates/publish/src/oci/client.rs

itowlson · 2023-01-24T00:54:10Z

crates/publish/src/oci/client.rs

+ .content
+ .source
+ .expect("file mount loaded from disk should contain a file source");
+ let source = spin_trigger::parse_file_url(source.as_str())?;


I get the convenience factor of spin_trigger having the function you need but I am uncomfortable about having the publishing crate depend on the trigger crate.

This is less about convenience for spin_trigger::parse_file_url (which can be easily duplicated) and more for spin_trigger::build_locked_app.

To expand on this a bit, my understanding is that we really want the functionality to build a locked application configuration from a spin.toml to live in the spin_app crate, and not spin_trigger. This is a refactoring I am not willing to take as part of this PR, which is intended to make as few structural changes as possible.

itowlson · 2023-01-24T00:55:43Z

crates/publish/src/oci/client.rs

+ // recursively traverse it and add layers for each file.
+ let mut files = Vec::new();
+ for f in c.files {
+ let source = f


Break out the loop body (or at least part of it) into a well-named function (or several) - it's quite long and quite deeply nested, which for me made it hard to follow.

crates/cache/src/oci.rs

lann · 2023-01-24T14:28:37Z

crates/cache/src/oci.rs

+ pub async fn new(root: Option<PathBuf>) -> Result<Self> {
+ let root = match root {
+ Some(root) => root,
+ None => dirs::config_dir()


I would agree this should be cache_dir.

crates/cache/src/lib.rs

crates/publish/src/oci/client.rs

radu-matei · 2023-01-28T17:12:03Z

Thanks for the review and feedback so far, everyone!
These are the main changes compared to the initial state of the PR:

moved the cache functionality inside spin_loader::oci::cache instead of a new crate (this should help refactoring the loaders cc @lann)
the registry cache now stores all data under $XDG_CACHE/spin/registry (TBD final location based on the ~/.spin vs. XDG debate). Instead of a directory for oci, this is renamed to registry, so that it can be used as a global cache (including for the HTTP component sources, which currently downloads the Wasm module on every spin up)
spin oci run now works with all trigger types (including potentially external triggers) (cc @itowlson) and can accept environment variables
the trigger CLI functionality still needs to be cleaned up — triggers currently expect an --oci flag, and I suspect this can be automatically inferred from the lockfile (i.e. if it has content digests instead of file paths, it is run from the registry). However, this is not something I want to address in this PR.
this still does not integrate with spin up. I would like to do this gradually for the next iteration.

Things to implement compared to the SIP:

spin up needs to be integrated — Implement spin up from an OCI registry #1086
spin oci login needs to be implemented — Implement spin registry login #1087
the spin bindle2oci plugin needs to be implemented

The main question I have for this PR is: do we think this is in a state where we could merge it, make it explicit it is experimental, and iterate on the implementation and continue to implement the SIP, or do we want to iterate in this PR without merging?

(Also a question that is probably better suited for the SIP: spin registry vs. spin oci for the top-level commands?)

See below an example of the current state of this PR:

❯ spin oci push ghcr.io/radu-matei/spin-redis-hello:v3
INFO spin_publish::oci::client: Pushed "https://ghcr.io/v2/radu-matei/spin-redis-hello/manifests/sha256:34e4f0133464a4271ba2189ad24ec07095c61c8f79dc4d753d32b1a0dc69c73a"

❯ spin oci run --env abc=def ghcr.io/radu-matei/spin-redis-hello:v3
INFO spin_publish::oci::client: Pulled ghcr.io/radu-matei/spin-redis-hello:v3@sha256:34e4f0133464a4271ba2189ad24ec07095c61c8f79dc4d753d32b1a0dc69c73a
INFO spin_redis_engine: Connecting to Redis server at redis:https://localhost:6379
INFO spin_redis_engine: Subscribing component "rust-test" to channel "messages"
INFO spin_redis_engine: Received message on channel "messages"
Env: {"abc": "def"},
Hello from Rust, message: hello

❯ tree /Users/radu/Library/Caches/spin/registry
/Users/radu/Library/Caches/spin/registry
├── data
├── manifests
│   └── ghcr.io
│       └── radu-matei
│           └── spin-redis-hello
│               └── v3
│                   ├── config.json                 // OCI configuration object that contains the Spin lockfile
│                   └── manifest.json               // OCI manifest that references all Wasm modules and static assets
└── wasm
    └── sha256:7f894f2523cf5f7b4596942a29c6f8ee0d24468e1f8951f4e4864794caf283de

itowlson

A few style/naming comments but nothing crucial - I feel like the priority at this point is to get something in so folks can start bashing on it, and try for a tidying pass later? eyes that long loop body hungrily

crates/loader/src/oci/cache.rs

crates/publish/src/oci/client.rs

flouthoc · 2023-01-31T05:45:25Z

crates/publish/src/oci/client.rs

+
+// TODO: the media types for application, wasm module, and data layer are not final.
+const SPIN_APPLICATION_MEDIA_TYPE: &str = "application/vnd.fermyon.spin.application.v1+config";
+const WASM_LAYER_MEDIA_TYPE: &str = "application/vnd.wasm.content.layer.v1+wasm";


Just a drive-by-comment: I think media type can be added to image-spec, see discussion here: opencontainers/image-spec#964 (comment)

Note that this is the media type of a layer, not of the top-level manifest or of the config.

In this scenario, the artifact is a Spin application, and it contains Wasm layers.
We could use the upcoming referrer API, but that's not something stable AFAIK.

Thanks for the clarification. SGTM :)

This commit adds experimental support for distributing Spin applications using OCI registries. Specifically, it uses the OCI Artifacts specification (https://github.com/opencontainers/artifacts) to package and distribute Spin applications. This PR implements `spin oci push`, `spin oci pull`, and `spin oci run` to interact with a supporting container registry - for example: ```bash $ spin oci push ghcr.io/<username>/my-spin-application:v1 INFO spin_publish::oci::client: Pushed "https://ghcr.io/v2/<username>/my-spin-application/manifests/sha256:9f4e7eebb27c0174fe6654ef5e0f908f1edc8f625324a9f49967ccde44a6516b" $ spin oci pull ghcr.io/<username>/my-spin-application:v1 INFO spin_publish::oci::client: Pulled ghcr.io/<username>/my-spin-application:v1@sha256:9f4e7eebb27c0174fe6654ef5e0f908f1edc8f625324a9f49967ccde44a6516b $ spin oci run ghcr.io/<username>/my-spin-application:v1 INFO spin_publish::oci::client: Pulled ghcr.io/<username>/my-spin-application:v1@sha256:9f4e7eebb27c0174fe6654ef5e0f908f1edc8f625324a9f49967ccde44a6516b ``` Following the SIP (fermyon#1033), this PR defines a new `config.mediaType` for a Spin application, `application/vnd.fermyon.spin.application.v1+config`, and two media types for the potential content that can be found in such an artifact: `application/vnd.wasm.content.layer.v1+wasm` for a Wasm module, and `application/vnd.wasm.content.layer.v1+data` for a static file. (Note that the media types *can* change in a future iteration of this experimental work if a community consensus on the media type used to represent Wasm is reached.) Following the SIP, this PR distributes the Spin lockfile for a given application as the OCI configuration object. This PR also introduces a global cache for layers and manifests pulled from the registry. This is a content addressable cache, and its use will be extended in the future for Wasm modules pulled from HTTP sources. Currently, `spin oci pull` (or `spin oci run`) will always make at least an initial request to the registry to fetch the manifest from the registry. After the manifest is fetched, already pulled layers will not be pulled again. In a future commit, the behavior of the initial manifest fetch regardless of its existence in the cache will be controllable by a flag. Signed-off-by: Radu Matei <[email protected]>

Before this commit, for a `spin.toml` file that had a URL module source for a component (such as the file server and redirect templates), subsequent runs of `spin up` would always download the module from the URL before starting the application. fermyon#1014 added a content-addressed registry cache that can be used to store remote module sources. This commit updates the Spin local loader to use that cache to store the component sources referenced through URL in `spin.toml`. Before fetching a component, the loader will check the local cache first. If present, it will read the file and return its content; otherwise, it will fetch it, store it in the cache, then return its content. Currently, the default cache root is always used. Once `spin up` will have a way to configure it, that will be propagated here as well. Signed-off-by: Radu Matei <[email protected]>

itowlson reviewed Jan 5, 2023

View reviewed changes

radu-matei commented Jan 5, 2023

View reviewed changes

docs/content/sips/008-using-oci-registries.md Outdated Show resolved Hide resolved

fibonacci1729 reviewed Jan 5, 2023

View reviewed changes

docs/content/sips/008-using-oci-registries.md Outdated Show resolved Hide resolved

kate-goldenring reviewed Jan 5, 2023

View reviewed changes

itowlson approved these changes Jan 17, 2023

View reviewed changes

docs/content/sips/008-using-oci-registries.md Outdated Show resolved Hide resolved

fibonacci1729 approved these changes Jan 18, 2023

View reviewed changes

lann reviewed Jan 18, 2023

View reviewed changes

docs/content/sips/008-using-oci-registries.md Outdated Show resolved Hide resolved

lann reviewed Jan 18, 2023

View reviewed changes

radu-matei mentioned this pull request Jan 18, 2023

Add SIP for distributing Spin applications using OCI registries #1033

Merged

radu-matei changed the title ~~Add prototype and SIP for distributing Spin applications using OCI~~ Add prototype for distributing Spin applications using OCI Jan 18, 2023

radu-matei marked this pull request as draft January 18, 2023 18:07

radu-matei requested review from itowlson and fibonacci1729 January 18, 2023 18:07