Change 904168 had a related patch set uploaded (by Hnowlan; author: Hnowlan):

[operations/deployment-charts@master] Thumbor: use emptyDir for /tmp

https://gerrit.wikimedia.org/r/904168

Change 904168 merged by jenkins-bot:

[operations/deployment-charts@master] Thumbor: use emptyDir for /tmp

https://gerrit.wikimedia.org/r/904168

I 've updated a bit the Thumbor dashboard. Aside from some performance changes (e.g. collapsing most rows by default) the main diff is adding 2 variables to the dashboard. Engine and percentile. I 've updated the Engines row panels to use them. And added 2 cluster comparison panels.

While trying to figure out diffs, that's where it hit me. Those are summaries and are in general not aggregatable across instances. That is, we can compare quantiles of one instance against quantiles of another instance but running max, sum, avg etc on those rarely, if ever, gives correct results (A good read is https://latencytipoftheday.blogspot.com/2014/06/latencytipoftheday-you-cant-average.html). So, we can't really rely on those percentile comparisons. We either need to turn them into histograms (relatively easy, it's a few lines of change in prometheus-statsd-exporter) and do more tests, or try to look at the data without looking into quantiles.

Change 904452 had a related patch set uploaded (by Alexandros Kosiaris; author: Alexandros Kosiaris):

[operations/deployment-charts@master] thumbor: Switch all summaries to histograms

https://gerrit.wikimedia.org/r/904452

Change 904456 had a related patch set uploaded (by Alexandros Kosiaris; author: Alexandros Kosiaris):

[operations/puppet@production] thumbor: Switch all summaries to histograms

https://gerrit.wikimedia.org/r/904456

I 've upload a couple of changes to switch summaries to histograms in both environments. That way we will be able to have aggregatable data across instances (summaries aren't aggregatable in most cases). We 'll need to give it a few days after merge to have a few days of useful data and we 'll need to adapt our dashboards.

Change 904456 merged by Alexandros Kosiaris:

[operations/puppet@production] thumbor: Switch all summaries to histograms

https://gerrit.wikimedia.org/r/904456

Change 904452 merged by jenkins-bot:

[operations/deployment-charts@master] thumbor: Switch all summaries to histograms

https://gerrit.wikimedia.org/r/904452

Change 905654 had a related patch set uploaded (by Hnowlan; author: Hnowlan):

[operations/deployment-charts@master] thumbor: increase memory quota, per-container memory

https://gerrit.wikimedia.org/r/905654

Change 905654 merged by jenkins-bot:

[operations/deployment-charts@master] thumbor: increase memory quota, per-container memory

https://gerrit.wikimedia.org/r/905654

Per my comment at T344233#9209303, it may help performance if you allow ImageMagick to use more than 256MB of memory, by increasing the limit in policy.xml. Prior to the migration to k8s, there was no memory limit in policy.xml.

Change 962061 had a related patch set uploaded (by Hnowlan; author: Hnowlan):

[operations/deployment-charts@master] thumbor: add imagemagick policy file

https://gerrit.wikimedia.org/r/962061

Some numbers to help us choose limits.

JPEG width statistics. Number of JPEG images in a sample with width exceeding the bucket size.

MariaDB [commonswiki]> select pow(2,floor(log2(img_width))) as width_bucket,count(*) from image where img_major_mime='image' and img_minor_mime='jpeg' and img_sha1 like '00%' group by width_bucket;
+--------------+----------+
| width_bucket | count(*) |
+--------------+----------+
|            8 |        1 |
|           32 |        7 |
|           64 |       55 |
|          128 |      418 |
|          256 |     2381 |
|          512 |     9344 |
|         1024 |    14725 |
|         2048 |    29644 |
|         4096 |    19160 |
|         8192 |      618 |
|        16384 |       34 |
|        32768 |        2 |
+--------------+----------+

JPEG area statistics:

MariaDB [commonswiki]> select pow(2,floor(log2(img_width*img_height))) as area_bucket,count(*) from image where img_major_mime='image' and img_minor_mime='jpeg' and img_sha1 like '00%' group by area_bucket;
+-------------+----------+
| area_bucket | count(*) |
+-------------+----------+
|         256 |        1 |
|         512 |        2 |
|        1024 |        1 |
|        2048 |        4 |
|        4096 |       14 |
|        8192 |       40 |
|       16384 |      109 |
|       32768 |      287 |
|       65536 |      610 |
|      131072 |     1606 |
|      262144 |     6427 |
|      524288 |     4688 |
|     1048576 |     5777 |
|     2097152 |     8486 |
|     4194304 |    13271 |
|     8388608 |    22961 |
|    16777216 |    10551 |
|    33554432 |     1330 |
|    67108864 |      183 |
|   134217728 |       32 |
|   268435456 |        8 |
|   536870912 |        1 |
+-------------+----------+

The total image count in this sample is 76389.

Change 962061 merged by jenkins-bot:

[operations/deployment-charts@master] thumbor: add imagemagick policy file

https://gerrit.wikimedia.org/r/962061

Change #1308156 had a related patch set uploaded (by Ladsgroup; author: Ladsgroup):

[operations/software/thumbor-plugins@master] swift: Load the file async

https://gerrit.wikimedia.org/r/1308156

A pretty major performance and architecture problem of thumbor at the current state is the fact that it's a broken mix of sync and async operations. The upstream thumbor is designed with the model of one worker thread + delegating as much as I/O-bound operation (or file being large or network) to async coroutines. Our current thumbor barely takes advantage of that and as such we are serving haproxy to act as load balancer on top of the worker that supposed to be the load balancer (see T357145: Consider moving to haproxy ingress for Thumbor workers) because the current worker gets locked on stuff that it really shouldn't. The patch above is an extremely low hanging fruit that moves at least loading of the files into the exec pool using the existing infra. It is already being used in some areas, for example for videos (see the use of tornado.Subprocess there) but it could be done a lot more from what I'm seeing.

I could be wrong. I'm new to the codebase (and haven't done async stuff for almost a decade now) so my apologies if I'm missing something super obvious.

Change #1308156 merged by jenkins-bot:

[operations/software/thumbor-plugins@master] swift: Load the file async

https://gerrit.wikimedia.org/r/1308156

Change #1309255 had a related patch set uploaded (by Ladsgroup; author: Ladsgroup):

[operations/deployment-charts@master] thumbor: Load files from swift async

https://gerrit.wikimedia.org/r/1309255

Change #1309255 merged by jenkins-bot:

[operations/deployment-charts@master] thumbor: Load files from swift async

https://gerrit.wikimedia.org/r/1309255

Cormac suggested we close this ticket since it has been a while and a lot has changed since then. And create a new ticket for issues we are finding. I agree.

In T333445#12107169, @Ladsgroup wrote:

Cormac suggested we close this ticket since it has been a while and a lot has changed since then. And create a new ticket for issues we are finding. I agree.

+1. I think much of the main body of the ticket has been addressed in various ways. For posterity, I think the filesystem-level concerns about running in k8s have been safely dismissed.