From 10185e1b3f5efe08376b468cc32cb2d9aa25fd2e Mon Sep 17 00:00:00 2001 From: Tim O'Farrell Date: Tue, 21 Jan 2025 13:27:51 -0700 Subject: [PATCH 1/8] Restarting containers --- openhands/runtime/impl/docker/containers.py | 7 ++-- .../runtime/impl/docker/docker_runtime.py | 42 +++++++++++++++---- .../server/routes/manage_conversations.py | 1 + 3 files changed, 40 insertions(+), 10 deletions(-) diff --git a/openhands/runtime/impl/docker/containers.py b/openhands/runtime/impl/docker/containers.py index e162b029c265..bed21d722b7f 100644 --- a/openhands/runtime/impl/docker/containers.py +++ b/openhands/runtime/impl/docker/containers.py @@ -1,18 +1,19 @@ import docker -def remove_all_containers(prefix: str): +def stop_all_containers(prefix: str): docker_client = docker.from_env() - try: containers = docker_client.containers.list(all=True) for container in containers: try: if container.name.startswith(prefix): - container.remove(force=True) + container.stop() # .remove(force=True) except docker.errors.APIError: pass except docker.errors.NotFound: pass except docker.errors.NotFound: # yes, this can happen! pass + finally: + docker_client.close() diff --git a/openhands/runtime/impl/docker/docker_runtime.py b/openhands/runtime/impl/docker/docker_runtime.py index bf06e00e854f..f0cd252b8a34 100644 --- a/openhands/runtime/impl/docker/docker_runtime.py +++ b/openhands/runtime/impl/docker/docker_runtime.py @@ -5,6 +5,7 @@ import docker import requests import tenacity +from docker.models.containers import Container from openhands.core.config import AppConfig from openhands.core.exceptions import ( @@ -18,7 +19,7 @@ from openhands.runtime.impl.action_execution.action_execution_client import ( ActionExecutionClient, ) -from openhands.runtime.impl.docker.containers import remove_all_containers +from openhands.runtime.impl.docker.containers import stop_all_containers from openhands.runtime.plugins import PluginRequirement from openhands.runtime.utils import find_available_tcp_port from openhands.runtime.utils.command import get_action_execution_server_startup_command @@ -35,8 +36,8 @@ APP_PORT_RANGE_2 = (55000, 59999) -def remove_all_runtime_containers(): - remove_all_containers(CONTAINER_NAME_PREFIX) +def stop_all_runtime_containers(): + stop_all_containers(CONTAINER_NAME_PREFIX) _atexit_registered = False @@ -68,7 +69,7 @@ def __init__( global _atexit_registered if not _atexit_registered: _atexit_registered = True - atexit.register(remove_all_runtime_containers) + atexit.register(stop_all_runtime_containers) self.config = config self._runtime_initialized: bool = False @@ -85,7 +86,7 @@ def __init__( self.base_container_image = self.config.sandbox.base_container_image self.runtime_container_image = self.config.sandbox.runtime_container_image self.container_name = CONTAINER_NAME_PREFIX + sid - self.container = None + self.container: Container | None = None self.runtime_builder = DockerRuntimeBuilder(self.docker_client) @@ -188,6 +189,31 @@ def _init_container(self): self.log('debug', 'Preparing to start container...') self.send_status_message('STATUS$PREPARING_CONTAINER') + try: + container = self.docker_client.containers.get(self.container_name) + container.restart() + self.container = container + config = container.attrs['Config'] + for env_var in config['Env']: + if env_var.startswith('port='): + self._host_port = int(env_var.split('port=')[1]) + self._container_port = self._host_port + elif env_var.startswith('VSCODE_PORT='): + self._vscode_port = int(env_var.split('VSCODE_PORT=')[1]) + self._app_ports = [] + for exposed_port in config['ExposedPorts'].keys(): + exposed_port = int(exposed_port.split('/tcp')[0]) + if ( + exposed_port != self._host_port + and exposed_port != self._vscode_port + ): + self._app_ports.append(exposed_port) + return + except docker.errors.NotFound: + pass + except docker.errors.APIError: + pass + self._host_port = self._find_available_port(EXECUTION_SERVER_PORT_RANGE) self._container_port = self._host_port self._vscode_port = self._find_available_port(VSCODE_PORT_RANGE) @@ -287,7 +313,7 @@ def _init_container(self): 'warning', f'Container {self.container_name} already exists. Removing...', ) - remove_all_containers(self.container_name) + stop_all_containers(self.container_name) return self._init_container() else: @@ -308,6 +334,8 @@ def _init_container(self): def _attach_to_container(self): self.container = self.docker_client.containers.get(self.container_name) + if self.container.status == 'exited': + self.container.start() for port in self.container.attrs['NetworkSettings']['Ports']: # type: ignore port = int(port.split('/')[0]) if ( @@ -368,7 +396,7 @@ def close(self, rm_all_containers: bool | None = None): close_prefix = ( CONTAINER_NAME_PREFIX if rm_all_containers else self.container_name ) - remove_all_containers(close_prefix) + stop_all_containers(close_prefix) def _is_port_in_use_docker(self, port): containers = self.docker_client.containers.list() diff --git a/openhands/server/routes/manage_conversations.py b/openhands/server/routes/manage_conversations.py index 3d8c0fe1523a..8ce7a8cec374 100644 --- a/openhands/server/routes/manage_conversations.py +++ b/openhands/server/routes/manage_conversations.py @@ -227,6 +227,7 @@ async def delete_conversation( is_running = await session_manager.is_agent_loop_running(conversation_id) if is_running: await session_manager.close_session(conversation_id) + # TODO: Delete await conversation_store.delete_metadata(conversation_id) return True From f5b6e3412a29b58889fb498bb8ba0730818bfb3a Mon Sep 17 00:00:00 2001 From: Tim O'Farrell Date: Tue, 21 Jan 2025 14:31:49 -0700 Subject: [PATCH 2/8] WIP --- .../runtime/impl/docker/docker_runtime.py | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/openhands/runtime/impl/docker/docker_runtime.py b/openhands/runtime/impl/docker/docker_runtime.py index f0cd252b8a34..d0e0d3a45706 100644 --- a/openhands/runtime/impl/docker/docker_runtime.py +++ b/openhands/runtime/impl/docker/docker_runtime.py @@ -190,8 +190,10 @@ def _init_container(self): self.send_status_message('STATUS$PREPARING_CONTAINER') try: + print('TRACE:101') container = self.docker_client.containers.get(self.container_name) container.restart() + print('TRACE:103') self.container = container config = container.attrs['Config'] for env_var in config['Env']: @@ -210,8 +212,10 @@ def _init_container(self): self._app_ports.append(exposed_port) return except docker.errors.NotFound: + print('TRACE:104') pass except docker.errors.APIError: + print('TRACE:105') pass self._host_port = self._find_available_port(EXECUTION_SERVER_PORT_RANGE) @@ -336,6 +340,20 @@ def _attach_to_container(self): self.container = self.docker_client.containers.get(self.container_name) if self.container.status == 'exited': self.container.start() + + config = self.container.attrs['Config'] + for env_var in config['Env']: + if env_var.startswith('port='): + self._host_port = int(env_var.split('port=')[1]) + self._container_port = self._host_port + elif env_var.startswith('VSCODE_PORT='): + self._vscode_port = int(env_var.split('VSCODE_PORT=')[1]) + self._app_ports = [] + for exposed_port in config['ExposedPorts'].keys(): + exposed_port = int(exposed_port.split('/tcp')[0]) + if exposed_port != self._host_port and exposed_port != self._vscode_port: + self._app_ports.append(exposed_port) + """ for port in self.container.attrs['NetworkSettings']['Ports']: # type: ignore port = int(port.split('/')[0]) if ( @@ -350,6 +368,7 @@ def _attach_to_container(self): elif port >= APP_PORT_RANGE_2[0] and port <= APP_PORT_RANGE_2[1]: self._app_ports.append(port) self._host_port = self._container_port + """ self.api_url = f'{self.config.sandbox.local_runtime_url}:{self._container_port}' self.log( 'debug', From 007b43d3fc932778dc982ebf1a054022704ee307 Mon Sep 17 00:00:00 2001 From: Tim O'Farrell Date: Tue, 21 Jan 2025 15:37:25 -0700 Subject: [PATCH 3/8] Stopping runtimes rather than deleting them --- openhands/core/config/sandbox_config.py | 2 +- openhands/runtime/base.py | 4 ++ .../runtime/impl/docker/docker_runtime.py | 61 +++++-------------- .../server/routes/manage_conversations.py | 4 +- 4 files changed, 22 insertions(+), 49 deletions(-) diff --git a/openhands/core/config/sandbox_config.py b/openhands/core/config/sandbox_config.py index 3ed15a01ecb2..f5b984fec0b9 100644 --- a/openhands/core/config/sandbox_config.py +++ b/openhands/core/config/sandbox_config.py @@ -39,7 +39,7 @@ class SandboxConfig(BaseModel): remote_runtime_api_url: str = Field(default='http://localhost:8000') local_runtime_url: str = Field(default='http://localhost') - keep_runtime_alive: bool = Field(default=True) + keep_runtime_alive: bool = Field(default=False) rm_all_containers: bool = Field(default=False) api_key: str | None = Field(default=None) base_container_image: str = Field( diff --git a/openhands/runtime/base.py b/openhands/runtime/base.py index 4e5729ac7789..9afbf6644e71 100644 --- a/openhands/runtime/base.py +++ b/openhands/runtime/base.py @@ -136,6 +136,10 @@ def setup_initial_env(self) -> None: def close(self) -> None: pass + @classmethod + def delete(cls, conversation_id: str) -> None: + pass + def log(self, level: str, message: str) -> None: message = f'[runtime {self.sid}] {message}' getattr(logger, level)(message, stacklevel=2) diff --git a/openhands/runtime/impl/docker/docker_runtime.py b/openhands/runtime/impl/docker/docker_runtime.py index d0e0d3a45706..86a31c7c2fbe 100644 --- a/openhands/runtime/impl/docker/docker_runtime.py +++ b/openhands/runtime/impl/docker/docker_runtime.py @@ -188,36 +188,6 @@ def _init_docker_client() -> docker.DockerClient: def _init_container(self): self.log('debug', 'Preparing to start container...') self.send_status_message('STATUS$PREPARING_CONTAINER') - - try: - print('TRACE:101') - container = self.docker_client.containers.get(self.container_name) - container.restart() - print('TRACE:103') - self.container = container - config = container.attrs['Config'] - for env_var in config['Env']: - if env_var.startswith('port='): - self._host_port = int(env_var.split('port=')[1]) - self._container_port = self._host_port - elif env_var.startswith('VSCODE_PORT='): - self._vscode_port = int(env_var.split('VSCODE_PORT=')[1]) - self._app_ports = [] - for exposed_port in config['ExposedPorts'].keys(): - exposed_port = int(exposed_port.split('/tcp')[0]) - if ( - exposed_port != self._host_port - and exposed_port != self._vscode_port - ): - self._app_ports.append(exposed_port) - return - except docker.errors.NotFound: - print('TRACE:104') - pass - except docker.errors.APIError: - print('TRACE:105') - pass - self._host_port = self._find_available_port(EXECUTION_SERVER_PORT_RANGE) self._container_port = self._host_port self._vscode_port = self._find_available_port(VSCODE_PORT_RANGE) @@ -340,7 +310,6 @@ def _attach_to_container(self): self.container = self.docker_client.containers.get(self.container_name) if self.container.status == 'exited': self.container.start() - config = self.container.attrs['Config'] for env_var in config['Env']: if env_var.startswith('port='): @@ -353,22 +322,6 @@ def _attach_to_container(self): exposed_port = int(exposed_port.split('/tcp')[0]) if exposed_port != self._host_port and exposed_port != self._vscode_port: self._app_ports.append(exposed_port) - """ - for port in self.container.attrs['NetworkSettings']['Ports']: # type: ignore - port = int(port.split('/')[0]) - if ( - port >= EXECUTION_SERVER_PORT_RANGE[0] - and port <= EXECUTION_SERVER_PORT_RANGE[1] - ): - self._container_port = port - if port >= VSCODE_PORT_RANGE[0] and port <= VSCODE_PORT_RANGE[1]: - self._vscode_port = port - elif port >= APP_PORT_RANGE_1[0] and port <= APP_PORT_RANGE_1[1]: - self._app_ports.append(port) - elif port >= APP_PORT_RANGE_2[0] and port <= APP_PORT_RANGE_2[1]: - self._app_ports.append(port) - self._host_port = self._container_port - """ self.api_url = f'{self.config.sandbox.local_runtime_url}:{self._container_port}' self.log( 'debug', @@ -451,3 +404,17 @@ def web_hosts(self): hosts[f'http://localhost:{port}'] = port return hosts + + @classmethod + def delete(cls, conversation_id: str): + docker_client = cls._init_docker_client() + try: + container_name = CONTAINER_NAME_PREFIX + conversation_id + container = docker_client.containers.get(container_name) + container.remove(force=True) + except docker.errors.APIError: + pass + except docker.errors.NotFound: + pass + finally: + docker_client.close() diff --git a/openhands/server/routes/manage_conversations.py b/openhands/server/routes/manage_conversations.py index 8ce7a8cec374..a91d6badda77 100644 --- a/openhands/server/routes/manage_conversations.py +++ b/openhands/server/routes/manage_conversations.py @@ -8,6 +8,7 @@ from openhands.core.logger import openhands_logger as logger from openhands.events.stream import EventStreamSubscriber +from openhands.runtime import get_runtime_cls from openhands.server.auth import get_user_id from openhands.server.routes.settings import ConversationStoreImpl, SettingsStoreImpl from openhands.server.session.conversation_init_data import ConversationInitData @@ -227,7 +228,8 @@ async def delete_conversation( is_running = await session_manager.is_agent_loop_running(conversation_id) if is_running: await session_manager.close_session(conversation_id) - # TODO: Delete + runtime_cls = get_runtime_cls(config.runtime) + await runtime_cls.delete(conversation_id) await conversation_store.delete_metadata(conversation_id) return True From b316c0e85adc03234abe78adef770dea24f18e79 Mon Sep 17 00:00:00 2001 From: Tim O'Farrell Date: Tue, 21 Jan 2025 15:39:48 -0700 Subject: [PATCH 4/8] Fix for delete --- openhands/runtime/base.py | 2 +- openhands/runtime/impl/docker/docker_runtime.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/openhands/runtime/base.py b/openhands/runtime/base.py index 9afbf6644e71..8362336a16b1 100644 --- a/openhands/runtime/base.py +++ b/openhands/runtime/base.py @@ -137,7 +137,7 @@ def close(self) -> None: pass @classmethod - def delete(cls, conversation_id: str) -> None: + async def delete(cls, conversation_id: str) -> None: pass def log(self, level: str, message: str) -> None: diff --git a/openhands/runtime/impl/docker/docker_runtime.py b/openhands/runtime/impl/docker/docker_runtime.py index 86a31c7c2fbe..91621c24906e 100644 --- a/openhands/runtime/impl/docker/docker_runtime.py +++ b/openhands/runtime/impl/docker/docker_runtime.py @@ -406,7 +406,7 @@ def web_hosts(self): return hosts @classmethod - def delete(cls, conversation_id: str): + async def delete(cls, conversation_id: str): docker_client = cls._init_docker_client() try: container_name = CONTAINER_NAME_PREFIX + conversation_id From 28be124d2f7707afe807f58163469cb7daf6a8c4 Mon Sep 17 00:00:00 2001 From: Tim O'Farrell Date: Tue, 21 Jan 2025 15:40:40 -0700 Subject: [PATCH 5/8] Always stop all runtime containers on exit --- openhands/runtime/impl/docker/docker_runtime.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/openhands/runtime/impl/docker/docker_runtime.py b/openhands/runtime/impl/docker/docker_runtime.py index e5276c6e4742..91621c24906e 100644 --- a/openhands/runtime/impl/docker/docker_runtime.py +++ b/openhands/runtime/impl/docker/docker_runtime.py @@ -67,7 +67,7 @@ def __init__( headless_mode: bool = True, ): global _atexit_registered - if not _atexit_registered and not config.sandbox.keep_runtime_alive: + if not _atexit_registered: _atexit_registered = True atexit.register(stop_all_runtime_containers) From 0287b0ea0f4d86ac0b97f49994e3d53c4db8a4bb Mon Sep 17 00:00:00 2001 From: Tim O'Farrell Date: Tue, 21 Jan 2025 15:51:00 -0700 Subject: [PATCH 6/8] Removed comment --- openhands/runtime/impl/docker/containers.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/openhands/runtime/impl/docker/containers.py b/openhands/runtime/impl/docker/containers.py index bed21d722b7f..b2c99f9825c5 100644 --- a/openhands/runtime/impl/docker/containers.py +++ b/openhands/runtime/impl/docker/containers.py @@ -8,7 +8,7 @@ def stop_all_containers(prefix: str): for container in containers: try: if container.name.startswith(prefix): - container.stop() # .remove(force=True) + container.stop() except docker.errors.APIError: pass except docker.errors.NotFound: From 6a9acbededd705362394c370afb9555bd82f1164 Mon Sep 17 00:00:00 2001 From: openhands Date: Wed, 22 Jan 2025 15:34:16 +0000 Subject: [PATCH 7/8] Add unit tests for DockerRuntime container cleanup behavior --- tests/unit/test_docker_runtime.py | 63 +++++++++++++++++++++++++++++++ 1 file changed, 63 insertions(+) create mode 100644 tests/unit/test_docker_runtime.py diff --git a/tests/unit/test_docker_runtime.py b/tests/unit/test_docker_runtime.py new file mode 100644 index 000000000000..54f899716b1b --- /dev/null +++ b/tests/unit/test_docker_runtime.py @@ -0,0 +1,63 @@ +import pytest +from unittest.mock import MagicMock, patch + +from openhands.core.config import AppConfig +from openhands.events import EventStream +from openhands.runtime.impl.docker.docker_runtime import DockerRuntime + + +@pytest.fixture +def mock_docker_client(): + with patch('docker.from_env') as mock_client: + container_mock = MagicMock() + container_mock.status = 'running' + container_mock.attrs = { + 'Config': { + 'Env': ['port=12345', 'VSCODE_PORT=54321'], + 'ExposedPorts': {'12345/tcp': {}, '54321/tcp': {}} + } + } + mock_client.return_value.containers.get.return_value = container_mock + mock_client.return_value.containers.run.return_value = container_mock + # Mock version info for BuildKit check + mock_client.return_value.version.return_value = {'Version': '20.10.0'} + yield mock_client.return_value + + +@pytest.fixture +def config(): + config = AppConfig() + config.sandbox.keep_runtime_alive = False + return config + + +@pytest.fixture +def event_stream(): + return MagicMock(spec=EventStream) + + +@patch('openhands.runtime.impl.docker.docker_runtime.stop_all_containers') +def test_container_stopped_when_keep_runtime_alive_false(mock_stop_containers, mock_docker_client, config, event_stream): + # Arrange + runtime = DockerRuntime(config, event_stream, sid='test-sid') + runtime.container = mock_docker_client.containers.get.return_value + + # Act + runtime.close() + + # Assert + mock_stop_containers.assert_called_once_with(f'openhands-runtime-test-sid') + + +@patch('openhands.runtime.impl.docker.docker_runtime.stop_all_containers') +def test_container_not_stopped_when_keep_runtime_alive_true(mock_stop_containers, mock_docker_client, config, event_stream): + # Arrange + config.sandbox.keep_runtime_alive = True + runtime = DockerRuntime(config, event_stream, sid='test-sid') + runtime.container = mock_docker_client.containers.get.return_value + + # Act + runtime.close() + + # Assert + mock_stop_containers.assert_not_called() \ No newline at end of file From 2027bae78e8d825412aa2d3161dc7b70c017ab4b Mon Sep 17 00:00:00 2001 From: Tim O'Farrell Date: Wed, 22 Jan 2025 11:54:59 -0700 Subject: [PATCH 8/8] Lint fixes --- openhands/server/session/manager.py | 5 ++++- tests/unit/test_docker_runtime.py | 25 +++++++++++++++---------- 2 files changed, 19 insertions(+), 11 deletions(-) diff --git a/openhands/server/session/manager.py b/openhands/server/session/manager.py index 203a8dc3b2ed..8dcead45ee67 100644 --- a/openhands/server/session/manager.py +++ b/openhands/server/session/manager.py @@ -456,7 +456,10 @@ async def maybe_start_agent_loop( response_ids = await self.get_running_agent_loops(user_id) if len(response_ids) >= MAX_RUNNING_CONVERSATIONS: logger.info('too_many_sessions_for:{user_id}') - await self.close_session(next(iter(response_ids))) + # Order is not guaranteed, but response_ids tend to be in descending chronological order + # By reversing, we are likely to pick the oldest (or at least an older) conversation + session_id = next(iter(reversed(list(response_ids)))) + await self.close_session(session_id) session = Session( sid=sid, diff --git a/tests/unit/test_docker_runtime.py b/tests/unit/test_docker_runtime.py index 54f899716b1b..9e44d73225d0 100644 --- a/tests/unit/test_docker_runtime.py +++ b/tests/unit/test_docker_runtime.py @@ -1,6 +1,7 @@ -import pytest from unittest.mock import MagicMock, patch +import pytest + from openhands.core.config import AppConfig from openhands.events import EventStream from openhands.runtime.impl.docker.docker_runtime import DockerRuntime @@ -14,7 +15,7 @@ def mock_docker_client(): container_mock.attrs = { 'Config': { 'Env': ['port=12345', 'VSCODE_PORT=54321'], - 'ExposedPorts': {'12345/tcp': {}, '54321/tcp': {}} + 'ExposedPorts': {'12345/tcp': {}, '54321/tcp': {}}, } } mock_client.return_value.containers.get.return_value = container_mock @@ -37,27 +38,31 @@ def event_stream(): @patch('openhands.runtime.impl.docker.docker_runtime.stop_all_containers') -def test_container_stopped_when_keep_runtime_alive_false(mock_stop_containers, mock_docker_client, config, event_stream): +def test_container_stopped_when_keep_runtime_alive_false( + mock_stop_containers, mock_docker_client, config, event_stream +): # Arrange runtime = DockerRuntime(config, event_stream, sid='test-sid') runtime.container = mock_docker_client.containers.get.return_value - + # Act runtime.close() - + # Assert - mock_stop_containers.assert_called_once_with(f'openhands-runtime-test-sid') + mock_stop_containers.assert_called_once_with('openhands-runtime-test-sid') @patch('openhands.runtime.impl.docker.docker_runtime.stop_all_containers') -def test_container_not_stopped_when_keep_runtime_alive_true(mock_stop_containers, mock_docker_client, config, event_stream): +def test_container_not_stopped_when_keep_runtime_alive_true( + mock_stop_containers, mock_docker_client, config, event_stream +): # Arrange config.sandbox.keep_runtime_alive = True runtime = DockerRuntime(config, event_stream, sid='test-sid') runtime.container = mock_docker_client.containers.get.return_value - + # Act runtime.close() - + # Assert - mock_stop_containers.assert_not_called() \ No newline at end of file + mock_stop_containers.assert_not_called()