Feat/docker sandbox - #394
Conversation
Signed-off-by: flb_ <floatlibai@gmail.com>
Signed-off-by: flb_ <floatlibai@gmail.com>
There was a problem hiding this comment.
Code Review
This pull request introduces a remote Docker sandbox (DockerSandbox) as a drop-in replacement for E2BSandbox, updates the agent sandbox logic to support it, and adds an end-to-end training shell script. The review feedback highlights several critical issues in the new sandbox implementation, including potential SSH connection leaks during startup and teardown, a bug in parsing the DOCKER_SANDBOX boolean environment variable, and API mismatches with the original sandbox interface (such as default argument values and exception handling). Additionally, the reviewer recommended making hardcoded IP addresses in the launch script configurable.
| async def __aenter__(self) -> "DockerSandbox": | ||
| loop = asyncio.get_event_loop() | ||
| docker_tarball_dir = os.environ.get("DOCKER_TARBALL_DIR") | ||
| def _start(): | ||
| self._client = _make_client() # 每个沙箱独立连接 | ||
| vime_head_host = os.environ.get("VIME_HEAD_HOST", "") | ||
| no_proxy=os.environ.get("no_proxy", f"127.0.0.1,localhost,{vime_head_host}") | ||
| NO_PROXY=os.environ.get("NO_PROXY", f"127.0.0.1,localhost,{vime_head_host}") | ||
| http_proxy=os.environ.get("http_proxy", "") | ||
| https_proxy=os.environ.get("https_proxy", "") | ||
| return self._client.containers.run( | ||
| self.image, | ||
| command="sleep infinity", | ||
| detach=True, | ||
| mem_limit=_MEM_LIMIT, | ||
| nano_cpus=int(_CPUS * 1e9), | ||
| network_mode="bridge", | ||
| cap_add=["SYS_PTRACE"], | ||
| remove=False, | ||
| volumes={ | ||
| docker_tarball_dir: { | ||
| "bind": docker_tarball_dir, | ||
| "mode": "rw" | ||
| } | ||
| }, | ||
| environment={ | ||
| "http_proxy": http_proxy, | ||
| "https_proxy": https_proxy, | ||
| "no_proxy": no_proxy, | ||
| "NO_PROXY": NO_PROXY, | ||
| }, | ||
| ) | ||
|
|
||
| self._container = await loop.run_in_executor(None, _start) | ||
| await self.exec("mkdir -p /workspace", user="root", check=False) | ||
| return self |
There was a problem hiding this comment.
If containers.run raises an exception during sandbox startup, the newly created DockerClient (which establishes an SSH connection) is never closed, leading to a resource/SSH connection leak. Additionally, if DOCKER_TARBALL_DIR is not set in the environment, passing None as a key in the volumes dictionary will cause a TypeError or AttributeError in docker-py.
We should wrap the container startup in a try...except block to ensure the client is closed on failure, and safely handle the volume mounting only when docker_tarball_dir is set.
async def __aenter__(self) -> "DockerSandbox":
loop = asyncio.get_event_loop()
docker_tarball_dir = os.environ.get("DOCKER_TARBALL_DIR")
def _start():
self._client = _make_client() # 每个沙箱独立连接
try:
vime_head_host = os.environ.get("VIME_HEAD_HOST", "")
no_proxy = os.environ.get("no_proxy", f"127.0.0.1,localhost,{vime_head_host}")
NO_PROXY = os.environ.get("NO_PROXY", f"127.0.0.1,localhost,{vime_head_host}")
http_proxy = os.environ.get("http_proxy", "")
https_proxy = os.environ.get("https_proxy", "")
volumes = {}
if docker_tarball_dir:
volumes[docker_tarball_dir] = {
"bind": docker_tarball_dir,
"mode": "rw"
}
return self._client.containers.run(
self.image,
command="sleep infinity",
detach=True,
mem_limit=_MEM_LIMIT,
nano_cpus=int(_CPUS * 1e9),
network_mode="bridge",
cap_add=["SYS_PTRACE"],
remove=False,
volumes=volumes,
environment={
"http_proxy": http_proxy,
"https_proxy": https_proxy,
"no_proxy": no_proxy,
"NO_PROXY": NO_PROXY,
},
)
except Exception:
self._client.close()
self._client = None
raise
self._container = await loop.run_in_executor(None, _start)
await self.exec("mkdir -p /workspace", user="root", check=False)
return self| async def __aexit__(self, *args): | ||
| if self._container is None: | ||
| return | ||
| loop = asyncio.get_event_loop() | ||
| container = self._container | ||
| client = self._client | ||
|
|
||
| def _stop(): | ||
| try: | ||
| container.remove(force=True) | ||
| except Exception: | ||
| pass | ||
| try: | ||
| client.close() | ||
| except Exception: | ||
| pass | ||
|
|
||
| await loop.run_in_executor(None, _stop) | ||
| self._container = None | ||
| self._client = None |
There was a problem hiding this comment.
If self._container is None (e.g., if container creation failed or was partially initialized), __aexit__ currently returns immediately without closing self._client. This leaks the SSH connection. We should ensure self._client is always closed if it exists, regardless of whether the container was successfully created.
| async def __aexit__(self, *args): | |
| if self._container is None: | |
| return | |
| loop = asyncio.get_event_loop() | |
| container = self._container | |
| client = self._client | |
| def _stop(): | |
| try: | |
| container.remove(force=True) | |
| except Exception: | |
| pass | |
| try: | |
| client.close() | |
| except Exception: | |
| pass | |
| await loop.run_in_executor(None, _stop) | |
| self._container = None | |
| self._client = None | |
| async def __aexit__(self, *args): | |
| loop = asyncio.get_event_loop() | |
| container = self._container | |
| client = self._client | |
| def _stop(): | |
| if container is not None: | |
| try: | |
| container.remove(force=True) | |
| except Exception: | |
| pass | |
| if client is not None: | |
| try: | |
| client.close() | |
| except Exception: | |
| pass | |
| await loop.run_in_executor(None, _stop) | |
| self._container = None | |
| self._client = None |
| async def exec( | ||
| self, | ||
| cmd: str, | ||
| *, | ||
| user: str = "root", | ||
| check: bool = True, | ||
| timeout: int = _DEFAULT_TIMEOUT, | ||
| env: dict[str, str] | None = None, | ||
| ) -> tuple[int, str, str]: |
There was a problem hiding this comment.
In the Sandbox protocol and the E2BSandbox implementation, check defaults to False. However, in DockerSandbox.exec, it currently defaults to True. This discrepancy breaks the "drop-in replacement" contract and can cause unexpected crashes when switching between sandbox backends. Let's change the default to False to match the protocol.
| async def exec( | |
| self, | |
| cmd: str, | |
| *, | |
| user: str = "root", | |
| check: bool = True, | |
| timeout: int = _DEFAULT_TIMEOUT, | |
| env: dict[str, str] | None = None, | |
| ) -> tuple[int, str, str]: | |
| async def exec( | |
| self, | |
| cmd: str, | |
| *, | |
| user: str = "root", | |
| check: bool = False, | |
| timeout: int = _DEFAULT_TIMEOUT, | |
| env: dict[str, str] | None = None, | |
| ) -> tuple[int, str, str]: |
| async def read_file(self, sandbox_path: str, user: str = "root") -> str: | ||
| loop = asyncio.get_event_loop() | ||
|
|
||
| def _get() -> str: | ||
| bits, _ = self._container.get_archive(sandbox_path) | ||
| buf = io.BytesIO(b"".join(bits)) | ||
| with tarfile.open(fileobj=buf) as tf: | ||
| member = tf.getmembers()[0] | ||
| f = tf.extractfile(member) | ||
| return f.read().decode("utf-8", errors="replace") if f else "" | ||
|
|
||
| return await loop.run_in_executor(None, _get) |
There was a problem hiding this comment.
In E2BSandbox.read_file, any exceptions (such as file not found) are caught and an empty string "" is returned. In DockerSandbox.read_file, if the file does not exist, get_archive will raise a docker.errors.NotFound exception, which propagates and crashes the caller. To ensure it acts as a true drop-in replacement, we should catch exceptions and return "".
| async def read_file(self, sandbox_path: str, user: str = "root") -> str: | |
| loop = asyncio.get_event_loop() | |
| def _get() -> str: | |
| bits, _ = self._container.get_archive(sandbox_path) | |
| buf = io.BytesIO(b"".join(bits)) | |
| with tarfile.open(fileobj=buf) as tf: | |
| member = tf.getmembers()[0] | |
| f = tf.extractfile(member) | |
| return f.read().decode("utf-8", errors="replace") if f else "" | |
| return await loop.run_in_executor(None, _get) | |
| async def read_file(self, sandbox_path: str, user: str = "root") -> str: | |
| loop = asyncio.get_event_loop() | |
| def _get() -> str: | |
| try: | |
| bits, _ = self._container.get_archive(sandbox_path) | |
| buf = io.BytesIO(b"".join(bits)) | |
| with tarfile.open(fileobj=buf) as tf: | |
| members = tf.getmembers() | |
| if not members: | |
| return "" | |
| f = tf.extractfile(members[0]) | |
| return f.read().decode("utf-8", errors="replace") if f else "" | |
| except Exception: | |
| return "" | |
| return await loop.run_in_executor(None, _get) |
| if os.environ.get("DOCKER_SANDBOX", True): | ||
| from .docker_sandbox import DockerSandbox as E2BSandbox, Sandbox | ||
| else: | ||
| from vime.agent.sandbox import E2BSandbox, Sandbox |
There was a problem hiding this comment.
Using os.environ.get("DOCKER_SANDBOX", True) will return a truthy string like "0" or "False" if the environment variable is set to disable the docker sandbox. In Python, any non-empty string is truthy, meaning the else block will never be reached. We should parse the environment variable as a boolean properly.
| if os.environ.get("DOCKER_SANDBOX", True): | |
| from .docker_sandbox import DockerSandbox as E2BSandbox, Sandbox | |
| else: | |
| from vime.agent.sandbox import E2BSandbox, Sandbox | |
| _use_docker = os.environ.get("DOCKER_SANDBOX", "1").lower() not in ("0", "false", "no") | |
| if _use_docker: | |
| from .docker_sandbox import DockerSandbox as E2BSandbox, Sandbox | |
| else: | |
| from vime.agent.sandbox import E2BSandbox, Sandbox |
| export MASTER_ADDR="192.168.13.190" | ||
| export VIME_HEAD_HOST="${MASTER_ADDR}" |
There was a problem hiding this comment.
The IP address 192.168.13.190 is hardcoded. It is better to allow overriding it via environment variables with this IP as a default fallback, making the script more portable across different cluster setups.
| export MASTER_ADDR="192.168.13.190" | |
| export VIME_HEAD_HOST="${MASTER_ADDR}" | |
| export MASTER_ADDR="${MASTER_ADDR:-192.168.13.190}" | |
| export VIME_HEAD_HOST="${MASTER_ADDR}" |
| export SWE_SANDBOX_METADATA_FILE="${SANDBOX_METADATA_FILE}" | ||
|
|
||
| export DOCKER_SANDBOX=1 | ||
| export DOCKER_SANDBOX_HOST="root@192.168.13.188" |
There was a problem hiding this comment.
Documentation build overview
40 files changed ·
|
No description provided.