Add support for multi-platform images #54

shanejbrown · 2023-08-07T21:33:58Z

Description

Adds support for docker multi-platform images using docker buildx. Because a common use case in buildrunner is to build a image locally, run some tests in the new image and then push the newly built image to a remote registry if the tests pass some specially steps had to be taken. In it's simplest form you can do docker buildx build --platform <list of platforms> -t <tag> --push . which will build and push a multi-platform image in a single command (but you may need to create a new builder for this). However, using this way will not allow for intermediate testing on the image prior to pushing it to a remote registry. For this reason the following methodology was contrived:

Start a ephemeral local docker registry container
Build each image for each platform with unique temporary images names pushed to the local docker registry container
Pull specific image for native architecture, else pull any of the created images
Run tests inside of pulled image
If tests pass, push multi-platform image to remote registry

During testing the local docker registry container was used since it appeared that a manifest was never created unless the --push flag was used. And the final push in step 5 is done by using docker buildx imagetools create which creates a list of existing manifests.

Related Issue

Motivation and Context

Multi-platform images are becoming for common and the standard for many images. As well as a push by some to begin to use ARM images.

How Has This Been Tested?

Unit tests have been created for the MultiplatformImageBuilder class which does most of this logic. I also created and used a simple repo with a buildrunner.yaml config to build and push single and multiple platform images.

Screenshots (if appropriate):

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

I have signed the Adobe Open Source CLA.
My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have added tests to cover my changes.
All new and existing tests passed.

bluesliverx · 2023-08-07T22:23:13Z

README.rst

@@ -432,6 +432,13 @@ shows the different configuration options available:
        <or>
        platform: linux/arm64/v8 # an apple m1 architecture

+        # Multi-platform images
+        # Specify a list of platforms to build for, and buildrunner will build a manifest
+        # list image with the given platforms


I think we need to make it clear that this uses the builders configured in build for these platforms, which may or may not be emulated.

I agree this should be more clear. But I am not sure what you mean by this uses the builders configured in build for these platforms. Can you elaborate on that some more?

builders configured in buildx is what I meant.

Oh I see what you mean. Right now, it doesn't use any configured buildx builders, it just uses the default builder. In the future we may use different builders.

However, for now how about this for the comment?
# For Multi-platform images please specify a list of platforms to build for
# and buildrunner will build images with the given platforms.
# Please note that these may be built using emulated architectures which
# will be slower than native builds.

Updated this, but let me know if you think it still needs work.

README.rst

buildrunner/__init__.py

bluesliverx · 2023-08-07T22:38:19Z

buildrunner/__init__.py

@@ -573,85 +574,92 @@ def run(self):  # pylint: disable=too-many-statements,too-many-branches,too-many

        exit_explanation = None
        try:  # pylint: disable=too-many-nested-blocks
+            with MultiplatformImageBuilder(keep_images=not self.cleanup_images) as multi_platform:


Does this always end up starting the local registry, or just when multiple platforms are specified?

Good catch! This is a bad design, since this will create a local registry every time. I'll need to make an update to parse the config to see if there are any multi-platform images in the platform.

Added new commit to fix this.

buildrunner/docker/multiplatform_image_builder.py

bluesliverx · 2023-08-08T18:28:46Z

buildrunner/steprunner/tasks/build.py

-            )
-            if exit_code != 0 or not builder.image:
-                raise BuildRunnerProcessingError('Error building image')
+            if isinstance(self.platform, list):


I think this should be:

if isinstance(self.platform, list) and len(self.platform) > 1:

bluesliverx · 2023-08-08T18:29:22Z

buildrunner/steprunner/tasks/build.py

+
+                assert len(built_images) == len(self.platform), \
+                    f'Number of built images ({len(built_images)}) does not match ' \
+                    'the number of platforms ({len(self.platform)})'


when splitting a string you need the f prefix on every formatted string

bluesliverx · 2023-08-08T18:31:40Z

buildrunner/steprunner/tasks/push.py

