Improve development experience with Docker #5966

victornoel · 2018-09-24T12:09:48Z

Improve Docker image
- smaller
- faster to build
- deterministic dependencies (see Error while installing (markdown() takes 1 positional argument but 2 were given) #5958)
Rework process to simplify setting things up
- updated documentation
- less commands to type
- no files to move and modify
- optional loading of samples
Still working in standalone mode (without volumes for superset)

This should close #5987 too

victornoel · 2018-09-27T07:15:30Z

@xiaohanyu I see you initially contributed this docker intregration, could it be possible to get a review maybe? Thanks :)

xiaohanyu · 2018-09-27T08:25:50Z

sure, will do that at this weekend, busy at daily work...

victornoel · 2018-09-30T14:16:54Z

@xiaohanyu have you had the time to check it out? :)

victornoel · 2018-10-01T10:16:42Z

@kakoni I have updated the PR to reflect what we discussed in #5802:

now the docker image watch both python and frontend code (so no need for extra dev-server except if desired by the dev)
the image is built directly by docker-compose.

codecov-io · 2018-10-01T16:34:30Z

Codecov Report

Merging #5966 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #5966   +/-   ##
=======================================
  Coverage   77.33%   77.33%           
=======================================
  Files          67       67           
  Lines        9578     9578           
=======================================
  Hits         7407     7407           
  Misses       2171     2171

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1a5ca35...12065ec. Read the comment docs.

victornoel · 2018-10-03T20:03:01Z

@mistercrunch I think this PR solves some of the problems people are having with the dev docker image and also improve the experience a bit, do you think I could get a review in order for it to get merged? thanks!

xiaohanyu · 2018-10-09T16:00:38Z

contrib/docker/Dockerfile

-
-# Add a normal user
-RUN useradd --user-group --create-home --shell /bin/bash work
-


I've read a lot docs about docker practice and it seems not good to run programs inside docker as root, so I create a new work user.

However, I think if we focus these docker files on development, then it's OK.

Oh, I didn't know, if you have some documentation on that I would be interested.

I removed it because it's a pain to manage a user stuffs in docker :)

https://medium.com/@mccode/processes-in-containers-should-not-run-as-root-2feae3f0df3b

See some articles like this.

However, I think for development purpose, it's OK to simplify things.

Wait, it's likely that people would want to use this docker image for production, why not keeping a user account?

