From 8e830941719c65926f41f3e2703abcc8f204da8e Mon Sep 17 00:00:00 2001 From: Shane Brown Date: Mon, 7 Aug 2023 17:21:00 -0600 Subject: [PATCH] Resolve some suggestions from PR comments --- README.rst | 14 +--- .../docker/multiplatform_image_builder.py | 77 ++++++++++--------- buildrunner/steprunner/tasks/build.py | 4 +- tests/test_multiplatform.py | 24 +++--- 4 files changed, 59 insertions(+), 60 deletions(-) diff --git a/README.rst b/README.rst index 8b19ed50..6b475490 100755 --- a/README.rst +++ b/README.rst @@ -432,9 +432,10 @@ shows the different configuration options available: 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 + # 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. platform: - linux/amd64 - linux/arm64/v8 @@ -727,13 +728,6 @@ the run step: 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 - platform: - - linux/amd64 - - linux/arm64/v8 - # systemd does not play well with docker typically, but you can # use this setting to tell buildrunner to set the necessary docker # flags to get systemd to work properly: diff --git a/buildrunner/docker/multiplatform_image_builder.py b/buildrunner/docker/multiplatform_image_builder.py index 63166a9d..dbd648fa 100644 --- a/buildrunner/docker/multiplatform_image_builder.py +++ b/buildrunner/docker/multiplatform_image_builder.py @@ -23,7 +23,7 @@ def __init__(self, repo: str, tags: List[str] = None): self._repo = repo if tags is None: - self._tags = ['latest'] + self._tags = ["latest"] else: self._tags = tags @@ -84,7 +84,7 @@ def __exit__(self, exc_type, exc_value, traceback): for image in images: for tag in image.tags: LOGGER.debug(f"Removing image {image.repo}:{tag} for {name}") - docker.image.remove(f'{image.repo}:{tag}', force=True) + docker.image.remove(f"{image.repo}:{tag}", force=True) # Removes all tagged images if keep_images is False if self._tagged_images_names and not self._keep_images: @@ -130,13 +130,13 @@ def _start_local_registry(self): # Something changed in the registry image, so we need to update this code assert len(ports) == 1, \ f"Expected 1 port, but got {len(ports)}" - assert isinstance(ports.get('5000/tcp')[0], dict), \ + assert isinstance(ports.get("5000/tcp")[0], dict), \ f"Expected dict, but got {type(ports.get('5000/tcp')[0])}" - assert ports.get('5000/tcp')[0].get('HostIp') == '0.0.0.0', \ + assert ports.get("5000/tcp")[0].get('HostIp') == "0.0.0.0", \ f"Expected HostIp to be 0.0.0.0 but got {ports.get('5000/tcp')[0].get('HostIp')}" - self._reg_ip = 'localhost' - self._reg_port = ports.get('5000/tcp')[0].get('HostPort') + self._reg_ip = "localhost" + self._reg_port = ports.get("5000/tcp")[0].get("HostPort") self._reg_name = container.name LOGGER.debug(f"Started local registry {self._reg_name} at {self._reg_ip}:{self._reg_port}") @@ -145,36 +145,39 @@ def _stop_local_registry(self): Stops and removes the local registry along with any images """ LOGGER.debug(f"Stopping and removing local registry {self._reg_name} at {self._reg_ip}:{self._reg_port}") - docker.stop(self._reg_name) - docker.remove(self._reg_name, volumes=True, force=True) + try: + docker.remove(self._reg_name, volumes=True, force=True) + except python_on_whales.exceptions.NoSuchContainer as err: + LOGGER.error(f"Failed to stop and remove local registry {self._reg_name}: {err}") + # pylint: disable=too-many-arguments - def build_image(self, + def build_single_image(self, name: str, platform: str, push: bool = True, - path: str = '.', - file: str = 'Dockerfile', + path: str = ".", + file: str = "Dockerfile", tags: List[str] = None,) -> None: """ - Builds the image for the given platform + 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'. + 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. """ if tags is None: - tags = ['latest'] + tags = ["latest"] - assert os.path.isdir(path) and os.path.exists(f'{file}'), \ + assert os.path.isdir(path) and os.path.exists(f"{file}"), \ f"Either path {path}({os.path.isdir(path)}) or file " \ "'{file}'({os.path.exists(f'{file}')}) does not exist!" - tagged_names = [f'{name}:{tag}' for tag in tags] + tagged_names = [f"{name}:{tag}" for tag in tags] LOGGER.debug(f"Building {tagged_names} for {platform}") # Build the image with the specified tags @@ -192,21 +195,21 @@ def build_image(self, raise err return image - def build(self, + def build_multiple_images(self, platforms: List[str], - path: str = '.', - file: str = 'Dockerfile', + path: str = ".", + file: str = "Dockerfile", name: str = None, tags: List[str] = None, push=True, do_multiprocessing: bool = False) -> List[ImageInfo]: """ - Builds the images for the given platforms + Builds multiple images for the given platforms. One image will be built for each platform. Args: 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'. + 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. 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. @@ -219,10 +222,10 @@ def build(self, LOGGER.debug(f"Building {name}:{tags} for platforms {platforms} from {file}") if tags is None: - tags = ['latest'] + tags = ["latest"] # Updates name to be compatible with docker - image_prefix = 'buildrunner-mp' + image_prefix = "buildrunner-mp" santized_name = f"{image_prefix}-{name.replace('/', '-').replace(':', '-')}" base_image_name = f"{self._reg_ip}:{self._reg_port}/{santized_name}" @@ -234,9 +237,9 @@ def build(self, curr_name = f"{base_image_name}-{platform.replace('/', '-')}" LOGGER.debug(f"Building {curr_name} for {platform}") if do_multiprocessing: - processes.append(Process(target=self.build_image, args=(curr_name, platform, push, path, file, tags))) + processes.append(Process(target=self.build_single_image, args=(curr_name, platform, push, path, file, tags))) else: - self.build_image(curr_name, platform, push, path, file, tags) + self.build_single_image(curr_name, platform, push, path, file, tags) self._intermediate_built_images[name].append(ImageInfo(curr_name, tags)) for proc in processes: @@ -275,13 +278,13 @@ def push(self, name: str, dest_names: List[str] = None) -> None: if dest_names is None: dest_names = name for tag in src_images[0].tags: - tagged_names.append(f'{dest_names}:{tag}') + tagged_names.append(f"{dest_names}:{tag}") else: tagged_names = dest_names for image in src_images: for tag in image.tags: - src_names.append(f'{image.repo}:{tag}') + src_names.append(f"{image.repo}:{tag}") timeout_seconds = initial_timeout_seconds while retries > 0: @@ -319,7 +322,7 @@ def _find_native_platform_images(self, name: str) -> str: host_os = system() host_arch = machine() LOGGER.debug(f"Finding native platform for {name} for {host_os}/{host_arch}") - pattern = f'{host_os}-{host_arch}' + pattern = f"{host_os}-{host_arch}" # No images built for this name if name not in self._intermediate_built_images.keys() \ @@ -330,8 +333,8 @@ def _find_native_platform_images(self, name: str) -> str: # No matches found, change os if match_platform == []: - if host_os == 'Darwin': - pattern = f'linux-{host_arch}' + if host_os == "Darwin": + pattern = f"linux-{host_arch}" match_platform = [image for image in self._intermediate_built_images[name] if image.repo.endswith(pattern)] assert len(match_platform) <= 1, f"Found more than one match for {name} and {pattern}: {match_platform}" @@ -349,13 +352,13 @@ def tag_single_platform(self, name: str, tags: List[str] = None, dest_name: str Args: name (str): The name of the image to load - tags (List[str], optional): The tags to load. Defaults to 'latest'. - dest_name (str, optional): The name to load the image as. Defaults to the 'name' arg. + tags (List[str], optional): The tags to load. Defaults to "latest". + dest_name (str, optional): The name to load the image as. Defaults to the "name" arg. """ # This is to handle pylint's "dangerous-default-value" error if tags is None: - tags = ['latest'] - LOGGER.debug(f'Tagging {name} with tags {tags} - Dest name: {dest_name}') + tags = ["latest"] + LOGGER.debug(f"Tagging {name} with tags {tags} - Dest name: {dest_name}") source_image = self._find_native_platform_images(name) if dest_name is None: dest_name = name @@ -369,7 +372,7 @@ def tag_single_platform(self, name: str, tags: List[str] = None, dest_name: str try: docker.pull(image) for tag in tags: - dest_tag = f'{dest_name}:{tag}' + dest_tag = f"{dest_name}:{tag}" docker.tag(image, dest_tag) LOGGER.debug(f"Tagged {image} as {dest_tag}") self._tagged_images_names[name].append(dest_tag) diff --git a/buildrunner/steprunner/tasks/build.py b/buildrunner/steprunner/tasks/build.py index 08ce01b8..0c3d64c9 100644 --- a/buildrunner/steprunner/tasks/build.py +++ b/buildrunner/steprunner/tasks/build.py @@ -217,8 +217,8 @@ def run(self, context): name=self.get_unique_build_name(),) 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)})" + f'Number of built images ({len(built_images)}) does not match ' \ + 'the number of platforms ({len(self.platform)})' else: exit_code = builder.build( diff --git a/tests/test_multiplatform.py b/tests/test_multiplatform.py index 55f060b3..13fd0780 100644 --- a/tests/test_multiplatform.py +++ b/tests/test_multiplatform.py @@ -11,6 +11,8 @@ TEST_DIR = os.path.basename(os.path.dirname(__file__)) +# 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]: missing_images = [] found = False @@ -133,7 +135,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(name=name, + built_images = mp.build_multiple_images(name=name, platforms=platforms, path=test_path, file=f'{test_path}/Dockerfile', @@ -166,7 +168,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(name=name, + built_images = mp.build_multiple_images(name=name, platforms=platforms, path=test_path, file=f'{test_path}/Dockerfile', @@ -201,7 +203,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(name=name, + built_images = mp.build_multiple_images(name=name, platforms=platforms, path=test_path, file=f'{test_path}/Dockerfile', @@ -241,7 +243,7 @@ def test_push(): test_path = f'{TEST_DIR}/test-files/multiplatform' with MultiplatformImageBuilder() as mp: - built_images = mp.build(name=build_name, + built_images = mp.build_multiple_images(name=build_name, platforms=platforms, path=test_path, file=f'{test_path}/Dockerfile', @@ -281,7 +283,7 @@ def test_push_with_dest_names(): test_path = f'{TEST_DIR}/test-files/multiplatform' with MultiplatformImageBuilder() as mp: - built_images = mp.build(name=build_name, + built_images = mp.build_multiple_images(name=build_name, platforms=platforms, path=test_path, file=f'{test_path}/Dockerfile', @@ -319,10 +321,10 @@ def test_push_with_dest_names(): ) ]) def test_build(name, platforms, expected_image_names): - with patch('buildrunner.docker.multiplatform_image_builder.MultiplatformImageBuilder.build_image'): + with patch('buildrunner.docker.multiplatform_image_builder.MultiplatformImageBuilder.build_single_image'): test_path = f'{TEST_DIR}/test-files/multiplatform' with MultiplatformImageBuilder() as mp: - built_images = mp.build(name=name, + built_images = mp.build_multiple_images(name=name, platforms=platforms, path=test_path, file=f'{test_path}/Dockerfile', @@ -344,17 +346,17 @@ def test_build_multiple_builds(): expected_image_names2 = ['test-build-multi-image-2002-linux-amd64', 'test-build-multi-image-2002-linux-arm64'] test_path = f'{TEST_DIR}/test-files/multiplatform' - with patch('buildrunner.docker.multiplatform_image_builder.MultiplatformImageBuilder.build_image'): + with patch('buildrunner.docker.multiplatform_image_builder.MultiplatformImageBuilder.build_single_image'): with MultiplatformImageBuilder() as mp: # Build set 1 - built_images1 = mp.build(name=name1, + built_images1 = mp.build_multiple_images(name=name1, platforms=platforms1, path=test_path, file=f'{test_path}/Dockerfile', do_multiprocessing=False) # Build set 2 - built_images2 = mp.build(name=name2, + built_images2 = mp.build_multiple_images(name=name2, platforms=platforms2, path=test_path, file=f'{test_path}/Dockerfile', @@ -387,7 +389,7 @@ def test_build_multiple_builds(): 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(name=name, + built_images = mp.build_multiple_images(name=name, platforms=platforms, path=test_path, file=f'{test_path}/Dockerfile',