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

Manifest v2 support #132

Merged
merged 3 commits into from
Oct 26, 2023
Merged

Manifest v2 support #132

merged 3 commits into from
Oct 26, 2023

Conversation

itowlson
Copy link
Contributor

For when spinframework/spin#1780 lands. The new load path moves all of the LockedApp fiddliness into the Spin loader, so we pretty much don't have to do anything to consume it any more!

The Spin crate references will need to be set back to the Fermyon repo at this point - this points them at @lann's branch just for development.

@itowlson itowlson force-pushed the manifest-v2 branch 3 times, most recently from 0afb576 to 5cff623 Compare October 17, 2023 23:51
Signed-off-by: itowlson <ivan.towlson@fermyon.com>
@itowlson
Copy link
Contributor Author

This seems to be having a problem with the Spin assets-test test case. The app deploys but never goes ready, and Cloud just says "failed to prepare your Spin application." Other apps with zero-length files work fine. Investigating!

@lann
Copy link
Collaborator

lann commented Oct 18, 2023

Might have a logical conflict with @vdice's OCI changes; it's an old branch!

@itowlson
Copy link
Contributor Author

The problem happens when a (v1, haven't tested with v2 yet) manifest omits the application HTTP base. The "failed to prepare" may be spurious, although earlier it was showing the domain as having a problem. For now I'll work on the basis that this is a client problem with the readiness detection.

@itowlson
Copy link
Contributor Author

Something amiss with the digest computation.

When it has base, the digest is 335de18dadb8ac201ef8e4a6252600948166d3bde7493de2abc4ac7b06e618bc

When it doesn't have base, the cloud plugin looks for digest 125f0c859c1a8b8ef8afd97ff1dcfaa24e400c5e6c8f736e239dcf177ffd6e31 but the cloud reports digest 335de18dadb8ac201ef8e4a6252600948166d3bde7493de2abc4ac7b06e618bc.

The digest is coming from within the spin_oci push_locked method. Next step is to try a spin registry push and see if the digest lines up with what the registry calculates.

@itowlson
Copy link
Contributor Author

itowlson commented Oct 18, 2023

spin registry push (to ghcr) comes up with the same value whether it has base or not.

@itowlson
Copy link
Contributor Author

Correction. spin registry push does come up with different digests depending on whether or not base was present, and these match the digests that the cloud plugin looks for.

@lann
Copy link
Collaborator

lann commented Oct 18, 2023

We should probably rethink how we track readiness. Cloud already assigns a unique ID to each deployment; we should be able to expose it in the .well-known info and check that instead.

@itowlson
Copy link
Contributor Author

Sure. That might need a Cloud API update - looking at the Swagger it seems like the only place the deployment ID is currently returned is in logs, though I could well be missing something! @bacongobbler is the deployment ID available during the deployment cycle itself?

@bacongobbler
Copy link
Member

Yes, it should be. Cloud assigns a unique ID to each deployment and passes that ID to the runtime.

@itowlson
Copy link
Contributor Author

The diagnosis of OCI digest weirdness was wrong. Baseless applications don't come ready at all - the supposed digest issue was down to the old application still being live.

Signed-off-by: itowlson <ivan.towlson@fermyon.com>
@itowlson itowlson marked this pull request as ready for review October 25, 2023 20:02
@itowlson
Copy link
Contributor Author

I think this is good to go. Thank you @lann for the HTTP base fix!

Signed-off-by: itowlson <ivan.towlson@fermyon.com>
Copy link
Collaborator

@lann lann left a comment

Choose a reason for hiding this comment

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

I guess @itowlson has reviewed my contribution here so its fine for me to review his...

@itowlson itowlson merged commit a0f3ee0 into fermyon:main Oct 26, 2023
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.

4 participants