Dockerfile with config.json templating - Closes #2053 #2056

fchavant · 2018-05-24T14:19:07Z

What was the problem?

Docker images are were being built from Dockerfile(s) in a different repository and were only configurable to a limited extent.

How did I fix it?

Added Dockerfile to this repository along with a template for config.json.

How to test it?

build docker image from top-level Dockerfile
ensure config.json can be customized by setting environment variables, e.g. LISK_DB_HOST

Review checklist

The PR solves Add Dockerfile to the lisk repository #2053
All new code is covered with unit tests
All new code was formatted with Prettier
Linting passes
Tests pass
Commit messages follow the commit guidelines
Documentation has been added/updated

hendrikhofstadt

@fchavant I appreciate that you added my config proposal/template to the repo here 👍 Is there a reason you are still using the full ubuntu image instead of the much smaller alpine one ?

Also will you completely remove liskhq/lisk-docker and keep the scripts in this repo for better managebility (as we also discussed a submodule approach)

hendrikhofstadt

Adding a peer list to the dockerfile and nethash should be required to create a straightforward build and release process.

hendrikhofstadt · 2018-05-25T04:11:39Z

Dockerfile

+ARG LISK_MIN_VERSION=${LISK_VERSION}
+ENV LISK_VERSION ${LISK_VERSION}
+ENV LISK_MIN_VERSION ${LISK_MIN_VERSION}
+


Is there a reason you are not adding the version specific seed peers here using

ENV LISK_PEERS_LIST_1 "94.237.41.99:5001" ENV LISK_PEERS_LIST_2 "209.50.52.217:5001" ENV LISK_PEERS_LIST_3 "94.237.26.150:5001" ENV LISK_PEERS_LIST_4 "83.136.249.102:5001" ENV LISK_PEERS_LIST_5 "94.237.65.179:5001"

Like here: LiskArchive/lisk-docker@c8b49ff#diff-ed9d43ac74c2d8e77f8497b190a5ab5aR23

For ease of release purposes the version specific nethash and seed peers should be set in this list, shouldn't they ?

If we add see nodes here then this Dockerfile becomes specific to a network (betanet given the peers you listed)

Ah I thought you might want to do this because there are other network specific files on the repo and to maintain compatibility with the old Docker images. But since the Docker image is using the "next" version they are breaking anyway and the proposed adding of an extra file for the peers list is a very good idea.

fchavant · 2018-05-25T08:22:47Z

@MaciejBaj not sure why the build was shown as "failing" (it was not); I started the build again and the checks are now I green.

fchavant · 2018-05-25T08:33:49Z

@SLAMPER

Is there a reason you are still using the full ubuntu image instead of the much smaller alpine one ?

I am concerned the packages that build native code (e.g. sodium) might not work (properly) -- I just haven't had time to look into it.

Also will you completely remove liskhq/lisk-docker and keep the scripts in this repo for better managebility (as we also discussed a submodule approach)

In a first step we'll probably keep the docker-compose based examples in the lisk-docker repository.

diego-G

How about the Docker Cloud check? Is this going to be mandatory for every PR or is optional?

diego-G · 2018-05-25T15:18:01Z

docker_files/etc/confd/templates/config.json.tmpl

+	},
+	"peers": {
+		"enabled": {{getv "/lisk/peers/enabled" "true"}},
+		"list": [


Is it possible to insert another variable which it'd read a file having the peers list? We need to bear in mind the goal of having an network agnostic template.

If such a file were to be introduced I suppose we could. Another option would be to add an ARG and set it at build time.

As discussed, we will add a new file containing the list of peers when we tackle #398.

hendrikhofstadt · 2018-05-25T16:52:09Z

@fchavant
The PR really looks good.

Another thing about nethash and seed peers. I think it's great that this is decoupled from the repo, which is a good step towards #398. Will you then add the required config variables to the docker-compose example.

This could be a good chance to you an .env-file for docker-compose which is network specific so the docker-compose.yml is not bloated and we have more decoupling.

About the Alpine image. I agree that there should be some testing before trying it out.
I tested a full Alpine version using confd some time ago and it worked just perfectly and had a much smaller size (about 70MB).

I can also open a PR with the adapted dockerfile so you can save time on it and request changes if you think any are necessary.

hendrikhofstadt · 2018-05-26T04:46:07Z

Dockerfile

+WORKDIR /home/lisk/lisk
+
+RUN npm install
+RUN ./node_modules/.bin/grunt release


What's the purpose of doing grunt release ? I see that it adds files that mark the revision which can help identify the version but it also creates a packaged tarball and copies it to a release directory creating unnecessary bulk for the docker image.

If the build and revision files are necessary grunt build/revision should be enough and not create unnecessary files.

Still the best would be to do npm install --production to omit all the unnecessary devDependencies (including grunt and other build utils)

@diego-G can you chime in on this?

@fchavant I agree we don't need grunt release in the docker image. Regarding to the other comment, I prefer to keep npm install without --production flag. But It is true we should use NODE_ENV environment variable to signalise what kind of installation we want in the image. By default npm listens it, not installing devDependencies if it is set to production.

hendrikhofstadt · 2018-05-26T05:54:46Z

@fchavant If you want to test the alpine image, here is a version of my alpine Dockerfile that I adapted with your changes: https://gist.github.com/Slamper/f96105fd4a130b15fd301c4315956fcf

It also omits the chown and uses COPY --chown instead which makes the build much quicker.

fchavant · 2018-05-28T06:36:55Z

@SLAMPER thanks; I'll have a look as soon as everything is done here. The reason I am not using the --chown flag is that it's not supported in docker cloud yet (it's running docker version 17.06.2-ee-6)
It might make sense to add a comment so that we can change this in the future.

nazarhussain · 2018-05-28T10:50:04Z

docker_files/home/lisk/run.sh

+#!/bin/bash
+
+confd -backend env -onetime
+node app.js


Why we are not using lisk.sh script here, if possible we should use it for consistency across distribution platforms.

We are not using lisk.sh here because

it is meant to manage an all-in-one installation which this is not and

it is meant to be run interactively: it starts processes and then returns which is not what the PID 1 of a container should do

nazarhussain · 2018-05-28T10:50:50Z

docker_files/home/lisk/run.sh

+#!/bin/bash
+
+confd -backend env -onetime
+node app.js


Normally I noticed entrypoint.sh as startup file in different docker images, should we not follow that naming convention.

Often a docker-entrypoint.sh script is used in combination with ENTRYPOINT; note that we use CMD however.

fchavant · 2018-05-29T08:18:04Z

@diego-G the ci/dockercloud check will only run for tags, not PR (except this one)

nazarhussain · 2018-06-05T14:14:23Z

Dockerfile

@@ -0,0 +1,53 @@
+FROM node:6 AS builder
+
+ENV ENV_NODE=production


Convention is to use NODE_ENV, many libraries use this convention to check environment variable for production use case. So better to rename it.

nazarhussain · 2018-06-05T14:19:00Z

Dockerfile

+
+ENV ENV_NODE=production
+
+RUN groupadd --gid 1100 lisk && \


I can't find any reference for using 1100 gid specific to distro. Why we are hardcoding the group id. What if some dependent docker container use the same group id?

Would it not be nice to add group only with name and then get its id afterwards. This may be helpful getent group lisk

The GID is not distribution specific. We chose 1100 arbitrarily.

What if some dependent docker container use the same group id?
That would be no problem in a different container. I you mean a container that mounts a volume from this one then that'd allow having the "same" lisk user (same UID and GID).

nazarhussain · 2018-06-05T14:23:08Z

Dockerfile

+ENV ENV_NODE=production
+
+RUN groupadd --gid 1100 lisk && \
+    useradd --create-home --home-dir /home/lisk --shell /bin/bash --uid 1100 --gid 1100 lisk


Same applies to uid here.

nazarhussain · 2018-06-05T14:26:33Z

Dockerfile

+    useradd --create-home --home-dir /home/lisk --shell /bin/bash --uid 1100 --gid 1100 lisk
+# As of May 2018 cloud.docker.com runs docker 17.06.1-ce
+# however version 17.12 is required to use the chown flag
+COPY . /home/lisk/lisk/


We shouldn't just copy every thing to docker. First run grunt release and copy the compiled source code. You may not find any noticeable difference, but any future change in Gruntfile may break the docker. So better to use compile release after grunt.

This is the build container, we must copy everything in order to be able to run npm install or grunt release. Regarding the latter I was told to remove it; @diego-G can you please confirm once more?

@fchavant @diego-G I am sure we need to run grunt release and copy only the archive files to container.

@fchavant We can run npm install and grunt release on host machine, where we are actually building container. I think we don't need a seperate container just to run grunt release.

I think we don't need a seperate container just to run grunt release.

It is a way to have a clean build environment w/o polluting the final containers with build time dependencies.

I am sure we need to run grunt release and copy only the archive files to container.

What would be the advantage in using the tarball grunt creates? It's meant to be consumed by lisk-build's build.sh script which makes sense for all-in-one release but not docker.

@fchavant Nothing will be polluted, if you run grunt release on host machine and copy the archive build file to container. It will not contain node_modules directory. So you will do a clean npm install in container.

1/ I don't want to run anything on the host: that way anyone can build the image (also in Docker Cloud)
2/ by using a build container the final image does not contain any of the build dependencies (e.g. gcc)

A nice bonus of using a build container is that we always build in a clean environment.

nazarhussain · 2018-06-05T14:28:51Z

Dockerfile

+RUN npm install
+
+
+FROM node:6


Duplicate line.

This is a different container (the final one, not the build container)

nazarhussain · 2018-06-05T14:31:22Z

docker_files/etc/confd/templates/config.json.tmpl

+	"httpPort": {{getv "/lisk/httpport" "8000"}},
+	"address": "{{getv "/lisk/address" "0.0.0.0"}}",
+	"version": "{{getenv "LISK_VERSION"}}",
+	"minVersion": "{{getenv "LISK_MIN_VERSION"}}",


If these env variables meant to run container with different min values, then I believe its not the right case. Once the docker container is built, its built for a specific version, which is bound to a specific minVersion. User should not be allowed to change those on runtime.

Once the docker container is built, its built for a specific version, which is bound to a specific minVersion.
That is exactly what will happen, those environment variables will come from the docker image (see https://github.com/LiskHQ/lisk/blob/83de84142be1c7603941278be0d51f0a6c833abb/Dockerfile#L44)

nazarhussain · 2018-06-05T14:34:43Z

docker_files/etc/confd/templates/config.json.tmpl

+	"trustProxy": {{getv "/lisk/trustproxy" "false"}},
+	"topAccounts": {{getv "/lisk/topaccounts" "false"}},
+	"cacheEnabled": {{getv "/lisk/cacheenabled" "false"}},
+	"wsWorkers": {{getv "/lisk/wsworkers" "1"}},


Why these variables are all small case? We follow either camel or snake can for readability.

confd will convert those depeding on the chosen backend: environment variables in our case. That means /lisk/wsworker will become LISK_WSWORKER and capitalization will get lost.

nazarhussain · 2018-06-05T14:35:36Z

docker_files/etc/confd/templates/config.json.tmpl

+		"host": "{{getv "/lisk/redis/host" "127.0.0.1"}}",
+		"port": {{getv "/lisk/redis/port" "6380"}},
+		"db": {{getv "/lisk/redis/db" "0"}},
+		"password": {{getv "/lisk/redis/password" "null"}}


I believe here you want to set null instead of "null", consider null without quote means no password. But if you quote it will check for that password.

This will output null not "null": notice there are not double quotes around the expression in double curly braces (compare to host for instance)

nazarhussain · 2018-06-05T14:39:43Z

docker_files/etc/confd/templates/config.json.tmpl

+	},
+	"peers": {
+		"enabled": {{getv "/lisk/peers/enabled" "true"}},
+		"list": [


This is the right use case for configurable values. User may want load its own list of peers. Can't understand why its should be done separately. Issue #398 is totally different thing, to support multiple networks in one installation.

nazarhussain · 2018-06-05T14:53:01Z

docker_files/home/lisk/wait-for-it.sh

@@ -0,0 +1,177 @@
+#!/usr/bin/env bash


This file is not relevant to lisk docker image, so it should not be included here. Even we can't see any usage of this script file in our Dockerfile.

Its dependency is used by docker-composer under lisk-docker repo, so this script should be kept and managed there.

I thought on the topic further. If you really want lisk container to wait for db port to be available before running. Then use this script inside run.sh not inside the docker-compose. This will make sure that our image will always have this behavior. And then it also makes sense to make this script file part of this repo.

That is exactly what I plan on doing, like so:
https://github.com/LiskHQ/lisk-docker/blob/2.1.0/examples/betanet/docker-compose.yml#L15

No you are running the script in docker-compose.

Please do it in run.sh and in compose start the container. The run.sh file should have usage of wait-for-it.sh to wait for db to appear online and then run node app.js

If we were to call wait-for-it.sh in run.sh then

it would not be optional any more and

passing parameters (host and port of the db to wait for) would be akward.

I think it makes sense to keep it where optional and as it is right now (wait-for-it.sh waiting for the DB and then calling run.sh)

If its an optional then don't copy it in container through docker file.

That's clearly seems a utility script used in docker-compose, then keep it there, mount the utility scripts volume to container and run it.

If something not required or not related to an image, don't copy it.

By optional I mean one can use it but one does not have to.
To give this possibility the script needs to be in the image though (the lisk container must wait for the postgres container; having it somewhere else would not work.

the lisk container must wait for the postgres container
Then make it part of run.sh

Having a utility script in image, but using it somewhere else (composer) does not make sense to me.

If we make it part of run.sh the it becomes very much non optional. Also it would make passing parameters hard; see above.

Having a utility script in image, but not using it somewhere else (composer) does not make sense to me.

We have a script in the image, it can be used in one's docker-compose.yml or by swarm or rancher or whatever. Or one may choose not to use it. That's why I call it "optional".

@fchavant Ok please document it somewhere our docker image have this script and the usage of this script.

nazarhussain

Please document it somewhere our docker image have waitf-for-it script and its usage.

fchavant added the 👁️ pending review label May 24, 2018

fchavant self-assigned this May 24, 2018

fchavant requested a review from diego-G May 24, 2018 14:19

fchavant changed the title ~~Dockerfile with config.json templating~~ Dockerfile with config.json templating - Closes #2053 May 24, 2018

MaciejBaj added 🛑 failing and removed 👁️ pending review labels May 24, 2018

hendrikhofstadt reviewed May 25, 2018

View reviewed changes

hendrikhofstadt suggested changes May 25, 2018

View reviewed changes

fchavant added 🏗️ in progress and removed 🛑 failing labels May 25, 2018

fchavant force-pushed the 2053-dockerfile-config-template branch from 4176dae to 2adbbae Compare May 25, 2018 11:29

Dockerfile with config.json templating

858c44e

fchavant force-pushed the 2053-dockerfile-config-template branch from 2adbbae to 858c44e Compare May 25, 2018 13:01

diego-G suggested changes May 25, 2018

View reviewed changes

hendrikhofstadt approved these changes May 25, 2018

View reviewed changes

hendrikhofstadt reviewed May 26, 2018

View reviewed changes

Add comment regarding (non) usage of chown flag for COPY

27a1700

nazarhussain reviewed May 28, 2018

View reviewed changes

fchavant added 👁️ pending review and removed 🏗️ in progress labels May 29, 2018

Set NODE_ENV=production; do not run grunt release

83de841

fchavant force-pushed the 2053-dockerfile-config-template branch from b0d2ced to 83de841 Compare May 30, 2018 13:28

MaciejBaj requested a review from diego-G June 5, 2018 13:46

diego-G approved these changes Jun 5, 2018

View reviewed changes

nazarhussain suggested changes Jun 5, 2018

View reviewed changes

francoilol added 2 commits June 6, 2018 09:44

Use NODE_ENV instead of ENV_NODE

6d9cd62

Correct typo, update comment, minor change to Dockerfile

2dfd350

fchavant force-pushed the 2053-dockerfile-config-template branch from 690a39e to 2dfd350 Compare June 6, 2018 08:38

nazarhussain approved these changes Jun 6, 2018

View reviewed changes

Add documentation for docker build

f256f10

fchavant mentioned this pull request Jun 6, 2018

Build from versioned tarballs LiskArchive/lisk-docker#86

Closed

Set version and minVersion from original config.json at build time

958ee97

MaciejBaj merged commit 38beb73 into 1.0.0-beta.8 Jun 6, 2018

MaciejBaj deleted the 2053-dockerfile-config-template branch June 6, 2018 16:58

This was referenced Jun 8, 2018

Remove Dockerfile, keep examples only. LiskArchive/lisk-docker#88

Closed

Remove Dockerfile - Closes #88 LiskArchive/lisk-docker#89

Merged

MaciejBaj added ✅ ready and removed 👁️ pending review labels Jun 21, 2018

		@@ -0,0 +1,53 @@
		FROM node:6 AS builder

		ENV ENV_NODE=production

Dockerfile with config.json templating - Closes #2053 #2056

Dockerfile with config.json templating - Closes #2053 #2056

Conversation

fchavant commented May 24, 2018

What was the problem?

How did I fix it?

How to test it?

Review checklist

hendrikhofstadt left a comment

Choose a reason for hiding this comment

hendrikhofstadt left a 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

fchavant commented May 25, 2018 • edited Loading

fchavant commented May 25, 2018

diego-G left a 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

hendrikhofstadt commented May 25, 2018

hendrikhofstadt May 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diego-G May 28, 2018 • edited Loading

Choose a reason for hiding this comment

hendrikhofstadt commented May 26, 2018

fchavant commented May 28, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fchavant May 28, 2018 • edited Loading

Choose a reason for hiding this comment

fchavant commented May 29, 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

Choose a reason for hiding this comment

fchavant Jun 6, 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

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

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

Choose a reason for hiding this comment

nazarhussain Jun 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nazarhussain left a comment

Choose a reason for hiding this comment

fchavant commented May 25, 2018 •

edited

Loading

hendrikhofstadt May 26, 2018 •

edited

Loading

diego-G May 28, 2018 •

edited

Loading

fchavant commented May 28, 2018 •

edited

Loading

fchavant May 28, 2018 •

edited

Loading

fchavant Jun 6, 2018 •

edited

Loading

nazarhussain Jun 6, 2018 •

edited

Loading