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

Dockerfile for zulip docker image #227

Closed
wants to merge 10 commits into
base: master
from

Conversation

Projects
None yet
6 participants
@galexrt
Contributor

galexrt commented Oct 25, 2015

This pull request includes all required files for zulip to run in a container smoothly.
The files are from my repository https://github.com/Galexrt/docker-zulip.


See #29.

The current docker-compose.yml points to my quay.io/galexrt repo. @timabbott said that I shouldn't add it, but I think because of the "many" service dependencies of zulip (postgresql database, memcached, redis, camo and rabbitmq), I think it's better to have a docker-compose.yml that gets zulip up and running in seconds, for easy use.

@smarx

This comment has been minimized.

Show comment
Hide comment
@smarx

smarx Oct 25, 2015

Automated message from Dropbox CLA bot

@galexrt, thanks for the pull request! It looks like you haven't yet signed the Dropbox CLA. Please sign it here and update the thread so we can consider merging your code.

smarx commented Oct 25, 2015

Automated message from Dropbox CLA bot

@galexrt, thanks for the pull request! It looks like you haven't yet signed the Dropbox CLA. Please sign it here and update the thread so we can consider merging your code.

@galexrt

This comment has been minimized.

Show comment
Hide comment
@galexrt

galexrt Oct 25, 2015

Contributor

I already signed up for the Dropbox CLA.

Contributor

galexrt commented Oct 25, 2015

I already signed up for the Dropbox CLA.

@lfaraone

This comment has been minimized.

Show comment
Hide comment
@lfaraone

lfaraone Oct 26, 2015

Member

Can all of the docker/zulip-puppet/* files be symlinks to the existing files in the repository?

Member

lfaraone commented Oct 26, 2015

Can all of the docker/zulip-puppet/* files be symlinks to the existing files in the repository?

@lfaraone

View changes

Show outdated Hide outdated docker/Dockerfile Outdated
@galexrt

This comment has been minimized.

Show comment
Hide comment
@galexrt

galexrt Oct 26, 2015

Contributor

@lfaraone Docker doesn't allow symlinks. But I could move the Dockerfile into the repo root and then use the files from there.
Do you mean by files, the files and manifests or only the files?
Because I need the manifests, they are edited by me to suit the docker container.

Contributor

galexrt commented Oct 26, 2015

@lfaraone Docker doesn't allow symlinks. But I could move the Dockerfile into the repo root and then use the files from there.
Do you mean by files, the files and manifests or only the files?
Because I need the manifests, they are edited by me to suit the docker container.

@djdefi

View changes

Show outdated Hide outdated docker/docker-compose.yml Outdated

galexrt added some commits Oct 27, 2015

Removed all "duplicate" files from the docker directory
It is now as @lfaraone wanted. I removed all "duplicate" files, expect my modified puppet manifests.
@galexrt

This comment has been minimized.

Show comment
Hide comment
@galexrt

galexrt Nov 7, 2015

Contributor

@lfaraone Sry, for the late response. I changed it as you wanted.
Could you look at it again?

Contributor

galexrt commented Nov 7, 2015

@lfaraone Sry, for the late response. I changed it as you wanted.
Could you look at it again?

@karambir

This comment has been minimized.

Show comment
Hide comment
@karambir

karambir Nov 17, 2015

Contributor

It would be helpful to have a one liner in README on how to run this. Thanks for the effort.

Contributor

karambir commented Nov 17, 2015

It would be helpful to have a one liner in README on how to run this. Thanks for the effort.

@galexrt

This comment has been minimized.

Show comment
Hide comment
@galexrt

galexrt Dec 12, 2015

Contributor

Ping @lfaraone @timabbott.
I removed all double files. Do you guys still want this pull request?

Contributor

galexrt commented Dec 12, 2015

Ping @lfaraone @timabbott.
I removed all double files. Do you guys still want this pull request?

@timabbott

This comment has been minimized.

Show comment
Hide comment
@timabbott

timabbott Dec 12, 2015

Member

Sorry for the slow reply to this PR! I really appreciate your working on this @galexrt and would like to figure out a good path forward. Here are some thoughts:

  • I just saw #365, which looks to be a docker implementation for running the entire Zulip development environment including dependencies within a single container. I imagine that's a model worth supporting (as an easy way to run the Zulip development environment for docker fans without needing to setup a prod environment), independent of whether we also merge this PR which supports running Zulip via a group of containers, right? I'd be excited if you took a look at that PR and posted any thoughts you have on that approach for the use case, since I'm by no means a docker expert.
  • I'd love to merge into the main Zulip codebase a docker file implementation that makes it easy to run a Zulip production environment inside a container as this pull request does; however ideally I'd like to figure out a way to make this not require 1280 lines of new code.

Even with your improvement of cleaning out the duplicated configuration files (which I definitely appreciate!), this pull request has a lot of code that duplicates logic we already have the maintain in the codebase. I spent some time trying to understand the code this adds, and here are the notes on the specific things I think we could improve:

  • docker/entrypoint.sh is 600 lines of code that seems basically like a reimplementation of our zproject/*settings*.py; I hope we can reuse the existing code here. For example, if you need to get access to a Django setting from a shell script or other non-Python file, you can do that with bin/get-django-setting.
  • docker/Dockerfile has a large amount of code that seems to overlap with the install script; would it work to just use install with some options to tweak the behavior instead (e.g. set which puppet rules are included)?
  • docker/puppet/zulip/files/nginx/nginx.conf differs from the version in Zulip in two ways -- one is that it doesn't include the gzip fixes I merged a few weeks ago (just version skew) and the other is that server_tokens is turned off -- what is the purpose of that change?
  • docker/puppet/zulip/files/createZulipAdmin.sh could be replaced with a new management command that lives under zerver/management/commands that includes its code; I'd be happy to just merge that as a standard feature of Zulip so that this doesn't need to be a docker-specific hack.
  • I think it should be possible to dramatically shrink the number of puppet manifests that need to be patched (or patched significantly) in order for this to work; e.g. I think some of the packages depended on by base.pp could be moved out that manifest. And secondarily I'd be willing to refactor a bit the hierarchy of manifests to make it easier to include a more minimal set of things.
  • One thing I wanted to ask you about is why in several of the puppet manifests you've removed notify => Service["nginx"], lines -- is that because you're running nginx in a separate docker container and thus nginx isn't running in the current container?

@galexrt if you can work on making docker/entrypoint.sh something more maintainable that uses the existing zproject/*settings.py* infrastructure, that is I think the essential huge piece that needs to be resolved before we can merge this. And if you can get this diffstat to be something more manageable by e.g. undoing the indentation changes, I'd be happy to work with you on a plan for how to refactor the puppet manifests to make that piece maintainable as well.

tabbott@monastery:~/zulip$ diff -ur docker/puppet/zulip/manifests/  puppet/zulip/manifests/ | diffstat
 /apache_sso.pp        |only
 /camo.pp              |only
 /localhost_sso.pp     |only
 /postfix_localmail.pp |only
 /redis.pp             |only
 app_frontend.pp       |  193 ++++++++++++++++++++++----------------------------
 base.pp               |  117 +++++++++++++++++-------------
 nginx.pp              |   33 ++++----
 postgres_appdb.pp     |   35 +++++++--
 postgres_common.pp    |   26 ++++--
 rabbit.pp             |   57 ++++++++++++--
 supervisor.pp         |   36 ++++++---
 voyager.pp            |   84 ++++++++++++++++-----
 13 files changed, 357 insertions(+), 224 deletions(-)
Member

timabbott commented Dec 12, 2015

Sorry for the slow reply to this PR! I really appreciate your working on this @galexrt and would like to figure out a good path forward. Here are some thoughts:

  • I just saw #365, which looks to be a docker implementation for running the entire Zulip development environment including dependencies within a single container. I imagine that's a model worth supporting (as an easy way to run the Zulip development environment for docker fans without needing to setup a prod environment), independent of whether we also merge this PR which supports running Zulip via a group of containers, right? I'd be excited if you took a look at that PR and posted any thoughts you have on that approach for the use case, since I'm by no means a docker expert.
  • I'd love to merge into the main Zulip codebase a docker file implementation that makes it easy to run a Zulip production environment inside a container as this pull request does; however ideally I'd like to figure out a way to make this not require 1280 lines of new code.

Even with your improvement of cleaning out the duplicated configuration files (which I definitely appreciate!), this pull request has a lot of code that duplicates logic we already have the maintain in the codebase. I spent some time trying to understand the code this adds, and here are the notes on the specific things I think we could improve:

  • docker/entrypoint.sh is 600 lines of code that seems basically like a reimplementation of our zproject/*settings*.py; I hope we can reuse the existing code here. For example, if you need to get access to a Django setting from a shell script or other non-Python file, you can do that with bin/get-django-setting.
  • docker/Dockerfile has a large amount of code that seems to overlap with the install script; would it work to just use install with some options to tweak the behavior instead (e.g. set which puppet rules are included)?
  • docker/puppet/zulip/files/nginx/nginx.conf differs from the version in Zulip in two ways -- one is that it doesn't include the gzip fixes I merged a few weeks ago (just version skew) and the other is that server_tokens is turned off -- what is the purpose of that change?
  • docker/puppet/zulip/files/createZulipAdmin.sh could be replaced with a new management command that lives under zerver/management/commands that includes its code; I'd be happy to just merge that as a standard feature of Zulip so that this doesn't need to be a docker-specific hack.
  • I think it should be possible to dramatically shrink the number of puppet manifests that need to be patched (or patched significantly) in order for this to work; e.g. I think some of the packages depended on by base.pp could be moved out that manifest. And secondarily I'd be willing to refactor a bit the hierarchy of manifests to make it easier to include a more minimal set of things.
  • One thing I wanted to ask you about is why in several of the puppet manifests you've removed notify => Service["nginx"], lines -- is that because you're running nginx in a separate docker container and thus nginx isn't running in the current container?

@galexrt if you can work on making docker/entrypoint.sh something more maintainable that uses the existing zproject/*settings.py* infrastructure, that is I think the essential huge piece that needs to be resolved before we can merge this. And if you can get this diffstat to be something more manageable by e.g. undoing the indentation changes, I'd be happy to work with you on a plan for how to refactor the puppet manifests to make that piece maintainable as well.

tabbott@monastery:~/zulip$ diff -ur docker/puppet/zulip/manifests/  puppet/zulip/manifests/ | diffstat
 /apache_sso.pp        |only
 /camo.pp              |only
 /localhost_sso.pp     |only
 /postfix_localmail.pp |only
 /redis.pp             |only
 app_frontend.pp       |  193 ++++++++++++++++++++++----------------------------
 base.pp               |  117 +++++++++++++++++-------------
 nginx.pp              |   33 ++++----
 postgres_appdb.pp     |   35 +++++++--
 postgres_common.pp    |   26 ++++--
 rabbit.pp             |   57 ++++++++++++--
 supervisor.pp         |   36 ++++++---
 voyager.pp            |   84 ++++++++++++++++-----
 13 files changed, 357 insertions(+), 224 deletions(-)
@galexrt

This comment has been minimized.

Show comment
Hide comment
@galexrt

galexrt Dec 13, 2015

Contributor

To your mentioned points:

  • docker/entrypoint.sh uses the zproject/*settings*.py and /etc/zulip/settings.py, it adds the vars to them (I can add a symlink from the settings.py to the data volume to allow manual configuration). The entrypoint.sh is "needed" because the settings in the /etc/zulip/settings.py are partly overriden by the "default" /home/zulip/deployments/current/zproject/settings.py.
  • docker/Dockerfile It is a modified version of your install script. (I could try to "recreate" this for the zulip/zulip repo here)
  • docker/puppet/zulip/files/nginx/nginx.conf I add server_tokens off option, because it stops sending the nginx server version out. By default this is on, it sends the, I think it was the X-Powered-By Header, containing the nginx version and the error pages from nginx don't show the nginx version (not sending the nginx version would "lower" the information needed for an attack).
  • base.pp If you refactor the manifests to be fully independent and seperate, I would be able to remove most of the puppet manifests then.
  • notify => Service["nginx"] In docker nobody hears you scream there's no init system, so puppet will sometimes fail (See https://ask.puppetlabs.com/question/13635/how-to-use-puppet-modules-that-require-upstart-with-docker/?answer=13785#post-id-13785).
  • docker/puppet/zulip/files/createZulipAdmin.sh would be good to see an official command in the manage.py to create an user with password from command line arguments. There is a command to create an user, but the command has no option to set the password from an command line argument, it would be enough to add an password command line argument. (Should I create a ticket for this feature?)

I think with refactored manifests, only having one small puppet manifest in the docker/ that includes the manifests from the repo, would be a possible simple solution.

I'll try my best to minimize the count of lines in my files.

@timabbott If you want, I can try to seperate the manifests in a seperate pull request to fit the needs of docker and a "normal" server.

Contributor

galexrt commented Dec 13, 2015

To your mentioned points:

  • docker/entrypoint.sh uses the zproject/*settings*.py and /etc/zulip/settings.py, it adds the vars to them (I can add a symlink from the settings.py to the data volume to allow manual configuration). The entrypoint.sh is "needed" because the settings in the /etc/zulip/settings.py are partly overriden by the "default" /home/zulip/deployments/current/zproject/settings.py.
  • docker/Dockerfile It is a modified version of your install script. (I could try to "recreate" this for the zulip/zulip repo here)
  • docker/puppet/zulip/files/nginx/nginx.conf I add server_tokens off option, because it stops sending the nginx server version out. By default this is on, it sends the, I think it was the X-Powered-By Header, containing the nginx version and the error pages from nginx don't show the nginx version (not sending the nginx version would "lower" the information needed for an attack).
  • base.pp If you refactor the manifests to be fully independent and seperate, I would be able to remove most of the puppet manifests then.
  • notify => Service["nginx"] In docker nobody hears you scream there's no init system, so puppet will sometimes fail (See https://ask.puppetlabs.com/question/13635/how-to-use-puppet-modules-that-require-upstart-with-docker/?answer=13785#post-id-13785).
  • docker/puppet/zulip/files/createZulipAdmin.sh would be good to see an official command in the manage.py to create an user with password from command line arguments. There is a command to create an user, but the command has no option to set the password from an command line argument, it would be enough to add an password command line argument. (Should I create a ticket for this feature?)

I think with refactored manifests, only having one small puppet manifest in the docker/ that includes the manifests from the repo, would be a possible simple solution.

I'll try my best to minimize the count of lines in my files.

@timabbott If you want, I can try to seperate the manifests in a seperate pull request to fit the needs of docker and a "normal" server.

@timabbott

This comment has been minimized.

Show comment
Hide comment
@timabbott

timabbott Dec 14, 2015

Member

Cool, yeah, refactoring the manifests would be great. I'd definitely prefer small pull requests (or at least small commits) so it's easy to review and merge them individually and I can give feedback on things before you go too far in a direction.

A good one to start with would be the base.pp package list -- I think we can move most of them to zulip_internal/manifests/base.pp. However, we should keep a couple that Zulip uses -- specifically ipython (makes manage.py shell way better) and ntp (correct timestamps are fairly important for a chat system).

Member

timabbott commented Dec 14, 2015

Cool, yeah, refactoring the manifests would be great. I'd definitely prefer small pull requests (or at least small commits) so it's easy to review and merge them individually and I can give feedback on things before you go too far in a direction.

A good one to start with would be the base.pp package list -- I think we can move most of them to zulip_internal/manifests/base.pp. However, we should keep a couple that Zulip uses -- specifically ipython (makes manage.py shell way better) and ntp (correct timestamps are fairly important for a chat system).

Merge pull request #3 from zulip/master
Merge latest zulip changes
@galexrt

This comment has been minimized.

Show comment
Hide comment
@galexrt

galexrt Dec 15, 2015

Contributor

@timabbott I tried my best to seperate the manifests, but I'm not knowing puppet well enough to seperate them to still work..
I would appreciate if you could look into trying to seperate the manifests.
After that I would recreate the pull request with the smaller updated version, if that's okay.

Contributor

galexrt commented Dec 15, 2015

@timabbott I tried my best to seperate the manifests, but I'm not knowing puppet well enough to seperate them to still work..
I would appreciate if you could look into trying to seperate the manifests.
After that I would recreate the pull request with the smaller updated version, if that's okay.

@timabbott

This comment has been minimized.

Show comment
Hide comment
@timabbott

timabbott Dec 25, 2015

Member

OK I can take a look at the modules piece of this...

Member

timabbott commented Dec 25, 2015

OK I can take a look at the modules piece of this...

@timabbott

This comment has been minimized.

Show comment
Hide comment
@timabbott

timabbott Dec 26, 2015

Member

I've done some work on refactoring the puppet modules in this branch: https://github.com/timabbott/zulip/commits/puppet-refactor

The branch is totally untested as of yet, and thus probably broken (so I wouldn't start building off it quite yet), but it should give you a sense of the direction I have in mind -- does that approach seem helpful to you?

Member

timabbott commented Dec 26, 2015

I've done some work on refactoring the puppet modules in this branch: https://github.com/timabbott/zulip/commits/puppet-refactor

The branch is totally untested as of yet, and thus probably broken (so I wouldn't start building off it quite yet), but it should give you a sense of the direction I have in mind -- does that approach seem helpful to you?

@timabbott

This comment has been minimized.

Show comment
Hide comment
@timabbott

timabbott Dec 26, 2015

Member

After some testing I've posted that code as #393

Member

timabbott commented Dec 26, 2015

After some testing I've posted that code as #393

@galexrt

This comment has been minimized.

Show comment
Hide comment
@galexrt

galexrt Dec 26, 2015

Contributor

Merry christmas to you and the zulip team! :)
@timabbott Thanks for the code. I'll look at integrating those in the dockerfile for the project.

Contributor

galexrt commented Dec 26, 2015

Merry christmas to you and the zulip team! :)
@timabbott Thanks for the code. I'll look at integrating those in the dockerfile for the project.

@galexrt

This comment has been minimized.

Show comment
Hide comment
@galexrt

galexrt Jan 20, 2016

Contributor

I'm closing this, because I had some troubles with git and now had to create a fresh fork of the repo.

@timabbott I'm going to create a new pull request within the next days, with the improved slimmed down docker zulip version without the 600+ lines of docker-entrypoint.sh and using your https://github.com/timabbott/zulip/commits/puppet-refactor manifests merged.

Contributor

galexrt commented Jan 20, 2016

I'm closing this, because I had some troubles with git and now had to create a fresh fork of the repo.

@timabbott I'm going to create a new pull request within the next days, with the improved slimmed down docker zulip version without the 600+ lines of docker-entrypoint.sh and using your https://github.com/timabbott/zulip/commits/puppet-refactor manifests merged.

@galexrt galexrt closed this Jan 20, 2016

@timabbott

This comment has been minimized.

Show comment
Hide comment
@timabbott

timabbott Jan 20, 2016

Member

Awesome, thanks @galexrt , I look forward to reviewing it!

Member

timabbott commented Jan 20, 2016

Awesome, thanks @galexrt , I look forward to reviewing it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment