Docker - Slow disk awareness

lohrbini commented 5 months ago

[ X ] I have read the FAQ.

Describe the bug

The issue with starting the Docker container lies in the fact that the pgid and puid are controlled via environment variables, allowing for flexible configuration. However, during the initial startup, when the entrypoint performs a chown operation, it exceeds the health checks defined in the compose file, leading to an error.

To Reproduce

The behavior can be reproduced by taking the example from the original documentation to set up the project locally and running it on non-fast disks. The Docker containers encounter errors every time because the health checks cannot function properly.

Versions:

Docker: [e.g. 20.10.12], run with docker version

Client: Docker Engine - Community
 Version:           26.0.1
 API version:       1.45
 Go version:        go1.21.9
 Git commit:        d260a54
 Built:             Thu Apr 11 10:53:21 2024
 OS/Arch:           linux/amd64
 Context:           default

Server: Docker Engine - Community
 Engine:
  Version:          26.0.1
  API version:      1.45 (minimum version 1.24)
  Go version:       go1.21.9
  Git commit:       60b9add
  Built:            Thu Apr 11 10:53:21 2024
  OS/Arch:          linux/amd64
  Experimental:     true
 containerd:
  Version:          1.6.31
  GitCommit:        e377cd56a71523140ca6ae87e30244719194a521
 runc:
  Version:          1.1.12
  GitCommit:        v1.1.12-0-g51d5e94
 docker-init:
  Version:          0.19.0
  GitCommit:        de40ad0

Suggestion It should be considered in the official documentation that not all instances run on fast disks. One way to address this could be to fix the user to a specific pgid and puid to prevent running the application as the root user. Additionally, the chown operation should be moved out of the entrypoint and be added into the image build process

karlomikus commented 5 months ago

Hello, not sure what you mean about aware method, but chown should definitely be moved to Dockerfile.

Thanks!

lohrbini commented 5 months ago

Hey 👋 ,

it maybe should also be considered if the user mapping should be static to avoid running the application as root.

Also updated the wording in the issue to clarify the problem properly

proffalken commented 5 months ago

Yes, please move the chown command into the Dockerfile - everytime I make a change to my setup and the api container is restarted, it takes up to 10 minutes just to get to a point where I can check and see what's going on!

lohrbini commented 5 months ago

This should fix the issue by adding the user and permissions into the build process. Needs to be validated exactly.

diff --git a/Dockerfile b/Dockerfile
index c122bf0..54c6101 100644
--- a/Dockerfile
+++ b/Dockerfile
@@ -55,6 +55,10 @@ ADD https://github.com/bar-assistant/data.git ./resources/data
 # Configure nginx
 COPY ./resources/docker/dist/nginx.conf /etc/nginx/sites-enabled/default

+# Setup user
+RUN groupmod -o -g "$PGID" www-data \
+    && usermod -o -u "$PUID" www-data
+
 # Add container entrypoint script
 COPY ./resources/docker/dist/entrypoint.sh /usr/local/bin/entrypoint

@@ -63,7 +67,10 @@ RUN chmod +x /usr/local/bin/entrypoint \
     && sed -i "s/{{VERSION}}/$BAR_ASSISTANT_VERSION/g" ./docs/open-api-spec.yml \
     && composer install --optimize-autoloader --no-dev \
     && mkdir -p /var/www/cocktails/storage/bar-assistant/ \
-    && echo "* * * * * www-data cd /var/www/cocktails && php artisan schedule:run >> /dev/null 2>&1" >> /etc/crontab
+    && echo "* * * * * www-data cd /var/www/cocktails && php artisan schedule:run >> /dev/null 2>&1" >> /etc/crontab \
+    && chown -R $PUID:PGID /var/www/cocktails
+
+USER www-data

 EXPOSE 3000

diff --git a/resources/docker/dist/entrypoint.sh b/resources/docker/dist/entrypoint.sh
index 641176c..c07d148 100644
--- a/resources/docker/dist/entrypoint.sh
:...skipping...
diff --git a/Dockerfile b/Dockerfile
index c122bf0..54c6101 100644
--- a/Dockerfile
+++ b/Dockerfile
@@ -55,6 +55,10 @@ ADD https://github.com/bar-assistant/data.git ./resources/data
 # Configure nginx
 COPY ./resources/docker/dist/nginx.conf /etc/nginx/sites-enabled/default

+# Setup user
+RUN groupmod -o -g "$PGID" www-data \
+    && usermod -o -u "$PUID" www-data
+
 # Add container entrypoint script
 COPY ./resources/docker/dist/entrypoint.sh /usr/local/bin/entrypoint

@@ -63,7 +67,10 @@ RUN chmod +x /usr/local/bin/entrypoint \
     && sed -i "s/{{VERSION}}/$BAR_ASSISTANT_VERSION/g" ./docs/open-api-spec.yml \
     && composer install --optimize-autoloader --no-dev \
     && mkdir -p /var/www/cocktails/storage/bar-assistant/ \
-    && echo "* * * * * www-data cd /var/www/cocktails && php artisan schedule:run >> /dev/null 2>&1" >> /etc/crontab
+    && echo "* * * * * www-data cd /var/www/cocktails && php artisan schedule:run >> /dev/null 2>&1" >> /etc/crontab \
+    && chown -R $PUID:PGID /var/www/cocktails
+
+USER www-data

 EXPOSE 3000

diff --git a/resources/docker/dist/entrypoint.sh b/resources/docker/dist/entrypoint.sh
index 641176c..c07d148 100644
--- a/resources/docker/dist/entrypoint.sh
+++ b/resources/docker/dist/entrypoint.sh
@@ -1,12 +1,6 @@
 #!/bin/bash
 set -e

-# Get PUID/PGID
-PUID=${PUID:-1000}
-PGID=${PGID:-1000}
-
-cd /var/www/cocktails
-
 echo "Starting Bar Assistant, this can take a few minutes depending on the system..."

 echo "
@@ -14,10 +8,6 @@ User uid:    $PUID
 User gid:    $PGID
 "

-groupmod -o -g "$PGID" www-data
-usermod -o -u "$PUID" www-data
-chown -R www-data:www-data /var/www/cocktails
-
 gosu www-data ./resources/docker/dist/run.sh

 php-fpm & nginx -g 'daemon off;'

@proffalken overwriting the entrypoint can solve the wait-for-startup issue.

proffalken commented 5 months ago

@proffalken overwriting the entrypoint can solve the wait-for-startup issue.

Yeah, good point, although I'm working with k3s (cut-down kubernetes) rather than Docker, so I'm finding a few "interesting" things along the way that don't quite translate over :)

karlomikus commented 5 months ago

This is a little more than moving chown into docker image, since file ownership gets lost if you mount existing directory.

I think the ideal thing is to refactor base image to use non-root user for everything, but this would probably be major breaking change.

Maybe temporary performance fix would be to chown only mounted directory instead of the whole project dir, but I need to test it out.

lohrbini commented 5 months ago

I tried a quick edit locally and building an image . But as you mentioned it is not that basic.

karlomikus commented 5 months ago

I think this should work as a "temporary" workaround:

Add custom user with a specific id (ex. 1000) in dockerfile
chown everything project related to that user
In entrypoint check if PUID/GUID env variables are different from previously assigned ID
- If they are then run chown to update the whole project to the new id
- If they are the same we don't have to do anything

This should stop changing permissions of the project directory on every restart.

karlomikus commented 4 months ago

I've pushed some update to docker startup script, and updated the docs to remove health checks temporary.

In future moving to non-privileged image should introduce further perf improvements. #287

karlomikus / bar-assistant

Docker - Slow disk awareness #276