Skip to content

fix(lib): discover Docker image tags for local profiles - #529

Open
cswaney wants to merge 2 commits into
mainfrom
fix/local-docker-image-discovery-520
Open

fix(lib): discover Docker image tags for local profiles#529
cswaney wants to merge 2 commits into
mainfrom
fix/local-docker-image-discovery-520

Conversation

@cswaney

@cswaney cswaney commented Aug 28, 2026

Copy link
Copy Markdown
Member

Summary

  • image_probe only listed SIF files, so local + Docker profiles reported zero staged tags and the launcher's Advanced Options rendered orphan help text with no version selector.
  • Add a Docker discovery path that queries the local daemon (docker image ls --format '{{.Repository}}:{{.Tag}}') and matches full refs against each configured ImageSpec.repo.
  • list_staged_tags now takes the container provider and dispatches: Docker uses the new path, everything else (Slurm profiles) keeps the SIF layout.

Closes #520

Test plan

  • uv run just lint
  • uv run just test (977 passed)
  • Manual smoke test on local + Docker profile: launcher shows a populated Version dropdown once an image is pulled.
  • Manual smoke test on Slurm profile: version list still comes from SIF filenames.

Follow-up (separate PR): alert in the launcher when no images are staged for the configured service — applies to both local and Slurm.

`image_probe` only listed SIF files, so local + Docker profiles reported
zero staged tags and the launcher's version selector rendered empty. Add
a Docker discovery path that queries the local daemon via
`docker image ls` and match refs against each configured `ImageSpec`.
`list_staged_tags` now accepts the container provider and dispatches to
the appropriate backend; Slurm profiles keep the SIF layout.

Closes #520
@claude

claude Bot commented Aug 28, 2026

Copy link
Copy Markdown

Review

Nice fix overall - the Docker discovery path, the <none> filtering, and the exact-repo matching in extract_docker_tag are all solid, and the test coverage for the new Docker branch is thorough.

Bug: provider is a global setting, not derived from the profile being queried

list_containers (lib/src/blackfish/server/asgi.py:1074-1078) passes blackfish_config.CONTAINER_PROVIDER straight through to list_staged_tags, regardless of which profile was resolved.

CONTAINER_PROVIDER is a single process-wide value, detected once at startup from whatever is installed on the server host (get_container_provider() in config.py, or BLACKFISH_CONTAINER_PROVIDER) - it has nothing to do with the target profile. _runner_for(profile), on the other hand, correctly picks SSHRunner vs LocalRunner based on the profile's type.

So on a server whose host has Docker installed (very common on a dev machine), querying a remote SlurmProfile will run docker image ls over SSH on the login node via _list_docker_refs. HPC clusters almost never run a Docker daemon, so this silently returns empty output (the trailing || true swallows the failure) and the launcher shows an empty Version dropdown for the Slurm profile - the exact bug this PR is fixing, just reintroduced for Slurm/Apptainer profiles.

This isn't hypothetical: the codebase already has the correct pattern for this exact decision in _resolve_image_and_provider (lib/src/blackfish/server/jobs/base.py:131-164):

if isinstance(profile, SlurmProfile):
    provider = ContainerProvider.Apptainer
else:
    provider = (
        getattr(app_config, "CONTAINER_PROVIDER", None)
        or _config.CONTAINER_PROVIDER
        or ContainerProvider.Apptainer
    )

list_containers should do the same instead of blindly forwarding the global config value, e.g.:

provider = (
    ContainerProvider.Apptainer
    if isinstance(resolved, SlurmProfile)
    else blackfish_config.CONTAINER_PROVIDER
)
staged = await list_staged_tags(resolved, blackfish_config.IMAGES, provider=provider)

(SlurmProfile is already imported in asgi.py.)

The updated module docstring in image_probe.py even claims "everything else (Slurm profiles) keeps the SIF layout" - that's only true if the caller passes the right provider, which today it doesn't unless the server host happens to lack Docker.

Test coverage gap

lib/tests/api/test_containers.py fully mocks out list_staged_tags, so it never exercises what provider value actually gets passed for a Slurm vs. local profile - this is why the bug above wasn't caught. Worth adding a test that patches list_staged_tags with a spy and asserts provider=Apptainer when profile resolves to a SlurmProfile, even if blackfish_config.CONTAINER_PROVIDER is Docker.

Minor

  • image_probe.py: _list_docker_refs and list_staged_tags's Docker branch look correct and safe - the command string is static (no interpolation of user input), so no injection concern there.
  • Nit: the two staged: dict[str, list[str]] = {} blocks plus their per-service loops in list_staged_tags (Docker branch and SIF branch) are near-identical aside from the tag-extraction call; not blocking, but could be collapsed by parameterizing on (source_list, extract_fn) if you want to touch it again later.

Nothing else stood out - lint/type coverage and the new unit tests otherwise look good.

CONTAINER_PROVIDER is a server-side setting detected at startup from the
host's installed tools, not a per-profile fact. Forwarding it blindly to
`list_staged_tags` meant a server with Docker installed (common on dev
boxes) would run `docker image ls` over SSH on a Slurm login node — which
has no Docker daemon — and silently return no tags. Force Apptainer for
`SlurmProfile`; only local profiles honor the configured provider.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

web: hide Advanced Options for local profiles until backend discovers Docker images

1 participant