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.

Sign up for GitHub

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

Bugfix for intermittent multi-platform build failures #87

Merged
merged 3 commits into from
Oct 27, 2023
Merged

Bugfix for intermittent multi-platform build failures #87

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ded83f7
Bugfix to remove image with tagged name instead of image digest
shanejbrown Oct 26, 2023
9749320
Refactor multi-platform build image
shanejbrown Oct 27, 2023
9e4bbcf
Improve readability and fix docstring error
shanejbrown Oct 27, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
235 changes: 125 additions & 110 deletions buildrunner/docker/multiplatform_image_builder.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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.")
Expand All @@ -256,50 +258,46 @@ 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,
tries=5,
tries=3,
delay=1,
backoff=3,
max_delay=30,
max_delay=10,
logger=logger)
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:
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

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. <path>/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. <path>/Dockerfile).
tags (List[str]): The tags to apply to the image.
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.
"""
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!"
Expand All @@ -308,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
Expand All @@ -333,17 +333,20 @@ 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:
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 """
Expand All @@ -367,29 +370,33 @@ 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",
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.

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. <path>/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
Expand All @@ -410,7 +417,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
Expand All @@ -421,31 +428,33 @@ 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,
platform,
push,
path,
dockerfile,
tags,
build_args,
self._intermediate_built_images[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"
Expand All @@ -455,29 +464,35 @@ 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,
platform,
push,
path,
dockerfile,
tags,
build_args,
self._intermediate_built_images[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(curr_name,
platform,
push,
path,
dockerfile,
tags,
build_args,
self._intermediate_built_images[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()
Expand All @@ -488,7 +503,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:
"""
Expand Down
8 changes: 4 additions & 4 deletions buildrunner/steprunner/tasks/build.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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:
Expand Down
Loading
Loading