Skip to content

Comments

Add additional networks#9566

Merged
enyst merged 10 commits intoOpenHands:mainfrom
Joeoc2001:joe/sandbox-additional-networks
Aug 20, 2025
Merged

Add additional networks#9566
enyst merged 10 commits intoOpenHands:mainfrom
Joeoc2001:joe/sandbox-additional-networks

Conversation

@Joeoc2001
Copy link
Contributor

  • This change is worth documenting at https://docs.all-hands.dev/
  • Include this change in the Release Notes. If checked, you must provide an end-user friendly description for your change below

End-user friendly description of the problem this fixes or functionality this introduces.

Allow additional docker networks to be specified to be attached to all sandbox instances, allowing sidecar containers (like build caches) to be securely shared between all runners.


Summarize what the PR does, explaining any non-trivial design decisions.

Adds additional networks to containers after they have been created. We delay network adding to ensure communication with the main app is still established correctly.


Link of any specific issues this addresses:

#6637

@Joeoc2001 Joeoc2001 requested a review from mamoodi as a code owner July 5, 2025 10:41
@Joeoc2001 Joeoc2001 marked this pull request as draft July 5, 2025 12:32
@Joeoc2001 Joeoc2001 marked this pull request as ready for review July 6, 2025 22:21
@enyst enyst added app server Related to OpenHands App Server docker Related to docker and removed app server Related to OpenHands App Server labels Jul 13, 2025
Copy link
Collaborator

@rbren rbren left a comment

Choose a reason for hiding this comment

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

This seems reasonable!

@enyst enyst enabled auto-merge (squash) July 31, 2025 16:44
@enyst
Copy link
Collaborator

enyst commented Jul 31, 2025

@Joeoc2001 There’s some type failure in CI. Could you perhaps take a look at it?

@enyst enyst self-requested a review August 13, 2025 14:42
Joeoc2001 and others added 2 commits August 20, 2025 15:08
…ntainer\n\nThe docker SDK types expect a str|Container for Network.connect, but our\ncontainer attribute is Optional[Container]. Add a simple None check and\nlog a warning if somehow unavailable.\n\nRan pre-commit (ruff, mypy) locally and it now passes.\n\nCo-authored-by: openhands <openhands@all-hands.dev>
Copy link
Collaborator

enyst commented Aug 20, 2025

I am AI Agent OpenHands-GPT-5. I investigated the CI failure on this PR and pushed a fix.

Summary of the issue:

  • The failing job was 'Lint python'.
  • mypy reported: openhands/runtime/impl/docker/docker_runtime.py:219: error: Argument 1 to "connect" of "Network" has incompatible type "Container | None"; expected "str | Container" [arg-type].
  • Root cause: self.container is typed as Container | None, but network.connect(...) expects a str | Container. Passing a possibly-None value triggered the type error.

What I changed:

  • In DockerRuntime.connect(), after the runtime is initialized and before connecting to any additional networks, I added a None-guard around the network.connect(self.container) call. If self.container is unexpectedly None, we log a warning and skip that network attach for safety.
  • This is the minimal change needed to satisfy mypy while keeping the intended behavior intact.

Commit with the fix:

  • 94554d7 (fix(docker): satisfy mypy by guarding network.connect against None container)

Verification:

  • Ran the repo's pre-commit (ruff + mypy) locally; it now passes. The push updated this PR's branch so CI should turn green for the Python lint step.

Notes:

  • The new additional_networks field and the network-attach logic remain functionally the same, now with a small safety/logging improvement.
  • No behavior change for valid cases (container present). Errors during attach continue to be logged, as before.

If you'd like me to also switch the additional_networks field to use default_factory=list (style preference in pydantic v2 to avoid mutable defaults), I can do that in a follow-up.

@enyst enyst merged commit bc8b995 into OpenHands:main Aug 20, 2025
17 checks passed
chuckbutkus pushed a commit that referenced this pull request Aug 26, 2025
Co-authored-by: Engel Nyst <enyst@users.noreply.github.com>
enyst added a commit to enyst/playground that referenced this pull request Sep 4, 2025
Co-authored-by: Engel Nyst <enyst@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

docker Related to docker

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants