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

Docker: Remove dist-upgrade #10822

Merged
merged 1 commit into from
May 26, 2019
Merged

Docker: Remove dist-upgrade #10822

merged 1 commit into from
May 26, 2019

Conversation

SuperSandro2000
Copy link
Contributor

Upgrading all packages is an anti pattern in docker. If there is no special reason to do that you should not do it. If any specific package is required to be update that individual package should be targeted.
The problem with that is that we potentially update many packages that increase the image size drastically and the end users needs to download more then necessary.

@Gargron Gargron merged commit 1e6a1ea into mastodon:master May 26, 2019
@SuperSandro2000 SuperSandro2000 deleted the docker-rm-dist-upgrade branch May 26, 2019 21:09
@Miaourt
Copy link

Miaourt commented May 26, 2019

Just my humble opinion, but updating existent package aren't increasing the size by much (worst case a few kilobytes), don't take much time, and provide latest upstream security fixes.
I think this should be reverted, but replacing dist-upgrade with only upgrade might be interesting, since the first one isn't really pertinent in our objective of having only security issues patched.

@SuperSandro2000
Copy link
Contributor Author

Well, wrong. It increases the image by the package size which best case is 1MB and worst case hundreds of MBs. Docker updates every file apt update touches and puts them in a new layer.
The build time is already insane so every saving is appreciated.
The Ubuntu image is almost daily updated with the latest packages so there is no need to do this if the image is build on CI or the builder pulls the ubuntu image before building.

with only upgrade might be interesting

Well, no. If you care that much about up to date run Arch and bare metal not docker.
Also this is against docker best practices which you should follow and only break if you have a valid reason to do so. https://docs.docker.com/develop/develop-images/dockerfile_best-practices/#run

If you're objective is to get this damn image to 2 GB you are on the best track. 1.4GB is already way to much for a docker image.

@Miaourt
Copy link

Miaourt commented May 26, 2019

It's not having bleeding edge stuff, it's having less security issue, docker is already a nightmare security-wise so it's quite important to do our best to have the very latest fixes.
The speed of built isn't the objective if this image. Faster the better, of course, but not on the detrimental of the security.
The very big size of this image is mostly due to the relatively big size of the mastodon software, relying on a bunch of gems and node packages, in addition to building all the styling at build time into the image for easier utilization, it's not a rush to save every bits can.

Also please try to stay courteous, we aren't here to attack each others.

@SuperSandro2000
Copy link
Contributor Author

SuperSandro2000 commented May 26, 2019

docker is already a nightmare security-wise

Most docker container that are well build are way more up do date then the typical Ubuntu or Debian machine. Also you give an potential attacker way less attack surface out of the box and don't allow him to sneak into all production databases when he finds one rce. So it is more secure if you know what you are doing like always.

The speed of built isn't the objective if this image.

It took bloody 47 minutes in circleci.....

Faster the better, of course, but not on the detrimental of the security.

If there is an important security update it can always be installed via apt install.
Also if the official Ubuntu base image is not up to date and there is an important update just nag the guys over here https://github.com/docker-library/official-images to update it asap.

it's not a rush to save every bits can.

Doesn't hurt. Speeds up my dev cycle.

Also please try to stay courteous, we aren't here to attack each others.

Please don't suggest anti patterns then.

Also apt-upgrade produces unpredictable builds and you can't reproduce an image in the future locally without pulling all updates. Good luck debugging a package issue with that.

@shleeable
Copy link
Contributor

docker-compose build --pull

problem solved.

@SuperSandro2000
Copy link
Contributor Author

@Shleeble good joke. One of the most request features that one if the maintainers simply does not want to implement.

@Sir-Boops
Copy link
Contributor

Sir-Boops commented Jun 13, 2019

I know this is thread is dead but @SuperSandro2000

If you're objective is to get this damn image to 2 GB you are on the best track. 1.4GB is already way to much for a docker image.

Is because the default docker build system is trash see these for what you can do using this dockerfile

Docker updates every file apt update touches and puts them in a new layer.

See --squash

It took bloody 47 minutes in circleci.....

That's because it builds nodejs, ruby, jemalloc, mastodon then finally compiles assets not from updates this was a choice made when this new dockerfile was added

see here for an example of the faster building version

@SuperSandro2000
Copy link
Contributor Author

SuperSandro2000 commented Jun 14, 2019

The docker build system is not trash this just uses it wrong. I don't wanna use squash.

Why even compile nodejs and jemalloc? Why not use preconpiled versions?
And I did PRs with that no one looked at yet.

@Sir-Boops
Copy link
Contributor

I don't wanna use squash.

What's wrong with squash?

Why even compile nodejs and jemalloc? Why not use preconpiled versions?

Allows masto to use any version at any time and this way the docker image dosn't depend on too many other images 😄

@SuperSandro2000
Copy link
Contributor Author

SuperSandro2000 commented Jun 14, 2019

You know apt? It can install all those things and it saves us time updating those things and debugging build failures and build time and complexity.
And right now we use a pretty old version of node that isn't even the latest version of that minor.

And squash is not the ideal way to do something proper.

@Sir-Boops
Copy link
Contributor

It can install all those things and it saves us time

Build caching for local/dev servers, then who cares how long it takes to build the release version

And right now we use a pretty old version of node that isn't even the latest version of that minor.

You build it in the container so you have version control even the slightly older version here it closer to current version then the version 18.04 is running src src2

And squash is not the ideal way to do something proper.

It shrinks image sizes massively why would you not build your release version using it

@SuperSandro2000
Copy link
Contributor Author

SuperSandro2000 commented Jun 14, 2019

I still care about build time when I need to develop that stuff. If it is not necessary we don't need to do it. We don't really gain anything except more complexity and work and I would like to keep it simple and understandable. If you want full control over everything feel safe to compile everything from source but I simply don't have the time and debugging that is required for that.

I am not referring to the version in Ubuntu. I am referring to the the official node binary releases for Ubuntu. Same thing with Ruby and python.

We can combine layers where it makes sense in the dockerfile. This makes the docker file 100% comparable to the final image and makes debugging easier. It also does not add unnecessary CI complexity that could fail.
Also we don't fix stuff afterwards that we could have prevented in the first place. I think this is a very bad pattern and should be avoided where necessary. If you can refractor something to make it easier do it instead of adding bulk around it.

hiyuki2578 pushed a commit to ProjectMyosotis/mastodon that referenced this pull request Oct 2, 2019
rtucker pushed a commit to vulpineclub/mastodon that referenced this pull request Jan 7, 2021
messenjahofchrist pushed a commit to Origin-Creative/mastodon that referenced this pull request Jul 30, 2021
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.

6 participants