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

Make layers manifest optional #29

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

mike-sul
Copy link
Collaborator

@mike-sul mike-sul commented Oct 2, 2024

  • Extend the test environment with the vanilla registry and tweak test fixtures so test can be run against different types of registry.
  • Make the app layers manifest creation and addition to an compose app manifest optional. It allows us creating an OCI-compliant app manifest that the vanilla registry can digest.

Add the vanilla registry to the test environment so we can run test
against it and make sure that our compose app is compatible with it.

Signed-off-by: Mike Sul <[email protected]>
Extend the existing smoke test so it runs against two types of registry,
the foundries one (with patches to support custom OCI manifest) and the
vanilla registry.

The smoke test obviously fails for the vanilla registry because it
rejects the non OCI-compliant manifest of compose app generated by
`compose publish` command.

The commits following this one will fix the failure.

Signed-off-by: Mike Sul <[email protected]>
Add a new parameter to the publish command to turn on/off app layers
manifest creation, posting, and inclusion its reference into the
manifest of app being published.

The layers manifest reference in a compose app manifest makes it non-OCI
compliant and the vanilla registry rejects requests to post such the
manifest.

The new parameter allows turning off app layers manifest addition to the
app manifest. As a result of it, a compose app manifest becomes OCI
compliant so it can be pushed to the vanilla registry which is proved by
the smoke test.

The app layers manifest is redundant in case of `composectl` usage for
app pulling since this utility retrieves meta about missing blobs from
registry and it knows sizes of missing blobs (archived one). The app
layers manifest is used by the older non composectl-based aklite
versions to avoid writing C++ code for communicating with registries and
retrieving blob metadata.

Signed-off-by: Mike Sul <[email protected]>
@mike-sul mike-sul marked this pull request as ready for review October 2, 2024 15:05
@mike-sul mike-sul requested a review from doanac October 2, 2024 15:08
It turned out that suppressing errors during gathering layers metadata
hides issues with the algorithm that parses a docker store and
calculates size of extracted layers.

Signed-off-by: Mike Sul <[email protected]>
@mike-sul
Copy link
Collaborator Author

mike-sul commented Oct 3, 2024

@doanac This change allows us not adding the "manifests" field into an app manifest which is against the OCI specification and makes us patch the registry.
Turning off of the "layers manifest" is optional and requires explicit parameter setting,so it is ON as it is now to avoid any regression.
The newer LmP/aklite versions (>= v93) does not need/use this field at all. The field/data are used by the older aklite versions, < v93, however its absence does not break aklite, just no app update size check.
My plan is to enable the "layers manifest" turning off for the dev and new factories, and optionally it can be done to existing production factories on request.

@doanac
Copy link
Member

doanac commented Oct 3, 2024

The field/data are used by the older aklite versions, < v93, however its absence does not break aklite, just no app update size check.

People do rely on that check, so we should assume nobody is going to want to use this <v93.

I wonder if there's something we need to document or convey to customer-success with regards to this?

@mike-sul
Copy link
Collaborator Author

mike-sul commented Oct 3, 2024

People do rely on that check, so we should assume nobody is going to want to use this <v93.

Yes, that's why we are not going to disable "layers manifest" for existing factories unless explicitly requested.

@mike-sul
Copy link
Collaborator Author

mike-sul commented Oct 3, 2024

I wonder if there's something we need to document or convey to customer-success with regards to this?

I don't see much reason. I don't see a good way to change it for existing factories because we are never know what LmP version their devices are on. So, we just keep doing the same hack for existing factories.
As for the new factory we just simply disable hacking app manifest, not sure there is any reason to inform customers about this low level detail.

@doanac
Copy link
Member

doanac commented Oct 3, 2024

I don't see a good way to change it for existing factories because we are never know what LmP version their devices are on

excellent point. it's about the version you are updating from.

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.

2 participants