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 PHP-FPM variant #4

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

fcharlaix-opendsi
Copy link

This PR add the support of "variants" images in the update process, like FPM.

To allow PHP-FPM to work properly with an external web server, the htdocs need to be accessible outside the PHP container.

This is done by storing Dolibarr original htdocs into /usr/src/dolibarr and on install or update, this folder is rsync into the /var/www/html volume.

This is not limited to a Nginx container, this could be used by a Kubernetes Nginx Ingress, Caddy or a host Nginx server.

@creekorful
Copy link
Member

Hello there,

First of all thanks for your contribution! Getting FPM support would be amazing.

I usually handle PHP-FPM and NGINX deployments by building separated containers with the same version number. For instance this is what we have done here at VOLD when we were first building our custom Dolibarr images. (See: https://github.com/vold-lu/docker-dolibarr).

We built two containers : dolibarr-php and dolibarr-nginx, one that bundle the source code and NGINX, the other that bundle the source code and PHP-FPM. The two containers then interact together using FastCGI.

I am not against your approach either but I don't understand the need to have a dedicated folder /usr/src/dolibarr. Why can't we just expose /var/www/html as readonly to the web container ?

Cheers,

@fcharlaix-opendsi
Copy link
Author

Hi,

I see, your way solve the need of a volume, but this requires to maintain another image and have the Dolibarr source code twice in image storage.

This PR use a separated directory because mounting the folder with a bind mount empty the content in the container :

services:
  test1:
    image: dolibarr/dolibarr:19
    container_name: test1
    entrypoint: /bin/ls
    command:
      - -a
      - /var/www/html

  test2:
    image: dolibarr/dolibarr:19
    container_name: test2
    volumes:
      - /tmp/test/vol:/var/www/html
    entrypoint: /bin/ls
    command:
      - -a
      - /var/www/html

  test3:
    image: dolibarr/dolibarr:19
    container_name: test3
    volumes:
      - test3:/var/www/html
    entrypoint: /bin/ls
    command:
      - -a
      - /var/www/html

volumes:
  test3:
Output logs
test1  | .
test1  | ..
test1  | accountancy
test1  | adherents
test1  | admin
test1  | api
test1  | asset
test1  | asterisk
test1  | barcode
test1  | blockedlog
test1  | bom
test1  | bookcal
test1  | bookmarks
test1  | categories
test1  | collab
test1  | comm
test1  | commande
test1  | compta
test1  | conf
test1  | contact
test1  | contrat
test1  | core
test1  | cron
test1  | custom
test1  | datapolicy
test1  | dav
test1  | debugbar
test1  | delivery
test1  | document.php
test1  | don
test1  | ecm
test1  | emailcollector
test1  | eventorganization
test1  | expedition
test1  | expensereport
test1  | exports
test1  | externalsite
test1  | favicon.ico
test1  | fichinter
test1  | filefunc.inc.php
test1  | fourn
test1  | ftp
test1  | holiday
test1  | hrm
test1  | imports
test1  | includes
test1  | index.php
test1  | install
test1  | intracommreport
test1  | knowledgemanagement
test1  | langs
test1  | loan
test1  | mailmanspip
test1  | main.inc.php
test1  | margin
test1  | master.inc.php
test1  | modulebuilder
test1  | mrp
test1  | multicurrency
test1  | opcachepreload.php
test1  | opensurvey
test1  | partnership
test1  | paybox
test1  | paypal
test1  | printing
test1  | product
test1  | projet
test1  | public
test1  | reception
test1  | recruitment
test1  | resource
test1  | robots.txt
test1  | salaries
test1  | security.txt
test1  | societe
test1  | stripe
test1  | supplier_proposal
test1  | support
test1  | takepos
test1  | theme
test1  | ticket
test1  | user
test1  | variants
test1  | viewimage.php
test1  | webhook
test1  | webservices
test1  | website
test1  | workstation
test1  | zapier
test2  | .
test2  | ..
test2  | custom
test3  | .
test3  | ..
test3  | accountancy
test3  | adherents
test3  | admin
test3  | api
test3  | asset
test3  | asterisk
test3  | barcode
test3  | blockedlog
test3  | bom
test3  | bookcal
test3  | bookmarks
test3  | categories
test3  | collab
test3  | comm
test3  | commande
test3  | compta
test3  | conf
test3  | contact
test3  | contrat
test3  | core
test3  | cron
test3  | custom
test3  | datapolicy
test3  | dav
test3  | debugbar
test3  | delivery
test3  | document.php
test3  | don
test3  | ecm
test3  | emailcollector
test3  | eventorganization
test3  | expedition
test3  | expensereport
test3  | exports
test3  | externalsite
test3  | favicon.ico
test3  | fichinter
test3  | filefunc.inc.php
test3  | fourn
test3  | ftp
test3  | holiday
test3  | hrm
test3  | imports
test3  | includes
test3  | index.php
test3  | install
test3  | intracommreport
test3  | knowledgemanagement
test3  | langs
test3  | loan
test3  | mailmanspip
test3  | main.inc.php
test3  | margin
test3  | master.inc.php
test3  | modulebuilder
test3  | mrp
test3  | multicurrency
test3  | opcachepreload.php
test3  | opensurvey
test3  | partnership
test3  | paybox
test3  | paypal
test3  | printing
test3  | product
test3  | projet
test3  | public
test3  | reception
test3  | recruitment
test3  | resource
test3  | robots.txt
test3  | salaries
test3  | security.txt
test3  | societe
test3  | stripe
test3  | supplier_proposal
test3  | support
test3  | takepos
test3  | theme
test3  | ticket
test3  | user
test3  | variants
test3  | viewimage.php
test3  | webhook
test3  | webservices
test3  | website
test3  | workstation
test3  | zapier
test1 exited with code 0
test2 exited with code 0
test3 exited with code 0

This could be needed for dev purpose (having access to the source code in an editor), or for people that want to store this in another disk without moving all docker data.

(I also think, if someone accidentally modifies the volume content, they're not getting the file updated with future releases)

If you have a cleaner way I'm very interested.

Thanks,

@creekorful
Copy link
Member

Hello,

This PR use a separated directory because mounting the folder with a bind mount empty the content in the container

And sadly using a volume mount won't be better... While the first time it will populate the volume with the container data, on upgrade the volume won't be updated and therefore no matter which volume type we are using, we will need a synchronisation mechanism like you suggest here.

I have mixed feelings about this implementation. Dolibarr source code by nature should be immutable and therefore does not really fit the volume use-case (persist data across containers restart). (This is only my opinion)

In addition to your suggestion I only have two others approach at the moment :

Provide another web container

Building another (let's call it web) container that duplicate the application source code and provide a web server / reverse proxy.

We will end-up having :

  • dolibarr/dolibarr:19.0.3-nginx-only
  • dolibarr/dolibarr:19.0.3-caddy-only
  • dolibarr/dolibarr:19.0.3-php-fpm-only

And the end user can choose which web flavour he want to deploy and deploy the php backend as well. This approach follow the volume best practices and ensure immutability of the source code. But the cost is a more complex deployment process.

Bundle everything into a single container

The idea here is to bundle process manager like supervisord that will start both nginx and php-fpm and supervise them. This way we only have one root process inside the container.

We will end-up having only :

  • dolibarr/dolibarr:19.0.3-php-fpm

While this approach clearly shit on (pardon my french) the unix philosophy I know a lot of folks who run similar deployment in production and never had any trouble despite angering some purists online. (And I too am a purist sometime)

IMO the main drawback of such approach is that you cannot scale separately the nginx or php-fpm process. By uncoupling the two containers one can easily scale the application by scaling each containers separately but having a single container with the two process mean you scale both nginx and php-fpm at the same time.

Would that be a trouble for a Dolibarr deployment? I am personally not sure about this.

What do you think about those suggestions? Do you have other ideas?

@fcharlaix-opendsi
Copy link
Author

Hi,

The proposed volume and sync mechanism is based on the official Nextcloud Docker image.
(The Dolibarr image I'm currently using is based on this)

I don't like my solution, but I don't see any way to improve this.

I'm strongly against multiples services in one container, this break the Docker principle of one service one container.

For the web container, this need to use a common base to share the source code and not storing the source code multiples time in the Docker image storage.
This will add the maintenance of multiples web images and configuration.

-e 's/^#\(ServerSignature Off\)$/\1/g' \
-e 's/^\(ServerTokens\) OS$/\1 Prod/g' \
/etc/apache2/conf-available/security.conf
%EXTRAS%
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you'll probably have to create a second template for the FPM version of your image instead of editing this one ...

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I want to minimize the number of files to update and avoid any inconsistency between variants

@mathieupotier
Copy link
Contributor

Hi there,

I think the FPM docker version should be available for those who wants to have plain control on their web server...
I mean FPM / FastCGI could work with a tomcat (or even an IIS ^_- ) if they want to ...

The dolibarr FPM should only have the source code and the crontab running inside the container (even if it's better to have it separated for concurrency reason), but I endorse the fact to have an image available that just run PHP-FPM with dolibarr app inside to let the people have their own web server dealing with an fpm capable service.

Upvoting for a FPM variant along with the full embed apache version... 👍

@creekorful
Copy link
Member

Hi,

Hello, sorry for the delay, I finally found the time to think more about this.

The proposed volume and sync mechanism is based on the official Nextcloud Docker image. (The Dolibarr image I'm currently using is based on this)

I don't like my solution, but I don't see any way to improve this.

I'm strongly against multiples services in one container, this break the Docker principle of one service one container.

Can you please explain why are strongly against it? Any production feedback where it was a problem?

IMHO I'd rather go with "breaking docker one service principle" (Actually Docker own documentation acknowledge such use-cases https://docs.docker.com/engine/containers/multi-service_container/) rather than using volume to share immutable data between containers.

I know the Nextcloud folks have used a similar approach and this had introduce several issues and I think some of them are more harmful than breaking the one service principle :

People confused about data being deleted after an upgrade

People usually NEVER expect container to remove data especially from a volume. Imagine mounting a volume to persist Dolibarr source code / and or modules. On every upgrade the container will erase people data.

Many end users are not aware of the proper way to use Docker, introducing such a change will definitely make some people having a very bad time.

Introducing scalability issues

Adding a bottleneck, this change will introduce severe scalability issue when it come to upgrade scaled Dolibarr instances. If we are targeting Kubernetes deployments for example this will definitely be a problem. Many complex tricks will be required in order to make this work. But at what cost?


I hope my answers are not coming rude or anything. I really appreciate what we are doing here and being a NGINX/PHP-FPM guy I really hope we could get this integrated very soon. But I have some strong concerns about the approach.

Cheers,

@creekorful creekorful added the question Further information is requested label Oct 2, 2024
@fcharlaix-opendsi
Copy link
Author

Hi,

Hello, sorry for the delay, I finally found the time to think more about this.

Don't worry, In my case this isn't urgent and this shouldn't be rushed.

Can you please explain why are strongly against it? Any production feedback where it was a problem?

When restarting a container I want to restart only one service that's misbehaving, and I expect the container healthcheck to report the state of one service, not a stack.

As an example, if I tweak the web server or PHP config, I want that docker to crash and don't take the rest of the stack with it (it's especially useful with Kubernetes deployment).

IMHO I'd rather go with "breaking docker one service principle" (Actually Docker own documentation acknowledge such use-cases https://docs.docker.com/engine/containers/multi-service_container/) rather than using volume to share immutable data between containers.

I do not agree with the immutability of the Dolibarr source files, some module adds files outside the custom folder, this is very bad practice but real production behavior.

As an example, this is used to bypass Dolibarr limitation, like a module adding his theme folder (example : Oblyon v3)

And patching the source code while waiting for a proper release need to stay even if the container is recreated with the same image version.

People confused about data being deleted after an upgrade

This is a big problem, but with proper documentation and warnings this can be mitigated.

People usually NEVER expect container to remove data especially from a volume. Imagine mounting a volume to persist Dolibarr source code / and or modules. On every upgrade the container will erase people data.

When you update an app, you expect the code to be replaced, and that's why custom and documents are both in separated volumes.

The container will not erase peoples data with proper configuration :

rsync -rlD --chown www-data:www-data --delete --exclude custom --exclude conf/conf.php /usr/src/dolibarr/ /var/www/html/

(I should add --one-file-system to rsync to be safe)

Many end users are not aware of the proper way to use Docker, introducing such a change will definitely make some people having a very bad time.

End user should not use Docker if they don't know how to use it, at least they should stick to premade example with documentation or use other means to host Dolibarr.

If we stick to the Nextcloud example, this is why Nextcloud All-In-One image exist.

Introducing scalability issues

Adding a bottleneck, this change will introduce severe scalability issue when it come to upgrade scaled Dolibarr instances. If we are targeting Kubernetes deployments for example this will definitely be a problem. Many complex tricks will be required in order to make this work. But at what cost?

The main problem reported on those issues is caused by slow storage and readiness probe failing.
This is avoidable by using an init container on Kubernetes with a lock that handle all those long tasks, it's more of a helm chart issue.


I like challenging my view of things, it is (for me) the best way to improve outside his comfort zone.
I really hope we can find a good solution to make the proper Dolibarr image everybody wants.

Regards,

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request question Further information is requested
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants