Additional Docker Features #1255
Conversation
There was a problem hiding this comment.
Great work @natankeddem. Thanks. Could you move the docker-compose.yml into a separate example? And maybe also providing a demo main.py and mounting the local folder as /app? That way users could see right away copy the folder and start hacking.
|
I've made some changes to the PR, @natankeddem. I hope you approve:
|
|
One other thing: could we get rid of the entrypoint by moving it all into the |
|
So a few things to unpack here. The reasons for using entrypoint script over cmd:
|
|
Some explanation on docker signal handling and why endpoint is needed. On a side note once signaling is enabled the container will not only shutdown gracefully but will also shutdown noticeably quicker. |
|
I think my original intent with the compose was to show more of an end user/end consumer usage. These end users would be people that had no need to manipulate the application code, they just want to utilize the functionality of the container. In this usage you would want to have persistent storage of just the .nicegui directory and would not share in your own code via a /app mount. Maybe it makes sense to remove the /app volume mount and create a 2nd compose or alter the development dockerfile in a separate PR (or in this one) to add these features since the development compose already mounts /app? |
|
After some soul searching here I want to quantify what we are trying to do here. We can categorize the NiceGUI audience in 3 different classes of users.
Perhaps all 3 classes of users deserve their own docker-compose examples/implementations? Sorry if I am complicating things I just want to clarify the customers for what we are developing here and what their specific needs might be. Hopefully this will get us something that is more useful and organized at the end of the process. |
to demonstrate the signal passing problem
Yes, it works for me: ➜ docker compose up
[+] Running 1/1
⠿ Container docker_features-nicegui-1 Recreated 0.1s
Attaching to docker_features-nicegui-1
docker_features-nicegui-1 | NiceGUI ready to go on http://localhost:8080, and http://192.168.16.2:8080
^CGracefully stopping... (press Ctrl+C again to force)
[+] Running 1/1
⠿ Container docker_features-nicegui-1 Stopped 0.5s
canceled
➜ docker compose logs
docker_features-nicegui-1 | NiceGUI ready to go on http://localhost:8080, and http://192.168.16.2:8080
docker_features-nicegui-1 | Shutdown has been initiated!This is in line to the reference you posted above.
True! The main downside of using the entrypoint besides more code is that the permissions of mounted Your categorization of the three audiences is very helpful. Thanks.
This PR should focus on 2 with some hints for 3 in my opinion. |
|
It is interesting that signal passing is working with CMD. At one point I thought I tested it and it didn't work and I saw the evidence for why in my research; reference previous link. Although, now when I test it again it seems to work. Perhaps I was mixing things up in my iterations of testing. I still think UID/GID mimicking is a good way to go. The user namespace remapping doesn't seem to be used by many and I myself will admit that I have a hard time wrapping my head around it. It seems rather complicated and requires the user have fairly advanced docker knowledge, while the ability to match your host user uid/gid is fairly simple as long as you have fairly basic linux knowledge. The PUID/PGID environmental variables have become popular since linuxserver.io seems to use that in all of there images so many people are already familiar with their usage. We could even probably use the same docker-entrypoint.sh for all of the dockerfiles by making it more generic as you have shown. |
|
I played around with different combinations a bit more and read up: So there appears to be major differences in how docker handles standard string CMD/ENTRYPOINT vs JSON-array notation. Therefore the current implementation: Now is the question of where do you want to go with the try entrypoint? Currently the .nicegui directory/contents are created with the following permissions: Where do you want to go from here? |
|
Thanks for digging so deep and being persistent, @natankeddem. I really like to get convinced :-)
The argument "behind" that sentence is really convincing: it's much simpler for users to set env variables than using remapping or creating custom Dockerfiles. Another benefit of using ENTRYPOINT to set the user would be that the images which are build upon the So let's go with the ENTRYPOINT version. |
|
I think the readability goes up with utilizing the ENTRYPOINT/CMD combo so I went that way. With this change to a generic entrypoint it should be useable with the development dockerfile in the future if needed. |
(uvicorn is doing it already)
Very cool. Unfortunately I had some issues with signal passing which I could only repair by using |
and adding PUID/PGID
|
I've used the following script to see how fast the shutdown is happening: #!/usr/bin/env bash
docker build -t nicegui:latest --build-arg VERSION=1.3.6 -f release.dockerfile .
docker run -d --name nicegui_container nicegui:latest
sleep 2 # give container some time to start properly
start_time=$(date +%s)
docker stop nicegui_container
end_time=$(date +%s)
elapsed_time=$((end_time - start_time))
echo "Shutdown time: $elapsed_time" seconds
echo "Logs:"
docker logs nicegui_container
echo "----"
docker container rm nicegui_container && echo 'removed container'Time to shutdown was 0 seconds with |
|
I've switched from |
The current release dockerfile is limited in features. I propose adding the following 2 additional features: