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

Add HEALTHCHECK instruction to Dockerfile #206

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

maesbrisa
Copy link

From this issue related to dispatcher and redis dependency.

Using this feature, we can use it in docker-compose.yml as follows:

 dispatcher:
    image: librenms/librenms:latest
    container_name: librenms_dispatcher
    hostname: librenms-dispatcher
    cap_add:
      - NET_ADMIN
      - NET_RAW
    depends_on:
      - librenms
      - redis
    volumes:
      - "./librenms:/data"
    env_file:
      - "./librenms.env"
    environment:
      - "TZ=${TZ}"
      - "PUID=${PUID}"
      - "PGID=${PGID}"
      - "DB_HOST=db"
      - "DB_NAME=${MYSQL_DATABASE}"
      - "DB_USER=${MYSQL_USER}"
      - "DB_PASSWORD=${MYSQL_PASSWORD}"
      - "DB_TIMEOUT=60"
      - "DISPATCHER_NODE_ID=dispatcher1"
      - "REDIS_HOST=redis"
      - "REDIS_PORT=6379"
      - "REDIS_DB=0"
      - "SIDECAR_DISPATCHER=1"
    restart: always
    healthcheck:
      test: ["CMD", "nc", "-w", "2", "redis", "6379"]
      interval: 10s
      timeout: 10s
      retries: 2

@maesbrisa maesbrisa requested a review from crazy-max as a code owner May 20, 2021 11:53
@CLAassistant
Copy link

CLAassistant commented May 20, 2021

CLA assistant check
All committers have signed the CLA.

@crazy-max
Copy link
Member

crazy-max commented May 26, 2021

@maesbrisa HEALTHCHECK NONE will "disable any healthcheck inherited from the base image" but there is none for the base image currently used.

@maesbrisa
Copy link
Author

maesbrisa commented May 26, 2021

I'm not sure I understand you. Yes, if the command HEALTHCHECK is used in previous images builds, the NONE value will overwrite any existent healthcheck, but I saw nothing related in previous Dockerfiles.

Anyway, if the NONE is the problem and if we consider my example that covers the use cases, we can substitute

HEALTHCHECK NONE

for:

HEALTHCHECK --interval=10s --timeout=10s CMD nc -w 2 redis 6379

@crazy-max
Copy link
Member

@maesbrisa There is no HEALTHCHECK currently used in this image or the based one, therefore it's not necessary to add this.

HEALTHCHECK --interval=10s --timeout=10s CMD nc -w 2 redis 6379

AFAIK there is no issue with the liveliness between the container and redis. librenms/librenms#12707 as well as #149 is smth that needs to be impl upstream on main repo. Not here.

@maesbrisa
Copy link
Author

Agree 100% about treating bugs related liveliness in the main repo, but the PR was about improving the monitoring during the containers execution, similar to this example: https://github.com/crazy-max/docker-msmtpd/blob/master/Dockerfile

@crazy-max
Copy link
Member

crazy-max commented May 27, 2021

@maesbrisa

Agree 100% about treating bugs related liveliness in the main repo, but the PR was about improving the monitoring during the containers execution, similar to this example: https://github.com/crazy-max/docker-msmtpd/blob/master/Dockerfile

HEALTHCHECK --interval=10s --timeout=10s CMD nc -w 2 redis 6379

Yes but for librenms image it should rely on an internal probe which must be implemented on main repo and not directly on an external service. For example this suggestion could be added on librenms repo. Do we have smth like that @murrant?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants