Skip to content
This repository has been archived by the owner on Nov 11, 2019. It is now read-only.

Add Dockerfile + docker-compose.yml #207

Closed
wants to merge 3 commits into from
Closed

Add Dockerfile + docker-compose.yml #207

wants to merge 3 commits into from

Conversation

bhearsum
Copy link
Contributor

@bhearsum bhearsum commented Mar 3, 2017

This isn't ready for merging yet, but it's a proof of concept for how we can build a container that lets anyone run the services apps, even if they don't have Nix installed. There's at least a few things todo still:

  • Stop hardcoding shipit_frontend everywhere. @garbas has some ideas about dynamically generated docker-compose.yml + Dockerfiles. I'm not entirely convinced that's a great idea, but it's probably worth exploring more. A simple alternative is to allow the APP to be passed in rather than hardcoded in the Dockerfile.
  • Fix SSL errors. I think these are caused by something inside the container listening on 127.0.0.1 instead of 0.0.0.0. Jordan previously found a workaround for this (https://people-mozilla.org/~jlund/cert_workaround). I suspect that only the APP_DEV_HOST part is necessary. Need to test this more.
  • Test with more apps than shipit_frontend
  • Update docker-compose.yml with the right image name/tag once we figure out what those will be.

Once this is merged, we'll need automated builds done and pushed somewhere public. I think dockerhub is probably the best place, because it's where Docker pulls from by default. It's much easier for new folks to get started if they don't need to specify some special host to find the image from.

@bhearsum
Copy link
Contributor Author

bhearsum commented Mar 6, 2017

Summary of the changes I just pushed:

  • Fix SSL handshake errors, which were caused by some apps binding to localhost instead of 0.0.0.0, which broke Docker port forwarding.
  • Switch to a docker-compose file per "family" of apps, eg: docker-compose-shipit.yml; enable containers for other Ship It webapps.
  • Install required dependencies for all Ship It webapps.

@lundjordan
Copy link
Contributor

fwiw - I tested this out and was able to hit the end points for the various shipit_* apps that stood up pretty smoothly

👍

@bhearsum bhearsum self-assigned this Mar 7, 2017
@bhearsum
Copy link
Contributor Author

bhearsum commented Mar 7, 2017

I'm trying to avoid expanding the scope of this too much more, but there's a few more things I want to do here still:

  • Figure out if we need a container for shipit_upliftbot (it sounds like this might be replaced or killed, so maybe nothing)
  • Purge sqlite references; use postgres wherever a database is needed
  • Figure out which containers need to be able to talk to one another
  • Ensure nothing is installed on first run (it should all be in the image)
  • Try to shrink the image a bit, if possible
  • Figure out where to build and push the image from CI

@garbas - when you have a few minutes, could you let me know if this PR looks OK so far, and if the above makes sense?

@bhearsum
Copy link
Contributor Author

bhearsum commented Mar 9, 2017

I filed #215, #216, #217, #218, #219, and #220 for all the things noted in my previous comment. Once #209 is done, we'll need to upgrade the Dockerfile here to use the new nix channel, and then I think we can merge this?

# TODO: what links do we need?
services:
releng_db:
image: postgres:9.6
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For postgresql we should use the same setup as in nix setup. and run the same command as if we would be running from Nix. That way we avoid "it works on my here" situations.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is significantly more work than using the Postgres image from the Docker library, which automatically handles creating users, databases, etc. How about pinning to a more specific version (9.6.2) as a compromise?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not really, we already support running postgresql via make develop-run-postgresql, we should make docker setup be a reflection of that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really don't think this is going to be a good way to go. The Makefile is focused around running one app at a time, and assumes that everything is running on the same host. None of this is the case in the Docker setup. We're going to have to do a bunch of extra work to have the Makefile's postgres targets support the Docker environment, including:

  • Make it learn how to create users
  • Make it possible to create multiple databases (not needed yet, but will be as soon as we have more than one Ship It app with a database)
  • Make it possible to conditionally bind to 0.0.0.0.

Basically, we have to more or less fork the postgres targets and use different ones in Docker. I do not think it's worth our time to write or maintain this code when the upstream postgres image will do it for us.

If the goal is reproducability and consistency across all machines, we can even pin to a specific digest of the postgres image, which should alleviate any concerns about things working on one machine and not another:

    expose:
      - 5432
    environment:
      - POSTGRES_PASSWORD=shipit
      - POSTGRES_USER=shipit
➜  services git:(docker) ✗ docker-compose -f docker-compose-shipit.yml up
Pulling shipit_db (postgres@sha256:6ac0fbeddde3bb7b0003a2ace8c126f742f8bdd90695801337d3edaaf1fcc478)...
sha256:6ac0fbeddde3bb7b0003a2ace8c126f742f8bdd90695801337d3edaaf1fcc478: Pulling from library/postgres

Makefile Outdated
@@ -37,7 +37,7 @@ VERSION=$(shell cat VERSION)

APP_DEV_DBNAME=services

APP_DEV_HOST=localhost
APP_DEV_HOST=0.0.0.0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should only be a case when inside docker and not when not using docker.

@bhearsum bhearsum mentioned this pull request Mar 17, 2017
healthcheck:
# TODO: this command generates spamming output from the releng_db container
# using the real pg client might fix this
test: ["CMD", "nc", "-vz", "releng_db", "5432"]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While these work, it works out I did them wrong. Healthchecks should be part of the container they're checking (so this one should be in the db container), and there's a special depends_on syntax for depending on a service being healthy. mozilla-releng/balrog#297 (review) has a good example of how to do it.

@bhearsum
Copy link
Contributor Author

@garbas - should this be closed now?

@garbas
Copy link
Contributor

garbas commented Jun 15, 2017

@mozbhearsum i was keeping it open to have a constant reminder that i need to finish this.

We need to fix docker+please command with @srfraser (hopefully this Friday)
I just created a PR #405 (to have one big docker image) that you can start development faster.
And to have the same functionality as this PR I just opened a issue #406 which I hope to come back in Q3.

I think it is now safe to close this ticket.

@garbas garbas closed this Jun 15, 2017
@garbas garbas mentioned this pull request Jun 15, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants