Add an official dockerfile #887

brownleej · 2018-11-01T17:13:45Z

We've had previous discussions and PRs about this, but I wanted to give it a fresh start. This image is taken from my local testing setup, and I think it is suitable for general testing use. I've tried to keep it pretty simple so that we can iterate on it and get it to something that we can commit to as a stable image.

Adds support for custom process classes in the docker image. Makes the scripts more robust against some variations in the environment.

alexmiller-apple

I apologize for the slow review.

I am all for any amount of incremental progress in this area, and having a docker image available for local testing sounds like a great improvement. #355 seemed to have a broader goal of supplying a docker image that could be used to run FDB in "production". Did you have a solid enough understanding of #355 to be able to describe what features existed in that PR that aren't present in this one (if any)? Is there any plan as to what will or will not be supported?

packaging/docker/create_cluster_file.bash

alexmiller-apple · 2018-11-07T00:09:51Z

packaging/docker/Dockerfile

+RUN chmod u+x fdb*
+RUN mv fdb* /usr/bin
+WORKDIR /var/fdb
+RUN rm -r tmp


Are these sorts of chains of commands supposed to be in one RUN line and chained with && ? I honestly don't understand dockerfile best practices.

Yes. That's the case because every RUN or any other command creates a new layer, which usually results in bigger overall image size. Just to illustrate:

... ADD $FDB_WEBSITE/downloads/$FDB_VERSION/linux/fdb_$FDB_VERSION.tar.gz . RUN tar -xzf fdb_$FDB_VERSION.tar.gz --strip-components=1 RUN rm fdb_$FDB_VERSION.tar.gz RUN chmod u+x fdb* RUN mv fdb* /usr/bin WORKDIR /var/fdb RUN rm -r tmp ...

Every line creates a new layer, meaning that even if have done rm fdb_$FDB_VERSION.tar.gz and rm -r tmp, the intermediate layers would still keep this files. This is effectively just a junk, because no one can access this file in the final image. Hence it's recommended to produce fewer number of layers by chaining commands:

RUN wget $FDB_WEBSITE/downloads/$FDB_VERSION/linux/fdb_$FDB_VERSION.tar.gz \ && tar -xzf fdb_$FDB_VERSION.tar.gz --strip-components=1 \ && rm fdb_$FDB_VERSION.tar.gz \ && chmod u+x fdb* \ && mv fdb* /usr/bin \ && cd /var/fdb \ && rm -r tmp

I've restructured the dockerfile a bit to reduce the number of layers.

I'm also thinking that in a future enhancement we may want to use a multistage build and just copy the binaries and libraries from an earlier stage into the final stage to reduce layering.

packaging/docker/README.md

alexmiller-apple · 2018-11-07T00:55:44Z

packaging/docker/fdb.bash

+# limitations under the License.
+#
+
+source $(/var/fdb/scripts/create_server_environment.bash)


I'm finding this a little weird for a couple reasons:

The failure handling here seems a little questionable. create_server_environment.bash will fail by outputting an error message, and then exit 1. This line will then try to execute that error message and source it.

create_server_environment.bash has a lot more side-effects than things that I generally see used in this form.

I'd suggest outputting to stderr in your scripts, which should just be tacking on a 1>&2 to the end of your echo lines for error messages.

However, reading through the PR further, I see why you preferred to have them as separate files instead of a library of code to source, as then it's easy to copy a single script to re-use and run in the client. However, calling it as a process and not as a library means that the exit 1 doesn't kill the current process as well, as it probably should.

You can either solve this via running set -e beforehand, which I've generally found to be brittle, or stealing a trick to be able to distinguish sourced vs directly run files, similar to python's if __name__ == '__main__'

if [[ "${BASH_SOURCE[0]}" == "${0}" ]]; then main "$@" fi

so that you can use the scripts either as callable standalone, or source-able and callable as a function.

Does that seem any better to you?

That makes sense. I've moved the cluster file creation logic into a function and redirected more outputs to standard error.

Ah, I had actually meant this line though. I see that you've moved create_cluster_file into a source-able function, but if /var/fdb/scripts/create_server_environment.bash does exit 1, we'll end up running source, which errors, and then continue onto starting up fdbserver below. I think it'd be clearer if we could just exit here. Could you do the same transformation to create_server_environment?

Add more copyright headers to the docker sample files. Make the cluster file creation logic in the docker image into a function.

