Skip to content
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

Get RUN_AS_DEVIN working with app sandbox #1426

Merged
merged 19 commits into from Apr 30, 2024

Conversation

xingyaoww
Copy link
Collaborator

@xingyaoww xingyaoww commented Apr 28, 2024

The reason why RUN_AS_DEVIN was not working properly is that SANDBOX_USER_ID is not properly setted up. os.getpid() from the app container will return UID=0, which creates the issue of #936.

This PR fix:

  • Allowing RUN_AS_DEVIN with app container. The caveat is that the user need to pass-in their UID when starting the app container. I have updated the readme accordingly
docker run \
    -e LLM_API_KEY \
    -e WORKSPACE_MOUNT_PATH=$WORKSPACE_BASE \
    -v $WORKSPACE_BASE:/opt/workspace_base \
    -e SANDBOX_USER_ID=$(id -u) \
    -v /var/run/docker.sock:/var/run/docker.sock \
    -p 4000:3000 \
    --add-host host.docker.internal=host-gateway \
    ghcr.io/opendevin/opendevin:latest
  • Make SANDBOX_USER_ID consistent: the app will run with a user with UID of SANDBOX_USER_ID, which won't cause a permission issue in writing to workspace; the sandbox will run with a UID of SANDBOX_USER_ID as well.

@li-boxuan
Copy link
Collaborator

li-boxuan commented Apr 28, 2024

Seems integration tests have captured a bug?https://github.com/OpenDevin/OpenDevin/actions/runs/8866160837/job/24343257827?pr=1426#step:6:66:

chmod: changing permissions of 'hello.sh': Operation not permitted

@xingyaoww
Copy link
Collaborator Author

Good catch! Seems like a mounting issue, already pushed a fix see if we can pass that
moby/moby#2259 (comment)

containers/app/Dockerfile Outdated Show resolved Hide resolved
containers/app/entrypoint.sh Outdated Show resolved Hide resolved
Copy link
Collaborator

@rbren rbren left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I get why this is necessary--we have to plumb the user's ID into both the app container and the sandbox to keep file ownership/permissions consistent. It's a lot to manage but I can't think of a better solution :/

The networking stuff here is what concerns me most though--I definitely don't think we should be changing permissions on docker.sock

@codecov-commenter
Copy link

codecov-commenter commented Apr 30, 2024

Codecov Report

Attention: Patch coverage is 66.66667% with 2 lines in your changes are missing coverage. Please review.

❗ No coverage uploaded for pull request base (main@31c1a2d). Click here to learn what that means.

❗ Current head b4d74c6 differs from pull request most recent head ec8e5ca. Consider uploading reports for the commit ec8e5ca to get more accurate results

Files Patch % Lines
opendevin/sandbox/docker/ssh_box.py 66.66% 2 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1426   +/-   ##
=======================================
  Coverage        ?   58.46%           
=======================================
  Files           ?       82           
  Lines           ?     3431           
  Branches        ?        0           
=======================================
  Hits            ?     2006           
  Misses          ?     1425           
  Partials        ?        0           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@xingyaoww xingyaoww force-pushed the run-as-devin-and-network-host branch from b4d74c6 to ec8e5ca Compare April 30, 2024 07:54
@xingyaoww xingyaoww changed the title Get RUN_AS_DEVIN and network=host working with app sandbox Get RUN_AS_DEVIN working with app sandbox Apr 30, 2024
@@ -42,8 +42,21 @@ jobs:
username: ${{ github.repository_owner }}
password: ${{ secrets.GITHUB_TOKEN }}

- name: Delete huge unnecessary tools folder
run: rm -rf /opt/hostedtoolcache
- name: Free Disk Space (Ubuntu)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we really need to fix the underlying issue here 😅

@rbren
Copy link
Collaborator

rbren commented Apr 30, 2024

Thanks for doing this!

@rbren rbren merged commit ccbbaba into OpenDevin:main Apr 30, 2024
44 checks passed
rbren added a commit that referenced this pull request May 1, 2024
rbren added a commit that referenced this pull request May 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants