Skip to content

fix(cli): diagnose Docker Desktop host networking - #3970

Draft
elezar wants to merge 2 commits into
mainfrom
codex/fix-doctor-host-networking/elezar
Draft

fix(cli): diagnose Docker Desktop host networking#3970
elezar wants to merge 2 commits into
mainfrom
codex/fix-doctor-host-networking/elezar

Conversation

@elezar

@elezar elezar commented Sep 30, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Summary

openshell doctor check reports success when Docker Desktop is running but host networking cannot reach the macOS gateway. Check container-to-host loopback connectivity and return an actionable error before sandbox provisioning.

Related Issue

No issue required: localized diagnostic bug fix for an unmet documented Docker Desktop prerequisite. Runtime routing, listener configuration, and gateway defaults remain unchanged.

Changes

  • Introduce a reusable prerequisite-provider interface and doctor runner; keep Docker diagnostics in a separate provider and share bounded command execution across providers. Additional runtime providers can be registered without changing the runner.
  • Detect local Docker Desktop and probe a temporary loopback listener from a host-networked container without requiring a running gateway.
  • Use the same shared default workload image as the sandbox drivers. Bash TCP redirection with a three-second timeout avoids installing additional tools in that image.
  • Return an actionable error identifying the host-networking and Enhanced Container Isolation requirements when connectivity fails.
  • Distinguish probe setup failures from failed connectivity, bound command execution, and clean up unsuccessful probes.
  • Skip the local probe for remote Docker endpoints and preserve native Docker Engine checks.
  • Add the prerequisites to the installation runtime requirements table and update the operator troubleshooting skill.

Testing

  • mise run pre-commit passes: attempted; blocked by the existing local sccache permission error during Cargo metadata/lockfile checks.
  • Unit tests added/updated: eight doctor tests cover reusable providers, success, failed connectivity including connection timeout, image-pull failure, native Engine, remote Desktop, and command timeout cleanup. Full CLI library suite: 334 tests passed with RUSTC_WRAPPER=.
  • CLI build and Clippy with warnings denied passed with RUSTC_WRAPPER=. Formatting, Markdown lint, and license checks passed.
  • Manual macOS ARM64 / Docker Desktop Engine 29.8.0 verification: the updated CLI reports Host networking .... FAILED and returns exit code 1 using the default sandbox image.
  • The default-image probe rejects 127.0.0.1 with connection refused and connects successfully through host.docker.internal to the same temporary listener. Test containers are removed.
  • Repository-wide mise run test and mise run ci were attempted; local checks remain blocked by the same sccache error.
  • E2E test changes: not applicable; gateway, sandbox, and policy behavior are unchanged.

Checklist

  • Follows Conventional Commits
  • Commits are signed off (DCO)
  • Related operator skill and published docs updated
  • Architecture docs: not applicable; this change is limited to CLI diagnostics.

@elezar
fix(cli): diagnose Docker Desktop host networking
f1f1dc3 
Signed-off-by: Evan Lezar <elezar@nvidia.com>
@copy-pr-bot

copy-pr-bot Bot commented Sep 30, 2026

Copy link
Copy Markdown

Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually.

Contributors can view more details about this message here.

@github-actions

github-actions Bot commented Sep 30, 2026

Copy link
Copy Markdown

🌿 Preview your docs: https://nvidia-preview-pr-3970.docs.buildwithfern.com/openshell

drew
drew reviewed Sep 30, 2026
View reviewed changes
Comment thread docs/how-it-works/gateways/overview.mdx Outdated
the check. Image-pull and container-start failures mean the check could not
verify connectivity. Remote Docker endpoints skip this test; run it on the
gateway host.

@drew drew Sep 30, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove, this is superfluous

drew
drew reviewed Sep 30, 2026
View reviewed changes
Comment thread crates/openshell-cli/src/commands/doctor.rs Outdated
use std::time::Duration;
use tokio::process::Command;

const PROBE_IMAGE: &str = "alpine:3.23";

@drew drew Sep 30, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how about we sync this with the default image the sandbox uses

drew
drew reviewed Sep 30, 2026
View reviewed changes
Comment thread docs/about/installation.mdx Outdated

The gateway reads `~/.config/openshell/gateway.toml` if it exists, otherwise the Homebrew config at `$(brew --prefix)/var/openshell/gateway.toml`.

For Docker Desktop, enable host networking and disable Enhanced Container Isolation. Run `openshell doctor check` to verify container-to-host loopback connectivity before creating a sandbox. The check may pull a small Alpine probe image.

@drew drew Sep 30, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just move this to requirements on line 72 above

krishicks
krishicks reviewed Sep 30, 2026
View reviewed changes
Comment thread crates/openshell-cli/src/commands/doctor.rs Outdated
// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved.
// SPDX-License-Identifier: Apache-2.0

//! Docker prerequisites, including Docker Desktop's container-to-host route.

@krishicks krishicks Sep 30, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This whole file is Docker-oriented, and does not seem extendable to add e.g. Podman, but the file is called doctor.rs. I feel like this is begging for a different design. I get that doctor_check() from before only did Docker, but we shouldn't maintain the status quo.

@drew drew Oct 1, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yea, thats a good call. this feels a little too specific for the driver/problem.

@elezar
refactor(cli): separate doctor prerequisite providers
13fd8ad 
Signed-off-by: Evan Lezar <elezar@nvidia.com>

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@drew drew drew left review comments
@krishicks krishicks krishicks left review comments
@mrunalp mrunalp Awaiting requested review from mrunalp mrunalp will be requested when the pull request is marked ready for review mrunalp is a code owner
@sjenning sjenning Awaiting requested review from sjenning sjenning will be requested when the pull request is marked ready for review sjenning is a code owner
@derekwaynecarr derekwaynecarr Awaiting requested review from derekwaynecarr derekwaynecarr will be requested when the pull request is marked ready for review derekwaynecarr is a code owner

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@elezar @drew @krishicks