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

New image structure based on design document #166

Merged
merged 21 commits into from
Sep 28, 2021

Conversation

humitos
Copy link
Member

@humitos humitos commented Mar 16, 2021

Dockerfile(s) required to build the new build images (ubuntu20-*) that use asdf to install languages requirements (python, node, rust, go)

Design documents:

Implementation:

How to build this image

  1. Clone this PR
  2. run docker build -t readthedocs/build:ubuntu-20.04 .

TODO

Closes #170
Closes #60
Closes #158
Closes #107
Closes #90
Closes #86
Closes #108
Closes #91
Closes #64
Closes #130
Closes #177
Closes #176
Closes #48

@humitos humitos force-pushed the humitos/build-images-design-doc branch from b063da2 to 966c509 Compare March 17, 2021 09:07
@humitos humitos requested a review from a team August 30, 2021 10:57
Copy link
Contributor

@astrojuanlu astrojuanlu left a comment

Choose a reason for hiding this comment

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

I wonder if it would be a good moment to try to optimize the Dockerfile: https://www.fromlatest.io/

in particular:

  • Couple apt-get update with rm -rf /var/lib/apt/lists/* in the same layer (although I don't think that plays along well with our multi-layered apt-get install - we would need to do update && install && rm -rf /var/lib/... in every layer, which will slow down the builds a bit)
  • Use --no-install-recommends to be more explicit about what are we installing and why

Dockerfile Outdated Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
@humitos
Copy link
Member Author

humitos commented Aug 31, 2021

Good feedback here!

Couple apt-get update with rm -rf /var/lib/apt/lists/* in the same layer (although I don't think that plays along well with our multi-layered apt-get install - we would need to do update && install && rm -rf /var/lib/... in every layer, which will slow down the builds a bit)

docker run -u root -it readthedocs/build:ubuntu20 /bin/bash
root@42d3d0f8fe1a:/home/docs# du -sh /var/lib/apt/lists/
29M	/var/lib/apt/lists/

I don't think we are spending a lot of disk space. However, I agree it could be a good optimization. As you already noticed, we will need to call apt-get update on each apt-get install command that we run.

On the other hands, packages (.deb files) are cleaned up automatically when using official Ubuntu images because they comes with a post-install hook for this:

docs@927b877e91c3:~$ cat /etc/apt/apt.conf.d/docker-clean
DPkg::Post-Invoke { "rm -f /var/cache/apt/archives/*.deb /var/cache/apt/archives/partial/*.deb /var/cache/apt/*.bin || true"; };
APT::Update::Post-Invoke { "rm -f /var/cache/apt/archives/*.deb /var/cache/apt/archives/partial/*.deb /var/cache/apt/*.bin || true"; };
Dir::Cache::pkgcache ""; Dir::Cache::srcpkgcache "";
docs@927b877e91c3:~$

Use --no-install-recommends to be more explicit about what are we installing and why

I'm not sure about this one. I think it's a good idea to know exactly what we are installing. However, if add this option now, I wonder if all the PDFs will keep building correctly (e.g. the right font, or stylesheets).

humitos added a commit to readthedocs/readthedocs.org that referenced this pull request Sep 1, 2021
Minimal implementation for POC of
#8447

It uses a Feature flag for now as a way to select the new
`readthedocs/build:ubuntu20` image and install Python versions via `asdf`
(readthedocs/readthedocs-docker-images#166)

MinIO requires a new bucket called `languages` with a pre-compiled Python 3.9.6
version to work (*) (this version is hardcoded for now). However, if a different
version is selected it will be downloaded from official mirrors, installed and
used.

Build times on `latest` version for `test-build`:

* using the new image + cached Python version: 112s
* using the new image + non cached Python version: 288s
* using old image (current production): 87s

> Note that all the parsing of the Config File to support `build.os` and
> `build.languages` is not included in this PR on purpose. That work can be
> split as a separate work and done in parallel with the rest of work required
> here.

(*) to pre-compile a Python version:

```bash
docker run -it readthedocs/build:ubuntu20 /bin/bash

asdf install python 3.9.6
asdf global python 3.9.6
python -m pip install -U pip setuptools virtualenv
cd /home/docs/.asdf/installs/python
tar -cfvz ubuntu20-python-3.9.6.tar.gz 3.9.6

docker cp <container id>:/home/docs/.asdf/installs/python/ubuntu20-python-3.9.6.tar.gz .
```

and upload the .tar.gz file to MinIO `languages` bucket using the web interface
Copy link
Contributor

@agjohnson agjohnson left a comment

Choose a reason for hiding this comment

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

So much red!

I don't quite follow the need for additional version information on the build however, it seems like we only wanted a ubuntu20 and ubuntu22 image. The additional granularity probably won't be used anywhere.

CONTRIBUTING.rst Outdated Show resolved Hide resolved
CONTRIBUTING.rst Outdated Show resolved Hide resolved
@humitos
Copy link
Member Author

humitos commented Sep 23, 2021

@agjohnson I addressed all your comments and made the tests run on CircleCI. Is there any blocker to merge this PR?

Copy link
Member

@ericholscher ericholscher left a comment

Choose a reason for hiding this comment

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

This looks like a good step forward to me. I didn't look super deeply into the DockerFile packages that are installed, but assume we've tested those.

CONTRIBUTING.rst Show resolved Hide resolved
CONTRIBUTING.rst Outdated Show resolved Hide resolved
@@ -1,11 +1,14 @@
# Read the Docs - Environment base
FROM ubuntu:20.04
LABEL mantainer="Read the Docs <support@readthedocs.com>"
LABEL version="ubuntu-20.04-2021.09.23"
Copy link
Member

Choose a reason for hiding this comment

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

Can we automate this with date or similar? Do we want to?

Copy link
Member Author

Choose a reason for hiding this comment

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

I haven't try that. Honestly, I don't think LABEL version is strictly needed; but I thought it could be useful to use with docker inspect to know what's the version you have downloaded:

▶ docker inspect c54fcbdfb6c8 | jq '.[0].Config.Labels.version'
"ubuntu-20.04-2021.09.23"

It's a nice to have to me. If we can automate it, it would be 💯

Comment on lines -26 to -40

`readthedocs/build:5.0`
``stable``
Ubuntu 18.04 supporting Python 2.7, 3.6, 3.7 and pypy3.5-7.0.0.
This is the **stable** image supported by Read the Docs.

`readthedocs/build:6.0`
``latest``
Ubuntu 18.04 supporting Python 2.7, 3.5, 3.6, 3.7, 3.8 and PyPy3.5-7.0.0.
This is the **latest** default image used for documentation builds and supported by Read the Docs.

`readthedocs/build:7.0`
``testing``
Ubuntu 18.04 supporting Python 2.7, 3.5, 3.6, 3.7, 3.8, 3.9, and PyPy3.5-7.0.0.
Available for public usage as **testing** image. You should expect some breaking changes here.
Copy link
Member

Choose a reason for hiding this comment

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

Are these not still important to document, since they are still in use?

Copy link
Member Author

Choose a reason for hiding this comment

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

This was a little (too much) opinionated 😄

  • I think we shouldn't explain these images here "to the final user"
  • I think users wanting to use these images should be pointed to the Read the Docs documentation instead (config file)
  • I would try to keep this repository hidden from the regular users because it only generates confusion
  • I wanted to only expose the newer image to avoid people finding this and using the old images

That said, it may make sense to remove the phrase "Available for public usage as build.os: ubuntu-20.04" as well.

Copy link
Member

Choose a reason for hiding this comment

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

I would try to keep this repository hidden from the regular users because it only generates confusion

Perhaps we should make it private, then? And move these docs to our public-facing docs & config file?

Copy link
Member Author

Choose a reason for hiding this comment

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

Make it private may be too much. I think we will lose some issues/prs references linked everywhere.

Definitely, how to use the docker image and what it contains should be documented in the Read the Docs documentation for the config file.

Co-authored-by: Eric Holscher <25510+ericholscher@users.noreply.github.com>
@humitos humitos merged commit dda2cad into master Sep 28, 2021
@humitos humitos deleted the humitos/build-images-design-doc branch September 28, 2021 11:42
@pradyunsg
Copy link

Hurray! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment