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

build: Update xml2rfc docker image #849

Merged
merged 7 commits into from
Aug 9, 2022
Merged

Conversation

kesara
Copy link
Member

@kesara kesara commented Aug 1, 2022

  • build: Change docker publish workflow to run manually [skip ci]
  • build: Install xml2rfc from PyPI [skip ci]
  • build: Add additional tools [skip ci]

Fixes #846

@kesara kesara requested a review from NGPixel August 1, 2022 06:41
Dockerfile Outdated Show resolved Hide resolved
@NGPixel
Copy link
Member

NGPixel commented Aug 1, 2022

I would suggest building the dev image on top of the xml2rfc-base image (used to generate the docs during CI):
https://github.com/ietf-tools/xml2rfc/blob/main/docker/base.Dockerfile

which is published at ghcr.io/ietf-tools/xml2rfc-base:latest

Basically add back all the Python versions I removed from the base image + additional dev tools

@kesara kesara marked this pull request as draft August 2, 2022 21:59
@kesara kesara force-pushed the fix/docker-update branch 3 times, most recently from 287c61d to 90aa6aa Compare August 2, 2022 23:34
@kesara kesara marked this pull request as ready for review August 2, 2022 23:46
@kesara kesara force-pushed the fix/docker-update branch 3 times, most recently from 4f33901 to 7caa642 Compare August 3, 2022 00:01
docker/dev.Dockerfile Outdated Show resolved Hide resolved
* Remove redundent docker publish workflow.
* Add instructions to build and publish dev docker image.
* Rename build-base workflow.
@kesara kesara merged commit d111945 into ietf-tools:main Aug 9, 2022
@kesara kesara deleted the fix/docker-update branch August 9, 2022 22:52
Comment on lines +53 to +58
COPY README.md .
COPY LICENSE .
COPY Makefile .
COPY configtest.py .
COPY xml2rfc ./xml2rfc
Copy link
Contributor

Choose a reason for hiding this comment

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

These steps, particularly the last one, involve files that will change frequently. If your goal is to improve layer caching, then moving those steps to the end will improve that greatly.

Comment on lines +64 to +70
RUN rm roboto-mono.zip
RUN apt-get remove --purge -y software-properties-common wget unzip
RUN apt-get autoclean
RUN apt-get clean
RUN pip3 uninstall -y decorator dict2xml pypdf2
RUN rm setup.py Makefile configtest.py requirements.txt
Copy link
Contributor

Choose a reason for hiding this comment

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

Running these as discrete steps won't reduce the size of the docker image, it only increases it. I'd cut these.

I'm interpreting this docker image as including a test, which is not really something that a dockerfile is well suited toward. If you want to test, maybe you can run these steps in the workflow instead.

Comment on lines +40 to +42
RUN apt-get remove --purge -y software-properties-common
RUN apt-get autoclean
RUN apt-get clean
Copy link
Contributor

Choose a reason for hiding this comment

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

To save space, cleanup should be run in the same step as the files that are being created.

Also, I wouldn't bother removing software-properties-common; it's just not that much space to save.

Comment on lines +44 to +46
RUN wget -q https://noto-website-2.storage.googleapis.com/pkgs/Noto-unhinted.zip
RUN unzip -q Noto-unhinted.zip -d ~/.fonts/opentype/
Copy link
Contributor

Choose a reason for hiding this comment

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

Running these as a single RUN step will reduce the number and size of the layers that are created.

Comment on lines +47 to +50
RUN unzip -q roboto-mono.zip -d ~/.fonts/opentype/
RUN ln -sf ~/.fonts/opentype/*.[to]tf /usr/share/fonts/truetype/
RUN fc-cache -f
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here.

@kesara
Copy link
Member Author

kesara commented Aug 10, 2022

@martinthomson, There's a workflow now that builds xml2rfc-dev image: https://github.com/ietf-tools/xml2rfc/pkgs/container/xml2rfc-dev

I have updated ./run to make use of that.
So anyone with amd64 or arm64 will not have to build the xml2rfc-dev image anymore.

@martinthomson
Copy link
Contributor

Thanks @kesara, that should work out to be a lot faster. I made a few suggestions that might help with image size, for those of us who live on the wrong side of the planet.

@kesara
Copy link
Member Author

kesara commented Aug 10, 2022

@martinthomson, Thanks for the suggestions and I'm looking to implement them.
I need to learn more about how docker layers get created.

@martinthomson
Copy link
Contributor

Think of each command in the dockerfile as making some changes on top of the previous state. Each change needs to be saved and adds to the size of the final image. If you create smaller changes or fewer changes, the final image can be a lot smaller.

This means consolidating actions into a single step is good. It also means that if you do something that creates temporary files (like entries in a cache), cleaning up those files during the step will produce less information to save for that step.

Of course, you want to create checkpoints at certain stages: because layers are cached, if you can find some steps that don't change often, creating a layer means that you can reuse that state in future builds. So don't just lump everything together either. Build the image in stages: the most stable stuff comes first, the least stable stuff comes last.

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.

Docker image performance
4 participants