Skip to content

Commit

Permalink
Add docker image output field to support publish to repository when u…
Browse files Browse the repository at this point in the history
…sing BuildKit (Cherry-pick of #20154) (#20185)

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](docker/buildx#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: riisi <rhysmadigan@gmail.com>
Co-authored-by: Rhys Madigan <rhys.madigan@accenture.com>
  • Loading branch information
3 people authored Nov 15, 2023
1 parent 55dcd4a commit 8bdf5a8
Show file tree
Hide file tree
Showing 7 changed files with 163 additions and 37 deletions.
12 changes: 3 additions & 9 deletions docs/markdown/Docker/docker.md
Original file line number Diff line number Diff line change
Expand Up @@ -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:

```
Expand All @@ -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"
]
```
Expand Down
47 changes: 26 additions & 21 deletions src/python/pants/backend/docker/goals/package_image.py
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -440,13 +443,15 @@ 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,
field_set=field_set,
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,
)
),
Expand Down
72 changes: 72 additions & 0 deletions src/python/pants/backend/docker/goals/package_image_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@

from pants.backend.docker.goals.package_image import (
DockerBuildTargetStageError,
DockerImageOptionValueError,
DockerImageTagValueError,
DockerInfoV1,
DockerPackageFieldSet,
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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",
Expand All @@ -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),
)


Expand All @@ -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",
Expand All @@ -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(
{
Expand Down
8 changes: 8 additions & 0 deletions src/python/pants/backend/docker/subsystems/docker_options.py
Original file line number Diff line number Diff line change
Expand Up @@ -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"""
Expand Down
52 changes: 46 additions & 6 deletions src/python/pants/backend/docker/target_types.py
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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(
Expand All @@ -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(
Expand All @@ -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
):
Expand Down Expand Up @@ -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}"

Expand All @@ -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))}"

Expand All @@ -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}"

Expand Down Expand Up @@ -547,6 +586,7 @@ class DockerImageTarget(Target):
DockerImageBuildPlatformOptionField,
DockerImageBuildImageCacheToField,
DockerImageBuildImageCacheFromField,
DockerImageBuildImageOutputField,
OutputPathField,
RestartableField,
)
Expand Down
Loading

0 comments on commit 8bdf5a8

Please sign in to comment.