Add a combined php-fpm-nginx image #102

deviantintegral · 2023-04-14T20:48:51Z

This PR adds support to build a single image that contains php-fpm and nginx.

The main advantage as compared to apache + mod_php is that this mirrors what Pantheon does for hosting. As well, while ddev supports apache as the web server, it defaults to nginx with php-fpm.

When I build this locally, I get the following image tags:

tugboatqa/php-fpm-nginx                                                 8.2.5-fpm-buster                  da523af7b96b   3 minutes ago       564MB
tugboatqa/php-fpm-nginx                                                 8.1.18-fpm-buster                 d07c9669284a   3 minutes ago       563MB
tugboatqa/php-fpm-nginx                                                 8.1.18-fpm-bullseye               9cf094317a20   3 minutes ago       614MB
tugboatqa/php-fpm-nginx                                                 8.0.28-fpm-bullseye               dcec48c190a6   3 minutes ago       610MB
tugboatqa/php-fpm-nginx                                                 8.0.28-fpm-buster                 36f58df88aaf   3 minutes ago       559MB
tugboatqa/php-fpm-nginx                                                 8.2.5-fpm-bullseye                b003e269c2c8   3 minutes ago       615MB

To test, I ran docker run --rm --name nginx -p 80:80 tugboatqa/php-fpm-nginx:8.2.5-fpm-bullseye, and then browsed to http://localhost:

It's not 1-1 with tugboat given I'm building arm images, but it should be pretty close.

I'm unsure how to test this on a full tugboat project. I'd like to see this work in a full Drupal site, certainly before we expose it in the Tugboat docs. Does it need to be merged first? I guess I'd need to force an amd64 build and push that if I tried to push an image before the PR is merged.

q0rban

Great stuff! This is quite an undertaking. Many folks will be using this, I foresee! Just some questions in here.

q0rban · 2023-04-14T21:29:13Z

Makefile

@@ -1,15 +1,17 @@
-ALL = $(shell ls services | grep -v -e elasticsearch- -e php- -e percona) elasticsearch php
+ALL = $(shell ls services | grep -v -e elasticsearch- -e php- -e percona) elasticsearch php php-fpm-nginx


If we renamed the directory to nginx-php-fpm I think we could avoid any modifications to the makefile. Thoughts?

I'm guessing you preferred php at the beginning because the tags will follow php? Not sure how important that is.

Yeah, that's it exactly. But I'm open either way.

The one slightly odd thing about the tags is that they also have fpm in them, so to get 8.2.x of php, you need to use tugboatqa/php-fpm-nginx:8.2-fpm, instead of tugboatqa/php-fpm-nginx:8.2. I suppose we could modify the getTags function to also do the filtering, and then strip the fpm from the tag.

Thinking about this some more, what if we did something like how the apache tags work and add a -nginx suffix: php:8.2-fpm-nginx

I like that, because it also follows the variant pattern other images use, such as node:18-alpine.

Thinking about this some more, what if we did something like how the apache tags work and add a -nginx suffix: php:8.2-fpm-nginx

I investigated this, and it would take a refactor of our scripts to accomplish. So I went instead with the approach in #112

services/php-fpm-nginx/Dockerfile

services/php-fpm-nginx/files/sites-enabled/nginx-site.conf

q0rban · 2023-04-14T21:43:50Z

services/php-fpm-nginx/manifest

@@ -0,0 +1,11 @@
+# Override getTags() to always point to the php image, not an invalid
+# php-fpm-nginx image.


I'm not following this. I'm guessing that NAME can't be specified in this file itself or that creates other issues? But I don't know what those issues are by this comment.

@deviantintegral can you weigh in on this and potentially update your comment here with the reasoning for the approach in this manifest?

If I remember right, the issue was that it would try to build from a base image called php-fpm-nginx. The existing scripts assume that the directory name in services/<service> match the upstream image name. There is no docker hub image called php-fpm-nginx so this was failing out.

Am I right this is needed? If there's a better way that's fine by me.

services/php-fpm-nginx/files/sites-enabled/nginx-site.conf

Co-authored-by: Sally Young <[email protected]>

q0rban

This isn't working as I expect. When I cd $DOCROOT and then modify the files there, I do not see those changes when curling http://127.0.0.1. Can we set up a time to pair program on this @deviantintegral ?

q0rban · 2023-07-03T17:08:39Z

services/php-fpm-nginx/Dockerfile

+    mv /root/tugboat.ini /usr/local/etc/php/conf.d/tugboat.ini && \
+    mv ${DOCROOT} ${DOCROOT}.original && \
+    ln -s ${DOCROOT}.original ${DOCROOT} && \
+    mkdir -p /var/lib/tugboat/web && \


This will cause conflicts when the repository is checked out to /var/lib/tugboat. What are you trying to achieve?

This is probably a drupalism sneaking into this work. I think what this means is that we'd need to make the docroot dynamic somehow in the nginx config?

deviantintegral · 2023-07-04T15:18:38Z

@q0rban re multiple RUN statements, it only matters if future statements modify or delete files from previous statements. If each RUN statement is additive to the previous layers, then there's actually effeciency gains to be had because:

More effective caching of layers. This speeds up builds and local testing in particular.
pulls can get multiple layers in parallel.

https://stackoverflow.com/questions/39223249/multiple-run-vs-single-chained-run-in-dockerfile-which-is-better

q0rban · 2023-07-04T15:26:20Z

@q0rban re multiple RUN statements, it only matters if future statements modify or delete files from previous statements. If each RUN statement is additive to the previous layers, then there's actually effeciency gains to be had because:

More effective caching of layers. This speeds up builds and local testing in particular.

pulls can get multiple layers in parallel.

https://stackoverflow.com/questions/39223249/multiple-run-vs-single-chained-run-in-dockerfile-which-is-better

IIRC, I believe the reason we stick to a single run is to intentionally reduce the number of layers, as having more layers limits how many layers Tugboat can then create, since Docker (I think?) imposes a hard limit.

deviantintegral · 2023-07-04T16:30:40Z

Ah! Makes sense. It sounds like that limit may now be a tunable: docker/docs#8230

