From 2e6b08db4fd12eadb993e32b418b62c289c34ffa Mon Sep 17 00:00:00 2001 From: Xingyao Wang Date: Fri, 9 Aug 2024 07:28:34 +0800 Subject: [PATCH] fix: workspace folder permission & app container cannot access client API (#3300) * also copy over pyproject and poetry lock * add missing readme * remove extra git config init since it is already done in client.py * only chown if the /workspace dir does not exists * Revert "remove extra git config init since it is already done in client.py" This reverts commit e8556cd76dcb1720b33f5e06904c56efda2e7d9f. * remove extra git config init since it is already done in client.py * fix test runtime * print container log while reconnecting * print log in more readable format * print log in more readable format * increase lines * clean up sandbox and ssh related stuff * remove ssh hostname * remove ssh hostname * fix docker app cannot access runtime API issue * remove ssh password * API HOSTNAME should be pre-fixed with SANDBOX * update config * fix typo that breaks the test --- containers/app/Dockerfile | 1 + opendevin/core/config.py | 2 ++ opendevin/runtime/client/client.py | 23 ++++++++++++++--------- opendevin/runtime/client/runtime.py | 29 +++++++++++++++-------------- tests/unit/test_runtime.py | 7 +++++-- 5 files changed, 37 insertions(+), 25 deletions(-) diff --git a/containers/app/Dockerfile b/containers/app/Dockerfile index efd36fd036..36ed2fbf06 100644 --- a/containers/app/Dockerfile +++ b/containers/app/Dockerfile @@ -37,6 +37,7 @@ ARG OPEN_DEVIN_BUILD_VERSION #re-declare for this section ENV RUN_AS_DEVIN=true # A random number--we need this to be different from the user's UID on the host machine ENV OPENDEVIN_USER_ID=42420 +ENV SANDBOX_API_HOSTNAME=host.docker.internal ENV USE_HOST_NETWORK=false ENV WORKSPACE_BASE=/opt/workspace_base ENV OPEN_DEVIN_BUILD_VERSION=$OPEN_DEVIN_BUILD_VERSION diff --git a/opendevin/core/config.py b/opendevin/core/config.py index 6d5d55637f..46217d3784 100644 --- a/opendevin/core/config.py +++ b/opendevin/core/config.py @@ -145,6 +145,7 @@ class SandboxConfig(metaclass=Singleton): """Configuration for the sandbox. Attributes: + api_hostname: The hostname for the EventStream Runtime API. container_image: The container image to use for the sandbox. user_id: The user ID for the sandbox. timeout: The timeout for the sandbox. @@ -164,6 +165,7 @@ class SandboxConfig(metaclass=Singleton): Default is None for general purpose browsing. Check evaluation/miniwob and evaluation/webarena for examples. """ + api_hostname: str = 'localhost' container_image: str = ( 'ubuntu:22.04' # default to ubuntu:22.04 for eventstream runtime ) diff --git a/opendevin/runtime/client/client.py b/opendevin/runtime/client/client.py index 572ba447eb..a1883bb829 100644 --- a/opendevin/runtime/client/client.py +++ b/opendevin/runtime/client/client.py @@ -128,14 +128,19 @@ class RuntimeClient: raise RuntimeError(f'Failed to add sudoer: {output.stderr.decode()}') logger.debug(f'Added sudoer successfully. Output: [{output.stdout.decode()}]') - # Add user and change ownership of the initial working directory + # Add user and change ownership of the initial working directory if it doesn't exist + command = ( + f'useradd -rm -d /home/{username} -s /bin/bash ' + f'-g root -G sudo -u {user_id} {username}' + ) + + if not os.path.exists(self.initial_pwd): + command += f' && mkdir -p {self.initial_pwd}' + command += f' && chown -R {username}:root {self.initial_pwd}' + command += f' && chmod g+s {self.initial_pwd}' + output = subprocess.run( - ( - f'useradd -rm -d /home/{username} -s /bin/bash ' - f'-g root -G sudo -u {user_id} {username} &&' - f'chown -R {username}:root {self.initial_pwd} && ' - f'chmod g+s {self.initial_pwd}' - ), + command, shell=True, capture_output=True, ) @@ -381,11 +386,11 @@ class RuntimeClient: assert file_stat is not None # restore the original file permissions if the file already exists os.chmod(filepath, file_stat.st_mode) - os.chown(filepath, file_stat.st_uid, ROOT_GID) + os.chown(filepath, file_stat.st_uid, file_stat.st_gid) else: # set the new file permissions if the file is new os.chmod(filepath, 0o644) - os.chown(filepath, self.user_id, ROOT_GID) + os.chown(filepath, self.user_id, self.user_id) except FileNotFoundError: return ErrorObservation(f'File not found: {filepath}') diff --git a/opendevin/runtime/client/runtime.py b/opendevin/runtime/client/runtime.py index d646b76fcc..e8733a7134 100644 --- a/opendevin/runtime/client/runtime.py +++ b/opendevin/runtime/client/runtime.py @@ -22,7 +22,6 @@ from opendevin.events.action import ( ) from opendevin.events.action.action import Action from opendevin.events.observation import ( - CmdOutputObservation, ErrorObservation, NullObservation, Observation, @@ -54,7 +53,7 @@ class EventStreamRuntime(Runtime): config, event_stream, sid, plugins ) # will initialize the event stream self._port = find_available_tcp_port() - self.api_url = f'http://localhost:{self._port}' + self.api_url = f'http://{self.config.sandbox.api_hostname}:{self._port}' self.session: Optional[aiohttp.ClientSession] = None self.instance_id = ( @@ -98,8 +97,6 @@ class EventStreamRuntime(Runtime): ) logger.info(f'Container initialized with env vars: {env_vars}') - await self._init_git_config() - @staticmethod def _init_docker_client() -> docker.DockerClient: try: @@ -183,16 +180,6 @@ class EventStreamRuntime(Runtime): await self.close(close_client=False) raise e - async def _init_git_config(self): - action = CmdRunAction( - 'git config --global user.name "opendevin" && ' - 'git config --global user.email "opendevin@all-hands.dev"' - ) - logger.info(f'Setting git config: {action}') - obs: Observation = await self.run_action(action) - assert isinstance(obs, CmdOutputObservation) - assert obs.exit_code == 0, f'Failed to set git config: {obs}' - async def _ensure_session(self): await asyncio.sleep(1) if self.session is None or self.session.closed: @@ -205,6 +192,20 @@ class EventStreamRuntime(Runtime): ) async def _wait_until_alive(self): logger.info('Reconnecting session') + container = self.docker_client.containers.get(self.container_name) + # print logs + _logs = container.logs(tail=10).decode('utf-8').split('\n') + # add indent + _logs = '\n'.join([f' |{log}' for log in _logs]) + logger.info( + '\n' + + '-' * 30 + + 'Container logs (last 10 lines):' + + '-' * 30 + + f'\n{_logs}' + + '\n' + + '-' * 90 + ) async with aiohttp.ClientSession() as session: async with session.get(f'{self.api_url}/alive') as response: if response.status == 200: diff --git a/tests/unit/test_runtime.py b/tests/unit/test_runtime.py index 0e1a402caf..9614f5adbf 100644 --- a/tests/unit/test_runtime.py +++ b/tests/unit/test_runtime.py @@ -1265,9 +1265,12 @@ async def test_keep_prompt(temp_dir): @pytest.mark.asyncio -async def test_git_operation(temp_dir, box_class): +async def test_git_operation(box_class): + # do not mount workspace, since workspace mount by tests will be owned by root + # while the user_id we get via os.getuid() is different from root + # which causes permission issues runtime = await _load_runtime( - temp_dir, + temp_dir=None, box_class=box_class, # Need to use non-root user to expose issues run_as_devin=True,