solgenomics / breedbase_dockerfile

The Dockerfiles for breeDBase
MIT License
8 stars 8 forks source link

Topic/multiple compose files #12

Closed bellerbrock closed 3 years ago

bellerbrock commented 4 years ago

Updates README to include table of contents and organize instructions by intended use.

Adds additional compose files to handle development vs production needs. Follows examples in docker docs https://docs.docker.com/compose/production/

Adds labels and build args to dockerfile to better document image

nathanweeks commented 3 years ago

Is this PR still under review? As is, the README.md directions for deploying a local breedbase using docker-compose with the docker-compose.yml don't appear to be sufficient to bootstrap a local instance.

bellerbrock commented 3 years ago

Hi @nathanweeks, good question. After some discussion I left this PR unmerged; we never came to a consensus regarding the best way to handle deployment in different contexts. But the current .yml files and README still have issues so thank you for the reminder.

Can you share the specific errors you encountered while trying to spin up an instance?

lukasmueller commented 3 years ago

HI Nathan,

try to use the breedbase/pg docker. I think it was updated to version 12 of postgres. We need to clean these up…

cheers Lukas

On Feb 11, 2021, at 8:27 AM, Nathan Weeks notifications@github.com wrote:

@nathanweeks commented on this pull request.

Hi @bellerbrock , after a few modifications to the docker-compose.yml in the master branch (and an additional postgres docker-entrypoint-initdb.d script or two), the issue that ultimately stopped me was the admin/password login not working. This branch looks closer to working out of the box; however, is breedbase/breedbase_pg12.4:v0.5 the right database image to use? After running prepare.sh, a docker-compose up results in:

/usr/local/bin/docker-entrypoint.sh: running /docker-entrypoint-initdb.d/empty_db_all_patches_20200915.sql.gz FATAL: database "breedbase" does not exist

If the POSTGRES_DB environment variable is set to create the "breedbase" db after the breedbase_db container is created; i.e.:

breedbase_db: image: breedbase/breedbase_pg12.4:v0.5 restart: always container_name: breedbase_db environment: POSTGRES_DB: breedbase

then some progress is made loading the .sql.gz file in docker-entrypoint-initdb.d, before the following error occurs:

ERROR: role "web_usr" does not exist [76] STATEMENT: ALTER FUNCTION public.refresh_materialized_phenotype_jsonb_table() OWNER TO web_usr;

In docker-compose.yml:

@@ -3,43 +3,12 @@ version: "3.7" services: breedbase: image: breedbase/breedbase:v0.17

  • restart: always

This is a good thing for production, though it might be unexpected in development (e.g., a developer who powers down their laptop might not want all containers that were running to automatically restart when they power it back on). Suggest moving the restart policy fields to the production.yml.

— You are receiving this because your review was requested. Reply to this email directly, view it on GitHub, or unsubscribe.

bellerbrock commented 3 years ago

@nathanweeks Haha yes sorry, that's my doing. I've been testing a fix to the postgres image dockerfile and just built the breedbase/pg:11 image locally. Unfortunately it turns out I can't push it to Dockerhub, I'll need to ask Lukas for the proper permissions.

I'll work on getting these PRs merged and the Pg:11 image pushed, in the meantime if you like it should be possible to build the missing image locally with these commands:

git clone --branch topic/use_empty_fixture https://github.com/solgenomics/postgres_dockerfile.git
docker build -t breedbase/pg:11 postgres_dockerfile/
nathanweeks commented 3 years ago

Thanks @bellerbrock ! That allowed the development deployment (docker-compose up -d) to work. I can't quite replicate that success with a production deployment (docker-compose -f docker-compose.yml -f production.yml up -d), I suspect due to incorrect db connection info in sgn.conf?

I'll proffer a few suggestions regarding the topic/use_empty_fixture branch of postgres_dockerfile:

  1. The most recent postgres base image is postgres:12.5 (and postgres:12.6 should be out any day now); it might be worth using the most-recent version before pushing a new breedbase/pg image to Docker Hub to include additional security updates, bug fixes, etc.
  2. For reproducibility & aid in troubleshooting, it could be beneficial to fetch empty_fixture.sql from a specific tag or commit (e.g., https://raw.githubusercontent.com/solgenomics/sgn/sgn-292.0/t/data/fixture/empty_fixture.sql). Also, postgres_dockerfile releases could be tagged to allow easy correlation with the image tag on Docker Hub (as a bonus, Docker Hub automated builds could be set up & configured to create a new image when a new tag is created in the postgresql_dockerfile git repo on GitHub).
  3. A couple initial minor/unnecessary simplifications that might be considered for postgresql_dockerfile: (1) Use the ADD directive to obviate the need for wget, and (2) set POSTGRES_DB=breedbase to avoid creating an apparently-unused "postgres" database; e.g.:
    
    FROM postgres:12.5

ENV POSTGRES_DB=breedbase ENV POSTGRES_PASSWORD=postgres

ADD --chown=postgres:postgres https://raw.githubusercontent.com/solgenomics/sgn/sgn-292.0/t/data/fixture/empty_fixture.sql /

COPY run.sh /docker-entrypoint-initdb.d/

(note then that run.sh would omit `CREATE DATABASE breedbase;`). These are purely optional and at your discretion, though (and I'm not sure if any users depend on the existence of a separate "postgres" database).

I'll add that in the development version, I encountered errors when visiting certain pages:

breedbase_web | [error] DBIx::Class::Storage::DBI::_dbh_execute(): DBI Exception: DBD::Pg::st execute failed: ERROR: column mbreedbase_web | LINE 1: ... me.name, me.description, me.dataset, me.is_live, me.is_publ... breedbase_web | ^ [for Statement "SELECT me.sp_datasetid, me.sp



I think it *may* be [this issue](https://github.com/solgenomics/sgn/issues/3319#issuecomment-773376786). If so, perhaps that suggests eventually updating prepare.sh to specify commits / tags (or perhaps more conventionally, including the repositories in breedbase_dockerfile as git submodules) to ensure consistency between the source code and schema (instead of compatibility being dependent on when the git repo is cloned)? But that could be beyond the scope of this PR.

Anyway, thanks again! I'm looking forward to some form of this PR being merged!
bellerbrock commented 3 years ago

@nathanweeks thanks for the extra info and advice! All the postgres dockerfile suggestions make sense, and you're right we need a better way to ensure consistency between the source code and schema. One solution I was thinking of would be to merge the postgres_dockerfile and breedbase_dockerfile into a single repo with each dockerfile in their own subdir. As I understand it that works fine for docker-compose. Then the new postgres and breedbase images could be built at the same time off of the same version of the source code cloned by prepare.sh.

For the errors you're still seeing:

I suspect due to incorrect db connection info in sgn.conf

Probably. the production deployment doesn't copy the sgn_local.conf file from the template automatically, so create it manually right before composing to make sure it's up to date cp sgn_local.conf.template sgn_local.conf

I think it may be this issue.

I think you're right. The empty_fixture is out of date compared to the source code version used in the breedbase/breedbase:v0.17 image. As we switch to using the empty_fixture to build the postgres image it will be extra important to update with each release tag.

Looks like the fixture was last updated when tag sgn-285.0 was released so in the meantime you should be able to revert the sgn repo to that tag in your container to fix the errors.

nathanweeks commented 3 years ago

Hi @bellerbrock ,

Probably. the production deployment doesn't copy the sgn_local.conf file from the template automatically, so create it manually right before composing to make sure it's up to date cp sgn_local.conf.template sgn_local.conf

For the production deployment, it seems that the Dockerfile would need to be modified to, e.g., COPY sgn_local.conf s/home/production/cxgn/sgn/, and a new breedbase image built?

Looks like the fixture was last updated when tag sgn-285.0 was released so in the meantime you should be able to revert the sgn repo to that tag in your container to fix the errors.

I tried reverting the sgn repo to sgn-285.0, and gave that a try, and still see the same error...

One solution I was thinking of would be to merge the postgres_dockerfile and breedbase_dockerfile into a single repo with each dockerfile in their own subdir. As I understand it that works fine for docker-compose. Then the new postgres and breedbase images could be built at the same time off of the same version of the source code cloned by prepare.sh.

That's a good suggestion; it would be more convenient to add a subdirectory with Dockerfile (e.g., db/Dockerfile) and other relevant files, and then docker-compose build breedbase_db to build a new image (or if the "breedbase" service were similarly modified, docker-compose up -d --build to build & start both breedbase & breedbase_db):

  breedbase_db:
    build: db
    image: breedbase/pg:11
...

OTOH, to more-tightly-couple the sgn source code with the database, could the "breedbase" service populate the database (if empty) with a suitable schema? Then a vanilla postgres base image could be used (whose version could be easily updated on new postgres release), instead of maintaining a custom image for the database; e.g.:

diff --git a/docker-compose.override.yml b/docker-compose.override.yml
index db3271a..c936ce7 100644
--- a/docker-compose.override.yml
+++ b/docker-compose.override.yml
@@ -12,6 +12,9 @@ services:
       - type: bind
         source: ./repos
         target: /home/production/cxgn
+      - type: bind
+        source: ./entrypoint.sh
+        target: /entrypoint.sh
       - archive:/home/production/archive
       - tmp:/home/production/tmp
       - cache:/home/production/cache
diff --git a/docker-compose.yml b/docker-compose.yml
index f6a2065..acfb675 100644
--- a/docker-compose.yml
+++ b/docker-compose.yml
@@ -3,10 +3,22 @@ version: "3.7"
 services:
   breedbase:
     image: breedbase/breedbase:v0.17
+    environment:
+      PGDATABASE: breedbase
+      PGHOST: breedbase_db
+      PGPASSWORD: postgres
+      PGUSER: postgres
     depends_on:
-      - breedbase_db
+      breedbase_db:
+        condition: service_healthy
     container_name: breedbase_web

   breedbase_db:
-    image: breedbase/pg:11
+    image: postgres:12.6
     container_name: breedbase_db
+    environment:
+      POSTGRES_DB: breedbase
+      POSTGRES_PASSWORD: postgres
+    healthcheck:
+      test: "pg_isready -U postgres -d $${POSTGRES_DB} || exit 1"
+      interval: 10s
diff --git a/entrypoint.sh b/entrypoint.sh
index 4e7b46d..d34d834 100644
--- a/entrypoint.sh
+++ b/entrypoint.sh
@@ -7,6 +7,12 @@ sed -i s/localhost/$HOSTNAME/g /etc/slurm-llnl/slurm.conf
 /etc/init.d/slurmd start
 #/etc/init.d/postgres start

+if [ $(psql -Atc 'select count(distinct table_schema) from information_schema.tables;') -eq 2 ]; then
+  psql -c "CREATE USER web_usr PASSWORD 'postgres';"
+  psql -f t/data/fixture/empty_fixture.sql
+  ( cd db && ./run_all_patches.pl -u ${PGUSER} -p "${PGPASSWORD}" -h ${PGHOST} -d ${PGDATABASE} -e janedoe )
+fi
+
 if [ "$MODE" == "DEVELOPMENT" ]; then
    /home/production/cxgn/sgn/bin/sgn_server.pl --fork -r -d -p 8080
 else

(where the postgres healthcheck is to guarantee that when the services are first created, the "breedbase_db" service starts before the "breedbase" service, so the latter is able to connect; note I tooko the run_all_patches.pl from https://github.com/FamosoLab/lsurice-breedbase-conf#run-patches-on-breedbase_fresh , and am not sure about it's appropriateness in this situation)