@mistercrunch I think it would be safer to use an image based on the pypi package for production use (as in the https://hub.docker.com/r/amancevice/superset/ image) but who knows what people want to do: I will put the user back then!

@mistercrunch @victornoel Would it make sense to create two docker images/docker files? One aimed for production and another one for development.

@kakoni if we do that, the advantage is that we could simply remove all the content with superset from the image and just provide the dev environment needed for it… but in that case, wouldn't it be easier to just use something like virtualenv? ;)

I think we can provide two docker images? one for development and one for production?

xiaohanyu · 2018-10-09T16:02:40Z

contrib/docker/Dockerfile

+RUN apt-get update -y \
+    && apt-get install -y build-essential libssl-dev \
+        libffi-dev python3-dev libsasl2-dev libldap2-dev \
+        vim less postgresql-client redis-tools


I put a separate RUN apt-get install -y vim less postgresql-client redis-tools because all vim less postgresql-client redis-tools are convenient tools for development, these are not necessary for production, while other packages like build-essential libssl-dev are needed for superset to run.

I think it's OK to merge here for development purpose.

Indeed, I will separate them again to make things clearer and fix the broken comment links.

contrib/docker/Dockerfile

xiaohanyu · 2018-10-09T16:15:36Z

contrib/docker/docker-compose.yml

 volumes:
  postgres:
    external: false
  redis:
    external: false
-  superset-node-modules:
+  frontend-dist:


So after you rename to frontend-dist, where's frontend-dist volume? it will be empty?

I forgot to remove it, it is not needed anymore

.dockerignore

.gitignore

xiaohanyu · 2018-10-09T16:31:17Z

Hi, @victornoel

Sorry for my late response...busy work.

I've checked your code and I think the code looks good to me. I've also left some comments, just for your reference.

Thanks.

xiaohanyu · 2018-10-09T16:35:39Z

@mistercrunch vote up for this PR

victornoel · 2018-10-09T17:21:25Z

@xiaohanyu thanks a lot for the feedbacks, I updated the PR to take your comments into account.

@mistercrunch just one open question is: should superset db upgrade and/or superset init be run everytime superset is started? For example for when schemas change, or there are migrations of the db?

mistercrunch · 2018-10-10T19:25:52Z

These two commands only need to run when upgrading Superset.

victornoel · 2018-10-11T13:46:50Z

@mistercrunch @xiaohanyu I'm having trouble with the current state of the image: for a reason I don't understand, running it started to show me lots of errors such as:

superset_1  | 2018-10-11 13:35:49,625:ERROR:flask_appbuilder.security.sqla.manager:Creation of Permission View Error: (sqlite3.IntegrityError) UNIQUE constraint failed: ab_permission_view.permission_id, ab_permission_view.view_menu_id [SQL: 'INSERT INTO ab_permission_view (permission_id, view_menu_id) VALUES (?, ?)'] [parameters: (6, 19)] (Background on this error at: http://sqlalche.me/e/gkpj)
superset_1  | 2018-10-11 13:35:49,641:ERROR:flask_appbuilder.security.sqla.manager:Add Permission to Role Error: Can't flush None value found in collection Role.permissions

Or problem connecting to AMQP:

[2018-10-11 13:46:07,595: ERROR/MainProcess] consumer: Cannot connect to amqp://guest:**@127.0.0.1:5672//: [Errno 111] Connection refused.

I'm not sure if something changed in the config or something like that for this to happen…

victornoel · 2018-10-11T14:00:39Z

Also @mistercrunch is sync-backend in assets meant to be run during image build? Because I ran it and I noticed that the content of the file is not the same one that is currently committed… that seems strange to me.

SuJiKiNen · 2018-10-15T04:16:46Z

@mistercrunch @xiaohanyu I'm having trouble with the current state of the image: for a reason I don't understand, running it started to show me lots of errors such as:

superset_1  | 2018-10-11 13:35:49,625:ERROR:flask_appbuilder.security.sqla.manager:Creation of Permission View Error: (sqlite3.IntegrityError) UNIQUE constraint failed: ab_permission_view.permission_id, ab_permission_view.view_menu_id [SQL: 'INSERT INTO ab_permission_view (permission_id, view_menu_id) VALUES (?, ?)'] [parameters: (6, 19)] (Background on this error at: http://sqlalche.me/e/gkpj)
superset_1  | 2018-10-11 13:35:49,641:ERROR:flask_appbuilder.security.sqla.manager:Add Permission to Role Error: Can't flush None value found in collection Role.permissions

That means the db already exists,it concerns the .dockerignore file, you built your local sqlite.db into the image.

a better approach is just converting .gitignore file to .dockeriginore file.

this PR need more refinement.

victornoel · 2018-10-15T08:22:44Z

@SuJiKiNen thanks for the feedback. But there is no sqlite db anywhere in the codebase, I never used that feature. Everything has been working without problem until a few days when this error started to appear. I noticed other opened issues about it (#5987), so I wonder if it's not a problem that was introduced lately in master…

For the record, the instructions in this PR to run superset are:

build image
start reddis/postgres containers and run the initialisation within the superset container (create admin, run superset db upgrade, run superset init)
start superset container (and that's when the problem appears).

I will update the dockerignore file nevertheless to not include the things declared in gitignore :)

victornoel · 2018-10-15T08:33:47Z

@SuJiKiNen I found the problem, my bad, there was a mistake with the volume where the configuration was mounted so it was using sqlite instead of posgres. You comment helped me realise that :)

victornoel · 2018-10-15T08:44:04Z

@SuJiKiNen @mistercrunch @xiaohanyu I pushed the latest fixes to the PR. It works well now.

- Improve Docker image - smaller - faster to build - deterministict dependencies (see apache#5958) - Rework process to simplify setting things up - updated documentation - less commands to type - no files to move and modify - optional loading of samples - Still working in standalone mode (without volumes for superset)

victornoel · 2018-11-12T14:20:45Z

@mistercrunch @xiaohanyu @SuJiKiNen I've updated this PR with respect to latest master.

oliviermichaelis · 2018-11-21T12:03:31Z

I tested the docker image with SUPERSET_ENV=production as well as SUPERSET_ENV=development for the last couple of days and it worked flawlessly for me. The build times were shorter and the image size was significantly smaller. Thanks!

KartikTaskhuman

mistercrunch · 2018-11-27T19:19:30Z

This looks great, merging!

victornoel · 2018-11-27T19:24:56Z

yay!

kakoni · 2018-11-27T20:15:27Z

Excellent! Thanks @victornoel !

mwcm · 2018-11-27T20:51:09Z

thank you @victornoel 👍

emacip · 2018-12-19T14:19:27Z

Sorry, @victornoel did you deleted the docker-build.sh?
I just following the https://superset.incubator.apache.org/installation.html# fails because the file does not exist anymore.

victornoel · 2018-12-19T14:30:57Z

@emacip it seems this documentation is not up to date

victornoel · 2018-12-19T14:32:57Z

@emacip see https://github.com/apache/incubator-superset/blob/master/docs/installation.rst

victornoel · 2018-12-19T14:33:15Z

if you find problems, just tell me here and I will try to fix them :)

emacip · 2018-12-19T14:36:57Z

thanks

- Improve Docker image - smaller - faster to build - deterministict dependencies (see apache#5958) - Rework process to simplify setting things up - updated documentation - less commands to type - no files to move and modify - optional loading of samples - Still working in standalone mode (without volumes for superset)

victornoel force-pushed the improve-docker-experience branch 2 times, most recently from 371574f to bd030d5 Compare September 26, 2018 09:45

This was referenced Sep 29, 2018

Error while installing (markdown() takes 1 positional argument but 2 were given) #5958

Closed

Simplifying docker contrib #5802

Closed

Failed testing Docker image #5987

Closed

victornoel force-pushed the improve-docker-experience branch 2 times, most recently from 1edf7ac to 2d17fb6 Compare October 1, 2018 10:16

victornoel mentioned this pull request Oct 3, 2018

build docker image fail #6011

Closed

victornoel force-pushed the improve-docker-experience branch from 2d17fb6 to 8ac8229 Compare October 9, 2018 07:20

xiaohanyu reviewed Oct 9, 2018

View reviewed changes

.gitignore Show resolved Hide resolved

victornoel force-pushed the improve-docker-experience branch from 8ac8229 to fbcc900 Compare October 9, 2018 17:21

victornoel force-pushed the improve-docker-experience branch from fbcc900 to 8f59d2c Compare October 15, 2018 08:43

victornoel force-pushed the improve-docker-experience branch from 8f59d2c to 006b69e Compare October 18, 2018 08:56

jnishiyama mentioned this pull request Nov 7, 2018

Cannot connect to amqp://guest:**@127.0.0.1:5672 #6291

Closed

victornoel force-pushed the improve-docker-experience branch from 006b69e to 12065ec Compare November 12, 2018 14:19

KartikTaskhuman reviewed Nov 22, 2018

View reviewed changes

mistercrunch merged commit 02aa3c6 into apache:master Nov 27, 2018

victornoel deleted the improve-docker-experience branch November 27, 2018 19:24

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.34.0 labels Feb 27, 2024


		# Add a normal user
		RUN useradd --user-group --create-home --shell /bin/bash work

Improve development experience with Docker #5966

Improve development experience with Docker #5966

Conversation

victornoel commented Sep 24, 2018 • edited Loading

victornoel commented Sep 27, 2018

xiaohanyu commented Sep 27, 2018

victornoel commented Sep 30, 2018

victornoel commented Oct 1, 2018

codecov-io commented Oct 1, 2018 • edited Loading

Codecov Report

victornoel commented Oct 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

victornoel Oct 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xiaohanyu commented Oct 9, 2018

xiaohanyu commented Oct 9, 2018

victornoel commented Oct 9, 2018

mistercrunch commented Oct 10, 2018

victornoel commented Oct 11, 2018

victornoel commented Oct 11, 2018

SuJiKiNen commented Oct 15, 2018

victornoel commented Oct 15, 2018

victornoel commented Oct 15, 2018

victornoel commented Oct 15, 2018

victornoel commented Nov 12, 2018

oliviermichaelis commented Nov 21, 2018

KartikTaskhuman left a comment

Choose a reason for hiding this comment

mistercrunch commented Nov 27, 2018

victornoel commented Nov 27, 2018

kakoni commented Nov 27, 2018

mwcm commented Nov 27, 2018

emacip commented Dec 19, 2018

victornoel commented Dec 19, 2018

victornoel commented Dec 19, 2018

victornoel commented Dec 19, 2018

emacip commented Dec 19, 2018

victornoel commented Sep 24, 2018 •

edited

Loading

codecov-io commented Oct 1, 2018 •

edited

Loading

victornoel Oct 11, 2018 •

edited

Loading