Skip to content

Extra changes for isolated builders on local development - #316

Open
humitos wants to merge 11 commits into
mainfrom
humitos/isolated-builders
Open

Extra changes for isolated builders on local development#316
humitos wants to merge 11 commits into
mainfrom
humitos/isolated-builders

Conversation

@humitos

@humitos humitos commented Jul 16, 2026

Copy link
Copy Markdown
Member
  • Mount rclone inside the container: this is required because we are uploading the artifacts from inside the builder now. The readthedocs/build:<os> image doesn't have rclone installed. Instead of rebuilding all those images, the easiest solution is to install rclone in the image spinning up the Docker container and mount the binary there. NOTE: we can find another way to do this later, but I want to have a solution in place for now. This is easiest one that I found.
  • Install rclone inside the Docker image
  • Mount Docker socket: this is required to allow the container to kill Docker containers
  • Pass extra environment variables
  • Install ssh: to be able to clone private repositories

@humitos
humitos marked this pull request as ready for review July 20, 2026 11:42
@humitos
humitos requested a review from a team as a code owner July 20, 2026 11:42
@humitos
humitos requested a review from stsewd July 20, 2026 11:42
@humitos humitos changed the title Mount the host's rclone binary into the build container Extra changes for isolated builders on local development Jul 23, 2026
@humitos
humitos requested a review from agjohnson July 28, 2026 14:43
@humitos

humitos commented Aug 6, 2026

Copy link
Copy Markdown
Member Author

@stsewd this is the only PR that's missing to merge to have the new builder fully working locally. With that, running inv docker.up + adding a project to ISOLATED_BUILDER feature should run those project's builds in the isolated-builder container.

Comment on lines +45 to +55
# 1b. Copy the image's rclone onto the host-visible bind-mount.
# The runner syncs artifacts to storage with rclone from inside the
# build container, and the worker bind-mounts the binary in rather
# than each build downloading its own. In production Packer bakes
# rclone into the AMI, so the worker mounts a real host path.
#
# Here the worker runs in THIS container, but ``docker run -v`` is
# resolved by the HOST daemon (docker-out-of-docker via the mounted
# socket) — so /usr/local/bin/rclone from this image is invisible to
# it. Copying into $RCLONE_DIR (a host bind-mount, see compose) puts
# the binary on the host filesystem, same trick as $RUNNER_VENV.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

In general, I think we need to tune the pattern that the llms use to generate comments around code or do more editing. I keep seeing this level of commenting generated and it makes me have to read through a lot of not super helpful comments -- and so I'm going to miss what is important if it's there. In this case, explaining docker and bind mounts is noisy information.

Suggested change
# 1b. Copy the image's rclone onto the host-visible bind-mount.
# The runner syncs artifacts to storage with rclone from inside the
# build container, and the worker bind-mounts the binary in rather
# than each build downloading its own. In production Packer bakes
# rclone into the AMI, so the worker mounts a real host path.
#
# Here the worker runs in THIS container, but ``docker run -v`` is
# resolved by the HOST daemon (docker-out-of-docker via the mounted
# socket) — so /usr/local/bin/rclone from this image is invisible to
# it. Copying into $RCLONE_DIR (a host bind-mount, see compose) puts
# the binary on the host filesystem, same trick as $RUNNER_VENV.
# This hack mounts the host rclone binary into the container so that it
# doesn't need to be installed into the container each build.

Comment thread dockerfiles/entrypoints/isolated-builder.sh Outdated
- No need for dual venvs
- Install rclone inside the Docker image
@humitos

humitos commented Aug 18, 2026

Copy link
Copy Markdown
Member Author

I updated this PR with the latest updates to use docker exec instead as we talked in Slack.

@stsewd stsewd left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What's the reason we need to share these directories with the host?

- ${PWD}/${RTDDEV_PATH_BUILDER:-../readthedocs-builder}/venv:/usr/src/builder/venv
- ${PWD}/${RTDDEV_PATH_BUILDER:-../readthedocs-builder}/uv-python:/usr/src/builder/uv-python

- RTD_S3_STATIC_STORAGE_BUCKET
- RTD_AWS_S3_REGION_NAME
- RTD_OPENAI_API_KEY
- RTD_BUILDER_TOKEN

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks like we no longer pass this env var, but the script still checks for it.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Good catch. We don't really need this env var locally because we require to have the readthedocs-builder cloned. We can remove it from the script as well, I guess.

@agjohnson agjohnson left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is easier to follow with the changes to use docker exec 👍

@humitos

humitos commented Aug 20, 2026

Copy link
Copy Markdown
Member Author

What's the reason we need to share these directories with the host?

These are leftovers from before. I will remove them.

- trim down comments
- remove unused variables
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.

3 participants