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

Support publishing of multi-architecture OCI container images #43085

Merged
merged 136 commits into from
Dec 2, 2024

Conversation

surayya-MS
Copy link
Member

@surayya-MS surayya-MS commented Aug 29, 2024

Context

Fixes dotnet/sdk-container-builds#87. Currently, customers use our tooling to publish single architectural container images providing architecture via RuntimeIdentifier or ContainerRuntimeIdentifier properties. It's not possible to publish multi-architectural container image. This change allows customers to do that.

Customer impact

With this change customers will be able to publish multi-arch container images to remote registries, or publish single-arch images for each architecture locally (either directly to docker/podman or as tarballs).

Details

It is not possible to create multi-arch image in docker because the individual images for each architecture must be available in the remote registry. For podman it is possible, however, that is not part of these PR. It will be the next iteration.

Changes

  1. PublishContainer target renamed to _PublishSingleContainer
  2. New _PublishMultiArchContainers target that calls _PublishSingleContainer for each arch and then creates image index
  3. PublishContainer target calls either _PublishSingleContainer or _PublishMultiArchContainers depending on a condition
  4. Implemented new task CreateImageIndex that creates the image index (manifest list it's same thing) and pushes to the remote registry.

Testing

  1. Added unit and integration tests
  2. Tested manually

Risk

Low - the existing tests ensure that other scenarios were not affected.

@dotnet-issue-labeler dotnet-issue-labeler bot added Area-Containers Related to dotnet SDK containers functionality untriaged Request triage from a team member labels Aug 29, 2024
@surayya-MS surayya-MS removed the untriaged Request triage from a team member label Aug 29, 2024
@baronfel
Copy link
Member

baronfel commented Sep 3, 2024

A few notes that we will need to handle based on my trials at baronfel/sdk-container-demo#15

  • The ContainerRepository property is only inferred for single-RID scenarios, not multi-RID so the CreateImageIndex task crashes.
    • We should be able to infer this property similarly to how we infer it for the single-RID publishes
  • ContainerImageTags needs to be supported for the inner builds - when ContainerImageTags is specified we get crashes on the single-image Publishes because the single-image publishes unconditionally forward along a single tag.
    • Instead of always forcing a single ContainerImageTag, we should append the RID to each ContainerImageTags value if specified
    • The CreateImageIndex Task should also accept ContainerImageTags similar to the single-image push
  • Using the package is rough right now - the in-SDK checks complain at you. In addition, if you have both the SDK and the package the SDK's Import always overrides the package. I had to work around this here
    • We should set the _ContainersTargetsDir property to point to the containers package path in the package .props so that users don't have to do all of this
    • We should add a Code to the Containers-checking target to allow users to suppress this warning - we can steal the code from my older PR: Add code to containers check to allow users to silence it #42501
  • CreateImageIndex crashes when no container registry is specified, often because a local publish was done
    • we should add a Condition that skips CreateImageIndex when local daemon publish was requested

@KalleOlaviNiemitalo
Copy link
Contributor

PublishContainer target calls either _PublishSingleContainer or _PublishSingleContainer depending on a condition

Or _PublishMultiArchContainers.

Copy link
Contributor

@donJoseLuis donJoseLuis left a comment

Choose a reason for hiding this comment

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

minor comments

Copy link
Member

@JanKrivanek JanKrivanek left a comment

Choose a reason for hiding this comment

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

Thanks - it looks good and shippable.

I have some nonblocking comments for consideration. I'd recommend to remove code repetition and define strong return types - it'll help maintainability

@baronfel
Copy link
Member

baronfel commented Dec 2, 2024

Very happy to merge this. Thank you for all of your work @surayya-MS, and for your deep review @SimaTian!

@baronfel baronfel merged commit 76d396c into dotnet:release/8.0.4xx Dec 2, 2024
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Containers Related to dotnet SDK containers functionality DO NOT MERGE Servicing-approved
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants