-
Notifications
You must be signed in to change notification settings - Fork 270
Add prototype for distributing Spin applications using OCI #1014
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just looked at the SIP to start -- thanks for walking through it with an example!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SIP looks good to me. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SIP LGTM
SIP LGTM! |
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! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
|
||
/// Commands for working with OCI registries to distribute applications. | ||
#[derive(Subcommand, Debug)] | ||
pub enum OciCommands { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
.content | ||
.source | ||
.expect("file mount loaded from disk should contain a file source"); | ||
let source = spin_trigger::parse_file_url(source.as_str())?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
// recursively traverse it and add layers for each file. | ||
let mut files = Vec::new(); | ||
for f in c.files { | ||
let source = f |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Outdated
pub async fn new(root: Option<PathBuf>) -> Result<Self> { | ||
let root = match root { | ||
Some(root) => root, | ||
None => dirs::config_dir() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would agree this should be cache_dir
.
75d3e51
to
49ec228
Compare
Thanks for the review and feedback so far, everyone!
Things to implement compared to the SIP:
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: See below an example of the current state of this PR:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
|
||
// 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"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a drive-by-comment: I think media type can be added to image-spec, see discussion here: opencontainers/image-spec#964 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 (spinframework#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. spinframework#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]>
See SIP — #1033
Signed-off-by: Radu Matei [email protected]