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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a prod-ready target to the Dockerfile #1141

Open
wants to merge 7 commits into
base: master
from

Conversation

Projects
None yet
6 participants
@NiR-
Copy link
Contributor

commented Oct 22, 2018

First of all, thank you a lot for that marvelous tool 馃榾

This change leverage multi-stage build feature to create build targets
specific to development and production environments. Both targets are
based on a primal target named base. Packages and common gems are
installed in the base target and env-specific gems are installed in
their respective target. This should solve #927.

Using the production environment: enables PumaWorkerKiller (this should
circumvent the issue described in #667), pull the logger level higher (so
no more sql queries got dumped) and speed up the application by
enabling some caching.

To minimize the image size, the build-base package is now installed
and removed every time bundle is run. This effectively reduce the size
of the dev build from 533MB to 364MB (for the dev target). And the
compressed size of the image on Docker Hub goes from 219MB to 159MB.

Also, note that Octobox now runs with uid 1000, to enforce better security
practices.

To get this PR fully effective, I think you have to change a bit the way you
deliver docker images: currently, it looks like all the branches are built for
dev environment. IMHO you should also (or instead), publish prod-ready
images based on git tags. It'd also help operators determine what version
of Octobox is used.

@NiR- NiR- force-pushed the NiR-:improve-dockerfile branch from 553186b to 4fa50fe Oct 22, 2018

@NiR-

This comment has been minimized.

Copy link
Contributor Author

commented Oct 22, 2018

Oh, I just realize @chrisarcand already opened a PR today to address some of the same issues this PR fixes.

The scope of his/their PR is wider than mine, however the introduction of the multi-stage feature in this PR, as well as the systematic deletion of build-base help to reduce the size of the image: while the final image from chrisarcand is ~800MB, mine is ~350MB (uncompressed). Thus, I think both are complementary.

@NiR- NiR- force-pushed the NiR-:improve-dockerfile branch from 4fa50fe to dee13ee Oct 22, 2018

@andrew

This comment has been minimized.

Copy link
Member

commented Oct 26, 2018

This will need a rebase now that #1139 has been merged

@NiR- NiR- force-pushed the NiR-:improve-dockerfile branch 2 times, most recently from b4524ee to e407f8b Oct 30, 2018

@NiR-

This comment has been minimized.

Copy link
Contributor Author

commented Oct 30, 2018

@andrew Done :)

Dockerfile Outdated
RUN RAILS_ENV=production rake assets:precompile \
&& rm -rf /usr/src/app/tmp/cache/

RUN RAILS_ENV=production rake assets:precompile \

This comment has been minimized.

Copy link
@andrew

andrew Oct 30, 2018

Member

Looks like this line might have been duplicated in the rebase?

This comment has been minimized.

Copy link
@NiR-

NiR- Oct 31, 2018

Author Contributor

Oh indeed, that was a rebase mistake. It's now fixed.

andrew added a commit that referenced this pull request Oct 30, 2018

@andrew

This comment has been minimized.

Copy link
Member

commented Oct 30, 2018

This is looking good to me, would be good for someone who's actually running Octobox with Docker to try this out before merging.

@andrew andrew requested a review from chrisarcand Oct 30, 2018

@andrew

This comment has been minimized.

Copy link
Member

commented Oct 30, 2018

I've pulled the docs fix out into a pr over here: #1168

andrew added a commit that referenced this pull request Oct 30, 2018

andrew added a commit that referenced this pull request Oct 30, 2018

@chrisarcand

This comment has been minimized.

Copy link
Member

commented Oct 30, 2018

Oh cool, thanks @NiR- ! I was reading about multistage stuff last week and figured something like this could be utilized as a followup to my recent PR, which you saw. Excellent!

I'll need to find a bit of time to sift through this and try it out; it's on my list to look at before the end of this week. Thanks for your patience!

@NiR- NiR- force-pushed the NiR-:improve-dockerfile branch from e407f8b to ba106e2 Oct 31, 2018

@chrisarcand
Copy link
Member

left a comment

Looking good! I haven't tried running it end to end but have enough feedback to address first:

  • Regarding the Docker Hub image and how images should be tailored per-environment, I generally agree but have tried to keep it 'backwards compatible' in that we've always supported dev environments in the images we provide. However, I really do think it makes sense to the image be a production-only environment and I don't really think we want to deal with providing multiple images. I'm still learning here so if there's an easy way to do it, sure - otherwise as long as @andrew is cool with it I'm all for just providing production images on Docker Hub.
  • It seems like some documentation might need updating, especially with the above consideration. To give Octobox a try in development at this point you'd need to pull down the repo and make your own image anyway, so some docs should be changed and I think the override file could probably just be removed, too.
&& chmod -R g=u $APP_ROOT

# Startup
CMD ["bin/docker-start"]
USER 1000

This comment has been minimized.

Copy link
@chrisarcand

chrisarcand Nov 2, 2018

Member

From the docs...

