From 9b17134d4d1422497b9f2e459f946c2c3219156c Mon Sep 17 00:00:00 2001 From: riisi Date: Mon, 13 Nov 2023 15:23:36 +0000 Subject: [PATCH] Add docker image output field to support publish to repository when using BuildKit (#20154) Currently, the `publish` goal doesn't work with docker images when buildkit is enabled, as by [default buildkit doesn't save the build output locally](https://github.com/docker/buildx/issues/166), and `publish` expects that the images were saved. This PR adds support for setting the output type, and defaults it to`docker`, which is the legacy docker build behavior, i.e. saves to the local image store. However, we only want to set that when buildkit is enabled. I thought it better to add an explicit option for that at the subsystem level; this allows for validation of buildkit-only options. This eliminates the need to set `DOCKER_BUILDKIT=1` in env vars - I need to update the docs on that actually. I have validated that with this change, docker images can be published to a registry. --------- Co-authored-by: Rhys Madigan --- docs/markdown/Docker/docker.md | 12 +--- .../backend/docker/goals/package_image.py | 47 ++++++------ .../docker/goals/package_image_test.py | 72 +++++++++++++++++++ .../docker/subsystems/docker_options.py | 8 +++ .../pants/backend/docker/target_types.py | 52 ++++++++++++-- .../docker/util_rules/docker_binary.py | 8 ++- .../docker/util_rules/docker_binary_test.py | 1 + 7 files changed, 163 insertions(+), 37 deletions(-) diff --git a/docs/markdown/Docker/docker.md b/docs/markdown/Docker/docker.md index 9200e39a77b..15fdbda5d7b 100644 --- a/docs/markdown/Docker/docker.md +++ b/docs/markdown/Docker/docker.md @@ -184,12 +184,6 @@ very-secret-value BuildKit supports exporting build cache to an external location, making it possible to import in future builds. Cache backends can be configured using the [`cache_to`](doc:reference-docker_image#codecache_tocode) and [`cache_from`](doc:reference-docker_image#codecache_fromcode) fields. -Enable BuildKit if necessary (it is the default in later versions of Docker): - -``` -❯ export DOCKER_BUILDKIT=1 -``` - Create a builder using a [build driver](https://docs.docker.com/build/drivers/) that is compatible with the cache backend: ``` @@ -205,17 +199,17 @@ Use the builder: Optionally, validate a build with the Docker CLI directly: ``` -❯ docker build -t pants-cache-test:latest \ +❯ docker buildx build -t pants-cache-test:latest \ --cache-to type=local,dest=/tmp/docker/pants-test-cache \ --cache-from type=local,src=/tmp/docker/pants-test-cache . ``` -Configure Pants: +Configure Pants to use buildx and pass the BUILDX_BUILDER environment variable: ```toml pants.toml [docker] +use_buildx = true env_vars = [ - "DOCKER_BUILDKIT", "BUILDX_BUILDER" ] ``` diff --git a/src/python/pants/backend/docker/goals/package_image.py b/src/python/pants/backend/docker/goals/package_image.py index 20259e0fceb..b035e5f8270 100644 --- a/src/python/pants/backend/docker/goals/package_image.py +++ b/src/python/pants/backend/docker/goals/package_image.py @@ -19,12 +19,12 @@ from pants.backend.docker.registries import DockerRegistries, DockerRegistryOptions from pants.backend.docker.subsystems.docker_options import DockerOptions from pants.backend.docker.target_types import ( + DockerBuildKitOptionField, DockerBuildOptionFieldMixin, + DockerBuildOptionFieldMultiValueDictMixin, DockerBuildOptionFieldMultiValueMixin, DockerBuildOptionFieldValueMixin, DockerBuildOptionFlagFieldMixin, - DockerImageBuildImageCacheFromField, - DockerImageBuildImageCacheToField, DockerImageContextRootField, DockerImageRegistriesField, DockerImageRepositoryField, @@ -316,28 +316,31 @@ def get_build_options( global_target_stage_option: str | None, global_build_hosts_options: dict | None, global_build_no_cache_option: bool | None, + use_buildx_option: bool, target: Target, ) -> Iterator[str]: # Build options from target fields inheriting from DockerBuildOptionFieldMixin for field_type in target.field_types: - if issubclass(field_type, DockerBuildOptionFieldMixin): - source = InterpolationContext.TextSource( - address=target.address, target_alias=target.alias, field_alias=field_type.alias - ) - format = partial( - context.interpolation_context.format, - source=source, - error_cls=DockerImageOptionValueError, - ) - yield from target[field_type].options(format, global_build_hosts_options) - elif issubclass(field_type, DockerBuildOptionFieldValueMixin): - yield from target[field_type].options() - elif issubclass(field_type, DockerBuildOptionFieldMultiValueMixin): - yield from target[field_type].options() - elif issubclass(field_type, DockerBuildOptionFlagFieldMixin): - yield from target[field_type].options() - elif issubclass(field_type, DockerImageBuildImageCacheToField) or issubclass( - field_type, DockerImageBuildImageCacheFromField + if issubclass(field_type, DockerBuildKitOptionField): + if use_buildx_option is not True: + if target[field_type].value != target[field_type].default: + raise DockerImageOptionValueError( + f"The {target[field_type].alias} field on the = `{target.alias}` target in `{target.address}` was set to `{target[field_type].value}`" + f" and buildx is not enabled. Buildx must be enabled via the Docker subsystem options in order to use this field." + ) + else: + # Case where BuildKit option has a default value - still should not be generated + continue + + if issubclass( + field_type, + ( + DockerBuildOptionFieldMixin, + DockerBuildOptionFieldMultiValueDictMixin, + DockerBuildOptionFieldValueMixin, + DockerBuildOptionFieldMultiValueMixin, + DockerBuildOptionFlagFieldMixin, + ), ): source = InterpolationContext.TextSource( address=target.address, target_alias=target.alias, field_alias=field_type.alias @@ -347,7 +350,7 @@ def get_build_options( source=source, error_cls=DockerImageOptionValueError, ) - yield from target[field_type].options(format) + yield from target[field_type].options(format, global_build_hosts_options=global_build_hosts_options) # type: ignore[attr-defined] # Target stage target_stage = None @@ -440,6 +443,7 @@ async def build_docker_image( context_root=context_root, env=env, tags=tags, + use_buildx=options.use_buildx, extra_args=tuple( get_build_options( context=context, @@ -447,6 +451,7 @@ async def build_docker_image( global_target_stage_option=options.build_target_stage, global_build_hosts_options=options.build_hosts, global_build_no_cache_option=options.build_no_cache, + use_buildx_option=options.use_buildx, target=wrapped_target.target, ) ), diff --git a/src/python/pants/backend/docker/goals/package_image_test.py b/src/python/pants/backend/docker/goals/package_image_test.py index b56e9792884..ac5df3ddc63 100644 --- a/src/python/pants/backend/docker/goals/package_image_test.py +++ b/src/python/pants/backend/docker/goals/package_image_test.py @@ -14,6 +14,7 @@ from pants.backend.docker.goals.package_image import ( DockerBuildTargetStageError, + DockerImageOptionValueError, DockerImageTagValueError, DockerInfoV1, DockerPackageFieldSet, @@ -171,6 +172,7 @@ def mock_get_info_file(request: CreateDigest) -> Digest: opts.setdefault("build_hosts", None) opts.setdefault("build_verbose", False) opts.setdefault("build_no_cache", False) + opts.setdefault("use_buildx", False) opts.setdefault("env_vars", []) docker_options = create_subsystem( @@ -1113,8 +1115,10 @@ def test_docker_cache_to_option(rule_runner: RuleRunner) -> None: def check_docker_proc(process: Process): assert process.argv == ( "/dummy/docker", + "buildx", "build", "--cache-to=type=local,dest=/tmp/docker/pants-test-cache", + "--output=type=docker", "--pull=False", "--tag", "img1:latest", @@ -1127,6 +1131,7 @@ def check_docker_proc(process: Process): rule_runner, Address("docker/test", target_name="img1"), process_assertions=check_docker_proc, + options=dict(use_buildx=True), ) @@ -1147,8 +1152,10 @@ def test_docker_cache_from_option(rule_runner: RuleRunner) -> None: def check_docker_proc(process: Process): assert process.argv == ( "/dummy/docker", + "buildx", "build", "--cache-from=type=local,dest=/tmp/docker/pants-test-cache", + "--output=type=docker", "--pull=False", "--tag", "img1:latest", @@ -1161,9 +1168,74 @@ def check_docker_proc(process: Process): rule_runner, Address("docker/test", target_name="img1"), process_assertions=check_docker_proc, + options=dict(use_buildx=True), ) +def test_docker_output_option(rule_runner: RuleRunner) -> None: + """Testing non-default output type 'image'. + + Default output type 'docker' tested implicitly in other scenarios + """ + rule_runner.write_files( + { + "docker/test/BUILD": dedent( + """\ + docker_image( + name="img1", + output={"type": "image"} + ) + """ + ), + } + ) + + def check_docker_proc(process: Process): + assert process.argv == ( + "/dummy/docker", + "buildx", + "build", + "--output=type=image", + "--pull=False", + "--tag", + "img1:latest", + "--file", + "docker/test/Dockerfile", + ".", + ) + + assert_build( + rule_runner, + Address("docker/test", target_name="img1"), + process_assertions=check_docker_proc, + options=dict(use_buildx=True), + ) + + +def test_docker_output_option_raises_when_no_buildkit(rule_runner: RuleRunner) -> None: + rule_runner.write_files( + { + "docker/test/BUILD": dedent( + """\ + docker_image( + name="img1", + output={"type": "image"} + ) + """ + ), + } + ) + + with pytest.raises( + DockerImageOptionValueError, + match=r"Buildx must be enabled via the Docker subsystem options in order to use this field.", + ): + assert_build( + rule_runner, + Address("docker/test", target_name="img1"), + ) + + def test_docker_build_network_option(rule_runner: RuleRunner) -> None: rule_runner.write_files( { diff --git a/src/python/pants/backend/docker/subsystems/docker_options.py b/src/python/pants/backend/docker/subsystems/docker_options.py index cdd6f917e2a..5857b95b2ea 100644 --- a/src/python/pants/backend/docker/subsystems/docker_options.py +++ b/src/python/pants/backend/docker/subsystems/docker_options.py @@ -143,6 +143,14 @@ def env_vars(self) -> tuple[str, ...]: """ ), ) + use_buildx = BoolOption( + default=False, + help=softwrap( + """ + Use [buildx](https://github.com/docker/buildx#buildx) (and BuildKit) for builds. + """ + ), + ) _build_args = ShellStrListOption( help=softwrap( f""" diff --git a/src/python/pants/backend/docker/target_types.py b/src/python/pants/backend/docker/target_types.py index 105c10e3736..4d8ddf9f7c1 100644 --- a/src/python/pants/backend/docker/target_types.py +++ b/src/python/pants/backend/docker/target_types.py @@ -37,6 +37,7 @@ ) from pants.engine.unions import union from pants.util.docutil import bin_name, doc_url +from pants.util.frozendict import FrozenDict from pants.util.strutil import help_text, softwrap # Common help text to be applied to each field that supports value interpolation. @@ -304,21 +305,33 @@ class DockerBuildOptionFieldMultiValueDictMixin(DictStringToStringField): docker_build_option: ClassVar[str] @final - def options(self, value_formatter: OptionValueFormatter) -> Iterator[str]: + def options(self, value_formatter: OptionValueFormatter, **kwargs) -> Iterator[str]: if self.value: yield f"{self.docker_build_option}=" + ",".join( f"{key}={value_formatter(value)}" for key, value in self.value.items() ) +class DockerBuildKitOptionField: + """Mixin to indicate a BuildKit-specific option.""" + + @abstractmethod + def options(self, value_formatter: OptionValueFormatter) -> Iterator[str]: + ... + + required_help = "This option requires BuildKit to be enabled via the Docker subsystem options." + + class DockerImageBuildImageCacheToField( - DockerBuildOptionFieldMultiValueDictMixin, DictStringToStringField + DockerBuildOptionFieldMultiValueDictMixin, DictStringToStringField, DockerBuildKitOptionField ): alias = "cache_to" help = help_text( f""" Export image build cache to an external cache destination. + {DockerBuildKitOptionField.required_help} + Example: docker_image( @@ -340,13 +353,15 @@ class DockerImageBuildImageCacheToField( class DockerImageBuildImageCacheFromField( - DockerBuildOptionFieldMultiValueDictMixin, DictStringToStringField + DockerBuildOptionFieldMultiValueDictMixin, DictStringToStringField, DockerBuildKitOptionField ): alias = "cache_from" help = help_text( f""" Use an external cache source when building the image. + {DockerBuildKitOptionField.required_help} + Example: docker_image( @@ -367,6 +382,30 @@ class DockerImageBuildImageCacheFromField( docker_build_option = "--cache-from" +class DockerImageBuildImageOutputField( + DockerBuildOptionFieldMultiValueDictMixin, DictStringToStringField, DockerBuildKitOptionField +): + alias = "output" + default = FrozenDict({"type": "docker"}) + help = help_text( + f""" + Sets the export action for the build result. + + {DockerBuildKitOptionField.required_help} + + When using `pants publish` to publish Docker images to a registry, the output type + must be 'docker', as `publish` expects that the built images exist in the local + image store. + + Currently, multi-platform images cannot be exported with the 'docker' export type, + although experimental support is available with the [containerd image store](https://docs.docker.com/desktop/containerd/) + + {_interpolation_help.format(kind="Values")} + """ + ) + docker_build_option = "--output" + + class DockerImageBuildSecretsOptionField( AsyncFieldMixin, DockerBuildOptionFieldMixin, DictStringToStringField ): @@ -441,7 +480,7 @@ class DockerBuildOptionFieldValueMixin(Field): docker_build_option: ClassVar[str] @final - def options(self) -> Iterator[str]: + def options(self, *args, **kwargs) -> Iterator[str]: if self.value is not None: yield f"{self.docker_build_option}={self.value}" @@ -453,7 +492,7 @@ class DockerBuildOptionFieldMultiValueMixin(StringSequenceField): docker_build_option: ClassVar[str] @final - def options(self) -> Iterator[str]: + def options(self, *args, **kwargs) -> Iterator[str]: if self.value: yield f"{self.docker_build_option}={','.join(list(self.value))}" @@ -479,7 +518,7 @@ class DockerBuildOptionFlagFieldMixin(BoolField, ABC): docker_build_option: ClassVar[str] @final - def options(self) -> Iterator[str]: + def options(self, *args, **kwargs) -> Iterator[str]: if self.value: yield f"{self.docker_build_option}" @@ -547,6 +586,7 @@ class DockerImageTarget(Target): DockerImageBuildPlatformOptionField, DockerImageBuildImageCacheToField, DockerImageBuildImageCacheFromField, + DockerImageBuildImageOutputField, OutputPathField, RestartableField, ) diff --git a/src/python/pants/backend/docker/util_rules/docker_binary.py b/src/python/pants/backend/docker/util_rules/docker_binary.py index 435bf36b32b..665243ea51b 100644 --- a/src/python/pants/backend/docker/util_rules/docker_binary.py +++ b/src/python/pants/backend/docker/util_rules/docker_binary.py @@ -62,9 +62,15 @@ def build_image( build_args: DockerBuildArgs, context_root: str, env: Mapping[str, str], + use_buildx: bool, extra_args: tuple[str, ...] = (), ) -> Process: - args = [self.path, "build", *extra_args] + if use_buildx: + build_commands = ["buildx", "build"] + else: + build_commands = ["build"] + + args = [self.path, *build_commands, *extra_args] for tag in tags: args.extend(["--tag", tag]) diff --git a/src/python/pants/backend/docker/util_rules/docker_binary_test.py b/src/python/pants/backend/docker/util_rules/docker_binary_test.py index 370c031cf36..3c1bbe489ee 100644 --- a/src/python/pants/backend/docker/util_rules/docker_binary_test.py +++ b/src/python/pants/backend/docker/util_rules/docker_binary_test.py @@ -36,6 +36,7 @@ def test_docker_binary_build_image(docker_path: str, docker: DockerBinary) -> No build_args=DockerBuildArgs.from_strings("arg1=2"), context_root="build/context", env=env, + use_buildx=False, extra_args=("--pull", "--squash"), )