Skip to content
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

employ docker-compose v3.4 and shared logging config #152

Merged
merged 5 commits into from
May 6, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion birdhouse/components/monitoring/docker-compose-extra.yml
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
version: '2.1'
version: "3.4"

services:
# https://github.com/google/cadvisor/blob/master/docs/running.md
Expand Down
2 changes: 1 addition & 1 deletion birdhouse/components/scheduler/docker-compose-extra.yml
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
version: '2.1'
version: "3.4"

services:
scheduler:
Expand Down
37 changes: 32 additions & 5 deletions birdhouse/docker-compose.yml
Original file line number Diff line number Diff line change
@@ -1,4 +1,12 @@
version: "2.1"
version: "3.4"

x-logging:
&default-logging
driver: "json-file"
options:
max-size: "200k"
Copy link
Collaborator

Choose a reason for hiding this comment

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

So we have not set any of those config currently (driver, max-size, max-file). What's was their default?

It's very nice to have a unified logging config for all containers but can each container, if needed, not use this config and roll their own?

And to loop back to the original Weaver PR, Weaver need to "hook" into other components logs and that's why it prefers other components to have unified logging config or is there another reason to introduce this unified logging?

Just to be clear, I like it to have a default and unified logging config, I just want to understand its relation to Weaver.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Docker's default is json so that remains the same.
The difference here is that we add log rotation over 10 files.
This is a config we have found a long time ago when deploying on our instances since twitcher and magpie can generate an immense quantity of logs (on each request). This allows discarding them gradually instead of accumulating GB of useless logs.
Each container can define its own logging section by using the relevant config instead of the default-logging reference.
I don't think there is a nice way to automate that though. Best could be to have an optional-component, but I think it is better to control the logs.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Using Weaver, it is a bit more tricky since a different docker-compose file is employed.
Depending on how docker-compose merges the files, the reference could or won't work across files.
Basically, if files are merged, and then references resolved, it would work. But if internally it uses PyYAML or similar lib, to read each file individually and merge them, Weaver's compose will raise a missing reference. So the config would need to be duplicated in its own file.

Copy link
Collaborator

Choose a reason for hiding this comment

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

So if I understand correctly, Weaver did not need this central logging and the log rotation to avoid accumulating infinite logs (which is indeed a nice thing).

From our recent "contributing guideline", this PR should not have been mixed with the Weaver PR to begin with. Or if mixed in, it should have been "well explained" that it is not required by Weaver?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, this is why it has been split by itself now that we updated the contribution guideline.
Please consider that Weaver PR was started in January and review only started in March, both long before we even started discussions of contribution guidelines.

max-file: "10"

services:
proxy:
image: pavics/canarieapi:0.3.5
Expand All @@ -25,6 +33,7 @@ services:
- thredds
entrypoint: /entrypoint
restart: always
logging: *default-logging

frontend:
image: pavics/pavics-frontend:1.0.5
Expand All @@ -34,6 +43,7 @@ services:
env_file:
- ./config/frontend/frontend.env
restart: always
logging: *default-logging

project-api:
image: pavics/pavics-project-api:0.9.0
Expand All @@ -47,6 +57,7 @@ services:
env_file:
- ./config/postgres/credentials.env
restart: always
logging: *default-logging

postgres:
image: postgres:9.6
Expand All @@ -62,6 +73,7 @@ services:
env_file:
- ./config/postgres/credentials.env
restart: always
logging: *default-logging

phoenix:
image: pavics/pyramid-phoenix:pavics-0.2.3
Expand All @@ -87,6 +99,7 @@ services:
- catalog
- proxy
restart: always
logging: *default-logging

catalog:
image: pavics/pavics-datacatalog:0.6.11
Expand All @@ -99,6 +112,7 @@ services:
depends_on:
- postgres
restart: always
logging: *default-logging

geoserver:
image: pavics/geoserver:2.9.3
Expand All @@ -116,6 +130,7 @@ services:
- postgis
entrypoint: /entrypointwrapper
restart: always
logging: *default-logging

malleefowl:
image: pavics/malleefowl:pavics-0.3.5
Expand All @@ -139,6 +154,7 @@ services:
depends_on:
- postgres
restart: always
logging: *default-logging

flyingpigeon:
image: birdhouse/flyingpigeon:1.6
Expand All @@ -154,6 +170,7 @@ services:
depends_on:
- postgres
restart: always
logging: *default-logging

finch:
image: ${FINCH_IMAGE}
Expand All @@ -170,6 +187,7 @@ services:
- wps_outputs:/data/wpsoutputs
- /tmp
restart: always
logging: *default-logging

raven:
image: pavics/raven:0.12.1
Expand All @@ -183,6 +201,7 @@ services:
- wps_outputs:/data/wpsoutputs
- /tmp
restart: always
logging: *default-logging

hummingbird:
image: pavics/hummingbird:0.5_dev
Expand All @@ -204,6 +223,7 @@ services:
depends_on:
- postgres
restart: always
logging: *default-logging

solr:
image: pavics/solr:5.2.1
Expand All @@ -213,9 +233,8 @@ services:
- "48983:9001"
volumes:
- ${DATA_PERSIST_ROOT}/solr:/data
volumes_from:
- mongodb
Copy link
Collaborator

Choose a reason for hiding this comment

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

From what I understood from our meeting, the removal of volumes_from directive was because of the update to format version 3.4, not because we want it or Weaver needs it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes. volumes_from is deprecated for >=3
There are a lot of improvements in docker-compose since v2, so better bump it while problems are still few.

This needs to be tested, but if I understand the compose correctly, that volumes_from directive specifically is useless, since mongodb doesn't even have a volumes section. Starting with Weaver though, I require a data persistence volume in mongo, so I would like to have this adjusted with docker-compose v3 before it other images are not using it anyway (and don't start breaking obviously).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Agreed, this looks harmless but should be tested to be sure. Has you or Mat gone over the failed build to determine the error is not related to this? Currently only the ESGF notebook should be intermittently failing because their server seems flaky but everything else should be green.

restart: always
logging: *default-logging

ncwms2:
image: pavics/ncwms2:2.0.4
Expand All @@ -228,9 +247,8 @@ services:
- wps_outputs:/pavics-data/wps_outputs
- ./config/ncwms2/custom.cfg:/opt/birdhouse/custom.cfg
- ./config/ncwms2/server.xml:/opt/birdhouse/eggs/birdhousebuilder.recipe.tomcat-0.2.9-py2.7.egg/birdhousebuilder/recipe/tomcat/server.xml
volumes_from:
- mongodb
restart: always
logging: *default-logging

thredds:
image: ${THREDDS_IMAGE}
Expand All @@ -254,6 +272,7 @@ services:
- ./config/thredds/entrypointwrapper:/entrypointwrapper:ro
entrypoint: /entrypointwrapper
restart: always
logging: *default-logging
healthcheck:
test:
[
Expand All @@ -271,11 +290,13 @@ services:
# Mongodb crash with permission denied errors if the command is not overriden like this
command: bash -c 'chown -R mongodb:mongodb /data && chmod -R 755 /data && mongod'
restart: always
logging: *default-logging

postgis:
image: pavics/postgis:2.2
container_name: postgis
restart: always
logging: *default-logging

portainer:
image: portainer/portainer
Expand All @@ -286,6 +307,7 @@ services:
ports:
- "9000:9000"
restart: always
logging: *default-logging

magpie:
image: pavics/magpie:3.8.0
Expand Down Expand Up @@ -313,6 +335,7 @@ services:
- ./config/magpie/permissions.cfg:/opt/local/src/magpie/config/permissions/permissions.cfg
- ./config/magpie/magpie.ini:/opt/local/src/magpie/config/magpie.ini
restart: always
logging: *default-logging

twitcher:
image: pavics/twitcher:magpie-3.8.0
Expand All @@ -330,6 +353,7 @@ services:
- ./config/twitcher/twitcher.ini:/opt/birdhouse/src/twitcher/twitcher.ini
command: "pserve /opt/birdhouse/src/twitcher/twitcher.ini"
restart: always
logging: *default-logging

postgres-magpie:
image: postgres:9.6
Expand All @@ -344,6 +368,7 @@ services:
- ${DATA_PERSIST_ROOT}/magpie_persist:/var/lib/postgresql/data/pgdata
- ./config/postgres-magpie/docker-entrypoint-initdb.d:/docker-entrypoint-initdb.d
restart: always
logging: *default-logging

jupyterhub:
image: pavics/jupyterhub:1.3.0-20201211
Expand Down Expand Up @@ -376,6 +401,7 @@ services:
- default
- jupyterhub_network
restart: always
logging: *default-logging

# need external network so the folder name is not prefixed to network name
networks:
Expand All @@ -392,4 +418,5 @@ volumes:
thredds_persistence:
external:
name: thredds_persistence

# vi: tabstop=8 expandtab shiftwidth=2 softtabstop=2
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
version: '2.1'
version: "3.4"
services:
magpie:
volumes:
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
version: '2.1'
version: "3.4"
services:
proxy:
volumes:
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
version: '2.1'
version: "3.4"
services:
emu:
image: ${EMU_IMAGE}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
version: '2.1'
version: "3.4"
services:
generic_bird:
image: ${GENERIC_BIRD_IMAGE}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
version: '2.1'
version: "3.4"
services:
magpie:
volumes:
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
version: '2.1'
version: "3.4"
services:
proxy:
volumes:
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
version: '2'
version: "3.4"
services:
thredds:
extra_hosts:
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
version: '2'
version: "3.4"
services:
thredds:
extra_hosts:
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
version: '2'
version: "3.4"
services:
thredds:
volumes:
Expand Down