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 deployment support #588

Merged
merged 35 commits into from
Oct 11, 2015

Conversation

meetwudi
Copy link
Contributor

@meetwudi meetwudi commented Oct 5, 2015

Add docker support (see #450).

Couple things to note:

  • I removed imagemin task in grunt since
    • It is obsolete and cannot be downloaded anymore (at least on my side). It can block building so I removed it.
    • It is useless, I mean, in re:dash
  • Example docker-compose.yaml is added.
  • Made service listen on 0.0.0.0 instead of only 127.0.0.1. So docker containers can accept incoming traffic then.

grunt-contrib-imagemin seems to be broken because several dependencies
are quite obsolete and cannot be downloaded.
Signed-off-by: John Wu <webmaster@leapoahead.com>
Signed-off-by: John Wu <webmaster@leapoahead.com>
Signed-off-by: John Wu <webmaster@leapoahead.com>
@@ -0,0 +1,25 @@
FROM debian:wheezy
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason to use Debian and not Ubuntu as the base image?

@arikfr
Copy link
Member

arikfr commented Oct 6, 2015

Thanks, @tjwudi ! This is much appreciated (and long awaited).

I've left some comments.

Signed-off-by: John Wu <webmaster@leapoahead.com>
Instead of binding to `0.0.0.0`, use `127.0.0.1` instead for security
concerns. "The Python Web server is more
vulnerable than nginx that proxies it."

Signed-off-by: John Wu <webmaster@leapoahead.com>
Signed-off-by: John Wu <webmaster@leapoahead.com>
Signed-off-by: John Wu <webmaster@leapoahead.com>
Signed-off-by: John Wu <webmaster@leapoahead.com>
@meetwudi
Copy link
Contributor Author

meetwudi commented Oct 6, 2015

Also, @arikfr is it a good practice to use db containers? Since files modified will be discarded when container stops, then all data will be lost if we do not commit the container regularly.

By using Dockerfile `RUN` command, we can enable docker to cache our
build. Also, much more easier to maintain.

Signed-off-by: John Wu <webmaster@leapoahead.com>
@meetwudi
Copy link
Contributor Author

meetwudi commented Oct 6, 2015

Updated!

@arikfr
Copy link
Member

arikfr commented Oct 6, 2015

@tjwudi that's what data volumes are for: https://docs.docker.com/userguide/dockervolumes/

ENV FILES_BASE_URL /opt/redash/current/setup/files/

# Base packages
RUN apt-get dist-upgrade
Copy link
Member

Choose a reason for hiding this comment

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

This line is probably not needed. I had some issues with AWS Ubuntu images, and added it. But normally everything should work fine without it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Which line is it? GitHub didn't point out which one exactly

Copy link
Member

Choose a reason for hiding this comment

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

RUN apt-get dist-upgrade

@meetwudi
Copy link
Contributor Author

meetwudi commented Oct 6, 2015

@arikfr then probably add data volumes in docker-compose-example.yaml?

@arthurpsmith
Copy link

FYI below is the docker-compose.yml file I've been using with success. Some notes on differences:

  • No need to expose the ports on the redis and postgres containers - these are automatically available to the redash container via the link, they do not need to be exposed on the docker host machine.
  • with a POSTGRES_PASSWORD environment variable, the default admin user (postgres) on the postgres server can be accessed by username/password authentication. In my "redash_start.sh" script I have a line export PGPASSWORD=${POSTGRES_PASSWORD} when I need to log in as the postgres user via psql (ie. psql -h postgres -U postgres postgres ... works by pulling the password from the environment).
  • Uses a volume on the host to have a persistent database between updates of the postgres container. A different volume choice might be suitable depending on your needs.
  • exposes the redash port (5000) via a port map on the host. If you want nginx in front you could, (that would be one more container with the nginx image + a custom config file) but it doesn't seem needed.
redis:
  image: redis

postgres:
  image: postgres
  environment:
    POSTGRES_PASSWORD: xxxxxxxx
  volumes:
   - /opt/postgres-data:/var/lib/postgresql/data

redash:
  build: .
  container_name: redash
  links:
   - redis
   - postgres 
  environment:
    POSTGRES_PASSWORD: xxxxxxxx
  ports:
   - "8080:5000"

@bobrik
Copy link
Contributor

bobrik commented Oct 8, 2015

I played with re:dash a bit and decided that it's nice to split celery and web into separate containers. It's easier to run on marathon and scale this way and you don't have to mess with init systems.

@meetwudi
Copy link
Contributor Author

meetwudi commented Oct 8, 2015

So I decided to create a separate image for redash nginx. It's now live on https://hub.docker.com/r/tjwudi/redash-nginx/ and repo https://github.com/tjwudi/redash-nginx.

Signed-off-by: John Wu <webmaster@leapoahead.com>
Signed-off-by: John Wu <webmaster@leapoahead.com>
Signed-off-by: John Wu <webmaster@leapoahead.com>
@arikfr
Copy link
Member

arikfr commented Oct 8, 2015

Why not keep the Dockerfile and nginx.conf for the nginx image in this
repo? It will make it easier to manage access and edit Docker related
configuration.

Also make sure to add daemon off directive to nginx.conf.
On Oct 8, 2015 9:34 PM, "John Wu" notifications@github.com wrote:

So I decided to create a separate image for redash nginx. It's now live on
https://hub.docker.com/r/tjwudi/redash-nginx/ and repo
https://github.com/tjwudi/redash-nginx.


Reply to this email directly or view it on GitHub
#588 (comment).

@meetwudi
Copy link
Contributor Author

meetwudi commented Oct 8, 2015

@arikfr Also makes sense. Though I prefer one image one repo, I can do it if that is what you guys want :)

@meetwudi
Copy link
Contributor Author

meetwudi commented Oct 8, 2015

@arikfr daemon is off by default

@meetwudi
Copy link
Contributor Author

meetwudi commented Oct 8, 2015

Well the thing is, the latest build does not contain supervisord_docker.conf so it can fail if you try to build. Anyway, it is now downloading from latest build instead :)

Signed-off-by: John Wu <webmaster@leapoahead.com>
@meetwudi
Copy link
Contributor Author

meetwudi commented Oct 8, 2015

@bobrik Separating celery is definitely what we want to do next, but probably in another branch maybe? b/c I don't know the complexity to do it yet.

@arikfr
Copy link
Member

arikfr commented Oct 8, 2015

I agree that we can split the images in another iteration/branch. Also the resulting image can still be used without the supervisord with custom run command.

error_log /var/log/nginx/log/error.log;

location / {
proxy_pass http://redashapp;
Copy link
Member

Choose a reason for hiding this comment

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

Unless in newer versions of nginx those options are on by default worth adding to the server directive:

  gzip on;
  gzip_types *;
  gzip_proxied any;

and to the location / directive:

    proxy_set_header Host $http_host;
    proxy_set_header X-Real-IP $remote_addr;
    proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for;
    proxy_set_header X-Forwarded-Proto $scheme;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for this!

Signed-off-by: John Wu <webmaster@leapoahead.com>
FROM ubuntu:trusty
MAINTAINER Di Wu <diwu@yelp.com>

ENV FILES_BASE_URL /opt/redash/current/setup/files/
Copy link
Member

Choose a reason for hiding this comment

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

this is no longer used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed :)

Signed-off-by: John Wu <webmaster@leapoahead.com>
@landscape-bot
Copy link

Code Health
Repository health increased by 0.45% when pulling e19962d on tjwudi:diwu/feature/docker-deployment into 5d1c75d on EverythingMe:master.

arikfr added a commit that referenced this pull request Oct 11, 2015
@arikfr arikfr merged commit 9e183f1 into getredash:master Oct 11, 2015
@arikfr
Copy link
Member

arikfr commented Oct 11, 2015

Merged. I will probably tweak it some more myself, to make it work with CI and everything.

Thanks a lot for the effort making this and patience during the review. :-) 👍

@meetwudi
Copy link
Contributor Author

My pleasure. Learned a lot here. Will go on contributing :) @arikfr ++

@meetwudi meetwudi mentioned this pull request Oct 12, 2015
@arikfr
Copy link
Member

arikfr commented Oct 12, 2015

Awesome 👍

@meetwudi
Copy link
Contributor Author

@arikfr are you working on separating celery tasks? If not, I am happy to take over.

@arikfr
Copy link
Member

arikfr commented Nov 22, 2015

@tjwudi you mean having Celery and gunicorn in separate containers?

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