Merge ~superalpaca/ols-charm-deps:SN1460-add-prometheus-multiproc-dir-config into ols-charm-deps:master

[Deep Fowdar]

Add prometheus multi-process support.

The PROMETHEUS_MULTIPROC_DIR env var can be specified via the config
option 'wsgi_prometheus_multiproc_dir'.

Part of: SN-1460

Follows on from the approach suggested by Przemek in https://code.launchpad.net/~superalpaca/snapstore-common/+git/snapstore-common/+merge/439018.

Ticket: https://warthogs.atlassian.net/browse/SN-1460

I've tested this change with the snapmodels charm, and it seems to work fine.

snapmodels MP: https://code.launchpad.net/~superalpaca/snapmodels/+git/snapmodels/+merge/439131

[Przemysław Suliga (suligap)]

Nice, thanks!

Just skimmed it but it looks good.

Thoughts on a boolean wsgi_prometheus_multiproc setting instead (off by default)?:

- on_starting() run in the arbiter could then simply always [re]create the dir if the setting is on (and add its PID in the name).

- The users of the layer probably don't care about the actual dir.

- It'd basically be an option that "handles proper prometheus client setup for multiproc".

Exposing wsgi_prometheus_multiproc_dir would need to be explained more, eg. "It will get wiped on restarts".

[Guillermo Gonzalez (verterok)]

how does this play with existing talisker based services that already are fiddling with on_starting and child_exit?

review: Needs Information

[Maximiliano Bertacchini (maxiberta)]

It looks good! As an alternative, what about skipping any extra config option and just add the multiproc logic if `worker_class == "uvicorn.workers.UvicornWorker"` with a temp dir? That's what talisker does automatically if running with multiple workers and no explicit env var has been set (which is how our services run in staging/prod afaics).

[Przemysław Suliga (suligap)]

> how does this play with existing talisker based services that already
> are fiddling with on_starting and child_exit?

This behavior is opt in and it's very unlikely you'd want to use it
with talisker is the simple answer I think. But that's a good question.
I'd expect talisker to do the right things: honor the env var and also
run the provided hooks. Don't know though.

> It looks good! As an alternative, what about skipping any extra config
> option and just add the multiproc logic if `worker_class ==
> "uvicorn.workers.UvicornWorker"` with a temp dir? That's what talisker
> does automatically if running with multiple workers and no explicit env
> var has been set (which is how our services run in staging/prod
> afaics).

I think these are separate concerns. Eg. you might want to use a single
uvicorn worker and not have the charm meddle with how prometheus is set
up?

Hmm, if anything I think the condition could be based on if you're using
talisker or not. But again, it's a bit magical then?

[Maximiliano Bertacchini (maxiberta)]

> > It looks good! As an alternative, what about skipping any extra config
> > option and just add the multiproc logic if `worker_class ==
> > "uvicorn.workers.UvicornWorker"` with a temp dir? That's what talisker
> > does automatically if running with multiple workers and no explicit env
> > var has been set (which is how our services run in staging/prod
> > afaics).
>
> I think these are separate concerns. Eg. you might want to use a single
> uvicorn worker and not have the charm meddle with how prometheus is set
> up?

You're right! But that was just an example. My question was really "Can we skip the config option and be smarter about it by handling this automatically? Not really sure how :/

> Hmm, if anything I think the condition could be based on if you're using
> talisker or not. But again, it's a bit magical then?

It's not too magical to me; it's preventing metrics corruption :). Otherwise, as I asked in the other MP, what should we do if running in async mode but the env var has not been set? Anyway, if there's no simple way to deduce it, let's just add the flag explicitly. I aggre that it should be a boolean.

[Deep Fowdar (superalpaca)]

Thanks for the reviews everyone!

I think it makes sense to update the config option to be a boolean (false/off by default), so I've gone ahead and done that. If this option is set, then gunicorn will override the PROMETHEUS_MULTIPROC_DIR env vars, provided we have more than 1 worker.

