From ded83f7f5f4b99c4af92ff7ab730274e910e58ae Mon Sep 17 00:00:00 2001 From: Shane Brown Date: Thu, 26 Oct 2023 16:31:41 -0600 Subject: [PATCH 1/3] Bugfix to remove image with tagged name instead of image digest --- buildrunner/docker/multiplatform_image_builder.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/buildrunner/docker/multiplatform_image_builder.py b/buildrunner/docker/multiplatform_image_builder.py index 2ea960fa..0b95e05c 100644 --- a/buildrunner/docker/multiplatform_image_builder.py +++ b/buildrunner/docker/multiplatform_image_builder.py @@ -265,10 +265,10 @@ def _build_with_inject(self, # pylint: disable=too-many-arguments @retry(python_on_whales.exceptions.DockerException, - tries=5, + tries=3, delay=1, backoff=3, - max_delay=30, + max_delay=10, logger=logger) def _build_single_image(self, name: str, @@ -333,7 +333,8 @@ def _build_single_image(self, image_id = docker.image.inspect(tag_name).id # Removes the image from host, if this fails it is considered a warning try: - docker.image.remove(images, force=True) + logger.debug(f"Removing {tag_name}") + docker.image.remove(tag_name, force=True) except python_on_whales.exceptions.DockerException as err: logger.warning(f"Failed to remove {images}: {err}") except python_on_whales.exceptions.DockerException as err: From 97493201d9d793113f9b5d32a582e12272e4eba9 Mon Sep 17 00:00:00 2001 From: Shane Brown Date: Fri, 27 Oct 2023 10:23:32 -0600 Subject: [PATCH 2/3] Refactor multi-platform build image --- .../docker/multiplatform_image_builder.py | 74 +++++++++---------- buildrunner/steprunner/tasks/build.py | 8 +- tests/test_multiplatform.py | 22 +++--- 3 files changed, 50 insertions(+), 54 deletions(-) diff --git a/buildrunner/docker/multiplatform_image_builder.py b/buildrunner/docker/multiplatform_image_builder.py index 0b95e05c..8991e899 100644 --- a/buildrunner/docker/multiplatform_image_builder.py +++ b/buildrunner/docker/multiplatform_image_builder.py @@ -273,33 +273,26 @@ def _build_with_inject(self, def _build_single_image(self, name: str, platform: str, - push: bool = True, - path: str = ".", - file: str = "Dockerfile", - tags: List[str] = None, - build_args: dict = None, - built_images: list = None, - inject: dict = None,) -> None: + push: bool, + path: str, + file: str, + tags: List[str], + build_args: dict, + mp_image_name: str, + inject: dict) -> None: """ Builds a single image for the given platform Args: name (str): The name of the image platform (str): The platform to build the image for (e.g. linux/amd64) - push (bool, optional): Whether to push the image to the registry. Defaults to True. - path (str, optional): The path to the Dockerfile. Defaults to ".". - file (str, optional): The path/name of the Dockerfile (ie. /Dockerfile). Defaults to "Dockerfile". - tags (List[str], optional): The tags to apply to the image. Defaults to None. - docker_registry (str, optional): The docker registry to push the image to. Defaults to None. - built_images (list, optional): A list of built images. Defaults to None. + push (bool): Whether to push the image to the registry. + path (str): The path to the Dockerfile. + file (str): The path/name of the Dockerfile (ie. /Dockerfile). + tags (List[str]): The tags to apply to the image. + docker_registry (str): The docker registry to push the image to. + mp_image_name (str): A list of built images. """ - if tags is None: - tags = ["latest"] - if built_images is None: - built_images = [] - if build_args is None: - build_args = {} - assert os.path.isdir(path) and os.path.exists(f"{file}"), \ f"Either path {path}({os.path.isdir(path)}) or file " \ f"'{file}'({os.path.exists(f'{file}')}) does not exist!" @@ -341,10 +334,10 @@ def _build_single_image(self, logger.error(f"Failed to build {tag_name}: {err}") raise err - built_images.append(ImageInfo(repo=name, - tags=tags, - platform=platform, - digest=image_id,)) + self._intermediate_built_images[mp_image_name].append(ImageInfo(repo=name, + tags=tags, + platform=platform, + digest=image_id,)) def get_single_platform_to_build(self, platforms: List[str]) -> str: """ Returns the platform to build for single platform flag """ @@ -372,7 +365,7 @@ def build_multiple_images(self, platforms: List[str], path: str = ".", file: str = "Dockerfile", - name: str = None, + mp_image_name: str = None, tags: List[str] = None, push=True, do_multiprocessing: bool = True, @@ -387,10 +380,13 @@ def build_multiple_images(self, platforms (List[str]): The platforms to build the image for (e.g. linux/amd64) path (str, optional): The path to the Dockerfile. Defaults to ".". file (str, optional): The path/name of the Dockerfile (ie. /Dockerfile). Defaults to "Dockerfile". - name (str, optional): The name of the image. Defaults to None. + mp_image_name (str, optional): The name of the image. Defaults to None. tags (List[str], optional): The tags to apply to the image. Defaults to None. push (bool, optional): Whether to push the image to the registry. Defaults to True. do_multiprocessing (bool, optional): Whether to use multiprocessing to build the images. Defaults to True. + docker_registry (str, optional): The docker registry to push the image to. Defaults to None. + build_args (dict, optional): The build args to pass to docker. Defaults to None. + inject (dict, optional): The files to inject into the build context. Defaults to None. Returns: List[ImageInfo]: The list of intermediate built images, these images are ephemeral @@ -411,7 +407,7 @@ def get_path(file): dockerfile, cleanup_dockerfile = get_dockerfile(file) - logger.debug(f"Building {name}:{tags} for platforms {platforms} from {dockerfile}") + logger.debug(f"Building {mp_image_name}:{tags} for platforms {platforms} from {dockerfile}") if self._use_local_registry and not self._local_registry_is_running: # Starts local registry container to do ephemeral image storage @@ -422,30 +418,30 @@ def get_path(file): # Updates name to be compatible with docker image_prefix = "buildrunner-mp" - santized_name = f"{image_prefix}-{name.replace('/', '-').replace(':', '-')}" + santized_name = f"{image_prefix}-{mp_image_name.replace('/', '-').replace(':', '-')}" base_image_name = f"{self._registry_info.ip_addr}:{self._registry_info.port}/{santized_name}" # Keeps track of the built images {name: [ImageInfo(image_names)]]} manager = Manager() - self._intermediate_built_images[name] = manager.list() + self._intermediate_built_images[mp_image_name] = manager.list() line = "-----------------------------------------------------------------" if self._disable_multi_platform: platform = self.get_single_platform_to_build(platforms) - curr_name = f"{base_image_name}-{platform.replace('/', '-')}" + platform_image_name = f"{base_image_name}-{platform.replace('/', '-')}" print(f"{line}\n" f"Note: Disabling multi-platform build, " "this will only build a single-platform image.\n" f"image: {santized_name} platform:{platform}\n" f"{line}") - self._build_single_image(curr_name, + self._build_single_image(platform_image_name, platform, push, path, dockerfile, tags, build_args, - self._intermediate_built_images[name], + mp_image_name, inject) else: processes = [] @@ -456,28 +452,28 @@ def get_path(file): "or set the 'disable-multi-platform' flag in the global config file.\n" f"{line}") for platform in platforms: - curr_name = f"{base_image_name}-{platform.replace('/', '-')}" - logger.debug(f"Building {curr_name} for {platform}") + platform_image_name = f"{base_image_name}-{platform.replace('/', '-')}" + logger.debug(f"Building {platform_image_name} for {platform}") if do_multiprocessing: processes.append(Process(target=self._build_single_image, - args=(curr_name, + args=(platform_image_name, platform, push, path, dockerfile, tags, build_args, - self._intermediate_built_images[name], + mp_image_name, inject))) else: - self._build_single_image(curr_name, + self._build_single_image(platform_image_name, platform, push, path, dockerfile, tags, build_args, - self._intermediate_built_images[name], + mp_image_name, inject) for proc in processes: @@ -489,7 +485,7 @@ def get_path(file): if cleanup_dockerfile and dockerfile and os.path.exists(dockerfile): os.remove(dockerfile) - return self._intermediate_built_images[name] + return self._intermediate_built_images[mp_image_name] def push(self, name: str, dest_names: List[str] = None) -> None: """ diff --git a/buildrunner/steprunner/tasks/build.py b/buildrunner/steprunner/tasks/build.py index d49dc9c5..06ba3e8f 100644 --- a/buildrunner/steprunner/tasks/build.py +++ b/buildrunner/steprunner/tasks/build.py @@ -214,11 +214,11 @@ def run(self, context): ) try: if self.platforms: - built_images = self.step_runner.multi_platform.build_multiple_images( + built_platform_images = self.step_runner.multi_platform.build_multiple_images( platforms=self.platforms, path=self.path, file=self.dockerfile, - name=self.get_unique_build_name(), + mp_image_name=self.get_unique_build_name(), docker_registry=docker_registry, build_args=self.buildargs, inject=self.to_inject, @@ -229,8 +229,8 @@ def run(self, context): if self.step_runner.multi_platform.disable_multi_platform: number_of_images = 1 - assert len(built_images) == number_of_images, \ - f'Number of built images ({len(built_images)}) does not match ' \ + assert len(built_platform_images) == number_of_images, \ + f'Number of built images ({len(built_platform_images)}) does not match ' \ f'the number of platforms ({number_of_images})' else: diff --git a/tests/test_multiplatform.py b/tests/test_multiplatform.py index bdbe43ef..efa308f4 100644 --- a/tests/test_multiplatform.py +++ b/tests/test_multiplatform.py @@ -69,7 +69,7 @@ def test_start_local_registry_on_build(): # Building should start the registry test_path = f'{TEST_DIR}/test-files/multiplatform' - mp.build_multiple_images(name='test-images-2000-start-on-build', + mp.build_multiple_images(mp_image_name='test-images-2000-start-on-build', platforms=['linux/arm64', 'linux/amd64'], path=test_path, file=f'{test_path}/Dockerfile', @@ -96,7 +96,7 @@ def test_start_local_registry_on_build(): assert registry_container.state.running # Building again should not start a new registry - mp.build_multiple_images(name='test-images-2000-start-on-build2', + mp.build_multiple_images(mp_image_name='test-images-2000-start-on-build2', platforms=['linux/arm64', 'linux/amd64'], path=test_path, file=f'{test_path}/Dockerfile', @@ -193,7 +193,7 @@ def test_tag_single_platform(name, platforms, expected_image_names): tag='latest' test_path = f'{TEST_DIR}/test-files/multiplatform' with MultiplatformImageBuilder() as mp: - built_images = mp.build_multiple_images(name=name, + built_images = mp.build_multiple_images(mp_image_name=name, platforms=platforms, path=test_path, file=f'{test_path}/Dockerfile', @@ -227,7 +227,7 @@ def test_tag_single_platform_multiple_tags(name, platforms, expected_image_names tags=['latest', '0.1.0'] test_path = f'{TEST_DIR}/test-files/multiplatform' with MultiplatformImageBuilder() as mp: - built_images = mp.build_multiple_images(name=name, + built_images = mp.build_multiple_images(mp_image_name=name, platforms=platforms, path=test_path, file=f'{test_path}/Dockerfile', @@ -263,7 +263,7 @@ def test_tag_single_platform_keep_images(name, platforms, expected_image_names): test_path = f'{TEST_DIR}/test-files/multiplatform' try: with MultiplatformImageBuilder(keep_images=True) as mp: - built_images = mp.build_multiple_images(name=name, + built_images = mp.build_multiple_images(mp_image_name=name, platforms=platforms, path=test_path, file=f'{test_path}/Dockerfile', @@ -304,7 +304,7 @@ def test_push(): test_path = f'{TEST_DIR}/test-files/multiplatform' with MultiplatformImageBuilder() as mp: - built_images = mp.build_multiple_images(name=build_name, + built_images = mp.build_multiple_images(mp_image_name=build_name, platforms=platforms, path=test_path, file=f'{test_path}/Dockerfile', @@ -346,7 +346,7 @@ def test_push_with_dest_names(): test_path = f'{TEST_DIR}/test-files/multiplatform' with MultiplatformImageBuilder() as mp: - built_images = mp.build_multiple_images(name=build_name, + built_images = mp.build_multiple_images(mp_image_name=build_name, platforms=platforms, path=test_path, file=f'{test_path}/Dockerfile', @@ -393,7 +393,7 @@ def test_build(mock_build, mock_pull, mock_inspect, mock_remove, name, platforms mock_inspect.return_value.id = 'myfakeimageid' test_path = f'{TEST_DIR}/test-files/multiplatform' with MultiplatformImageBuilder() as mp: - built_images = mp.build_multiple_images(name=name, + built_images = mp.build_multiple_images(mp_image_name=name, platforms=platforms, path=test_path, file=f'{test_path}/Dockerfile', @@ -424,14 +424,14 @@ def test_build_multiple_builds(mock_build, mock_pull, mock_inspect, mock_remove) test_path = f'{TEST_DIR}/test-files/multiplatform' with MultiplatformImageBuilder() as mp: # Build set 1 - built_images1 = mp.build_multiple_images(name=name1, + built_images1 = mp.build_multiple_images(mp_image_name=name1, platforms=platforms1, path=test_path, file=f'{test_path}/Dockerfile', do_multiprocessing=False) # Build set 2 - built_images2 = mp.build_multiple_images(name=name2, + built_images2 = mp.build_multiple_images(mp_image_name=name2, platforms=platforms2, path=test_path, file=f'{test_path}/Dockerfile', @@ -465,7 +465,7 @@ def test_build_multiple_builds(mock_build, mock_pull, mock_inspect, mock_remove) def test_build_with_tags(name, tags, platforms, expected_image_names): test_path = f'{TEST_DIR}/test-files/multiplatform' with MultiplatformImageBuilder() as mp: - built_images = mp.build_multiple_images(name=name, + built_images = mp.build_multiple_images(mp_image_name=name, platforms=platforms, path=test_path, file=f'{test_path}/Dockerfile', From 9e4bbcf7e8c25da1578efa9b141c0d5e0baa966d Mon Sep 17 00:00:00 2001 From: Shane Brown Date: Fri, 27 Oct 2023 11:38:59 -0600 Subject: [PATCH 3/3] Improve readability and fix docstring error --- .../docker/multiplatform_image_builder.py | 194 ++++++++++-------- 1 file changed, 106 insertions(+), 88 deletions(-) diff --git a/buildrunner/docker/multiplatform_image_builder.py b/buildrunner/docker/multiplatform_image_builder.py index 8991e899..fbf6e2c1 100644 --- a/buildrunner/docker/multiplatform_image_builder.py +++ b/buildrunner/docker/multiplatform_image_builder.py @@ -117,11 +117,12 @@ def __repr__(self): class MultiplatformImageBuilder: # pylint: disable=too-many-instance-attributes """Multiple platform image builder""" - def __init__(self, - use_local_registry: bool = True, - keep_images: bool = False, - temp_dir: str = os.getcwd(), - disable_multi_platform: bool = False,): + def __init__( + self, + use_local_registry: bool = True, + keep_images: bool = False, + temp_dir: str = os.getcwd(), + disable_multi_platform: bool = False,): self._registry_info = None self._use_local_registry = use_local_registry self._keep_images = keep_images @@ -224,14 +225,15 @@ def _stop_local_registry(self): logger.warning("Local registry is not running when attempting to stop it") # pylint: disable=too-many-arguments,too-many-locals - def _build_with_inject(self, - inject: dict, - tagged_names: List[str], - platform: str, - push: bool, - path: str, - file: str, - build_args: dict,) -> None: + def _build_with_inject( + self, + inject: dict, + tagged_names: List[str], + platform: str, + push: bool, + path: str, + file: str, + build_args: dict,) -> None: if not path or not os.path.isdir(path): logger.warning(f"Failed to inject {inject} for {tagged_names} since path {path} isn't a directory.") @@ -256,12 +258,13 @@ def _build_with_inject(self, assert os.path.isdir(context_dir), f"Failed to create context dir {context_dir}" - docker.buildx.build(context_dir, - tags=tagged_names, - platforms=[platform], - push=push, - file=file, - build_args=build_args) + docker.buildx.build( + context_dir, + tags=tagged_names, + platforms=[platform], + push=push, + file=file, + build_args=build_args) # pylint: disable=too-many-arguments @retry(python_on_whales.exceptions.DockerException, @@ -270,16 +273,17 @@ def _build_with_inject(self, backoff=3, max_delay=10, logger=logger) - def _build_single_image(self, - name: str, - platform: str, - push: bool, - path: str, - file: str, - tags: List[str], - build_args: dict, - mp_image_name: str, - inject: dict) -> None: + def _build_single_image( + self, + name: str, + platform: str, + push: bool, + path: str, + file: str, + tags: List[str], + build_args: dict, + mp_image_name: str, + inject: dict) -> None: """ Builds a single image for the given platform @@ -290,8 +294,9 @@ def _build_single_image(self, path (str): The path to the Dockerfile. file (str): The path/name of the Dockerfile (ie. /Dockerfile). tags (List[str]): The tags to apply to the image. - docker_registry (str): The docker registry to push the image to. - mp_image_name (str): A list of built images. + build_args (dict): The build args to pass to docker. + mp_image_name (str): The multi-platform name of the image. + inject (dict): The files to inject into the build context. """ assert os.path.isdir(path) and os.path.exists(f"{file}"), \ f"Either path {path}({os.path.isdir(path)}) or file " \ @@ -301,20 +306,22 @@ def _build_single_image(self, logger.debug(f"Building {tagged_names} for {platform}") if inject and isinstance(inject, dict): - self._build_with_inject(inject=inject, - tagged_names=tagged_names, - platform=platform, - push=push, - path=path, - file=file, - build_args=build_args) + self._build_with_inject( + inject=inject, + tagged_names=tagged_names, + platform=platform, + push=push, + path=path, + file=file, + build_args=build_args) else: - docker.buildx.build(path, - tags=tagged_names, - platforms=[platform], - push=push, - file=file, - build_args=build_args) + docker.buildx.build( + path, + tags=tagged_names, + platforms=[platform], + push=push, + file=file, + build_args=build_args) # Check that the images were built and in the registry # Docker search is not currently implemented in python-on-wheels @@ -334,10 +341,12 @@ def _build_single_image(self, logger.error(f"Failed to build {tag_name}: {err}") raise err - self._intermediate_built_images[mp_image_name].append(ImageInfo(repo=name, - tags=tags, - platform=platform, - digest=image_id,)) + self._intermediate_built_images[mp_image_name].append(ImageInfo( + repo=name, + tags=tags, + platform=platform, + digest=image_id, + )) def get_single_platform_to_build(self, platforms: List[str]) -> str: """ Returns the platform to build for single platform flag """ @@ -361,18 +370,19 @@ def get_single_platform_to_build(self, platforms: List[str]) -> str: return platforms[0] # pylint: disable=too-many-locals - def build_multiple_images(self, - platforms: List[str], - path: str = ".", - file: str = "Dockerfile", - mp_image_name: str = None, - tags: List[str] = None, - push=True, - do_multiprocessing: bool = True, - docker_registry: str = None, - build_args: dict = None, - inject: dict = None, - ) -> List[ImageInfo]: + def build_multiple_images( + self, + platforms: List[str], + path: str = ".", + file: str = "Dockerfile", + mp_image_name: str = None, + tags: List[str] = None, + push=True, + do_multiprocessing: bool = True, + docker_registry: str = None, + build_args: dict = None, + inject: dict = None, + ) -> List[ImageInfo]: """ Builds multiple images for the given platforms. One image will be built for each platform. @@ -434,15 +444,17 @@ def get_path(file): "this will only build a single-platform image.\n" f"image: {santized_name} platform:{platform}\n" f"{line}") - self._build_single_image(platform_image_name, - platform, - push, - path, - dockerfile, - tags, - build_args, - mp_image_name, - inject) + self._build_single_image( + platform_image_name, + platform, + push, + path, + dockerfile, + tags, + build_args, + mp_image_name, + inject + ) else: processes = [] print(f"{line}\n" @@ -455,26 +467,32 @@ def get_path(file): platform_image_name = f"{base_image_name}-{platform.replace('/', '-')}" logger.debug(f"Building {platform_image_name} for {platform}") if do_multiprocessing: - processes.append(Process(target=self._build_single_image, - args=(platform_image_name, - platform, - push, - path, - dockerfile, - tags, - build_args, - mp_image_name, - inject))) + processes.append(Process( + target=self._build_single_image, + args=( + platform_image_name, + platform, + push, + path, + dockerfile, + tags, + build_args, + mp_image_name, + inject + ) + )) else: - self._build_single_image(platform_image_name, - platform, - push, - path, - dockerfile, - tags, - build_args, - mp_image_name, - inject) + self._build_single_image( + platform_image_name, + platform, + push, + path, + dockerfile, + tags, + build_args, + mp_image_name, + inject + ) for proc in processes: proc.start()