Skip to content

Optimize dockerfiles for local rebuilds - #20238

Merged
potiuk merged 1 commit into
apache:mainfrom
potiuk:optimizes-dockerfile
Jan 11, 2022
Merged

Optimize dockerfiles for local rebuilds#20238
potiuk merged 1 commit into
apache:mainfrom
potiuk:optimizes-dockerfile

Conversation

@potiuk

@potiuk potiuk commented Dec 12, 2021

Copy link
Copy Markdown
Member

This is one of the last final refactorings of the image before
it is eligible to become an "official image".

When you build dockerfiles locally for development the layer
invalidation could happen earlier than you wanted - some of the
variables (like COMMIT_SHA) were affecting the cache of Docker
in the way that they forced either invalidation of the pre-cached
packages installed or forced to recreate assets when they were
not touched.

Similarly when no webpack/yarn/packages/static are modified,
the node asset compilation should not happen. It makes
no sense to compile all the assets on docker rebuild when
none of the www files changed.

In case of CI build we can also separate node modules
preparation and asset compilation, because node modules
should remain in the image anyway for incremental changes.

Fixes: #20259

This PR improves the experience of iterating over docker image
building by decreasing unnecesary layer invalidations.


^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code change, Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in UPDATING.md.

@potiuk

potiuk commented Dec 13, 2021

Copy link
Copy Markdown
Member Author

This is one of the final optimizations/refactorings and cleanups of the image before I will submit it to become an official image. Some last optimizations/clarifications (cleaning up the stderr output etc. )

If you think it is to big I can attempt to split it into smaller pieces, but there are a number of changes in there that invalidate current layers of Airflow images, making the cache less effective and merging it in one go would only require image refresh once.

Since the image is going to be "official" I had to make sure that all the potential "errors" are either gone or are explained. There re many requirements to fulfill to become an "official image" - and I think with that one, we are getting closer to having them all fulfilled (https://github.com/docker-library/official-images).

One thing that I had to do however is to add this reassuring message that we know what we are doing by using root and not using virtualenv. I tried to solve the problem in PIP or even PEP 668 level but I failed, so I had to revert to this reassuring message:

Screenshot from 2021-12-13 16-55-29

Looking forward to reviews!

@potiuk

potiuk commented Dec 14, 2021

Copy link
Copy Markdown
Member Author

Looking forward to reviews on that one- it should be helpful in finalizing AIP-26 (finally)

@potiuk

potiuk commented Dec 16, 2021

Copy link
Copy Markdown
Member Author

Anyone :) ?

@potiuk
potiuk force-pushed the optimizes-dockerfile branch from b55b3a2 to abcd0c8 Compare December 20, 2021 16:55
@raphaelauv

Copy link
Copy Markdown
Contributor

python2 is still in the DockerFile , maybe the opportunity to remove it ?

@potiuk

potiuk commented Dec 21, 2021

Copy link
Copy Markdown
Member Author

python2 is still in the DockerFile , maybe the opportunity to remove it ?

It's there for a reason. Python2 + Python Virtualenv is our offer to the users who still (?) have a need to run Python2 code.

It's part of the migration process we have:

We have a lot of enterprise users who are (hopefully) going to move away soon from it. And since it does not cost us much, we have Python 2 added to our image and even tests in our CI that test if PythonVirtualenv work for Python2.

I think removal of Python 2 from our tests/image should be done after voting that we want to drop it.

@potiuk

potiuk commented Dec 21, 2021

Copy link
Copy Markdown
Member Author

I started PROPOSAL thread on the devlist: https://lists.apache.org/thread/rjyqw3cwsh4vgg6jycsbr1jr0slnych3 @raphaelauv . I think 2 years of EOL anniversary is a good time to put the final nail in the coffin for it.

@potiuk
potiuk force-pushed the optimizes-dockerfile branch 2 times, most recently from 36df2da to 74bd750 Compare December 22, 2021 00:24
@potiuk

potiuk commented Dec 22, 2021

Copy link
Copy Markdown
Member Author

I updated the images to remove Python 2.

I also got rid of the warning from PIP in a different way. I complicated the Base image to add airflow user there (even if it is not needed), Just to make sure there are no warnings whe I pass it to the official image verification.

@potiuk

potiuk commented Dec 22, 2021

Copy link
Copy Markdown
Member Author

I would love some reviews - happy to split off some things (though it would be rather difficult to split).

But it would be great to get this one in to (almost) end the prod image story.

@potiuk
potiuk force-pushed the optimizes-dockerfile branch 2 times, most recently from 611bf18 to 67a594c Compare December 22, 2021 00:31
potiuk added a commit to potiuk/airflow that referenced this pull request Jan 5, 2022
* removes PIP_INSTALL_USER variable
* upgrades PIP to 21.3.1
* removes AIRFLOW_INSTALL_USER_FLAG as it is not needed
* removes spurious usage of --upgrade flag for PIP
* adds better diagnostics during the build for PIP location and version

Separated out from apache#20238
potiuk added a commit to potiuk/airflow that referenced this pull request Jan 5, 2022
There was some "junk" output generated by the scripts that are
used in Airflow image building. The junk has been cleaned up so
that no unnecessary warnings are generated.

Separated out from apache#20238
@potiuk
potiuk force-pushed the optimizes-dockerfile branch from e8821dc to 905b421 Compare January 5, 2022 16:49
@potiuk
potiuk force-pushed the optimizes-dockerfile branch from 905b421 to 99209ea Compare January 6, 2022 11:01
potiuk added a commit to potiuk/airflow that referenced this pull request Jan 6, 2022
* remove PIP_INSTALL_USER variable
* upgrade PIP to 21.3.1
* remove AIRFLOW_INSTALL_USER_FLAG as it is not needed
* remove spurious usage of --upgrade flag for PIP
* add better diagnostics during the build for PIP location and version

Separated out from apache#20238
potiuk added a commit that referenced this pull request Jan 7, 2022
* remove PIP_INSTALL_USER variable
* upgrade PIP to 21.3.1
* remove AIRFLOW_INSTALL_USER_FLAG as it is not needed
* remove spurious usage of --upgrade flag for PIP
* add better diagnostics during the build for PIP location and version

Separated out from #20238
potiuk added a commit to potiuk/airflow that referenced this pull request Jan 7, 2022
There was some "junk" output generated by the scripts that are
used in Airflow image building. The junk has been cleaned up so
that no unnecessary warnings are generated.

Separated out from apache#20238
potiuk added a commit to potiuk/airflow that referenced this pull request Jan 8, 2022
There was some "junk" output generated by the scripts that are
used in Airflow image building. The junk has been cleaned up so
that no unnecessary warnings are generated.

This change includes:

* making sure that when everything is fine, there are no
  warnnings generated by PROD docker build proces

* making sure that when CI image is build the only remaining
  warning is "Using root" - this warning cannot be silenced
  pypa/pip#10556 and instead
  in CI build we explain in green that this is invalid warning

* the "scripted" steps of docker build have nicely blue headers
  that visually separate steps of building the iamge and give
  more information on what's going on

* the current way of printing ouput will play very nicely with
  BUILDKIT UI where Blue color indicates progress in building

Separated out from apache#20238
@potiuk
potiuk force-pushed the optimizes-dockerfile branch 2 times, most recently from d04782a to f75e61e Compare January 8, 2022 19:21
@potiuk

potiuk commented Jan 8, 2022

Copy link
Copy Markdown
Member Author

This one is also updated - based on improved #20744 and #20747

potiuk added a commit to potiuk/airflow that referenced this pull request Jan 8, 2022
There was some "junk" output generated by the scripts that are
used in Airflow image building. The junk has been cleaned up so
that no unnecessary warnings are generated.

This change includes:

* making sure that when everything is fine, there are no
  warnnings generated by PROD docker build proces

* making sure that when CI image is build the only remaining
  warning is "Using root" - this warning cannot be silenced
  pypa/pip#10556 and instead
  in CI build we explain in green that this is invalid warning

* the "scripted" steps of docker build have nicely blue headers
  that visually separate steps of building the iamge and give
  more information on what's going on

* the current way of printing ouput will play very nicely with
  BUILDKIT UI where Blue color indicates progress in building

Separated out from apache#20238
@potiuk
potiuk force-pushed the optimizes-dockerfile branch from f75e61e to 1cd4e3c Compare January 8, 2022 19:51
@potiuk