The USER instruction sets the user name (or UID) and optionally the user group (or GID) to use when running the image and for any RUN, CMD and ENTRYPOINT instructions that follow it in the Dockerfile.

Doesn't that mean this should be before all the other commands in each section?

This comment has been minimized.

Copy link
@NiR-

NiR- Nov 25, 2018

Author Contributor

Unfortunately this would mean running apk with user 1000 and that won't work. Since bundle install commands are all surrounded by apk commands, the only place where we can set the USER is at the end of each target.

However, AFAIK bundler and RubyGems does not have the same security issue than, for instance, npm: there's no external code run during installation phase, so I believe there's no reason for the build environment to be compromised (from that vector). In the end, it's only there to not run Octobox as a root user, as this is considered a bad practice (see here).

This comment has been minimized.

Copy link
@andrew

andrew Nov 26, 2018

Member

Rubygems does actually allow for arbitrary code execution during installation via the option to build native extensions: http://blog.costan.us/2008/11/post-install-post-update-scripts-for.html although it requires jumping through more hoops than with npm.

This comment has been minimized.

Copy link
@NiR-

NiR- Nov 26, 2018

Author Contributor

Ha too bad :/ I'm not really sure what to do then. I tried to find out articles about "dockerizing" ruby apps, but almost none talks about USER instruction. The only one I found using this instruction, does so after installing the dependencies (see here).