bluk

Tried out this branch to attempt some automation. Hope you don't mind the random comments.

bluk · 2018-11-16T01:16:01Z

packaging/docker/README.md

+
+The port that FoundationDB should bind to. The default is 4500. 
+
+### FDB_NETWORKING


This seems to have changed from FDB_NETWORKING to FDB_NETWORKING_MODE.

Suggested change

### FDB_NETWORKING

### FDB_NETWORKING_MODE

bluk · 2018-11-16T01:16:22Z

packaging/docker/README.md

+
+This directory provides a Docker image for running FoundationDB.
+
+The image in this directory is based on Ubuntu 18.0.4, but the commands and


Minor nitpick.

Suggested change

The image in this directory is based on Ubuntu 18.0.4, but the commands and

The image in this directory is based on Ubuntu 18.04, but the commands and

bluk · 2018-11-16T01:21:44Z

packaging/docker/create_server_environment.bash

+	public_ip=127.0.0.1
+	FDB_CLUSTER_FILE_CONTENTS="docker:docker@$public_ip:4500"
+elif [[ "$FDB_NETWORKING_MODE" == "container" ]]; then
+	public_ip=$(grep `hostname` /etc/hosts | sed -e "s/\s *`hostname`.*//")


Should the FDB_CLUSTER_FILE_CONTENTS be set here as well? I tried:

docker build . -t fdb:latest --build-arg FDB_VERSION=5.2.5 docker run -ti --rm -e "FDB_NETWORKING_MODE=container" -e "FDB_PORT=4500" fdb:latest

I got:

Starting FDB server on 172.17.0.2:4500 Invalid cluster file `/var/fdb/fdb.cluster': 2104 Connection string invalid Error: Connection string invalid

Adding the FDB_CLUSTER_FILE_CONTENTS line 32 in this elsif block seemed to make things better but I'm not too familiar with FDB to know if this should have been written out by default and should "just work" or my setup is just wrong.

Yes, good catch! I think I was trying to accomplish this through the FDB_COORDINATOR option, but the DNS resolution wasn't working properly, so I've tweaked this to use FDB_CLUSTER_FILE_CONTENTS in both networking modes.

bluk · 2018-11-16T01:27:27Z

packaging/docker/Dockerfile

+ARG FDB_WEBSITE=https://www.foundationdb.org
+
+WORKDIR /var/fdb/tmp
+RUN wget $FDB_WEBSITE/downloads/$FDB_VERSION/linux/fdb_$FDB_VERSION.tar.gz && \


You may want to consider adding a SHA256 checksum check here. Adding another argument after line 31 like:

ARG FDB_VERSION_SHA256SUM

and then running the check in the RUN block on line 35.

RUN wget $FDB_WEBSITE/downloads/$FDB_VERSION/linux/fdb_$FDB_VERSION.tar.gz && \ echo "${FDB_VERSION_SHA256SUM} fdb_${FDB_VERSION}.tar.gz" > checksum && sha256sum -c checksum && \ tar -xzf fdb_$FDB_VERSION.tar.gz --strip-components=1 && \

Then running:

docker build . -t fdb:latest --build-arg FDB_VERSION=5.2.5 --build-arg FDB_VERSION_SHA256SUM=275b9f541590d145136061cee6427103b347e1ff0e851b6556671e46102c1ad9

Just a "maybe nice to have".

ofek · 2018-11-20T05:53:56Z

@brownleej Thanks so much for doing this! Do you think you'll have time this week to address the final review comments?

mexisme · 2018-11-21T19:33:27Z

I'm also thinking that in a future enhancement we may want to use a multistage build and just copy the binaries and libraries from an earlier stage into the final stage to reduce layering.

I'd love to see that.
I might be able to do a follow-up PR for it, if that helps?

…the docker image for the two different networking options. Fixes some inaccurate text in the docker image README.

…urceable function. Adds an explicit coordinator container in the docker-compose sample to support recent changes in the name generation in docker-compose. Bump a dependency version in the dockerfile. Improves handling of coordinator lookup failures in the docker scripts.

alexmiller-apple

Did you still want @AlvinMooreSr to approve, or are we good to merge?

ofek · 2018-11-30T07:59:08Z

This is very exciting! Will an image be released soon after merging as well?

jdotjdot · 2018-11-30T20:23:37Z

Not only an image, but if there were instructions on how to start a production-ready multi-node FoundationDB cluster with Docker, that would be great. Given the lack of an existing AMI/Cloudformation/Terraform set up, at least with this Docker image, a docker-compose, Docker Swarm, or Kubernetes example template could be set up.

alexmiller-apple · 2018-12-01T02:08:40Z

@jdotjdot There was an example of terraform scripts posted on the forums.

AlvinMooreSr

LGTM

brownleej · 2018-12-03T21:47:47Z

@jdotjdot We're definitely interested in building out more examples on how you would run this image in production on something like Kubernetes. I'm hoping to be able to put something like that together this month; it will probably go along with more work on the image itself.

lxcid · 2018-12-03T21:51:53Z

Looking forward to it!

Add an official dockerfile

brownleej added 3 commits September 9, 2018 16:28

Adds a dockerfile for building an image to run FoundationDB.

6e076df

Adds a sample docker-compose setup that uses the new docker image.

ab60d94

Adds support for basic locality information in the docker image.

dac41b8

Adds support for custom process classes in the docker image. Makes the scripts more robust against some variations in the environment.

This was referenced Nov 1, 2018

Added preliminary docker image #355

Closed

Create an official foundationDB image on DockerHub #300

Closed

AlvinMooreSr assigned AlvinMooreSr and alexmiller-apple Nov 1, 2018

AlvinMooreSr requested review from AlvinMooreSr and alexmiller-apple November 1, 2018 17:22

alexmiller-apple suggested changes Nov 7, 2018

View reviewed changes

Combine multiple run commands into one command in the dockerfile.

60908f7

Add more copyright headers to the docker sample files. Make the cluster file creation logic in the docker image into a function.

bluk reviewed Nov 16, 2018

View reviewed changes

brownleej added 2 commits November 25, 2018 16:47

Ensures that we set the cluster file contents in a consistent way in …

bbf7a44

…the docker image for the two different networking options. Fixes some inaccurate text in the docker image README.

alexmiller-apple approved these changes Nov 27, 2018

View reviewed changes

apkar mentioned this pull request Dec 3, 2018

Create official Docker image on Docker Hub FoundationDB/fdb-document-layer#13

Closed

AlvinMooreSr approved these changes Dec 3, 2018

View reviewed changes

brownleej merged commit 33d2ad5 into apple:master Dec 3, 2018

etschannen pushed a commit to etschannen/foundationdb that referenced this pull request Mar 26, 2019

Merge pull request apple#887 from brownleej/packaged-dockerfile

3b4bc64

Add an official dockerfile

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add an official dockerfile #887

Add an official dockerfile #887

brownleej commented Nov 1, 2018

alexmiller-apple left a comment

alexmiller-apple Nov 7, 2018

alvelcom Nov 8, 2018

brownleej Nov 14, 2018

brownleej Nov 14, 2018

alexmiller-apple Nov 7, 2018

brownleej Nov 14, 2018

alexmiller-apple Nov 27, 2018

bluk left a comment

bluk Nov 16, 2018

bluk Nov 16, 2018

bluk Nov 16, 2018

brownleej Nov 26, 2018

bluk Nov 16, 2018

ofek commented Nov 20, 2018

mexisme commented Nov 21, 2018

alexmiller-apple left a comment

ofek commented Nov 30, 2018

jdotjdot commented Nov 30, 2018

alexmiller-apple commented Dec 1, 2018

AlvinMooreSr left a comment

brownleej commented Dec 3, 2018

lxcid commented Dec 3, 2018


		The port that FoundationDB should bind to. The default is 4500.

		### FDB_NETWORKING


		This directory provides a Docker image for running FoundationDB.

		The image in this directory is based on Ubuntu 18.0.4, but the commands and

Add an official dockerfile #887

Add an official dockerfile #887

Conversation

brownleej commented Nov 1, 2018

alexmiller-apple 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

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

bluk 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ofek commented Nov 20, 2018

mexisme commented Nov 21, 2018

alexmiller-apple left a comment

Choose a reason for hiding this comment

ofek commented Nov 30, 2018

jdotjdot commented Nov 30, 2018

alexmiller-apple commented Dec 1, 2018

AlvinMooreSr left a comment

Choose a reason for hiding this comment

brownleej commented Dec 3, 2018

lxcid commented Dec 3, 2018