Template out docker repository #670

shreshthkhilani · 2018-01-30T20:34:29Z

Overview

Template out docker repository to help decentralize architecture.

Notes

Makefile needs to run with DOCKER_REPOSITORY environment variable, as does forecast_segment_incidents.py.

Testing

Test build on local development machine. How can we test with a new docker repository?

ddohler

This looks like it's a mixture of two approaches:

removing the {{docker_repository}} prefix on development and
using variable substitution with a blank variable to achieve the same effect

I think the same approach should be used as much as possible (although there are a few spots where it isn't possible); I've added my thoughts on how to make everything line up. Let me know if you have questions!

ddohler · 2018-01-31T13:47:07Z

deployment/ansible/roles/driver.app/templates/upstart-app.conf.j2

@@ -41,7 +41,7 @@ exec /usr/bin/docker run \
  --env DRIVER_ADMIN_GROUP='{{ driver_admin_group }}' \
  --log-driver syslog \
 {% if celery %} --entrypoint '/bin/bash -c' \
-{% endif %}  quay.io/azavea/driver-app:{{ docker_image_tag }} {% if celery %} \
+{% endif %}  {{ docker_repository }}/driver-app:{{ docker_image_tag }} {% if celery %} \


I think there needs to be another {% if 'development' not in group_names %} added here to choose the correct image (and same comment for the other upstart scripts). --Edit-- or if you're following the blank-variable approach, this just needs to be updated to account for the leading slash.

ddohler · 2018-01-31T13:47:36Z

deployment/ansible/group_vars/all.example

@@ -15,6 +15,7 @@ docker_options: "--storage-driver=aufs"
 # In local development, always use the 'latest' tag regardless of what app_version says;
 # users need to checkout a tag in order to get a specific version.
 docker_image_tag: "{% if developing %}latest{% else %}{{app_version}}{% endif %}"
+docker_repository: "quay.io/azavea"


I don't think this is quite the right place for this, or rather, it should get overridden in both group_vars/development and group_vars/production so that in development it's blank and in production it's set to quay.io/azavea by default. It also needs to get added to the configuration wizard.

ddohler · 2018-01-31T13:49:32Z

deployment/ansible/roles/driver.gradle/tasks/main.yml

@@ -17,12 +17,12 @@
  command: >
    docker build
      -f {{ gradle_dir }}/Dockerfile
-      -t quay.io/azavea/driver-gradle:{{ docker_image_tag }}
+      -t {{ docker_repository }}/driver-gradle:{{ docker_image_tag }}


I think this use of {{ docker_repository }} should be removed because it's building on development. Same comment for driver.r-docker/tasks/main.yml and deployment/ansible/roles/driver.web/tasks/development.yml. --Edit-- Or if you're following the blank-variable approach, just adjust for the slash.

ddohler · 2018-01-31T13:52:14Z

web/test/karma.conf.js

@@ -60,9 +60,12 @@ module.exports = function(config) {
      'bower_components/Leaflet.utfgrid/dist/leaflet.utfgrid.js',


I wasn't expecting to see changes to this file as part of this PR; were these necessary to get it working?

ddohler · 2018-01-31T13:57:22Z

app/black_spots/tasks/forecast_segment_incidents.py

@@ -18,12 +19,16 @@ def forecast_segment_incidents(segments_csv, output_path):
    Returns:
        Path of forecast file generated by the R script
    """
+    analysis_container = 'quay.io/azavea/driver-analysis'
+    docker_repository = os.environ.get('DOCKER_REPOSITORY')


If this environment variable is necessary for this script to run then I think it needs to get provided to the celery process in upstart-celery.conf.j2.

ddohler · 2018-01-31T14:01:54Z

Makefile

@@ -1,13 +1,17 @@
 all: app editor web

+ifeq ($(DOCKER_REPOSITORY),)
+    DOCKER_REPOSITORY := quay.io/azavea


I'm not sure that the default is necessary here; this script is only used in local development, so there shouldn't be any need for the prefix in most cases.

ddohler · 2018-01-31T14:15:21Z

deployment/ansible/roles/driver.app/tasks/main.yml

@@ -17,20 +17,20 @@
  command: >
    docker build
      -f {{ root_app_dir }}/Dockerfile.base
-      -t quay.io/azavea/driver-app:{{ docker_image_tag }}
+      -t driver-app:{{ docker_image_tag }}


This is inconsistent with the other docker build ... commands in other parts of the Ansible provisioning process. Initially I made comments that those other commands should be made to match this one, but I think that substituting a blank variable for {{docker_repository}} like you suggested yesterday is a better approach. I think you could update this to keep the {{docker_repository}} prefix that the other docker build ... commands use and then set the blank variable in the development group_vars (see the comment above), and it should work after that.

ddohler · 2018-01-31T14:25:14Z

deployment/ansible/roles/driver.windshaft/templates/upstart-windshaft.conf.j2

@@ -16,7 +16,7 @@ pre-start script
  /usr/bin/docker rm windshaft || true

  {% if 'development' not in group_names -%}
-  /usr/bin/docker pull quay.io/azavea/windshaft:0.1.0
+  /usr/bin/docker pull {{ docker_repository }}/windshaft:0.1.0


Hmm, this is an interesting wrinkle, I had forgotten about this. We don't actually build a windshaft image anywhere in the deployment process, we just use an existing Azavea image and use it to run server.js. So I think this should actually be hard-coded as quay.io/azavea (probably with a comment). Unless someone wanted to like, upgrade to a different version of Windshaft, they should be able to do everything they want by just modifying server.js so there'd be no need to use a custom docker image.

ddohler · 2018-01-31T14:28:59Z

scripts/grunt.sh

@@ -24,15 +24,23 @@ case "$1" in
    PUBLISHED_PORT=9000
    LIVERELOAD_PORT=35731
    VOLUME_ROOT="/opt/schema_editor"
-    IMAGE_NAME="quay.io/azavea/driver-editor:latest"
+    if [[ -z "${DOCKER_REPOSITORY}" ]]; then
+      IMAGE_NAME="quay.io/azavea/driver-editor:latest"


This script can only get run in development, so I think this can just be driver-editor:latest and remove the check for DOCKER_REPOSITORY (in both cases here).

ddohler

This is pretty close; I found one more thing that needs to be changed, and I think this also needs to be added to the deployment wizard in ./scripts; after that it should be good to go. I was able to provision and verified that the docker images are named correctly.

ddohler · 2018-01-31T20:50:45Z

deployment/ansible/roles/driver.celery/templates/upstart-celery.conf.j2

  {% endif %}
 end script

 exec /usr/bin/docker run \
  --name driver-celery \
+  --env DOCKER_REPOSITORY={{ docker_repository }}


This line is missing a trailing \.

shreshthkhilani · 2018-02-02T19:27:57Z

Added to config wizard. Ready for a last look before I merge.

pcaisse · 2018-02-02T21:53:33Z

@shreshthkhilani I would wait for #672 to be merged and then rebase since it has a lot of changes to the wizard.

ddohler · 2018-02-05T15:00:22Z

deployment/ansible/group_vars/production.j2

+## DOCKER SETTINGS
+# The docker repository you want your app to pull images from. The default is "quay.io/azavea/".
+# The trailing / is required.
+docker_repository: {{ docker_repository }}


I chatted with @pcaisse about this and I think we can just have this line be docker_repository: "quay.io/azavea/" -- the wizard doesn't need to prompt for it. There are a bunch of things that aren't covered by the wizard that people will need to go into the group_vars to edit manually anyway, so if they ever need to fork the app they'll probably have a pretty good command of the group_vars by that point.

ddohler

This looks good to me; I made one last minor suggestion but this should be ready to go after it's rebased on #672 .

In terms of testing, the way to go about it would be to set up a second repository somewhere like AWS ECR, and then push images to that repository and try to deploy using it. That seems like a pretty big task, so I added a story to the icebox to cover doing that.

shreshthkhilani · 2018-02-05T18:52:11Z

I made the fix and now that docker_repository isn't prompted for in the wizard, do I still need to wait for #672 to be merged and then rebase? I think the merge will be automatic?

ddohler · 2018-02-05T18:56:43Z

Yeah, you're right, I tested rebasing from that branch locally and it worked fine, so I think this is good to go now.

shreshthkhilani requested a review from ddohler January 30, 2018 20:34

ddohler suggested changes Jan 31, 2018

View reviewed changes

ddohler approved these changes Jan 31, 2018

View reviewed changes

shreshthkhilani force-pushed the feature/template-container-repo-154217004 branch from b96ac2f to a6838dd Compare January 31, 2018 21:42

Template out docker repository

7469931

shreshthkhilani force-pushed the feature/template-container-repo-154217004 branch from a6838dd to 7469931 Compare February 2, 2018 16:17

ddohler reviewed Feb 5, 2018

View reviewed changes

ddohler approved these changes Feb 5, 2018

View reviewed changes

Add docker_repository to production group_vars

94cdaa1

shreshthkhilani force-pushed the feature/template-container-repo-154217004 branch from 8fbda88 to 94cdaa1 Compare February 5, 2018 15:57

shreshthkhilani merged commit d5ce4cd into develop Feb 5, 2018

shreshthkhilani deleted the feature/template-container-repo-154217004 branch February 5, 2018 18:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Template out docker repository #670

Template out docker repository #670

shreshthkhilani commented Jan 30, 2018

ddohler left a comment

ddohler Jan 31, 2018

ddohler Jan 31, 2018

ddohler Jan 31, 2018

ddohler Jan 31, 2018

ddohler Jan 31, 2018

ddohler Jan 31, 2018

ddohler Jan 31, 2018

ddohler Jan 31, 2018

ddohler Jan 31, 2018

ddohler left a comment

ddohler Jan 31, 2018

shreshthkhilani commented Feb 2, 2018

pcaisse commented Feb 2, 2018

ddohler Feb 5, 2018 •

edited

Loading

ddohler left a comment

shreshthkhilani commented Feb 5, 2018

ddohler commented Feb 5, 2018

		@@ -60,9 +60,12 @@ module.exports = function(config) {
		'bower_components/Leaflet.utfgrid/dist/leaflet.utfgrid.js',

Template out docker repository #670

Template out docker repository #670

Conversation

shreshthkhilani commented Jan 30, 2018

Overview

Notes

Testing

ddohler 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ddohler left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shreshthkhilani commented Feb 2, 2018

pcaisse commented Feb 2, 2018

ddohler Feb 5, 2018 • edited Loading

Choose a reason for hiding this comment

ddohler left a comment

Choose a reason for hiding this comment

shreshthkhilani commented Feb 5, 2018

ddohler commented Feb 5, 2018

ddohler Feb 5, 2018 •

edited

Loading