From e8169860428a8ff4e9c9bb7ac90d06335e6f92b9 Mon Sep 17 00:00:00 2001 From: Shane Brown Date: Mon, 4 Nov 2024 15:27:25 -0700 Subject: [PATCH 1/2] Rollback python_on_whales conversion --- buildrunner/__init__.py | 73 ++++++++------------------- buildrunner/docker/__init__.py | 17 +++---- buildrunner/docker/builder.py | 4 +- buildrunner/docker/daemon.py | 17 ++----- buildrunner/docker/importer.py | 8 +-- buildrunner/docker/runner.py | 59 ++++++---------------- buildrunner/sshagent/__init__.py | 57 +++++---------------- buildrunner/steprunner/tasks/build.py | 4 +- buildrunner/steprunner/tasks/run.py | 17 ++----- tests/test_builders.py | 2 +- tests/test_caching.py | 18 +++---- 11 files changed, 78 insertions(+), 198 deletions(-) diff --git a/buildrunner/__init__.py b/buildrunner/__init__.py index f8a133c7..a5814929 100644 --- a/buildrunner/__init__.py +++ b/buildrunner/__init__.py @@ -21,7 +21,6 @@ import types from typing import List, Optional -import python_on_whales import requests from retry import retry @@ -39,7 +38,7 @@ ) from buildrunner.steprunner import BuildStepRunner from buildrunner.docker.multiplatform_image_builder import MultiplatformImageBuilder -import buildrunner.docker.builder as legacy_builder +import buildrunner.docker.builder LOGGER = logging.getLogger(__name__) @@ -288,37 +287,15 @@ def get_source_image(self): source_archive_path: "source.tar", SOURCE_DOCKERFILE: "Dockerfile", } - if self.buildrunner_config.run_config.use_legacy_builder: - image = legacy_builder.build_image( - temp_dir=self.buildrunner_config.global_config.temp_dir, - inject=inject, - timeout=self.docker_timeout, - docker_registry=self.buildrunner_config.global_config.docker_registry, - nocache=True, - pull=False, - ) - self._source_image = image - else: - # Use buildx builder - native_platform = self._step_runner.multi_platform.get_native_platform() - LOGGER.info(f"Setting platforms to [{native_platform}]") - platforms = [native_platform] - built_images_info = ( - self._step_runner.multi_platform.build_multiple_images( - platforms=platforms, - inject=inject, - cache=False, - pull=False, - build_args={ - "BUILDRUNNER_DISTRO": os.environ.get("BUILDRUNNER_DISTRO") - }, - ) - ) - if len(built_images_info.built_images) != 1: - raise BuildRunnerProcessingError( - "Failed to build source image. Retrying the build may resolve the issue." - ) - self._source_image = built_images_info.built_images[0].trunc_digest + image = buildrunner.docker.builder.build_image( + temp_dir=self.buildrunner_config.global_config.temp_dir, + inject=inject, + timeout=self.docker_timeout, + docker_registry=self.buildrunner_config.global_config.docker_registry, + nocache=True, + pull=False, + ) + self._source_image = image return self._source_image @@ -519,16 +496,11 @@ def run(self): # pylint: disable=too-many-statements,too-many-branches,too-many if self._source_image: self.log.write(f"Destroying source image {self._source_image}\n") try: - if self.buildrunner_config.run_config.use_legacy_builder: - _docker_client.remove_image( - self._source_image, - noprune=False, - force=True, - ) - else: - python_on_whales.docker.image.remove( - self._source_image, prune=True, force=True - ) + _docker_client.remove_image( + self._source_image, + noprune=False, + force=True, + ) except ImageNotFound: self.log.warning( f"Failed to remove source image {self._source_image}\n" @@ -540,16 +512,11 @@ def run(self): # pylint: disable=too-many-statements,too-many-branches,too-many # reverse the order of the images since child images would likely come after parent images for _image in self.generated_images[::-1]: try: - if self.buildrunner_config.run_config.use_legacy_builder: - _docker_client.remove_image( - _image, - noprune=False, - force=True, - ) - else: - python_on_whales.docker.image.remove( - _image, prune=True, force=True - ) + _docker_client.remove_image( + _image, + noprune=False, + force=True, + ) except Exception as _ex: # pylint: disable=broad-except self.log.write(f"Error removing image {_image}: {str(_ex)}\n") else: diff --git a/buildrunner/docker/__init__.py b/buildrunner/docker/__init__.py index 07a2e842..a57dc9e6 100644 --- a/buildrunner/docker/__init__.py +++ b/buildrunner/docker/__init__.py @@ -12,7 +12,6 @@ from typing import Tuple import urllib.parse import docker -import python_on_whales from buildrunner.errors import BuildRunnerError, BuildRunnerConfigurationError @@ -96,21 +95,17 @@ def new_client( ) -def force_remove_container(docker_client, container, use_legacy_builder: bool): +def force_remove_container(docker_client, container): """ Force removes a container from the given docker client. :param docker_client: the docker client :param container: the container - :param use_legacy_builder: whether to use the legacy builder """ - if use_legacy_builder: - docker_client.remove_container( - container, - force=True, - v=True, - ) - else: - python_on_whales.docker.remove(container, force=True, volumes=True) + docker_client.remove_container( + container, + force=True, + v=True, + ) def get_dockerfile(dockerfile: str, temp_dir: str = None) -> Tuple[str, bool]: diff --git a/buildrunner/docker/builder.py b/buildrunner/docker/builder.py index 05f6b278..962346b9 100644 --- a/buildrunner/docker/builder.py +++ b/buildrunner/docker/builder.py @@ -277,9 +277,7 @@ def cleanup(self): # iterate through and destroy intermediate containers for container in self.intermediate_containers: try: - force_remove_container( - self.docker_client, container, use_legacy_builder=True - ) + force_remove_container(self.docker_client, container) except docker.errors.APIError as err: logger.debug( f"Error removing intermediate container {container}: {err}" diff --git a/buildrunner/docker/daemon.py b/buildrunner/docker/daemon.py index 1c5e52f7..c398764e 100644 --- a/buildrunner/docker/daemon.py +++ b/buildrunner/docker/daemon.py @@ -8,9 +8,7 @@ import os -import python_on_whales -from buildrunner.config import BuildRunnerConfig from buildrunner.docker import DOCKER_DEFAULT_DOCKERD_URL DAEMON_IMAGE_NAME = "busybox:latest" @@ -99,13 +97,8 @@ def stop(self): f"Destroying Docker daemon container {self._daemon_container:.10}\n" ) if self._daemon_container: - if BuildRunnerConfig.get_instance().run_config.use_legacy_builder: - self.docker_client.remove_container( - self._daemon_container, - force=True, - v=True, - ) - else: - python_on_whales.docker.remove( - self._daemon_container, force=True, volumes=True - ) + self.docker_client.remove_container( + self._daemon_container, + force=True, + v=True, + ) diff --git a/buildrunner/docker/importer.py b/buildrunner/docker/importer.py index 0f727741..074cfab9 100644 --- a/buildrunner/docker/importer.py +++ b/buildrunner/docker/importer.py @@ -6,13 +6,11 @@ with the terms of the Adobe license agreement accompanying it. """ -import python_on_whales import yaml import docker import docker.errors -from buildrunner.config import BuildRunnerConfig from buildrunner.docker import new_client from buildrunner.errors import BuildRunnerProcessingError from buildrunner.utils import is_dict @@ -43,11 +41,7 @@ def import_image(self): """ try: - import_return = None - if BuildRunnerConfig.get_instance().run_config.use_legacy_builder: - import_return = self.docker_client.import_image(self.src) - else: - import_return = python_on_whales.docker.import_(self.src) + import_return = self.docker_client.import_image(self.src) except docker.errors.APIError as apie: raise BuildRunnerProcessingError( f"Error importing image from archive file {self.src}: {apie}" diff --git a/buildrunner/docker/runner.py b/buildrunner/docker/runner.py index 9ee367bb..25f8de44 100644 --- a/buildrunner/docker/runner.py +++ b/buildrunner/docker/runner.py @@ -23,11 +23,9 @@ from docker.utils import compare_version from retry import retry import docker.errors -import python_on_whales import six import timeout_decorator -from buildrunner.config import BuildRunnerConfig from buildrunner.docker import ( new_client, force_remove_container, @@ -133,10 +131,6 @@ def __init__(self, image_config, dockerd_url=None, log=None): if log: log.write("\nImage pulled successfully\n") - self.use_docker_py = ( - BuildRunnerConfig.get_instance().run_config.use_legacy_builder - ) - def start( self, shell="/bin/sh", @@ -242,10 +236,7 @@ def start( # start the container self.container = self.docker_client.create_container(self.image_name, **kwargs) - if self.use_docker_py: - self.docker_client.start(self.container["Id"]) - else: - python_on_whales.docker.start(self.container["Id"]) + self.docker_client.start(self.container["Id"]) # run any supplied provisioners if provisioners: @@ -263,27 +254,19 @@ def stop(self): Stop the backing Docker container. """ if self.container: - if self.use_docker_py: - self.docker_client.stop( - self.container["Id"], - timeout=0, - ) - else: - python_on_whales.docker.stop(self.container["Id"], time=0) + self.docker_client.stop( + self.container["Id"], + timeout=0, + ) def cleanup(self): """ Cleanup the backing Docker container, stopping it if necessary. """ if self.container: - use_docker_py = ( - BuildRunnerConfig.get_instance().run_config.use_legacy_builder - ) for container in self.containers: try: - force_remove_container( - self.docker_client, container, use_legacy_builder=use_docker_py - ) + force_remove_container(self.docker_client, container) except docker.errors.NotFound: try: container_ids = self.docker_client.containers( @@ -291,16 +274,11 @@ def cleanup(self): ) if container_ids: for container_id in container_ids: - if use_docker_py: - self.docker_client.remove_container( - container_id["Id"], - force=True, - v=True, - ) - else: - python_on_whales.docker.remove( - container_id["Id"], force=True, volumes=True - ) + self.docker_client.remove_container( + container_id["Id"], + force=True, + v=True, + ) else: print( f'Unable to find docker container with name or label "{container}"' @@ -309,16 +287,11 @@ def cleanup(self): print( f'Unable to find docker container with name or label "{container}"' ) - if use_docker_py: - self.docker_client.remove_container( - self.container["Id"], - force=True, - v=True, - ) - else: - python_on_whales.docker.remove( - self.container["Id"], force=True, volumes=True - ) + self.docker_client.remove_container( + self.container["Id"], + force=True, + v=True, + ) self.container = None diff --git a/buildrunner/sshagent/__init__.py b/buildrunner/sshagent/__init__.py index 17248a2a..9db7195b 100644 --- a/buildrunner/sshagent/__init__.py +++ b/buildrunner/sshagent/__init__.py @@ -27,10 +27,8 @@ from paramiko.common import io_sleep from paramiko.util import asbytes from paramiko.message import Message -import python_on_whales -import buildrunner.config -import buildrunner.docker.builder as legacy_builder +import buildrunner.docker.builder from buildrunner.errors import ( BuildRunnerConfigurationError, BuildRunnerProcessingError, @@ -245,52 +243,25 @@ def stop(self): self.log.write( f"Destroying ssh-agent container {self._ssh_agent_container:.10}\n" ) - if buildrunner.config.BuildRunnerConfig.get_instance().run_config.use_legacy_builder: - self.docker_client.remove_container( - self._ssh_agent_container, - force=True, - v=True, - ) - else: - python_on_whales.docker.remove( - self._ssh_agent_container, force=True, volumes=True - ) + self.docker_client.remove_container( + self._ssh_agent_container, + force=True, + v=True, + ) def get_ssh_agent_image(self): """ Get and/or create the image used to proxy the ssh agent to a container. """ - buildrunner_config = buildrunner.config.BuildRunnerConfig.get_instance() if not self._ssh_agent_image: - if buildrunner_config.run_config.use_legacy_builder: - self.log.write("Creating ssh-agent image\n") - image = legacy_builder.build_image( - path=SSH_AGENT_PROXY_BUILD_CONTEXT, - docker_registry=self.docker_registry, - nocache=False, - pull=False, - ) - self._ssh_agent_image = image - else: - native_platform = ( - self._multiplatform_image_builder.get_native_platform() - ) - platforms = [native_platform] - built_images_info = ( - self._multiplatform_image_builder.build_multiple_images( - platforms=platforms, - path=SSH_AGENT_PROXY_BUILD_CONTEXT, - file=f"{SSH_AGENT_PROXY_BUILD_CONTEXT}/Dockerfile", - cache=True, - pull=False, - use_threading=False, - ) - ) - if len(built_images_info.built_images) != 1: - raise BuildRunnerProcessingError( - "Failed to build ssh-agent image. Retrying the build may resolve the issue." - ) - self._ssh_agent_image = built_images_info.built_images[0].trunc_digest + self.log.write("Creating ssh-agent image\n") + image = buildrunner.docker.builder.build_image( + path=SSH_AGENT_PROXY_BUILD_CONTEXT, + docker_registry=self.docker_registry, + nocache=False, + pull=False, + ) + self._ssh_agent_image = image return self._ssh_agent_image diff --git a/buildrunner/steprunner/tasks/build.py b/buildrunner/steprunner/tasks/build.py index d6ef0f35..cc9c308f 100644 --- a/buildrunner/steprunner/tasks/build.py +++ b/buildrunner/steprunner/tasks/build.py @@ -16,7 +16,7 @@ from buildrunner.config import BuildRunnerConfig from buildrunner.config.models_step import StepBuild from buildrunner.docker.importer import DockerImporter -import buildrunner.docker.builder as legacy_builder +import buildrunner.docker.builder from buildrunner.errors import ( BuildRunnerConfigurationError, ) @@ -262,7 +262,7 @@ def run(self, context): else: # Use the legacy builder - image = legacy_builder.build_image( + image = buildrunner.docker.builder.build_image( path=self.path, inject=self.to_inject, dockerfile=self.dockerfile, diff --git a/buildrunner/steprunner/tasks/run.py b/buildrunner/steprunner/tasks/run.py index 35ac22b0..c677b85a 100644 --- a/buildrunner/steprunner/tasks/run.py +++ b/buildrunner/steprunner/tasks/run.py @@ -1134,18 +1134,11 @@ def cleanup(self, context): # pylint: disable=unused-argument self.step_runner.log.write( f"Destroying source container {self._source_container:.10}\n" ) - if BuildRunnerConfig.get_instance().run_config.use_legacy_builder: - self._docker_client.remove_container( - self._source_container, - force=True, - v=True, - ) - else: - python_on_whales.docker.container.remove( - self._source_container, - force=True, - volumes=True, - ) + self._docker_client.remove_container( + self._source_container, + force=True, + v=True, + ) for image in self.images_to_remove: python_on_whales.docker.image.remove(image, force=True) diff --git a/tests/test_builders.py b/tests/test_builders.py index 400d7dc4..605ccc6e 100644 --- a/tests/test_builders.py +++ b/tests/test_builders.py @@ -183,7 +183,7 @@ def fixture_set_env(): ], ) @mock.patch( - "tests.test_runner.buildrunner.steprunner.tasks.build.legacy_builder.build_image" + "tests.test_runner.buildrunner.steprunner.tasks.build.buildrunner.docker.builder.build_image" ) @mock.patch( "tests.test_runner.buildrunner.steprunner.MultiplatformImageBuilder.build_multiple_images" diff --git a/tests/test_caching.py b/tests/test_caching.py index 135786c8..817e540e 100644 --- a/tests/test_caching.py +++ b/tests/test_caching.py @@ -34,19 +34,15 @@ def fixture_setup_runner(): "docker.io/ubuntu:19.04", pull_image=False, ) - with mock.patch( - "buildrunner.docker.runner.BuildRunnerConfig.get_instance" - ) as mock_build_runner_config: - # Set to use docker-py to be used over python on whales - mock_build_runner_config.run_config.use_legacy_builder.return_value = True - runner = DockerRunner( - image_config=image_config, - ) - runner.start(working_dir="/root") - yield runner + runner = DockerRunner( + image_config=image_config, + ) + runner.start(working_dir="/root") + + yield runner - runner.cleanup() + runner.cleanup() @pytest.fixture(name="log_output") From 28fcaaeeadb67424ee41196a6d0f670b9fe9e136 Mon Sep 17 00:00:00 2001 From: Shane Brown Date: Tue, 5 Nov 2024 09:24:39 -0700 Subject: [PATCH 2/2] Add random rerun delay ond increase reruns test_buildrunner_scan_dir --- tests/test_buildrunner_files.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/test_buildrunner_files.py b/tests/test_buildrunner_files.py index 77e83b93..1f028ce4 100644 --- a/tests/test_buildrunner_files.py +++ b/tests/test_buildrunner_files.py @@ -1,4 +1,5 @@ import os +import random import pytest import platform import subprocess @@ -185,7 +186,7 @@ def test_buildrunner_arm_dir(test_dir: str, file_name, args, exit_code): _test_buildrunner_file(test_dir, file_name, args, exit_code) -@pytest.mark.flaky(reruns=2, reruns_delay=1) +@pytest.mark.flaky(reruns=5, reruns_delay=random.randint(1, 5)) @pytest.mark.parametrize( "test_dir, file_name, args, exit_code", _get_test_runs(test_dir=f"{TEST_DIR}/test-files/scan", serial_tests=False),