OpenHands icon indicating copy to clipboard operation
OpenHands copied to clipboard

Update Makefile to skip sandbox Docker pull

Open risingsunomi opened this issue 1 year ago • 5 comments

Adding a build-nodocker option to make OpenDevin not pull the sandbox Docker. This needed in the dockerizing process for OpenDevin I have been working on here https://github.com/risingsunomi/OpenDevin-Docker

We cannot start a docker in a docker so will need to remove it.

risingsunomi avatar Apr 12 '24 21:04 risingsunomi

I agree with the need, not fully onboard with the implementation.

If you make a copy of the existing make target and skip just the docker pull step, every time someone makes changes to make build they'll have to remember to make the same change to build-nodocker, and that's easy to miss.

Is there a way to make the regular make build skip a step? Like perhaps setting an env var. making things up: $ SKIP_SANDBOX_IMAGE_PULL=1 make build

foragerr avatar Apr 14 '24 04:04 foragerr

Maybe -

ifndef SKIP_SANDBOX_IMAGE_PULL
	@$(MAKE) -s pull-docker-image
endif

(untested)

foragerr avatar Apr 14 '24 04:04 foragerr

Maybe -

ifndef SKIP_SANDBOX_IMAGE_PULL
	@$(MAKE) -s pull-docker-image
endif

(untested)

I agree, this implementation is much better. Have it control by env we can possibly use the same env for other things. Will test this and see.

risingsunomi avatar Apr 14 '24 04:04 risingsunomi

You can implement Docker-in-Docker with this project. But I as someone who has is not going to recommend not having control of when...just as a major correction about that statement, @risingsunomi . It can be done but it required major modification to the way the Docker image is built (in multiple steps, committing image changes from a builder container, and using a local registry), so what that ultimately means from my experiment is it must be done at container runtime and it is not recommended to pull it by default, and instead at runtime from /docker-entrypoint.sh with a Docker-in-Docker privileged solution. Which in itself is a good case for SKIP_SANDBOX_IMAGE_PULL to avoid the type of complexity I had to deal with.

I was able to get DinD working without modifying any of the repository code, but it did require restructuring of the way that the image is built (as a builder container must be able to run, and multistage builds with a container registry must be implemented, and have to avoid using docker compose until when everything is built and the runtime image is cooked successfully, a couple thousand lines of shell code had to be written, ouch). Privileged containers cannot happen at BuildKit time. Which is the entire reason that solution is extremely complex. I've done so.

Major restructure of the way the Docker image is built

  • use a local container registry via registry:2 docker image.
            docker run -it \
                --name "registry" \
                --network "opendevin_net" \
                --port "5000:5000" \
                --label "com.docker.stack.namespace=opendevin-admin" \
               docker.io/registry:2 || return $?
  • split the project image into 2 dockerfiles: Dockerfile.builder and Dockerfile.runtime.
FROM debian:bookworm-slim AS localhost:5000/opendevin-builder
  • installing docker in opendevin-builder image in the Dockerfile.builder by using the following RUN command (in a debian-bookworm image is where I tested this):
# I run in user mode, not as root, so I define a build argument that lets me set a username:
ARG APP_USER
ENV APP_USER ${APP_USER:-nobody}

# ... after installing base dependencies, creating user, and their home directory, etc. ...