Do you think this is not an issue in the end, or should I do the required changes to not run bundler as root (that would mean we couldn't install/deinstall build-tools before/after running bundler)?

@@ -0,0 +1,25 @@
version: '3.4'

This comment has been minimized.

Copy link
@chrisarcand

chrisarcand Nov 2, 2018

Member

Why is this file added? An old artifact from before I did my own PR where the separate dev compose file was removed? Is this necessary?

This comment has been minimized.

Copy link
@chrisarcand

chrisarcand Nov 2, 2018

Member

(to combine my feedback of 'we should just provide production images', if we do that I believe the target added in this file should just be edited in the existing compose file and this file removed)

This comment has been minimized.

Copy link
@NiR-

NiR- Nov 7, 2018

Author Contributor

Indeed, it's an old artifact, my bad.

the target added in this file should just be edited in the existing compose file

If developers have to update their docker-compose.yml file to run dev versions, this will not be really practical as it'll be marked as an "unstage change" by git. This might introduce too much friction. However, we can use an env var, defined in .env, so people can easily switch to any env.

This comment has been minimized.

Copy link
@nesl247

nesl247 Nov 15, 2018

My recommendation is to have docker-compose.yml only include what is useful in EVERY environment, and add a docker-compose.override.yml.dist file which is designed for environment specific purposes, with defaults for development. Users then copy it to docker-compose.override.yml and make any changes necessary. You add the .dist to .gitignore and then it won't show up as an unstaged change.

@chrisarcand

This comment has been minimized.

Copy link
Member

commented Nov 2, 2018

Followup: I just looked and it seems that you can't actually specify separate build targets to automatically build on Docker Hub, furthering the thought that we shouldn't bother providing anything but production images. 馃憤

We should definitely start building images on tagged releases as you say, will follow up with that after this PR is approved and merged 馃挒

@andrew andrew dismissed their stale review Nov 15, 2018

outdated

@jakubgs

This comment has been minimized.

Copy link

commented Nov 22, 2018

Any news on this?

NiR- added some commits Oct 22, 2018

Add a prod-ready target to the Dockerfile
This change leverage multi-stage build feature to create build targets
specific to development and production environments. Both targets are
based on a primal target named base. Packages and common gems are
installed in the base target and env-specific gems are installed in
their respective target.

Using the production environment: enables PumaWorkerKiller, pull the
logger level higher (so no more sql queries got dumped) and speed up
the application by enabling some caching.

To minimize the image size, the `build-base` package is now installed
and removed every time `bundle` is run. This effectively reduce the size
of the dev build from 533MB to 364MB (for the dev target). And the
compressed size of the image on Docker Hub goes from 219MB to 159MB.

The impact of both modifications on the build time is negligable:
it gets from 7m57 to 7m47 (on a single build on my computer though),
for the development target. However, the build time for the prod env
is a bit higher: ~9m. That's because the dev target appear first in
the Dockerfile and has to be built first (env if it's not used in
the prod target though).

@NiR- NiR- force-pushed the NiR-:improve-dockerfile branch from ba106e2 to 482ef1d Nov 26, 2018

@NiR-

This comment has been minimized.

Copy link
Contributor Author

commented Nov 26, 2018

I updated the PR to include some more changes:

  • The .dockerignore file now works like a whitelist instead of the usual blacklist mode. That should prevent anyone building images from sources to inadvertently include files with credentials (like .env.backup or anything like that).
  • Docs has been updated to contain instructions to set up prod instances, with an example docker-compose.yml and a .env exhaustively listing config parameters.
  • The app container in dev env now has a volume to bind-mount the whole project. This is better since cache_classes is turned to false in that env.
@andrew

This comment has been minimized.

Copy link
Member

commented May 3, 2019

This has some conflicts but I'd really like to parts of it merged, would be good to test what it's like upgrading an existing docker install with this new image and config.

@andrew

This comment has been minimized.

Copy link
Member

commented May 13, 2019

Fixed up the conflicts here, going to look at how to get docker hub to build the production version on push.

@BenJam BenJam referenced this pull request May 14, 2019

Closed

Delayed comments #1791

BenJam added some commits May 23, 2019

@andrew

This comment has been minimized.

Copy link
Member

commented May 28, 2019

Pushing it to docker hub didn't really work 馃

@NiR-

This comment has been minimized.

Copy link
Contributor Author

commented May 28, 2019

Oh, what's the issue?

@andrew

This comment has been minimized.

Copy link
Member

commented May 28, 2019

app_1                                | `/` is not writable.
app_1                                | Bundler will use `/tmp/bundler/home/unknown' as your home directory temporarily.
app_1                                | rake aborted!
app_1                                | LoadError: cannot load such file -- rubocop/rake_task
app_1                                | /usr/local/bundle/gems/bootsnap-1.4.4/lib/bootsnap/load_path_cache/core_ext/kernel_require.rb:22:in `require'
app_1                                | /usr/local/bundle/gems/bootsnap-1.4.4/lib/bootsnap/load_path_cache/core_ext/kernel_require.rb:22:in `block in require_with_bootsnap_lfi'
app_1                                | /usr/local/bundle/gems/bootsnap-1.4.4/lib/bootsnap/load_path_cache/loaded_features_index.rb:89:in `register'
app_1                                | /usr/local/bundle/gems/bootsnap-1.4.4/lib/bootsnap/load_path_cache/core_ext/kernel_require.rb:21:in `require_with_bootsnap_lfi'
app_1                                | /usr/local/bundle/gems/bootsnap-1.4.4/lib/bootsnap/load_path_cache/core_ext/kernel_require.rb:40:in `rescue in require'
app_1                                | /usr/local/bundle/gems/bootsnap-1.4.4/lib/bootsnap/load_path_cache/core_ext/kernel_require.rb:26:in `require'
app_1                                | /usr/local/bundle/gems/activesupport-5.2.3/lib/active_support/dependencies.rb:291:in `block in require'
app_1                                | /usr/local/bundle/gems/activesupport-5.2.3/lib/active_support/dependencies.rb:257:in `load_dependency'
app_1                                | /usr/local/bundle/gems/activesupport-5.2.3/lib/active_support/dependencies.rb:291:in `require'
app_1                                | /usr/src/app/rakefile:26:in `<top (required)>'
app_1                                | /usr/local/bundle/gems/rake-12.3.2/exe/rake:27:in `<top (required)>'
app_1                                | /usr/local/bundle/gems/bundler-2.0.1/lib/bundler/cli/exec.rb:74:in `load'
app_1                                | /usr/local/bundle/gems/bundler-2.0.1/lib/bundler/cli/exec.rb:74:in `kernel_load'
app_1                                | /usr/local/bundle/gems/bundler-2.0.1/lib/bundler/cli/exec.rb:28:in `run'
app_1                                | /usr/local/bundle/gems/bundler-2.0.1/lib/bundler/cli.rb:463:in `exec'
app_1                                | /usr/local/bundle/gems/bundler-2.0.1/lib/bundler/vendor/thor/lib/thor/command.rb:27:in `run'
app_1                                | /usr/local/bundle/gems/bundler-2.0.1/lib/bundler/vendor/thor/lib/thor/invocation.rb:126:in `invoke_command'
app_1                                | /usr/local/bundle/gems/bundler-2.0.1/lib/bundler/vendor/thor/lib/thor.rb:387:in `dispatch'
app_1                                | /usr/local/bundle/gems/bundler-2.0.1/lib/bundler/cli.rb:27:in `dispatch'
app_1                                | /usr/local/bundle/gems/bundler-2.0.1/lib/bundler/vendor/thor/lib/thor/base.rb:466:in `start'
app_1                                | /usr/local/bundle/gems/bundler-2.0.1/lib/bundler/cli.rb:18:in `start'
app_1                                | /usr/local/bundle/gems/bundler-2.0.1/exe/bundle:30:in `block in <top (required)>'
app_1                                | /usr/local/bundle/gems/bundler-2.0.1/lib/bundler/friendly_errors.rb:124:in `with_friendly_errors'
app_1                                | /usr/local/bundle/gems/bundler-2.0.1/exe/bundle:22:in `<top (required)>'
app_1                                | /usr/local/bundle/bin/bundle:23:in `load'
app_1                                | /usr/local/bundle/bin/bundle:23:in `<main>'
app_1                                | 
app_1                                | Caused by:
app_1                                | Bootsnap::LoadPathCache::FallbackScan: 
app_1                                | 
app_1                                | (See full trace by running task with --trace)
octobox_app_1 exited with code 1
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can鈥檛 perform that action at this time.