-
Notifications
You must be signed in to change notification settings - Fork 100
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 healthchecks to docker-compose.yml #36
Conversation
Due to Redis licensing not being opensource anymore, here's a drop-in replacement with an opensource license. Source: https://redis.com/blog/redis-adopts-dual-source-available-licensing/ https://github.com/Snapchat/KeyDB/blob/main/COPYING
Added healthchecks for redis, db and misp-core services. misp-core and misp-modules now wait for redis and db to be healthy before starting.
docker-compose.yml
Outdated
@@ -12,7 +12,7 @@ services: | |||
- "SMARTHOST_ALIASES=${SMARTHOST_ALIASES}" | |||
|
|||
redis: | |||
image: redis:7.2 | |||
image: eqalpha/keydb:x86_64_v6.3.4 |
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.
This image is x64 only while the Redis image is ARM as well. Would it be possible to remove the arch modifier here?
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.
Note that unless MISP changes the default key-value storage, we should only document this alternative in the README.md rather than modify the docker-compose.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.
Fine by me. All their images are arch-specific except eqalpha/keydb:latest
which is both for arm64 and x86_64.
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 should also reference https://github.com/valkey-io/valkey if a docker image is available.
db: | ||
condition: service_healthy | ||
healthcheck: | ||
test: curl -ks https://localhost/users/login > /dev/null || exit 1 |
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.
Is this check run inside the container? In other words, Is localhost
the correct setting regardless of our BASE_URL
?
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.
Yes, it's run inside the container.
You can see below I don't expose ports in my stack and it's still healthy:
$ docker compose ps 2> /dev/null
NAME IMAGE COMMAND SERVICE CREATED STATUS PORTS
misp-db-1 mariadb:10.11 "docker-entrypoint.s…" db 43 hours ago Up 43 hours (healthy) 3306/tcp
misp-mail-1 ixdotai/smtp "/bin/entrypoint.sh …" mail 43 hours ago Up 43 hours 25/tcp
misp-misp-core-1 ghcr.io/misp/misp-docker/misp-core:latest "/entrypoint.sh" misp-core 42 hours ago Up 42 hours (healthy)
misp-misp-modules-1 ghcr.io/misp/misp-docker/misp-modules:latest "/usr/local/bin/misp…" misp-modules 43 hours ago Up 43 hours
misp-redis-1 eqalpha/keydb:x86_64_v6.3.4 "docker-entrypoint.s…" redis 43 hours ago Up 43 hours (healthy) 6379/tcp
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.
This is good stuff, but I think it's too early to replace redis.
Can we add the healthchecks first, and only when better docker images are published, move to a new key value store?
Alternatively, it should be easy to just document the alternatives available in the README.
docker-compose.yml
Outdated
image: redis:7.2 | ||
image: eqalpha/keydb:x86_64_v6.3.4 | ||
healthcheck: | ||
test: keydb-cli ping || exit 1 |
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.
What is the alternative command if were to use Redis here?
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.
redis-cli ping
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.
Will merge if changes to the underlying image are removed.
Also, note that we decided to migrate to valkey, so healthcheck commands might need updating.
I figured everyone would move to valkey but it wasn't available at the time. This is better? |
Looks good. Can you rebase and squash? Otherwise I will do it when merging (will wait for next MISP release). |
Add healthchecks
Hello,
here's a small PR to replace redis with keydb due to redis license changes and to add healthchecks to services.