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 tini as init system to improve signal handling in docker container #6892

Merged
merged 3 commits into from
Mar 11, 2024

Conversation

yubiuser
Copy link
Contributor

@yubiuser yubiuser commented Mar 9, 2024

So far, there was no init system on the docker container and any SIGTERM is lost somewhere between the host and mkdocs. The consequence is that while stopping the container, nothing signaled docker that the process successful finished and docker waited the default 10 seconds before it killed the container.

root@s740:/home/pi/docker/mkdocs-material# docker compose down
[+] Running 1/1
 ✔ Container mkdocs-material  Removed                                                10.2s 

This PR adds tini as an init system which handles such signals and boost the shutdown

root@s740:/home/pi/docker/mkdocs-material# docker compose down
[+] Running 1/1
 ✔ Container mkdocs-material  Removed                                                 0.2s

Signed-off-by: Christian König <ckoenig@posteo.de>
Signed-off-by: Christian König <ckoenig@posteo.de>
@squidfunk
Copy link
Owner

Thanks for suggesting. So, as I read correctly, we would need to change all commands in our documentation to explicitly include mkdocs at the end, and all users that run the container would need to change all invocations?

@polarathene
Copy link
Contributor

tini is available by default, no need to include in Dockerfile (unless you want make it used by default instead of opt-in).


So, as I read correctly, we would need to change all commands in our documentation to explicitly include mkdocs at the end, and all users that run the container would need to change all invocations?

No, the contributor has just followed the advice from the tini README.

When starting a container it will run the entrypoint + cmd (default or user supplied), tini supports -- like many CLI programs to communicate where it's options input stops. After that is your command for tini to run, so you can use:

ENTRYPOINT ["/sbin/tini", "--", "mkdocs"]
CMD ["serve", "--dev-addr=0.0.0.0:8000"]

This is only relevant to Docker Compose for the shutdown time concern being addressed however (doesn't apply to docker run), which could alternative be configured in compose.yaml via stop_grace_period: 0s.

For reference here is a compose.yaml with all 3 options if you'd like to try them easily:

services:
  docs:
    #image: squidfunk/mkdocs-material
    # Either of these would resolve the concern:
    #init: true
    #stop_grace_period: 0s
    # Or extend the image (_bit pointless in this case_):
    image: mkdocs-material-with-tini
    build:
       dockerfile_inline: |
         FROM squidfunk/mkdocs-material
         RUN apk add --no-cache tini
         ENTRYPOINT ["/sbin/tini", "--", "mkdocs"]
         CMD ["serve", "--dev-addr=0.0.0.0:8000"]
    configs:
      - source: mkdocs
        target: /docs/mkdocs.yml
      - source: hello
        target: /docs/docs/index.md
    ports:
      - "8000:8000"

# Static inline config files for a quick example,
# normally these would be separate files mounted via `volumes` key.
configs:
  # mkdocs.yml:
  mkdocs:
    content: |
      site_name: Example
      theme:
        name: material

  # index.md:
  hello:
    content: |
      Hello **world**!

@squidfunk
Copy link
Owner

Thanks for the clarification. I was interested whether we would need to change the API, as we would need to issue a new major release because all CI scripts using our Docker image would break – nothing were keen on doing for obvious reasons 😅 What's the upside of using tini outside of docker compose?

Signed-off-by: Christian König <ckoenig@posteo.de>
@yubiuser
Copy link
Contributor Author

unless you want make it used by default instead of opt-in

This is what I intended to do. So it is independent of users who might not know --tini or services.<service name>.init: true.


I moved mkdocs back into the ENTRYPOINT so no doc/API change is necessary and all user scripts should work as before.

@yubiuser
Copy link
Contributor Author

What's the upside of using tini outside of docker compose?

Prevents the creation of zombie processes, ensures default signal handlers work even if mkdocs does not handle them by itself.

For a detailed explanation see krallin/tini#8

@squidfunk
Copy link
Owner

Thanks!

@squidfunk squidfunk merged commit 354491e into squidfunk:master Mar 11, 2024
4 checks passed
@yubiuser yubiuser deleted the tini branch March 11, 2024 04:52
Dockerfile Show resolved Hide resolved
@iBug
Copy link
Contributor

iBug commented Mar 14, 2024

Any chance this gets released soon? I run multiple Docker containers as development servers on my machine and they aren't responsive for a shutdown event (I can see systemd-shutdown[1]: Waiting for processes: bash, python3, python3, python3, python3...). I'd love to see this update go into the Docker image so they're less painful.

@polarathene
Copy link
Contributor

@iBug you can just use any of the alternatives I suggested in the meantime: #6892 (comment)

Just add init: true or stop_grace_period: 0s to your compose.yaml service.

@squidfunk
Copy link
Owner

Yes, it will be released soon. We do a release every 1-2 weeks, sometimes even more often. In the meantime, thanks to @polarathene for providing a workaround!

@squidfunk
Copy link
Owner

I also feel I need to mention this again: mkdocs serve is not meant for production. The Docker image is only provided for previewing or building your documentation, not for serving it. The fact that you're using Docker Compose to knit this together with other services is already a smell. MkDocs should not be running in production.

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.

4 participants