pulp / pulp_container

Pulp Container Registry
https://docs.pulpproject.org/pulp_container/
GNU General Public License v2.0
23 stars 45 forks source link

Add manifest arch, os, and compressed layers size fields #1782

Closed git-hyagi closed 2 weeks ago

git-hyagi commented 1 month ago

closes: #1767

ianballou commented 1 month ago

4. If we decided to create another django-admin command, we should align with Katello to see if it makes sense for them to run a management command reading data from the storage.

From the Katello side, we're okay with the command being the same as before or a new one. Choose whatever is most efficient from the Pulp side.

ianballou commented 1 month ago

Also I wanted to clarify to be super sure -- OCI Image Index manifests will always have a null arch, OS, and size? I'm not super clear since, according to https://specs.opencontainers.org/image-spec/image-index/?v=v1.0.1, the optional platform property can have an arch and an OS,

git-hyagi commented 1 month ago

Also I wanted to clarify to be super sure -- OCI Image Index manifests will always have a null arch, OS, and size? I'm not super clear since, according to https://specs.opencontainers.org/image-spec/image-index/?v=v1.0.1, the optional platform property can have an arch and an OS,

Thank you for bringing this! We reviewed and discussed the os and architecture fields from manifest list, and we will work on it in this PR. If the manifest list (or oci index) contains the platform (an optional) field, we will populate pulp manifest with arch and os (whenever platform is defined, os and arch are required).

git-hyagi commented 1 month ago

After re-reading the specs, I realized that manifestlist or oci-index do not have a platform field. Actually, platform is a field from manifests. In this link, we can see it better: https://github.com/opencontainers/image-spec/blob/main/image-index.md (platform is a bullet inside manifests).

So, please, ignore my last comment and yes, "OCI Image Index manifests will always have a null arch, OS, and size".

ianballou commented 1 month ago

After re-reading the specs, I realized that manifestlist or oci-index do not have a platform field. Actually, platform is a field from manifests. In this link, we can see it better: https://github.com/opencontainers/image-spec/blob/main/image-index.md (platform is a bullet inside manifests).

So, please, ignore my last comment and yes, "OCI Image Index manifests will always have a null arch, OS, and size".

Nice catch! I didn't notice it was under manifests, so that's perfect.

git-hyagi commented 1 month ago

If we need to store the "architecture" and "os" fields on the Manifest model, we should try to fetch the values from a manifest list first. An OCI Index has the "architecture" and "os" fields required. There is no need to read the data from a config blob if a manifest is listed within an index (https://github.com/opencontainers/image-spec/blob/main/image-index.md#image-index-property-descriptions).

I did some investigation in each workflow, and here are my findings:

I couldn't find how to fetch these values from the manifest list in advance. I'm not sure if I overlooked something or misunderstood the code workflow.

lubosmj commented 1 month ago
git-hyagi commented 1 month ago

Thank you for the help and the suggestions/optimizations!

ianballou commented 1 month ago

cc @sjha4 @qcjames53 we should keep an eye on this and integrate with it in Katello sooner rather than later to avoid excess reindexing of container manifests.

lubosmj commented 3 weeks ago

@git-hyagi, please, rebase this PR against the main branch and prepare it for the final review.