Add a Dockerfile for synapse #2846

kaiyou · 2018-02-03T19:26:12Z

This conflicts with #2482 and we should discuss merging the two patches before going further.

Changes

Issues

SQL schema files are not copied properly when building
The process is running as root
Travis build is failing

Signed-off-by: Pierre Jaury pierre@jaury.eu

matrixbot · 2018-02-03T19:26:13Z

Can one of the admins verify this patch?

matrixbot · 2018-02-03T19:26:13Z

Can one of the admins verify this patch?

kaiyou · 2018-02-04T13:24:14Z

The image is working properly, I'll try and deploy it in production tonight. The PR still need a proper turn server working alongside with synapse in the example.

turt2live

I'm by no means a docker expert, but here's a few things I noticed.

turt2live · 2018-02-05T21:00:24Z

contrib/docker/README.md

+    --name synapse \
+    -v ${DATA_PATH}:/data \
+    -e SYNAPSE_SERVER_NAME=my.matrix.host \
+    matrixdotorg/synapse:v0.22.1


This should probably be pointed at master, otherwise the version will be forgotten about and a bunch of old servers will be spun up.

Pointing at master sounds a bit risky as well. Maybe point to latest, that the team will have a responsibility to point to the current working version?

master is effectively latest, but yes: latest would be safer.

Oh yes, I had forgotten about that regarding this repo. But sure, let's go for latest anyway.

turt2live · 2018-02-05T21:00:49Z

contrib/docker/README.md

+The image expects a single volume, located at ``/data``, that will hold:
+
+* temporary files during uploads;
+* uploaded media and thumbnais;


*thumbnails

turt2live · 2018-02-05T21:02:21Z

contrib/docker/README.md

+* ``UID``, the user id Synapse will run as [default 991]
+* ``GID``, the group id Synapse will run as [default 991]
+
+Synapse specific settings:


An environment variable for the cache factor would be neat, although most people aren't going to need it.

The environment variable SYNAPSE_CACHE_FACTOR is supported by default if passed to the container. Just added some documentation about it.

uhoreg · 2018-02-05T21:32:16Z

Dockerfile

+
+VOLUME ["/data"]
+
+EXPOSE 8448


should also EXPOSE 8008

uhoreg · 2018-02-05T21:33:27Z

Dockerfile

+ && pip install --upgrade pip setuptools psycopg2 \
+ && mkdir -p /synapse/cache \
+ && pip install -f /synapse/cache --upgrade --process-dependency-links . \
+ && mv /synapse/contrib/docker/* / \


It seems strange to me to litter / with so many files. Why not just keep them all there?

I usually prefer to keep short and simple path for files in Docker images, like /start.py and /conf. But you are right, other files should not be copied.

uhoreg · 2018-02-05T21:37:31Z

contrib/docker/conf/homeserver.yaml

+#   notif_for_new_users: True
+#   riot_base_url: "http://localhost/riot"
+
+enable_group_creation: true


Would be nice to have some way to add appservices somehow (maybe in a future version of the Dockerfile).

Definitely agree, I was thinking maybe have all yaml files in a folder in /data be included as appservices.

Just added this in the latest commit.

Wouldn't it make a lot more sense to let the regular generate command take care of this? Having a complete config file here feels like duplicating what the generate command does. Adding the used environment variables as either command line flags to the generate command or having the generate command read the env vars itself would make this a lot more portable.

I want the image to have two different modes of operation: either generating and customizing a config file, or using the dnyamically generated one. I agree that the dynamically generated one could be populated using the generate command instead of Jinja inside the start script.

I'd say this is room for later improvement, as I'm not experimented enough to play with Synapse core code yet :)

uhoreg · 2018-02-05T21:46:23Z

contrib/docker/start.py

+# Parse the configuration file
+if not os.path.exists("/compiled"): os.mkdir("/compiled")
+convert("/conf/homeserver.yaml", "/compiled/homeserver.yaml", environ)
+convert("/conf/log.config", "/compiled/%s.log.config" % environ.get("SYNAPSE_SERVER_NAME"), environ)


I guess it should only do these if SYNAPSE_CONFIG_PATH is not set, and if mode is not generate

Indeed, harmless but will save some milliseconds.

neilisfragile · 2018-03-28T13:55:55Z

Hi @gwend4l @michaelkaye has joined the core team in an SRE capacity - he needs to take a look before merge - but it is on his immediate todo list

Ezwen · 2018-03-30T10:43:38Z

sounds great! then good luck @michaelkaye :)

michaelkaye · 2018-04-20T14:59:49Z

Hi there @kaiyou - apologies for it taking so long to get back to you.

Currently we've been doing the packaging of synapse out of the main tree - for instance the Debian build lives over https://github.com/matrix-org/package-synapse-debian in it's own packaging.

I know that makes it a bit more of a pain to package, but at the moment we want to keep things separated.

I've gone and made a new repository in the matrix-org org to host this : https://github.com/matrix-org/matrix-synapse-docker - if we can move the code over to be a PR against that then I think we're happy to merge it in.

Once we've got the image building, I'd like to get some automation on builds into the docker-hub repos, and we can make doing a commit to bump the version part of our general synapse release process.

Cheers,
Michael

michaelkaye · 2018-04-20T15:05:07Z

It is probably worth also mentioning that I'd like to find a better way for these to be configured than the current (slightly fragile) way of writing a new config file on the fly. It's something that needs a bit of careful thought and probably will involve splitting out the way the configuration parsing is happening, as not everyone is deploying synapse now by installing it then running it immediately for the configuration file. Still, something for the future, that.

turt2live · 2018-04-20T15:11:00Z

contrib/docker/README.md

+* ``SYNAPSE_SERVER_NAME`` (mandatory), the current server public hostname.
+* ``SYNAPSE_REPORT_STATS``, (mandatory, ``yes`` or ``no``), enable anonymous
+  statistics reporting back to the Matrix project which helps us to get funding.
+* ``SYNAPSE_MACAROON_SECRET_KEY`` (mandatory) secret for signing access tokens


iirc this can't change after initially being set otherwise synapse can't understand previous access tokens.

Yes, I did hesitate with a default behavior generating a random value and storing it to the data dir.

Some added documentation to say "don't ever change this" would be nice :)

kaiyou · 2018-04-21T08:37:53Z

I must admit I have a strong opinion about keeping the Dockerfile inside the repository, so it evolves with it and one can build any commit with a simple checkout.

Regarding the configuration being generated on the fly, I find it nice to have both options, which the current Dockerfile does provide, so that inexperienced users can still run an instance with sane defaults for a small-sized server.

jcgruenhage · 2018-04-21T10:49:47Z

For building the specific commit, you can make it into a build arg in the docker file and pass it when building the container. That even has the advantage that you can build an older version where the Dockerfile wasn't merged.

kaiyou · 2018-04-21T10:51:54Z

Except the Dockerfile depends on the content of the repo itself. Now it is running on Python2, maybe tomorrow Synapse will run on Python3, maybe some dependencies will change and require library headers, specific library versions, etc. My experience with Dockerfiles is: keeping them outside the main repository is a burden.

Ezwen · 2018-04-29T16:25:05Z

contrib/docker/README.md

+
+Database specific values (will use SQLite if not set):
+
+* `POSTGRES_DATABASE` - The database name for the synapse postgres database. [default: `matrix`]


Shouldn't that be POSTGRES_DB, as in https://github.com/kaiyou/synapse/blob/feat-dockerfile/contrib/docker/conf/homeserver.yaml#L55 ?

If so, the default also seems to be "synapse" too.

kaiyou · 2018-05-02T06:02:02Z

The latest build is affected by #3135, I will fix tonight.

almereyda · 2018-05-02T10:09:30Z

@kaiyou If you're at it, please also consider to include the recent security release https://github.com/matrix-org/synapse/releases/tag/v0.28.1

I'm going to update matrix.allmende.io from 0.26 to your branch in the next days and can provide feedback then.

michaelkaye · 2018-05-04T15:10:14Z

Hey, so having had a bit of a think about what would be good as a whole for packaging, and a chat with a few people, and also seeing the support for in-tree packaging in the comments here.

The logic about "the debian packaging and the docker packaging should be supported to the same level" and be treated similarly still applies - but maybe that actually means bringing both into this repository (or in the case of debian, into a branch in the repository).

What I suppose we were worried about is a free for all for all the varied packaging solutions that are around - we can't afford to support all the options that are out there, so limiting ourselves to tarballs, docker images and debian packages seems sensible - things we want to make sure are pushed as part of a synapse release.

tl;dr:

@kaiyou - thank you very much for your work and for waiting - Can you have a look at https://github.com/matrix-org/synapse/blob/master/CONTRIBUTING.rst and make an appropriate entry in the PR description, and we'll be able to merge once that's done.

michaelkaye · 2018-05-04T15:11:22Z

I should probably also ask @jcgruenhage to do something about the status of his review - i believe the comments there have been addressed?

michaelkaye · 2018-05-04T16:42:44Z

Probably also worth thinking about what we'll be doing to push these packages. I was thinking to make something visible on docker.io/matrixdotorg/synapse we setup the automated builds there to trigger only when there's a new tag pushed - eg "v*".

If we run into release problems with this, we can just make a little bugfix branch and push the matrixdotorg image out by hand, so they'll be available there.

kaiyou · 2018-05-04T19:00:32Z

Docker cloud will basically build every branch and tag automatically, or you can provide a filtering regex.

I signed off the pull request. Also, the PR is based on master because the idea was to build the stable synapse, but it should merge just as fine into develop.

jcgruenhage · 2018-05-05T12:14:46Z

@michaelkaye must have forgottten to set it to approved back when I did the review and @kaiyou changed what I wanted

@kaiyou

Changes in synapse v0.29.1 (2018-05-17) ========================================== Changes: * Update docker documentation (PR #3222) Changes in synapse v0.29.0 (2018-05-16) =========================================== Not changes since v0.29.0-rc1 Changes in synapse v0.29.0-rc1 (2018-05-14) =========================================== Notable changes, a docker file for running Synapse (Thanks to @kaiyou!) and a closed spec bug in the Client Server API. Additionally further prep for Python 3 migration. Potentially breaking change: * Make Client-Server API return 401 for invalid token (PR #3161). This changes the Client-server spec to return a 401 error code instead of 403 when the access token is unrecognised. This is the behaviour required by the specification, but some clients may be relying on the old, incorrect behaviour. Thanks to @NotAFile for fixing this. Features: * Add a Dockerfile for synapse (PR #2846) Thanks to @kaiyou! Changes - General: * nuke-room-from-db.sh: added postgresql option and help (PR #2337) Thanks to @rubo77! * Part user from rooms on account deactivate (PR #3201) * Make 'unexpected logging context' into warnings (PR #3007) * Set Server header in SynapseRequest (PR #3208) * remove duplicates from groups tables (PR #3129) * Improve exception handling for background processes (PR #3138) * Add missing consumeErrors to improve exception handling (PR #3139) * reraise exceptions more carefully (PR #3142) * Remove redundant call to preserve_fn (PR #3143) * Trap exceptions thrown within run_in_background (PR #3144) Changes - Refactors: * Refactor /context to reuse pagination storage functions (PR #3193) * Refactor recent events func to use pagination func (PR #3195) * Refactor pagination DB API to return concrete type (PR #3196) * Refactor get_recent_events_for_room return type (PR #3198) * Refactor sync APIs to reuse pagination API (PR #3199) * Remove unused code path from member change DB func (PR #3200) * Refactor request handling wrappers (PR #3203) * transaction_id, destination defined twice (PR #3209) Thanks to @damir-manapov! * Refactor event storage to prepare for changes in state calculations (PR #3141) * Set Server header in SynapseRequest (PR #3208) * Use deferred.addTimeout instead of time_bound_deferred (PR #3127, #3178) * Use run_in_background in preference to preserve_fn (PR #3140) Changes - Python 3 migration: * Construct HMAC as bytes on py3 (PR #3156) Thanks to @NotAFile! * run config tests on py3 (PR #3159) Thanks to @NotAFile! * Open certificate files as bytes (PR #3084) Thanks to @NotAFile! * Open config file in non-bytes mode (PR #3085) Thanks to @NotAFile! * Make event properties raise AttributeError instead (PR #3102) Thanks to @NotAFile! * Use six.moves.urlparse (PR #3108) Thanks to @NotAFile! * Add py3 tests to tox with folders that work (PR #3145) Thanks to @NotAFile! * Don't yield in list comprehensions (PR #3150) Thanks to @NotAFile! * Move more xrange to six (PR #3151) Thanks to @NotAFile! * make imports local (PR #3152) Thanks to @NotAFile! * move httplib import to six (PR #3153) Thanks to @NotAFile! * Replace stringIO imports with six (PR #3154, #3168) Thanks to @NotAFile! * more bytes strings (PR #3155) Thanks to @NotAFile! Bug Fixes: * synapse fails to start under Twisted >= 18.4 (PR #3157) * Fix a class of logcontext leaks (PR #3170) * Fix a couple of logcontext leaks in unit tests (PR #3172) * Fix logcontext leak in media repo (PR #3174) * Escape label values in prometheus metrics (PR #3175, #3186) * Fix 'Unhandled Error' logs with Twisted 18.4 (PR #3182) Thanks to @Half-Shot! * Fix logcontext leaks in rate limiter (PR #3183) * notifications: Convert next_token to string according to the spec (PR #3190) Thanks to @mujx! * nuke-room-from-db.sh: fix deletion from search table (PR #3194) Thanks to @rubo77! * add guard for None on purge_history api (PR #3160) Thanks to @krombel!

superdump and others added 3 commits September 28, 2017 12:12

docker: Initial Dockerfile and docker-compose.yaml

95e02b8

docker: s/matrix-org/matrixdotorg/g

24d1628

Initial commit including a Dockerfile for synapse

431476f

kaiyou added 7 commits February 3, 2018 20:30

Add template config files for the Docker image

d434ae3

Allow for a wheel cache and include missing files in the build

48bc22f

Generate any missing keys before starting synapse

6d1e28a

Generate shared secrets if not defined in the environment

f2bf0cd

Support an external postgresql config in the Docker image

886c2d5

Install the postgres dependencies

042757f

Provide an example docker compose file

1ba2fe1

kaiyou added 10 commits February 4, 2018 15:04

Reuse environment variables of the postgres container

a207ccc

Merge remote-tracking branch 'origin/rob/docker' into feat-dockerfile

5396533

Remove etc/service files from rob's branch

84a9209

Update sumperdump Docker readme to match this image properties

9a87b8a

Add the build cache/ folder to gitignore

b8ab78b

Fix multiple typos

f72c9c1

Run the server as an unprivileged user

e9021e1

Remove docker related files from the python manifest

8db84e9

Add dynamic recaptcha configuration in the Docker image

81010a1

Add dynamic TURN configuration in the Docker image

cd51931

turt2live reviewed Feb 5, 2018

View reviewed changes

kaiyou added 3 commits February 5, 2018 22:10

Document the cache factor environment variable for Docker

cf4ef60

Fix a typo in the Docker README

d8c7da5

Point to the 'latest' tag in the Docker documentation

f5364b4

uhoreg reviewed Feb 5, 2018

View reviewed changes

kaiyou added 2 commits February 5, 2018 22:57

Do not copy documentation files to the Docker root folder

630573a

Only generate configuration files when necessary

ee3b160

kaiyou added 2 commits April 8, 2018 17:56

Merge remote-tracking branch 'upstream/master' into feat-dockerfile

a13b786

Merge remote-tracking branch 'upstream/master' into feat-dockerfile

041b41a

michaelkaye mentioned this pull request Apr 20, 2018

Official Docker image #2482

Closed

turt2live reviewed Apr 20, 2018

View reviewed changes

Ezwen reviewed Apr 29, 2018

View reviewed changes

kaiyou added 2 commits May 1, 2018 20:47

Fix the documentation about 'POSTGRES_DB'

d4c14e1

Make the logging level configurable

4f2e898

Merge remote-tracking branch 'upstream/master' into feat-dockerfile

9a779c2

Add Docker packaging in the author list

5addeaa

jcgruenhage approved these changes May 4, 2018

View reviewed changes

michaelkaye changed the base branch from master to develop May 11, 2018 15:38

michaelkaye merged commit 16f4123 into matrix-org:develop May 11, 2018

richvdh mentioned this pull request May 15, 2018

Consider to include a Dockerfile #969

Closed

jcgruenhage mentioned this pull request Nov 5, 2018

Make LDAP configurable in the container image #4144

Closed

richvdh mentioned this pull request Feb 15, 2022

Docker: remove VOLUME directive #11997

Merged


		Database specific values (will use SQLite if not set):

		* `POSTGRES_DATABASE` - The database name for the synapse postgres database. [default: `matrix`]

Add a Dockerfile for synapse #2846

Add a Dockerfile for synapse #2846

Conversation

kaiyou commented Feb 3, 2018 • edited Loading

matrixbot commented Feb 3, 2018

matrixbot commented Feb 3, 2018

kaiyou commented Feb 4, 2018

turt2live 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

kaiyou Feb 5, 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

neilisfragile commented Mar 28, 2018

Ezwen commented Mar 30, 2018

michaelkaye commented Apr 20, 2018

michaelkaye commented Apr 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaiyou commented Apr 21, 2018 • edited Loading

jcgruenhage commented Apr 21, 2018

kaiyou commented Apr 21, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaiyou commented May 2, 2018

almereyda commented May 2, 2018 • edited Loading

michaelkaye commented May 4, 2018

michaelkaye commented May 4, 2018

michaelkaye commented May 4, 2018

kaiyou commented May 4, 2018 • edited Loading

jcgruenhage commented May 5, 2018

kaiyou commented Feb 3, 2018 •

edited

Loading

kaiyou Feb 5, 2018 •

edited

Loading

kaiyou commented Apr 21, 2018 •

edited

Loading

almereyda commented May 2, 2018 •

edited

Loading

kaiyou commented May 4, 2018 •

edited

Loading