Skip to content

Commit d324e64

Browse files
committed
fix: wrap docker buildx ls/create failures as ImageBuildError
`docker buildx create --name flytex ...` (and `docker buildx ls`) run with `check=True`, so a CalledProcessError bubbled out of `_ensure_buildx_builder` raw and was reported to Sentry as an SDK crash (FLYTE-SDK-4R) even though the failure is a local Docker environment problem (daemon down, driver missing, or a leftover/locked builder), not an SDK bug. Wrap both subprocess calls in `ImageBuildError` (a `RuntimeUserError`, filtered from Sentry) with an actionable message that includes the captured stderr and points at `docker buildx rm flytex` / the remote image builder. If the create fails because the builder already exists (concurrent build), reuse it instead of failing. fixes FLYTE-SDK-4R Signed-off-by: Haytham Abuelfutuh <haytham@afutuh.com>
1 parent 6cd1b09 commit d324e64

2 files changed

Lines changed: 115 additions & 18 deletions

File tree

src/flyte/_internal/imagebuild/docker_builder.py

Lines changed: 43 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -688,9 +688,16 @@ async def _ensure_buildx_builder():
688688
raise ImageBuildError("Docker buildx is not available. Make sure BuildKit is installed and enabled.")
689689

690690
# List builders
691-
result = await run_sync_with_loop(
692-
subprocess.run, ["docker", "buildx", "ls"], capture_output=True, text=True, check=True
693-
)
691+
try:
692+
result = await run_sync_with_loop(
693+
subprocess.run, ["docker", "buildx", "ls"], capture_output=True, text=True, check=True
694+
)
695+
except subprocess.CalledProcessError as e:
696+
raise ImageBuildError(
697+
f"Failed to list docker buildx builders: {(e.stderr or '').strip() or e}. "
698+
"Ensure the Docker daemon is running, or use the remote image builder by setting "
699+
"`image_builder='remote'` on your `flyte.Image`."
700+
) from e
694701
builders = result.stdout
695702

696703
# Check if there's any usable builder with the correct driver options
@@ -716,21 +723,39 @@ async def _ensure_buildx_builder():
716723
else:
717724
logger.info("No buildx builder found, creating one...")
718725

719-
await run_sync_with_loop(
720-
subprocess.run,
721-
[
722-
"docker",
723-
"buildx",
724-
"create",
725-
"--name",
726-
DockerImageBuilder._builder_name,
727-
"--platform",
728-
"linux/amd64,linux/arm64",
729-
"--driver-opt",
730-
"network=host",
731-
],
732-
check=True,
733-
)
726+
# Create the builder. Wrap subprocess failures in ImageBuildError so a broken or
727+
# locked Docker environment surfaces as an actionable user error instead of leaking
728+
# as a raw CalledProcessError crash report (FLYTE-SDK-4R).
729+
try:
730+
await run_sync_with_loop(
731+
subprocess.run,
732+
[
733+
"docker",
734+
"buildx",
735+
"create",
736+
"--name",
737+
DockerImageBuilder._builder_name,
738+
"--platform",
739+
"linux/amd64,linux/arm64",
740+
"--driver-opt",
741+
"network=host",
742+
],
743+
capture_output=True,
744+
text=True,
745+
check=True,
746+
)
747+
except subprocess.CalledProcessError as e:
748+
stderr = (e.stderr or "").strip()
749+
# A concurrent build may have created the builder between our `ls` check and now;
750+
# if it already exists we can just reuse it instead of failing.
751+
if "already exists" in stderr.lower():
752+
logger.info(f"Buildx builder {DockerImageBuilder._builder_name!r} already exists, reusing it.")
753+
return
754+
raise ImageBuildError(
755+
f"Failed to create docker buildx builder {DockerImageBuilder._builder_name!r}: {stderr or e}. "
756+
f"Try removing it with `docker buildx rm {DockerImageBuilder._builder_name}`, or use the remote "
757+
"image builder by setting `image_builder='remote'` on your `flyte.Image`."
758+
) from e
734759