+            for repo in self._repos:
+                # Always add default tag
+                repo.tags.append(default_tag)
+                self.step_runner.multi_platform.tag_single_platform(name=self.get_unique_build_name(),


Personally I feel like this kind of parameter indentation can be hard to read, especially with very long prefixes. I much prefer something like this:

self.step_runner.multi_platform.tag_single_platform( name=self.get_unique_build_name(), tags=repo.tags, dest_name=repo.repository, )

I'm not asking you to change this, I just thought I should mention it as I think it's much easier to parse what's going on since you don't have to read across the entire line for the indent.

I like your suggestion better.

bluesliverx · 2023-08-08T18:32:39Z

tests/test_multiplatform.py

+            missing_images.append(expected_image)
+    return missing_images
+
+def test_use_local_registry():


Should have two lines in between methods.

bluesliverx · 2023-08-08T18:32:50Z

tests/test_multiplatform.py

+
+# FIXME: These tests can be broken if a custom buildx builder is set as default  # pylint: disable=fixme
+
+def actual_images_match_expected(actual_images, expected_images) -> List[str]:


Should have two lines in between methods/declarations.

bluesliverx · 2023-08-08T18:33:16Z

tests/test_multiplatform.py

+    assert not docker.volume.exists(volume_name)
+
+@pytest.mark.parametrize("name, in_mock_os, in_mock_arch, built_images, expected_image",
+                         [


This indent seems a little much... but maybe it's just github doing weird things.

No you are right, it is extreme. I'll make it less.

bluesliverx · 2023-08-08T19:50:32Z

README.rst

@@ -432,6 +432,14 @@ shows the different configuration options available:
        <or>
        platform: linux/arm64/v8 # an apple m1 architecture

+        # For Multi-platform images please specify a list of platforms to build for


How about this instead?

To build multi-platform images, add each platform to be built to this list and buildrunner will use docker buildx to build and tag a single image containing all architectures specified. Note that buildx may be configured to build some platforms with emulation and therefore builds may take longer with this option specified.

I made a minor change to that.

buildrunner will use docker buildx to build and provide a single tag containing all architectures specified.

docker buildx will actually create multiple images, one for each architecture, and then combine them into a single tag.

Add support for multi-platform images

59a580b

shanejbrown marked this pull request as ready for review August 7, 2023 22:08

bluesliverx reviewed Aug 7, 2023

View reviewed changes

README.rst Outdated Show resolved Hide resolved

bluesliverx reviewed Aug 7, 2023

View reviewed changes

buildrunner/__init__.py Show resolved Hide resolved

bluesliverx reviewed Aug 7, 2023

View reviewed changes

buildrunner/docker/multiplatform_image_builder.py Outdated Show resolved Hide resolved

bluesliverx reviewed Aug 7, 2023

View reviewed changes

buildrunner/docker/multiplatform_image_builder.py Outdated Show resolved Hide resolved

Resolve some suggestions from PR comments

71583eb

shanejbrown force-pushed the main branch from 8e83094 to 71583eb Compare August 7, 2023 23:25

bluesliverx reviewed Aug 8, 2023

View reviewed changes

Shane Brown added 2 commits August 8, 2023 12:37

Start local reg only when building multi-platform

669704a

Clean-up from PR comments

ff637ce

shanejbrown force-pushed the main branch from 094b7cb to ff637ce Compare August 8, 2023 19:42

bluesliverx reviewed Aug 8, 2023

View reviewed changes

Update readme for multi-platform option

8d9c267

bluesliverx approved these changes Aug 8, 2023

View reviewed changes

bluesliverx merged commit 018cfa9 into adobe:main Aug 8, 2023
8 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for multi-platform images #54

Add support for multi-platform images #54

shanejbrown commented Aug 7, 2023 •

edited

Loading

bluesliverx Aug 7, 2023

shanejbrown Aug 7, 2023

bluesliverx Aug 7, 2023

shanejbrown Aug 7, 2023

shanejbrown Aug 7, 2023

bluesliverx Aug 7, 2023

shanejbrown Aug 7, 2023

shanejbrown Aug 8, 2023

bluesliverx Aug 8, 2023

shanejbrown Aug 8, 2023

bluesliverx Aug 8, 2023

shanejbrown Aug 8, 2023

bluesliverx Aug 8, 2023

shanejbrown Aug 8, 2023

shanejbrown Aug 8, 2023

bluesliverx Aug 8, 2023

shanejbrown Aug 8, 2023

bluesliverx Aug 8, 2023

shanejbrown Aug 8, 2023

bluesliverx Aug 8, 2023

shanejbrown Aug 8, 2023

shanejbrown Aug 8, 2023

bluesliverx Aug 8, 2023

shanejbrown Aug 8, 2023


		# FIXME: These tests can be broken if a custom buildx builder is set as default # pylint: disable=fixme

		def actual_images_match_expected(actual_images, expected_images) -> List[str]:

Add support for multi-platform images #54

Add support for multi-platform images #54

Conversation

shanejbrown commented Aug 7, 2023 • edited Loading

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shanejbrown commented Aug 7, 2023 •

edited

Loading