potiuk commented Jan 10, 2022

Copy link
Copy Markdown
Member Author

With this change I think I finally implemented an optimised sequence of layers for the CI image:

  1. Pre-install PYPI deps from main
  2. Install node deps if changed
  3. Install "current" PYPI deps if changed
  4. Compile www assets if asset files changed
  5. Copy sources from airflow

This brings really optimized image building and witht the follow-up buildx #20258 change and build-kit --cache-from, it will be always minimum time needd to rebuilld exactly what needs to be rebuilt.

When you build dockerfiles locally for development the layer
invalidation could happen earlier than you wanted - some of the
variables (like COMMIT_SHA) were affecting the cache of Docker
in the way that they forced either invalidation of the pre-cached
packages installed or forced to recreate assets when they were
not touched.

Similarly when no webpack/yarn/packages/static are modified,
the node asset compilation should not happen. It makes
no sense to compile all the assets on docker rebuild when
none of the www files changed.

In case of CI build we can also separate node modules
preparation and asset compilation, because node modules
should remain in the image anyway for incremental changes.

Fixes: apache#20259

This PR improves the experience of iterating over docker image
building by decreasing unnecesary layer invalidations.
@potiuk

potiuk commented Jan 11, 2022

Copy link
Copy Markdown
Member Author

I Look for some reviews. As part of the optimization I also reviewed the image with Dive (cc: @malthe @mik-laj ) and made sure that some of the remainig remnants that were "bloating" the image were removed

  • we had (unnecesary) PIP install in the final image - this caused (a small) number of .pyc files to be embedded in the image
  • we also had a lastlog produced during apt installl which had 15MB - I made sure it is removed as the last step of the RUN instruction that created it (thanks @malthe for pointing that out!).
  • I also reviewed and improved the instructions which copied the .local folder and performed permission - one of the problems noted in Write access to /home/airflow for "default" user in docker #20776 that there was no "group write" permission for the home directory of Airflow (which could be problematic in some open-shift cases). It had to be done carefully - changing of the permissions has to be done in the right place bacause changing the permission after the files are stored as layer effectively duplicates the layer (the new layer with pemissions creates effectively a copy o all the files) 😱

As result the efficiency score of our image jumped from 97% to 99%:

Screenshot 2022-01-11 02 08 24

I am thinking about adding some more automated tests for the presence of unwanted files and automating the tests for the image "efficiency" in our CI, but I would like to do it after this one and #20258 as switching to buildx significantly improves the experience of iterating over the images and building them in small increments.

Looking forward to reviews!

@malthe

malthe commented Jan 11, 2022

Copy link
Copy Markdown
Contributor

FWIW, the gzipped size comes to about 300 megs. I suppose when the image is mapped to a filesystem then it has to uncompress each layer. There's some discussion about that here: moby/moby#24515.

@github-actions

Copy link
Copy Markdown
Contributor

The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest main at your convenience, or amend the last commit of the PR, and push it with --force-with-lease.

@potiuk

potiuk commented Jan 11, 2022

Copy link
Copy Markdown
Member Author

FWIW, the gzipped size comes to about 300 megs. I suppose when the image is mapped to a filesystem then it has to uncompress each layer. There's some discussion about that here: moby/moby#24515.

Yeah - compressed size is much smaller - and that's the size that "matters" when the file gets pulled really. The moby discussion is about users who are already dynamically decompressing the data they store. This I think is kind of antipattern when you use images - the image layers are compressed by default - maybe not most efficient compression on the planet because it is a "generic" compression - but it does the job. in vast majority of cases I saw it is 1:3 compression rate for most binary data and 1:10 at least for text data (roughly - I never made a detailed calculation), But if someone attempts to store compressed data that reaches similar levels of compression in an already compressed image layer, it's a pretty much loss (unless you care about the final space used when image is decompressed and you decompress on-the-flight and never store the decompressed data).

So in essence I just take the compression done by container layers as "granted" and don't try to tweak around it.

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

Labels

area:dev-tools area:production-image Production image improvements and fixes changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) full tests needed We need to run full set of tests for this PR to merge kind:documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Write access to /home/airflow for "default" user in docker Make sure that stderr is "clean" while building the images.

5 participants