735760
async def _build_image(self, image: Image, *, push: bool = True, dry_run: bool = False, wait: bool = True) -> str:
736761
"""

tests/flyte/imagebuild/test_docker_builder.py

Lines changed: 72 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -883,6 +883,78 @@ def mock_run(cmd, **kwargs):
883883
assert "network=host" in create_cmds[0]
884884

885885

886+
@pytest.mark.asyncio
887+
async def test_ensure_buildx_builder_wraps_create_failure_as_image_build_error():
888+
"""When `docker buildx create` fails, the raw CalledProcessError should not bubble out.
889+
890+
Previously this leaked into Sentry as a RuntimeSystem/CalledProcessError crash report
891+
(FLYTE-SDK-4R). It should be wrapped in an actionable ImageBuildError, which is filtered.
892+
"""
893+
from flyte.errors import ImageBuildError
894+
895+
def mock_run(cmd, **kwargs):
896+
result = subprocess.CompletedProcess(cmd, 0)
897+
if cmd == ["docker", "buildx", "ls"]:
898+
result.stdout = "default"
899+
result.stderr = ""
900+
return result
901+
if "create" in cmd:
902+
raise subprocess.CalledProcessError(returncode=1, cmd=cmd, stderr="ERROR: failed to find driver")
903+
return result
904+
905+
with patch(
906+
"flyte._internal.imagebuild.docker_builder.run_sync_with_loop", side_effect=lambda fn, *a, **kw: fn(*a, **kw)
907+
):
908+
with patch("subprocess.run", side_effect=mock_run):
909+
with pytest.raises(ImageBuildError, match="Failed to create docker buildx builder"):
910+
await DockerImageBuilder._ensure_buildx_builder()
911+
912+
913+
@pytest.mark.asyncio
914+
async def test_ensure_buildx_builder_reuses_existing_on_already_exists():
915+
"""If `docker buildx create` fails because the builder already exists (e.g. a concurrent
916+
build created it), it should be reused rather than raising."""
917+
918+
def mock_run(cmd, **kwargs):
919+
result = subprocess.CompletedProcess(cmd, 0)
920+
if cmd == ["docker", "buildx", "ls"]:
921+
result.stdout = "default"
922+
result.stderr = ""
923+
return result
924+
if "create" in cmd:
925+
raise subprocess.CalledProcessError(
926+
returncode=1,
927+
cmd=cmd,
928+
stderr=f'ERROR: existing instance for "{DockerImageBuilder._builder_name}" already exists',
929+
)
930+
return result
931+
932+
with patch(
933+
"flyte._internal.imagebuild.docker_builder.run_sync_with_loop", side_effect=lambda fn, *a, **kw: fn(*a, **kw)
934+
):
935+
with patch("subprocess.run", side_effect=mock_run):
936+
# Should not raise.
937+
await DockerImageBuilder._ensure_buildx_builder()
938+
939+
940+
@pytest.mark.asyncio
941+
async def test_ensure_buildx_builder_wraps_ls_failure_as_image_build_error():
942+
"""When `docker buildx ls` fails, surface an actionable ImageBuildError instead of a crash."""
943+
from flyte.errors import ImageBuildError
944+
945+
def mock_run(cmd, **kwargs):
946+
if cmd == ["docker", "buildx", "ls"]:
947+
raise subprocess.CalledProcessError(returncode=1, cmd=cmd, stderr="Cannot connect to the Docker daemon")
948+
return subprocess.CompletedProcess(cmd, 0)
949+
950+
with patch(
951+
"flyte._internal.imagebuild.docker_builder.run_sync_with_loop", side_effect=lambda fn, *a, **kw: fn(*a, **kw)
952+
):
953+
with patch("subprocess.run", side_effect=mock_run):
954+
with pytest.raises(ImageBuildError, match="Failed to list docker buildx builders"):
955+
await DockerImageBuilder._ensure_buildx_builder()
956+
957+
886958
@pytest.mark.asyncio
887959
async def test_build_image_uses_custom_builder_from_env(monkeypatch):
888960
"""When FLYTE_DOCKER_BUILDKIT_BUILDER_NAME is set, _build_image should use it and skip _ensure_buildx_builder."""

0 commit comments

Comments
 (0)