Skip to content

fix: workspace folder permission & app container cannot access client API #3300

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 24 commits into from
Aug 8, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
fa71d50
also copy over pyproject and poetry lock
xingyaoww Aug 8, 2024
5fdd04b
add missing readme
xingyaoww Aug 8, 2024
e8556cd
remove extra git config init since it is already done in client.py
xingyaoww Aug 8, 2024
bef58ce
only chown if the /workspace dir does not exists
xingyaoww Aug 8, 2024
1dc6412
Revert "remove extra git config init since it is already done in clie…
xingyaoww Aug 8, 2024
8bd23e6
remove extra git config init since it is already done in client.py
xingyaoww Aug 8, 2024
7e92201
fix test runtime
xingyaoww Aug 8, 2024
563198e
print container log while reconnecting
xingyaoww Aug 8, 2024
96756a5
print log in more readable format
xingyaoww Aug 8, 2024
ee15525
print log in more readable format
xingyaoww Aug 8, 2024
6415384
increase lines
xingyaoww Aug 8, 2024
782ded9
clean up sandbox and ssh related stuff
xingyaoww Aug 8, 2024
67806b4
Merge commit '782ded98a488660aaa14e6ef1808ea40734c66ae' into xw/fix-g…
xingyaoww Aug 8, 2024
352728c
remove ssh hostname
xingyaoww Aug 8, 2024
c3b395c
remove ssh hostname
xingyaoww Aug 8, 2024
645d957
Merge commit 'c3b395c52806a4eaf1939888071bef9c3e5a4c40' into xw/fix-g…
xingyaoww Aug 8, 2024
52af538
fix docker app cannot access runtime API issue
xingyaoww Aug 8, 2024
1ac8109
remove ssh password
xingyaoww Aug 8, 2024
9e7db4e
Merge commit '1ac81094bc503075e16f6eb6450060eba3a1abea' into xw/fix-g…
xingyaoww Aug 8, 2024
bd0f1d7
API HOSTNAME should be pre-fixed with SANDBOX
xingyaoww Aug 8, 2024
27da169
update config
xingyaoww Aug 8, 2024
6cc4d6d
fix typo that breaks the test
xingyaoww Aug 8, 2024
c8d9dc4
Merge commit '6cc4d6d57b569ace4f30e5e7a4efc628523170b4' into xw/fix-g…
xingyaoww Aug 8, 2024
5702c6e
Merge branch 'main' into xw/fix-git-ownership
xingyaoww Aug 8, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions containers/app/Dockerfile
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 2 additions & 0 deletions opendevin/core/config.py
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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
)
Expand Down
23 changes: 14 additions & 9 deletions opendevin/runtime/client/client.py
Original file line number Diff line number Diff line change
Expand Up @@ -128,14 +128,19 @@ def _init_user(self, username: str, user_id: int) -> None:
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,
)
Expand Down Expand Up @@ -381,11 +386,11 @@ async def write(self, action: FileWriteAction) -> Observation:
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}')
Expand Down
29 changes: 15 additions & 14 deletions opendevin/runtime/client/runtime.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@
)
from opendevin.events.action.action import Action
from opendevin.events.observation import (
CmdOutputObservation,
ErrorObservation,
NullObservation,
Observation,
Expand Down Expand Up @@ -54,7 +53,7 @@ def __init__(
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 = (
Expand Down Expand Up @@ -98,8 +97,6 @@ async def ainit(self, env_vars: dict[str, str] | None = None):
)
logger.info(f'Container initialized with env vars: {env_vars}')

await self._init_git_config()

@staticmethod
def _init_docker_client() -> docker.DockerClient:
try:
Expand Down Expand Up @@ -183,16 +180,6 @@ async def _init_container(
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 "[email protected]"'
)
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:
Expand All @@ -205,6 +192,20 @@ async def _ensure_session(self):
)
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:
Expand Down
7 changes: 5 additions & 2 deletions tests/unit/test_runtime.py
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down