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

Dockerfile improvements again #198

Merged
merged 6 commits into from
Apr 16, 2020
Merged

Dockerfile improvements again #198

merged 6 commits into from
Apr 16, 2020

Conversation

obilodeau
Copy link
Collaborator

@obilodeau obilodeau commented Mar 25, 2020

Since we merged #190 we can apply further sizing improvements to our docker image

  • Split GUI and not GUI into two images
  • Staged build to drastically reduce image size (no more build-essentials package required)
  • verify docker size improvements gains
  • Apply staged build to GUI Dockerfile
  • Add integration-gui integration-slim and latest-gui latest-slim tags to dockerhub automatic builds
  • update docker-compose.yml and documentation
  • add a Dockerfile.slim build in CI too

Size reduction

  • The integration image is 63.24 MB now. That is down from 252.39 MB now in master or down from 570.26 MB for 0.4.1.
  • The integration-gui image is 293.32 MB now. That is up from 252.39 MB now in master (which I suspect was broken for the GUI since I needed to add a package to make it work on my system) or down from 570.26 MB for 0.4.1.

Feedback required for Tag names

Because of @alxbl's upcoming work with video encoding and the needed dependencies, I'm not sure that -gui is the proper approach. Candidates:

  • a) Current proposal: latest with minimal dependencies and latest-gui (with GUI but more later?)
  • b) latest with minimal dependencies and latest-full (with everything)
  • c) Turn it the other way around: latest with everything (GUI + video encoding, etc.) and latest-slim (I think python does it that way for official images)

@obilodeau obilodeau added the enhancement New feature or request label Mar 25, 2020
@obilodeau obilodeau added this to the vNext milestone Mar 25, 2020
@obilodeau
Copy link
Collaborator Author

New Dockerfile worked locally. Triggering an integration build to see how it goes over there and it's resulting size.

Dockerfile Outdated Show resolved Hide resolved
@obilodeau
Copy link
Collaborator Author

Verified image size in integration. Added at the top of the issue.

The integration image is 63.24 MB now. That is down from 252.39 MB now in master or down from 570.26 MB for 0.4.1.

Also added a question about tag names alternatives

@obilodeau
Copy link
Collaborator Author

Verified image size in integration-gui. Added at the top of the issue.

The integration-gui image is 293.32 MB now. That is up from 252.39 MB now in master (which I suspect was broken for the GUI since I needed to add a package to make it work on my system) or down from 570.26 MB for 0.4.1.

@obilodeau
Copy link
Collaborator Author

@alxbl when you have a minute, let's discuss the tag name approach that I added at the top of the PR. @Res260 feel free to chime in as well.

Once this will be figured out, I'll setup dockerhub and update the documentation accordingly.

@alxbl
Copy link
Collaborator

alxbl commented Mar 25, 2020

@alxbl when you have a minute, let's discuss the tag name approach that I added at the top of the PR.

I feel like a lot of images take the -slim tag approach, so I kinda like that one. By default you get everything (batteries included), and if you have specific needs, then you read the docs and find out that there's a slim build or a specific tag for your needs.

@obilodeau
Copy link
Collaborator Author

Changed the Dockerfile to use the slim approach. Configured an integration-slim build rule in DockerHub.

@obilodeau
Copy link
Collaborator Author

Image builds that happened during lunch:

  • integration: 295.44 MB
  • integration-slim: 65.37 MB

@obilodeau
Copy link
Collaborator Author

Merged this into integration to see if it builds. I'll finish and submit the PR tomorrow.

* Renamed setup.py optional deps from GUI to full
* Aligned docker-compose.yml
* Documentation updated

Some of these changes took for granted that pyrdp-convert is going in.
Which I think it will so this is why I introduced full, because we are
going to pull ffmpeg-dev and pyav pretty soon.
@obilodeau obilodeau marked this pull request as ready for review April 15, 2020 15:15
@Res260
Copy link
Collaborator

Res260 commented Apr 15, 2020

@obilodeau I don't know if you wanted to do it like that, but you can separate the two docker builds in two different jobs in the CI so they can run in parrallel instead of sequentially

@obilodeau
Copy link
Collaborator Author

@obilodeau I don't know if you wanted to do it like that, but you can separate the two docker builds in two different jobs in the CI so they can run in parrallel instead of sequentially

I decided to keep it into the same task consciously. My rationale was that this is one less ubuntu to start so we save a few seconds of CI time (not sure how much we use though). The two builds are closely related and some of the docker layers might be locally cached. I also believe that if one fails the other will probably too.

I haven't thought of the parallel argument. Good call. If I ever find myself waiting too much on this specific CI task I'll think about it.

@obilodeau obilodeau requested review from alxbl and Res260 April 15, 2020 23:02
README.md Show resolved Hide resolved
docker-compose.yml Outdated Show resolved Hide resolved
@obilodeau obilodeau merged commit 3335c2d into master Apr 16, 2020
@obilodeau obilodeau deleted the dockerfile-split branch April 16, 2020 15:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants