From dcb2e21b87b87d9a52c23c8a44f3d907f0648f60 Mon Sep 17 00:00:00 2001 From: Saurya Velagapudi Date: Wed, 18 Mar 2026 17:07:19 -0700 Subject: [PATCH] feat: Auto-forward LLM_* env vars to agent-server and fix host network config (#13192) Co-authored-by: openhands --- .../sandbox/docker_sandbox_service.py | 21 +- .../sandbox/sandbox_spec_service.py | 53 ++- .../test_agent_server_env_override.py | 422 +++++++++++++++++- .../app_server/test_docker_sandbox_service.py | 53 +++ 4 files changed, 528 insertions(+), 21 deletions(-) diff --git a/openhands/app_server/sandbox/docker_sandbox_service.py b/openhands/app_server/sandbox/docker_sandbox_service.py index 6c692a680a..f5a302fa73 100644 --- a/openhands/app_server/sandbox/docker_sandbox_service.py +++ b/openhands/app_server/sandbox/docker_sandbox_service.py @@ -43,6 +43,16 @@ _logger = logging.getLogger(__name__) STARTUP_GRACE_SECONDS = 15 +def _get_use_host_network_default() -> bool: + """Get the default value for use_host_network from environment variables. + + This function is called at runtime (not at class definition time) to ensure + that environment variable changes are picked up correctly. + """ + value = os.getenv('AGENT_SERVER_USE_HOST_NETWORK', '') + return value.lower() in ('true', '1', 'yes') + + class VolumeMount(BaseModel): """Mounted volume within the container.""" @@ -591,18 +601,13 @@ class DockerSandboxServiceInjector(SandboxServiceInjector): ), ) use_host_network: bool = Field( - default=os.getenv('SANDBOX_USE_HOST_NETWORK', '').lower() - in ( - 'true', - '1', - 'yes', - ), + default_factory=_get_use_host_network_default, description=( - 'Whether to use host networking mode for sandbox containers. ' + 'Whether to use host networking mode for agent-server containers. ' 'When enabled, containers share the host network namespace, ' 'making all container ports directly accessible on the host. ' 'This is useful for reverse proxy setups where dynamic port mapping ' - 'is problematic. Configure via OH_SANDBOX_USE_HOST_NETWORK environment variable.' + 'is problematic. Configure via AGENT_SERVER_USE_HOST_NETWORK environment variable.' ), ) diff --git a/openhands/app_server/sandbox/sandbox_spec_service.py b/openhands/app_server/sandbox/sandbox_spec_service.py index 4034af1f5b..5025bdee6b 100644 --- a/openhands/app_server/sandbox/sandbox_spec_service.py +++ b/openhands/app_server/sandbox/sandbox_spec_service.py @@ -69,27 +69,58 @@ def get_agent_server_image() -> str: return AGENT_SERVER_IMAGE +# Prefixes for environment variables that should be auto-forwarded to agent-server +# These are typically configuration variables that affect the agent's behavior +AUTO_FORWARD_PREFIXES = ('LLM_',) + + def get_agent_server_env() -> dict[str, str]: """Get environment variables to be injected into agent server sandbox environments. - This function reads environment variable overrides from the OH_AGENT_SERVER_ENV - environment variable, which should contain a JSON string mapping variable names - to their values. + This function combines two sources of environment variables: + + 1. **Auto-forwarded variables**: Environment variables with certain prefixes + (e.g., LLM_*) are automatically forwarded to the agent-server container. + This ensures that LLM configuration like timeouts and retry settings + work correctly in the two-container V1 architecture. + + 2. **Explicit overrides via OH_AGENT_SERVER_ENV**: A JSON string that allows + setting arbitrary environment variables in the agent-server container. + Values set here take precedence over auto-forwarded variables. + + Auto-forwarded prefixes: + - LLM_* : LLM configuration (timeout, retries, model settings, etc.) Usage: - Set OH_AGENT_SERVER_ENV to a JSON string: - OH_AGENT_SERVER_ENV='{"DEBUG": "true", "LOG_LEVEL": "info", "CUSTOM_VAR": "value"}' + # Auto-forwarding (no action needed): + export LLM_TIMEOUT=3600 + export LLM_NUM_RETRIES=10 + # These will automatically be available in the agent-server - This will inject the following environment variables into all sandbox environments: - - DEBUG=true - - LOG_LEVEL=info - - CUSTOM_VAR=value + # Explicit override via JSON: + OH_AGENT_SERVER_ENV='{"DEBUG": "true", "CUSTOM_VAR": "value"}' + + # Override an auto-forwarded variable: + export LLM_TIMEOUT=3600 # Would be auto-forwarded as 3600 + OH_AGENT_SERVER_ENV='{"LLM_TIMEOUT": "7200"}' # Overrides to 7200 Returns: dict[str, str]: Dictionary of environment variable names to values. - Returns empty dict if OH_AGENT_SERVER_ENV is not set or invalid. + Returns empty dict if no variables are found. Raises: JSONDecodeError: If OH_AGENT_SERVER_ENV contains invalid JSON. """ - return env_parser.from_env(dict[str, str], 'OH_AGENT_SERVER_ENV') + result: dict[str, str] = {} + + # Step 1: Auto-forward environment variables with recognized prefixes + for key, value in os.environ.items(): + if any(key.startswith(prefix) for prefix in AUTO_FORWARD_PREFIXES): + result[key] = value + + # Step 2: Apply explicit overrides from OH_AGENT_SERVER_ENV + # These take precedence over auto-forwarded variables + explicit_env = env_parser.from_env(dict[str, str], 'OH_AGENT_SERVER_ENV') + result.update(explicit_env) + + return result diff --git a/tests/unit/app_server/test_agent_server_env_override.py b/tests/unit/app_server/test_agent_server_env_override.py index 61d851590e..5c1c1ea208 100644 --- a/tests/unit/app_server/test_agent_server_env_override.py +++ b/tests/unit/app_server/test_agent_server_env_override.py @@ -2,10 +2,11 @@ This module tests the environment variable override functionality that allows users to inject custom environment variables into sandbox environments via -OH_AGENT_SERVER_ENV_* environment variables. +OH_AGENT_SERVER_ENV environment variable and auto-forwarding of LLM_* variables. The functionality includes: -- Parsing OH_AGENT_SERVER_ENV_* environment variables +- Auto-forwarding of LLM_* environment variables to agent-server containers +- Explicit overrides via OH_AGENT_SERVER_ENV JSON - Merging them into sandbox specifications - Integration across different sandbox types (Docker, Process, Remote) """ @@ -25,6 +26,7 @@ from openhands.app_server.sandbox.remote_sandbox_spec_service import ( get_default_sandbox_specs as get_default_remote_sandbox_specs, ) from openhands.app_server.sandbox.sandbox_spec_service import ( + AUTO_FORWARD_PREFIXES, get_agent_server_env, ) @@ -185,6 +187,114 @@ class TestGetAgentServerEnv: assert result == expected +class TestLLMAutoForwarding: + """Test cases for automatic forwarding of LLM_* environment variables.""" + + def test_auto_forward_prefixes_contains_llm(self): + """Test that LLM_ is in the auto-forward prefixes.""" + assert 'LLM_' in AUTO_FORWARD_PREFIXES + + def test_llm_timeout_auto_forwarded(self): + """Test that LLM_TIMEOUT is automatically forwarded.""" + env_vars = { + 'LLM_TIMEOUT': '3600', + 'OTHER_VAR': 'should_not_be_included', + } + + with patch.dict(os.environ, env_vars, clear=True): + result = get_agent_server_env() + assert 'LLM_TIMEOUT' in result + assert result['LLM_TIMEOUT'] == '3600' + assert 'OTHER_VAR' not in result + + def test_llm_num_retries_auto_forwarded(self): + """Test that LLM_NUM_RETRIES is automatically forwarded.""" + env_vars = { + 'LLM_NUM_RETRIES': '10', + } + + with patch.dict(os.environ, env_vars, clear=True): + result = get_agent_server_env() + assert 'LLM_NUM_RETRIES' in result + assert result['LLM_NUM_RETRIES'] == '10' + + def test_multiple_llm_vars_auto_forwarded(self): + """Test that multiple LLM_* variables are automatically forwarded.""" + env_vars = { + 'LLM_TIMEOUT': '3600', + 'LLM_NUM_RETRIES': '10', + 'LLM_MODEL': 'gpt-4', + 'LLM_BASE_URL': 'https://api.example.com', + 'LLM_API_KEY': 'secret-key', + 'NON_LLM_VAR': 'should_not_be_included', + } + + with patch.dict(os.environ, env_vars, clear=True): + result = get_agent_server_env() + assert result['LLM_TIMEOUT'] == '3600' + assert result['LLM_NUM_RETRIES'] == '10' + assert result['LLM_MODEL'] == 'gpt-4' + assert result['LLM_BASE_URL'] == 'https://api.example.com' + assert result['LLM_API_KEY'] == 'secret-key' + assert 'NON_LLM_VAR' not in result + + def test_explicit_override_takes_precedence(self): + """Test that OH_AGENT_SERVER_ENV overrides auto-forwarded variables.""" + env_vars = { + 'LLM_TIMEOUT': '3600', # Auto-forwarded value + 'OH_AGENT_SERVER_ENV': '{"LLM_TIMEOUT": "7200"}', # Explicit override + } + + with patch.dict(os.environ, env_vars, clear=True): + result = get_agent_server_env() + # Explicit override should win + assert result['LLM_TIMEOUT'] == '7200' + + def test_combined_auto_forward_and_explicit(self): + """Test combining auto-forwarded and explicit variables.""" + env_vars = { + 'LLM_TIMEOUT': '3600', # Auto-forwarded + 'LLM_NUM_RETRIES': '10', # Auto-forwarded + 'OH_AGENT_SERVER_ENV': '{"DEBUG": "true", "CUSTOM_VAR": "value"}', # Explicit + } + + with patch.dict(os.environ, env_vars, clear=True): + result = get_agent_server_env() + # Auto-forwarded + assert result['LLM_TIMEOUT'] == '3600' + assert result['LLM_NUM_RETRIES'] == '10' + # Explicit + assert result['DEBUG'] == 'true' + assert result['CUSTOM_VAR'] == 'value' + + def test_no_llm_vars_returns_empty_without_explicit(self): + """Test that no LLM_* vars and no explicit env returns empty dict.""" + env_vars = { + 'SOME_OTHER_VAR': 'value', + 'ANOTHER_VAR': 'another_value', + } + + with patch.dict(os.environ, env_vars, clear=True): + result = get_agent_server_env() + assert result == {} + + def test_llm_prefix_is_case_sensitive(self): + """Test that LLM_ prefix matching is case-sensitive.""" + env_vars = { + 'LLM_TIMEOUT': '3600', # Should be included + 'llm_timeout': 'lowercase', # Should NOT be included (wrong case) + 'Llm_Timeout': 'mixed', # Should NOT be included (wrong case) + } + + with patch.dict(os.environ, env_vars, clear=True): + result = get_agent_server_env() + assert 'LLM_TIMEOUT' in result + assert result['LLM_TIMEOUT'] == '3600' + # Lowercase variants should not be included + assert 'llm_timeout' not in result + assert 'Llm_Timeout' not in result + + class TestDockerSandboxSpecEnvironmentOverride: """Test environment variable override integration in Docker sandbox specs.""" @@ -476,3 +586,311 @@ class TestEnvironmentOverrideIntegration: # Should not have the old variables assert 'VAR1' not in spec_2.initial_env assert 'VAR2' not in spec_2.initial_env + + +class TestDockerSandboxServiceEnvIntegration: + """Integration tests for environment variable propagation to Docker sandbox containers. + + These tests verify that environment variables are correctly propagated through + the entire flow from the app-server environment to the agent-server container. + """ + + def test_llm_env_vars_propagated_to_container_run(self): + """Test that LLM_* env vars are included in docker container.run() environment argument.""" + from unittest.mock import patch + + # Set up environment with LLM_* variables + env_vars = { + 'LLM_TIMEOUT': '3600', + 'LLM_NUM_RETRIES': '10', + 'LLM_MODEL': 'gpt-4', + 'OTHER_VAR': 'should_not_be_forwarded', + } + + with patch.dict(os.environ, env_vars, clear=True): + # Create a sandbox spec using the actual factory to get LLM_* vars + specs = get_default_docker_sandbox_specs() + sandbox_spec = specs[0] + + # Verify the sandbox spec has the LLM_* variables + assert 'LLM_TIMEOUT' in sandbox_spec.initial_env + assert sandbox_spec.initial_env['LLM_TIMEOUT'] == '3600' + assert 'LLM_NUM_RETRIES' in sandbox_spec.initial_env + assert sandbox_spec.initial_env['LLM_NUM_RETRIES'] == '10' + assert 'LLM_MODEL' in sandbox_spec.initial_env + assert sandbox_spec.initial_env['LLM_MODEL'] == 'gpt-4' + # Non-LLM_* variables should not be included + assert 'OTHER_VAR' not in sandbox_spec.initial_env + + def test_explicit_oh_agent_server_env_overrides_llm_vars(self): + """Test that OH_AGENT_SERVER_ENV can override auto-forwarded LLM_* variables.""" + env_vars = { + 'LLM_TIMEOUT': '3600', # Auto-forwarded value + 'OH_AGENT_SERVER_ENV': '{"LLM_TIMEOUT": "7200"}', # Override value + } + + with patch.dict(os.environ, env_vars, clear=True): + specs = get_default_docker_sandbox_specs() + sandbox_spec = specs[0] + + # OH_AGENT_SERVER_ENV should take precedence + assert sandbox_spec.initial_env['LLM_TIMEOUT'] == '7200' + + def test_multiple_llm_vars_combined_with_explicit_overrides(self): + """Test complex scenario with multiple LLM_* vars and explicit overrides.""" + env_vars = { + 'LLM_TIMEOUT': '3600', + 'LLM_NUM_RETRIES': '10', + 'LLM_MODEL': 'gpt-4', + 'LLM_TEMPERATURE': '0.7', + 'OH_AGENT_SERVER_ENV': '{"LLM_MODEL": "gpt-3.5-turbo", "CUSTOM_VAR": "custom_value"}', + } + + with patch.dict(os.environ, env_vars, clear=True): + specs = get_default_docker_sandbox_specs() + sandbox_spec = specs[0] + + # Auto-forwarded LLM_* vars that weren't overridden + assert sandbox_spec.initial_env['LLM_TIMEOUT'] == '3600' + assert sandbox_spec.initial_env['LLM_NUM_RETRIES'] == '10' + assert sandbox_spec.initial_env['LLM_TEMPERATURE'] == '0.7' + + # LLM_MODEL should be overridden by OH_AGENT_SERVER_ENV + assert sandbox_spec.initial_env['LLM_MODEL'] == 'gpt-3.5-turbo' + + # Custom variable from OH_AGENT_SERVER_ENV + assert sandbox_spec.initial_env['CUSTOM_VAR'] == 'custom_value' + + def test_sandbox_spec_env_passed_to_docker_container_run(self): + """Test that sandbox spec's initial_env is passed to docker container run.""" + from unittest.mock import AsyncMock, MagicMock, patch + + import httpx + + from openhands.app_server.sandbox.docker_sandbox_service import ( + DockerSandboxService, + ExposedPort, + ) + + # Create mock docker client + mock_docker_client = MagicMock() + mock_container = MagicMock() + mock_container.name = 'oh-test-abc123' + mock_container.image.tags = ['test-image:latest'] + mock_container.attrs = { + 'Created': '2024-01-01T00:00:00Z', + 'Config': { + 'Env': ['SESSION_API_KEY=test-key'], + 'WorkingDir': '/workspace', + }, + 'NetworkSettings': {'Ports': {'8000/tcp': [{'HostPort': '32768'}]}}, + 'HostConfig': {'NetworkMode': 'bridge'}, + } + mock_container.status = 'running' + mock_docker_client.containers.run.return_value = mock_container + mock_docker_client.containers.list.return_value = [] + + # Create mock sandbox spec service + mock_spec_service = MagicMock() + + # Create sandbox spec with LLM_* environment variables + env_vars = { + 'LLM_TIMEOUT': '3600', + 'LLM_NUM_RETRIES': '10', + } + + with patch.dict(os.environ, env_vars, clear=True): + specs = get_default_docker_sandbox_specs() + sandbox_spec = specs[0] + + mock_spec_service.get_default_sandbox_spec = AsyncMock( + return_value=sandbox_spec + ) + + # Create service + service = DockerSandboxService( + sandbox_spec_service=mock_spec_service, + container_name_prefix='oh-test-', + host_port=3000, + container_url_pattern='http://localhost:{port}', + mounts=[], + exposed_ports=[ + ExposedPort( + name='AGENT_SERVER', + description='Agent server', + container_port=8000, + ) + ], + health_check_path='/health', + httpx_client=MagicMock(spec=httpx.AsyncClient), + max_num_sandboxes=5, + docker_client=mock_docker_client, + ) + + # Start sandbox + import asyncio + + asyncio.get_event_loop().run_until_complete(service.start_sandbox()) + + # Verify docker was called with environment variables including LLM_* + call_kwargs = mock_docker_client.containers.run.call_args[1] + container_env = call_kwargs['environment'] + + # LLM_* variables should be in the container environment + assert 'LLM_TIMEOUT' in container_env + assert container_env['LLM_TIMEOUT'] == '3600' + assert 'LLM_NUM_RETRIES' in container_env + assert container_env['LLM_NUM_RETRIES'] == '10' + + # Default variables should also be present + assert 'OPENVSCODE_SERVER_ROOT' in container_env + assert 'LOG_JSON' in container_env + + def test_host_network_mode_with_env_var(self): + """Test that AGENT_SERVER_USE_HOST_NETWORK affects container network mode.""" + from unittest.mock import AsyncMock, MagicMock, patch + + import httpx + + from openhands.app_server.sandbox.docker_sandbox_service import ( + DockerSandboxService, + ExposedPort, + _get_use_host_network_default, + ) + + # Test with environment variable set + with patch.dict( + os.environ, {'AGENT_SERVER_USE_HOST_NETWORK': 'true'}, clear=True + ): + assert _get_use_host_network_default() is True + + # Create mock docker client + mock_docker_client = MagicMock() + mock_container = MagicMock() + mock_container.name = 'oh-test-abc123' + mock_container.image.tags = ['test-image:latest'] + mock_container.attrs = { + 'Created': '2024-01-01T00:00:00Z', + 'Config': { + 'Env': ['SESSION_API_KEY=test-key'], + 'WorkingDir': '/workspace', + }, + 'NetworkSettings': {'Ports': {}}, + 'HostConfig': {'NetworkMode': 'host'}, + } + mock_container.status = 'running' + mock_docker_client.containers.run.return_value = mock_container + mock_docker_client.containers.list.return_value = [] + + # Create mock sandbox spec service + mock_spec_service = MagicMock() + specs = get_default_docker_sandbox_specs() + mock_spec_service.get_default_sandbox_spec = AsyncMock( + return_value=specs[0] + ) + + # Create service with host network enabled + service = DockerSandboxService( + sandbox_spec_service=mock_spec_service, + container_name_prefix='oh-test-', + host_port=3000, + container_url_pattern='http://localhost:{port}', + mounts=[], + exposed_ports=[ + ExposedPort( + name='AGENT_SERVER', + description='Agent server', + container_port=8000, + ) + ], + health_check_path='/health', + httpx_client=MagicMock(spec=httpx.AsyncClient), + max_num_sandboxes=5, + docker_client=mock_docker_client, + use_host_network=True, + ) + + # Start sandbox + import asyncio + + asyncio.get_event_loop().run_until_complete(service.start_sandbox()) + + # Verify docker was called with host network mode + call_kwargs = mock_docker_client.containers.run.call_args[1] + assert call_kwargs['network_mode'] == 'host' + # Port mappings should be None in host network mode + assert call_kwargs['ports'] is None + + def test_bridge_network_mode_without_env_var(self): + """Test that default (bridge) network mode is used when env var is not set.""" + from unittest.mock import AsyncMock, MagicMock, patch + + import httpx + + from openhands.app_server.sandbox.docker_sandbox_service import ( + DockerSandboxService, + ExposedPort, + _get_use_host_network_default, + ) + + # Test without environment variable + with patch.dict(os.environ, {}, clear=True): + assert _get_use_host_network_default() is False + + # Create mock docker client + mock_docker_client = MagicMock() + mock_container = MagicMock() + mock_container.name = 'oh-test-abc123' + mock_container.image.tags = ['test-image:latest'] + mock_container.attrs = { + 'Created': '2024-01-01T00:00:00Z', + 'Config': { + 'Env': ['SESSION_API_KEY=test-key'], + 'WorkingDir': '/workspace', + }, + 'NetworkSettings': {'Ports': {'8000/tcp': [{'HostPort': '32768'}]}}, + 'HostConfig': {'NetworkMode': 'bridge'}, + } + mock_container.status = 'running' + mock_docker_client.containers.run.return_value = mock_container + mock_docker_client.containers.list.return_value = [] + + # Create mock sandbox spec service + mock_spec_service = MagicMock() + specs = get_default_docker_sandbox_specs() + mock_spec_service.get_default_sandbox_spec = AsyncMock( + return_value=specs[0] + ) + + # Create service with bridge network (default) + service = DockerSandboxService( + sandbox_spec_service=mock_spec_service, + container_name_prefix='oh-test-', + host_port=3000, + container_url_pattern='http://localhost:{port}', + mounts=[], + exposed_ports=[ + ExposedPort( + name='AGENT_SERVER', + description='Agent server', + container_port=8000, + ) + ], + health_check_path='/health', + httpx_client=MagicMock(spec=httpx.AsyncClient), + max_num_sandboxes=5, + docker_client=mock_docker_client, + use_host_network=False, + ) + + # Start sandbox + import asyncio + + asyncio.get_event_loop().run_until_complete(service.start_sandbox()) + + # Verify docker was called with bridge network mode (network_mode=None) + call_kwargs = mock_docker_client.containers.run.call_args[1] + assert call_kwargs['network_mode'] is None + # Port mappings should be present in bridge mode + assert call_kwargs['ports'] is not None + assert 8000 in call_kwargs['ports'] diff --git a/tests/unit/app_server/test_docker_sandbox_service.py b/tests/unit/app_server/test_docker_sandbox_service.py index 23a6d51b04..f6ae716eef 100644 --- a/tests/unit/app_server/test_docker_sandbox_service.py +++ b/tests/unit/app_server/test_docker_sandbox_service.py @@ -1254,6 +1254,59 @@ class TestDockerSandboxServiceInjector: injector = DockerSandboxServiceInjector(use_host_network=True) assert injector.use_host_network is True + def test_use_host_network_from_agent_server_env_var(self): + """Test that AGENT_SERVER_USE_HOST_NETWORK env var enables host network mode.""" + import os + from unittest.mock import patch + + from openhands.app_server.sandbox.docker_sandbox_service import ( + DockerSandboxServiceInjector, + ) + + env_vars = { + 'AGENT_SERVER_USE_HOST_NETWORK': 'true', + } + + with patch.dict(os.environ, env_vars, clear=True): + injector = DockerSandboxServiceInjector() + assert injector.use_host_network is True + + def test_use_host_network_env_var_accepts_various_true_values(self): + """Test that use_host_network accepts various truthy values.""" + import os + from unittest.mock import patch + + from openhands.app_server.sandbox.docker_sandbox_service import ( + DockerSandboxServiceInjector, + ) + + for true_value in ['true', 'TRUE', 'True', '1', 'yes', 'YES', 'Yes']: + env_vars = {'AGENT_SERVER_USE_HOST_NETWORK': true_value} + with patch.dict(os.environ, env_vars, clear=True): + injector = DockerSandboxServiceInjector() + assert injector.use_host_network is True, ( + f'Failed for value: {true_value}' + ) + + def test_use_host_network_env_var_defaults_to_false(self): + """Test that unset or empty env var defaults to False.""" + import os + from unittest.mock import patch + + from openhands.app_server.sandbox.docker_sandbox_service import ( + DockerSandboxServiceInjector, + ) + + # Empty environment + with patch.dict(os.environ, {}, clear=True): + injector = DockerSandboxServiceInjector() + assert injector.use_host_network is False + + # Empty string + with patch.dict(os.environ, {'AGENT_SERVER_USE_HOST_NETWORK': ''}, clear=True): + injector = DockerSandboxServiceInjector() + assert injector.use_host_network is False + class TestDockerSandboxServiceInjectorFromEnv: """Test cases for DockerSandboxServiceInjector environment variable configuration."""