# Setup Docker CE
RUN set -eux; \
    curl -fsSL https://download.docker.com/linux/debian/gpg | gpg --dearmor -o /usr/share/keyrings/docker-archive-keyring.gpg; \
    echo "deb [arch=amd64 signed-by=/usr/share/keyrings/docker-archive-keyring.gpg] https://download.docker.com/linux/debian $(lsb_release -cs) stable" | tee /etc/apt/sources.list.d/docker.list; \
    apt-get update -qqy ;\
    DEBIAN_FRONTEND=noninteractive apt-get install -qqy --no-install-recommends \
        docker-ce; \
    apt-get clean -y; \
    rm -rf /var/lib/apt/lists/*; \
    rm -rf /var/cache/apt; \
    usermod -aG docker $(id -un); \
    # issue: https://github.com/docker/cli/issues/4807#issuecomment-1903950217 \
    sed -i 's/ulimit -Hn/# ulimit -Hn/g' /etc/init.d/docker; \
    touch /var/run/docker.sock; \
    chown ${APP_USER}:docker /var/run/docker.sock;\ 
    chmod 4775 /var/run/docker.sock
  • Then inherit from it in opendevin-runtime:
FROM localhost:5000/opendevin-builder AS opendevin-runtime
  • allowing /docker-entrypoint.sh to accept build and run arguments. This paste was too lengthy so I won't share, but I have a custom /docker-entrypoint.sh that does that here that supports building the project while the container is running, instead of at BuildKit time where docker in docker isn't supported and docker cannot run with service docker start.

  • passing --priviledged to the docker run command in the runtime image

  • using --volume to mount /var/run/docker.sock

  • setting permissions on /var/run/docker.sock to >= 4755 and having it owned by the runtime user:docker, which is what happens in the example RUN command I gave while installing docker.

Here's some code from my project.

            docker build \
                --file "./docker/Dockerfile.builder" \
                --tag "localhost:5000/opendevin-builder:latest" \
                --label "com.docker.stack.namespace=opendevin" \
                "./docker" || return $?
            docker build \
                --file "./docker/OpenDevin/Dockerfile.runtime" \
                --tag "localhost:5000/opendevin-runtime:latest" \
                --label "com.docker.stack.namespace=opendevin" \
                "./docker/OpenDevin" || return $?
            docker run -it \
                --name "opendevin-prod" \
                --network "opendevin_net" \
                --volume "/var/run/docker.sock:/var/run/docker.sock" \
                --label "com.docker.stack.namespace=opendevin" \
                localhost:5000/opendevin-runtime:latest \
                build || return $?

The working proof of concept of a builder vs. a runtime image with a working Docker-in-Docker with zero modification to Makefile is here.

This required some extensive modification to the way the docker image itself is built.

I recommend the flag because of a different reason: control of when the Sandbox image actually gets pulled which totally would have saved me from writing a couple thousand lines of shell boilerplate and additional Dockerfile to work around the issue of not being able to DinD with --priviledged at BuildKit time. NOT because Docker-in-Docker isn't possible with this project. That is incorrect. It is, but will vastly inflate code complexity as in my experiment.

This doesn't cover all the lengths and footwork I myself had to do, but for obvious reasons related to not increasing complexity of docker solution to the level that I did, I vote yes on SKIP_SANDBOX_IMAGE_PULL, especially because I'm exhausted after my experiment doing without it!

The moral of the story is don't do what I did and a Makefile flag with a minor modification to /docker-entrypoint.sh is probably the cleanest solution I have seen to all the PR whiteout I have witnessed promoting a working docker image.

By the way I wish people would look before submitting PRs, we have #1097 #940 #377 #481 and #1023 . This is not directed at this particular PR, which makes a simple change and doesn't repeat what I've seen a lot of other PR's doing on this repository....

I think #1023 has the right idea staying a Draft and letting a group collaborate on it.... trying to implement docker for this project has become a bit of a mess! Please end the chaos! I highly recommend that if you want to work on Docker, currently the best place is in the Draft on #1023 that has been working collaboratively on one solution.

loopyd avatar Apr 14 '24 15:04 loopyd

You can implement Docker-in-Docker with this project. But I as someone who has is not going to recommend not having control of when...just as a major correction about that statement, @risingsunomi . It can be done but it required major modification to the way the Docker image is built (in multiple steps, committing image changes from a builder container, and using a local registry), so what that ultimately means from my experiment is it must be done at container runtime and it is not recommended to pull it by default, and instead at runtime from /docker-entrypoint.sh with a Docker-in-Docker privileged solution. Which in itself is a good case for SKIP_SANDBOX_IMAGE_PULL to avoid the type of complexity I had to deal with.

I was able to get DinD working without modifying any of the repository code, but it did require restructuring of the way that the image is built (as a builder container must be able to run, and multistage builds with a container registry must be implemented, and have to avoid using docker compose until when everything is built and the runtime image is cooked successfully, a couple thousand lines of shell code had to be written, ouch). Privileged containers cannot happen at BuildKit time. Which is the entire reason that solution is extremely complex. I've done so.

Major restructure of the way the Docker image is built

  • use a local container registry via registry:2 docker image.

            docker run -it \

                --name "registry" \

                --network "opendevin_net" \

                --port "5000:5000" \

                --label "com.docker.stack.namespace=opendevin-admin" \

               docker.io/registry:2 || return $?

  • split the project image into 2 dockerfiles: Dockerfile.builder and Dockerfile.runtime.

FROM debian:bookworm-slim AS localhost:5000/opendevin-builder

  • installing docker in opendevin-builder image in the Dockerfile.builder by using the following RUN command (in a debian-bookworm image is where I tested this):

# I run in user mode, not as root, so I define a build argument that lets me set a username:

ARG APP_USER

ENV APP_USER ${APP_USER:-nobody}



# ... after installing base dependencies, creating user, and their home directory, etc. ...



# Setup Docker CE

RUN set -eux; \

    curl -fsSL https://download.docker.com/linux/debian/gpg | gpg --dearmor -o /usr/share/keyrings/docker-archive-keyring.gpg; \

    echo "deb [arch=amd64 signed-by=/usr/share/keyrings/docker-archive-keyring.gpg] https://download.docker.com/linux/debian $(lsb_release -cs) stable" | tee /etc/apt/sources.list.d/docker.list; \

    apt-get update -qqy ;\

    DEBIAN_FRONTEND=noninteractive apt-get install -qqy --no-install-recommends \

        docker-ce; \

    apt-get clean -y; \

    rm -rf /var/lib/apt/lists/*; \

    rm -rf /var/cache/apt; \

    usermod -aG docker $(id -un); \

    # issue: https://github.com/docker/cli/issues/4807#issuecomment-1903950217 \

    sed -i 's/ulimit -Hn/# ulimit -Hn/g' /etc/init.d/docker; \

    touch /var/run/docker.sock; \

    chown ${APP_USER}:docker /var/run/docker.sock;\ 

    chmod 4775 /var/run/docker.sock

  • Then inherit from it in opendevin-runtime:

FROM localhost:5000/opendevin-builder AS opendevin-runtime

  • allowing /docker-entrypoint.sh to accept build and run arguments. This paste was too lengthy so I won't share, but I have a custom /docker-entrypoint.sh that does that here that supports building the project while the container is running, instead of at BuildKit time where docker in docker isn't supported and docker cannot run with service docker start.

  • passing --priviledged to the docker run command in the runtime image

  • using --volume to mount /var/run/docker.sock

  • setting permissions on /var/run/docker.sock to >= 4755 and having it owned by the runtime user:docker, which is what happens in the example RUN command I gave while installing docker.

Here's some code from my project.


            docker build \

                --file "./docker/Dockerfile.builder" \

                --tag "localhost:5000/opendevin-builder:latest" \

                --label "com.docker.stack.namespace=opendevin" \

                "./docker" || return $?

            docker build \

                --file "./docker/OpenDevin/Dockerfile.runtime" \

                --tag "localhost:5000/opendevin-runtime:latest" \

                --label "com.docker.stack.namespace=opendevin" \

                "./docker/OpenDevin" || return $?

            docker run -it \

                --name "opendevin-prod" \

                --network "opendevin_net" \

                --volume "/var/run/docker.sock:/var/run/docker.sock" \

                --label "com.docker.stack.namespace=opendevin" \

                localhost:5000/opendevin-runtime:latest \

                build || return $?

The working proof of concept of a builder vs. a runtime image with a working Docker-in-Docker with zero modification to Makefile is here.

This required some extensive modification to the way the docker image itself is built.

I recommend the flag because of a different reason: control of when the Sandbox image actually gets pulled which totally would have saved me from writing a couple thousand lines of shell boilerplate and additional Dockerfile to work around the issue of not being able to DinD with --priviledged at BuildKit time. NOT because Docker-in-Docker isn't possible with this project. That is incorrect. It is, but will vastly inflate code complexity as in my experiment.

This doesn't cover all the lengths and footwork I myself had to do, but for obvious reasons related to not increasing complexity of docker solution to the level that I did, I vote yes on SKIP_SANDBOX_IMAGE_PULL, especially because I'm exhausted after my experiment doing without it!

The moral of the story is don't do what I did and a Makefile flag with a minor modification to /docker-entrypoint.sh is probably the cleanest solution I have seen to all the PR whiteout I have witnessed promoting a working docker image.

By the way I wish people would look before submitting PRs, we have #1097 #940 #377 #481 and #1023 . This is not directed at this particular PR, which makes a simple change and doesn't repeat what I've seen a lot of other PR's doing on this repository....

I think #1023 has the right idea staying a Draft and letting a group collaborate on it.... trying to implement docker for this project has become a bit of a mess! Please end the chaos! I highly recommend that if you want to work on Docker, currently the best place is in the Draft on #1023 that has been working collaboratively on one solution.

Thank you for the explanation and correction. Will make sure to make this more of a collab effort than all these different solutions. The docker stuff is kinda wild right now and possibly too many people working on it independently. Will consolidate the effort.

risingsunomi avatar Apr 14 '24 18:04 risingsunomi

I think we can close this one now that we have a Dockerfile set up.

rbren avatar Apr 16 '24 14:04 rbren