I'm not too sure how this will interact with talisker, but it's probably best to not set this when talisker is being used. Maybe adding a check for talisker (like https://pastebin.canonical.com/p/yVRpQ5k9jt/) could work?

[Guillermo Gonzalez (verterok)]

Maybe could add to the config decription this is only needed for non-wsgi/talisker services? (as those will do the right thing)

[Deep Fowdar (superalpaca)]

> Maybe could add to the config decription this is only needed for non-
> wsgi/talisker services? (as those will do the right thing)

Makes sense, will add this

[Przemysław Suliga (suligap)]

This looks great, but check some small comments.

[Przemysław Suliga (suligap)]

Found one more issue and proposed a solution.

[Deep Fowdar (superalpaca)]

Thanks for checking this again. I've replied to the diff comments and will apply the suggested changes/fixes

[Przemysław Suliga (suligap)]

Thank you, +1.

review: Approve

[Guillermo Gonzalez (verterok)]

Hi,

I think it would be nice if we can re-use the existing mechanisms in ols-layers to set environment variables and handle gunicorn restarts

Here is a quick example: https://pastebin.canonical.com/p/G5T8ZjVnbv/

Some context on the chat with suligap about this: https://chat.canonical.com/canonical/pl/3zk7zkzyqj8kdrmswbyq61n43e

Let me know what you think about the proposed changes.

Also, I'm not sure this is covering all the edge cases (after looking at talisker), but let's get this first and worry about the edge cases later ;-)

[Deep Fowdar (superalpaca)]

I've had a look at the new changes and I think they're better than the current ones. I had no idea about the systemd private tmp dirs, so TIL. I tried out the patch you linked but got the following error on charm startup:
"""
File "/srv/snapmodels/code/env/lib/python3.10/site-packages/gunicorn/arbiter.py", line 534, in reap_workers
    self.cfg.child_exit(self, worker)
  File "/srv/gunicorn/snapmodels.py", line 31, in child_exit
    multiprocess.mark_process_dead(worker.pid)
AttributeError: partially initialized module 'prometheus_client.multiprocess' has no attribute 'mark_process_dead' (most likely due to a circular import)
"""
I moved the import out of the function into the top-level and that seems to have fixed it.

I'll push through the changes soon enough though.

[Guillermo Gonzalez (verterok)]

review: Approve

diff --git a/layer/ols-wsgi/config.yaml b/layer/ols-wsgi/config.yaml
index df316d9..daad490 100644
--- a/layer/ols-wsgi/config.yaml
+++ b/layer/ols-wsgi/config.yaml
@@ -49,3 +49,13 @@ options:
     type: int
     default: 0
     description: "The max. number of seconds to wait before actually restarting/reloading"
+  wsgi_use_prometheus_multiproc:
+    type: boolean
+    default: false
+    description: >
+      Handles proper prometheus client setup for multiproc with gunicorn.
+      NOTE: This requires the prometheus client from https://github.com/prometheus/client_python.
+      Toggling this option results in a gunicorn restart.
+      This will also set the PROMETHEUS_MULTIPROC_DIR (and lowercase version) env vars if set.
+      This should not be set to true if using talisker-based services, as those should
+      handle prometheus multiproc metrics properly.
\ No newline at end of file
diff --git a/layer/ols-wsgi/lib/ols/gunicorn.py b/layer/ols-wsgi/lib/ols/gunicorn.py
index b5520db..60a3d60 100755
--- a/layer/ols-wsgi/lib/ols/gunicorn.py
+++ b/layer/ols-wsgi/lib/ols/gunicorn.py
@@ -107,8 +107,15 @@ def configure(working_dir, port, user, group, env_extra, access_logfile='-'):
         'wsgi_max_requests',
         'wsgi_logrotate_count',
         'wsgi_reuse_port',
+        'wsgi_use_prometheus_multiproc',
     ):
         wsgi_config[conf_name.replace('wsgi_', '')] = charm_config[conf_name]
+    if charm_config['wsgi_use_prometheus_multiproc']:
+        # as systemd provides private temp directories there is no need to make
+        # this unique
+        multiproc_tmp = '/tmp/prometheus_multiproc'
+        env_extra['prometheus_multiproc_dir'] = multiproc_tmp
+        env_extra['PROMETHEUS_MULTIPROC_DIR'] = multiproc_tmp
 
     wsgi_config['working_dir'] = working_dir
     service_name = sanitized_service_name()
diff --git a/layer/ols-wsgi/templates/config.tmpl b/layer/ols-wsgi/templates/config.tmpl
index 2ebc7f4..32450b6 100644
--- a/layer/ols-wsgi/templates/config.tmpl
+++ b/layer/ols-wsgi/templates/config.tmpl
@@ -20,3 +20,21 @@ reuse_port = True
 {% if access_logfile -%}
 accesslog = "{{ access_logfile }}"
 {% endif -%}
+
+{% if use_prometheus_multiproc -%}
+
+from prometheus_client import multiprocess
+
+def setup_prometheus_multiproc():
+    import os
+    os.makedirs(os.environ['prometheus_multiproc_dir'], mode=0o700, exist_ok=True)
+
+
+def on_starting(server):
+    setup_prometheus_multiproc()
+
+
+def child_exit(server, worker):
+    multiprocess.mark_process_dead(worker.pid)
+
+{% endif -%}