I wonder if we should eventually either squash all tugboat images as a final step (since we can't control how many layers upstream images use), or see about increasing that tunable.

I also found this experimental option via https://stackoverflow.com/questions/22713551/how-to-flatten-a-docker-image: https://docs.docker.com/engine/reference/commandline/build/#squash

While squashing layers may produce smaller images, it may have a negative impact on performance, as a single layer takes longer to extract, and downloading a single layer cannot be parallelized.

q0rban · 2023-07-04T17:55:55Z

Ooooo, neat! Good finds @deviantintegral

apotek · 2023-07-12T20:24:37Z

Thank you @deviantintegral @justafish @q0rban for this work that I have been wanting to help with but never found much time to get at.
But I figured testing and maybe contributing some docs counts too?
Just reporting back that changing my drupal service to use nginx rather than apache:

 services:
   drupal:
-    image: chromatichq/php-apache-node:8.1
+    image: tugboatqa/php-nginx:8.1

yields a hung build when built as its own base preview:

2023-07-12T20:17:23.301Z - Building preview: 64af0a5365d7012a0bd345f4 (pr627)

25 minutes pass before

2023-07-12T20:43:17.433Z - Image Not Found (Tugboat Error 1025): tugboatqa/php-nginx:8.1

apotek · 2023-07-13T16:04:02Z

2023-07-12T20:43:17.433Z - Image Not Found (Tugboat Error 1025): tugboatqa/php-nginx:8.1

@q0rban I am looking at docker hub now and it looks like there are no tags available for specifying the php version. There are tags for (obviously) nginx versions as well as stable and latest, and also perl versions! But nothing for php.

I'm going to re-test without any tags at all (thereby using latest), but I can also maybe create a merge request where I create some php-version tagged images and contribute those. Would you like that or is that something that is better handled in-house?

apotek · 2023-07-13T16:07:16Z

Update: Even with simply using

services:
  drupal:
    image: tugboatqa/php-nginx

with no tags at all, I get the same result: Hung builds unable to find the image.

Should I best create a separate issue rather than tack all this feedback into a completed merge request?

q0rban · 2023-07-16T13:44:32Z

Hi @apotek ! Apologies for the confusion here, as we don't have the tags documented on docs.tugboatqa.com yet while we are still testing this. But the tags all follow the official PHP image with fpm, i.e. 8.1-fpm if you want php 8.1.

You can view all the tags here for now: https://hub.docker.com/r/tugboatqa/php-nginx/tags.
They are also published here: https://github.com/TugboatQA/dockerfiles/blob/main/php-nginx/TAGS.md

Once we have this tested, we will add more details to docs.tugboatqa.com with how to use the image.

Add a combined php-fpm-nginx image

527eeb8

deviantintegral requested review from q0rban and rabbitlair April 14, 2023 20:49

deviantintegral added 2 commits April 14, 2023 17:21

Add php-fpm-nginx in the makefile

20cb37d

Fix wrong phony syntax

17f8e54

q0rban reviewed Apr 14, 2023

View reviewed changes

This was referenced Apr 21, 2023

Provide a Tugboat template for Pantheon review environments Lullabot/drainpipe#96

Closed

Build select images using arm64 archicecture #104

Closed

baluv3 mentioned this pull request May 3, 2023

Provide a Tugboat template for Acquia hosted sites Lullabot/drainpipe#204

Open

justafish reviewed Jun 23, 2023

View reviewed changes

services/php-fpm-nginx/files/sites-enabled/nginx-site.conf Outdated Show resolved Hide resolved

deviantintegral and others added 7 commits June 23, 2023 08:48

Link to upstream webserver configs

b6e67d2

Co-authored-by: Sally Young <[email protected]>

Compress run statements into one

e703f5e

Fix run statement

bc62f93

fix spacing

da574be

fix spacing

29d1245

Make sure directory exists

0fa71c4

Compress run statements into one (#111)

aa0a1ad

q0rban requested changes Jul 3, 2023

View reviewed changes

q0rban added 2 commits July 4, 2023 12:48

rename to php-nginx; simplify copy steps; rename service to nginx

c45e0a9

update makefile to build php-nginx when building php

1be1edd

q0rban added 2 commits July 6, 2023 09:07

renaming / simplifying php-nginx config (#112)

fc8d12d

feat: allow root for WordPress CLI #113

2643e94

q0rban approved these changes Jul 7, 2023

View reviewed changes

q0rban merged commit 77a7265 into main Jul 7, 2023

q0rban deleted the php-fpm-nginx branch July 7, 2023 11:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a combined php-fpm-nginx image #102

Add a combined php-fpm-nginx image #102

deviantintegral commented Apr 14, 2023

q0rban left a comment

q0rban Apr 14, 2023

q0rban Apr 14, 2023

deviantintegral Jul 4, 2023

q0rban Jul 4, 2023

q0rban Jul 4, 2023

deviantintegral Jul 4, 2023

q0rban Jul 4, 2023

q0rban Apr 14, 2023

q0rban Jul 3, 2023

deviantintegral Jul 4, 2023

q0rban left a comment

q0rban Jul 3, 2023

deviantintegral Jul 4, 2023

deviantintegral commented Jul 4, 2023

q0rban commented Jul 4, 2023 •

edited

Loading

deviantintegral commented Jul 4, 2023

q0rban commented Jul 4, 2023

apotek commented Jul 12, 2023 •

edited

Loading

apotek commented Jul 13, 2023

apotek commented Jul 13, 2023 •

edited

Loading

q0rban commented Jul 16, 2023 •

edited

Loading

		@@ -1,15 +1,17 @@
		ALL = $(shell ls services \| grep -v -e elasticsearch- -e php- -e percona) elasticsearch php
		ALL = $(shell ls services \| grep -v -e elasticsearch- -e php- -e percona) elasticsearch php php-fpm-nginx

		@@ -0,0 +1,11 @@
		# Override getTags() to always point to the php image, not an invalid
		# php-fpm-nginx image.

Add a combined php-fpm-nginx image #102

Add a combined php-fpm-nginx image #102

Conversation

deviantintegral commented Apr 14, 2023

q0rban left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

q0rban left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deviantintegral commented Jul 4, 2023

q0rban commented Jul 4, 2023 • edited Loading

deviantintegral commented Jul 4, 2023

q0rban commented Jul 4, 2023

apotek commented Jul 12, 2023 • edited Loading

apotek commented Jul 13, 2023

apotek commented Jul 13, 2023 • edited Loading

q0rban commented Jul 16, 2023 • edited Loading

q0rban commented Jul 4, 2023 •

edited

Loading

apotek commented Jul 12, 2023 •

edited

Loading

apotek commented Jul 13, 2023 •

edited

Loading

q0rban commented Jul 16, 2023 •

edited

Loading