-
-
Notifications
You must be signed in to change notification settings - Fork 997
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
Add docker-compose.healthcheck.yml #384
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very cool, thanks for this. Some questions/comments posted.
compose/bin/setup
Outdated
--elasticsearch-host=$ES_HOST \ | ||
--elasticsearch-port=$ES_PORT \ | ||
--elasticsearch-host=elasticsearch \ | ||
--elasticsearch-port=9200 \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'll want to back out this update so a user can override the Elasticsearch host/port with env vars.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, I will recover this part.
But I think this setting is very common.
If someone need change Elasticsearch host/port, change here is not enough.
docker-compose.yml (just guess, not test yet)
elasticsearch:
image: markoshust/magento-elasticsearch:7.6.2-2
ports:
- "9200:9200" # this also need to change
- "9300:9300"
environment:
- "discovery.type=single-node"
- ES_HOST=elasticsearch # add new value
- ES_PORT=9200 # add new value
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, I'd avoid the vars being set here though and set defaults within the bash file (see #384 (comment)). But, this is how & where someone would override these vars. This would be useful to add to the documentation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this push request (commit) should only keep changes about HEALTHCHECK
.
For other environmental variables, I will create another one for it.
compose/bin/setup
Outdated
@@ -3,14 +3,10 @@ set -o errexit | |||
source env/db.env | |||
BASE_URL=${1:-magento2.test} | |||
|
|||
ES_HOST=elasticsearch | |||
ES_PORT=9200 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to add these back in
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is where we need updates. These settings are file, but need to pull in from envvar if it exists.
I didn't test but something like this pseudo code may work:
ES_HOST="${ES_HOST:=elasticsearch}"
ES_PORT="${ES_PORT:=9200}"
This way we don't need to define anything in the XML files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On second thought, maybe be better to create an env/elasticsearch.env
file for these similar to https://github.com/markshust/docker-magento/blob/master/compose/env/db.env
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@markshust I think this push request (commit) should only keep changes about HEALTHCHECK
.
For other environmental variables, I will create another one for it.
compose/docker-compose.yml
Outdated
@@ -19,12 +19,27 @@ services: | |||
- appdata:/var/www/html | |||
- sockdata:/sock | |||
- ssldata:/etc/nginx/certs | |||
healthcheck: | |||
test: 'curl --fail http://127.0.0.1:8000' | |||
interval: 1m30s |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is the interval
a different value from the docker-compose.dev.yml
file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think remove all interval, timeout, and start-period will be fine.
Using their default value may a better choice
The options that can appear before CMD are:
--interval=DURATION (default: 30s)
--timeout=DURATION (default: 30s)
--start-period=DURATION (default: 0s)
--retries=N (default: 3)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool, definitely prefer the defaults 👍
compose/docker-compose.yml
Outdated
healthcheck: | ||
test: 'curl --fail http://127.0.0.1:8000' | ||
interval: 1m30s | ||
timeout: 10s |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same with timeout, I would think these would be consistent between the files unless there was some other reason to do so (which should probably then be documented).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove all interval, timeout, and start-period, and use default value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
compose/docker-compose.yml
Outdated
@@ -53,6 +78,11 @@ services: | |||
- "5672:5672" | |||
volumes: | |||
- rabbitmqdata:/var/lib/rabbitmq | |||
healthcheck: | |||
test: 'rabbitmq-diagnostics -q ping' | |||
interval: 30s |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The healthcheck params are repeated a few times, can we make this into a YAML anchor like volumes are done?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove all interval, timeout, and start-period, and use default value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
@rangerz this has been sitting here a long time. Should it be worked on & merged in? It's a bit invasive, I want to make sure we need this, as there seems to be many moving pieces to implement this. I've really never had a problem with my Docker containers being in an unhealthy state. |
Forget the above message I added Please review it. |
Hi @rangerz, all looks good! But I don't see any additions to I just merged your other big PR in for docker-compose/docker compose, so perhaps that needed to be merged in first? |
@markshust Yes, here is waiting for the It's updated to put |
Awesome... this is sweet!!! 🧡🧡🧡 |
According to the
HEALTHCHECK
for docker containers I mentioned in here@markshust, please rebuild all of php (7.3, 7.4, 8.0) and elasticsearch(6.8, 7.6) images. Thanks 👍
In
bin/status
also can view all health statusRefs:
magento/magento-cloud-docker
sun-asterisk-research/docker-php