Open tomkralidis opened 7 months ago
Some considerations on this:
The config is read in the global scope in e.g. flask_app.py
, so every worker processes has its own copy.
If there's only one worker process (with possibly threads), it would be possible to change the config in place by doing something like self.config.update(data)
when receiving update requests. However the docker entrypoint starts gunicorn with 4 processes, so only one of them would effectively receive the update in this case.
To update all workers, a graceful restart is necessary and in gunicorn can be done by using kill -HUP masterpid
. To get the masterpid, we can run gunicorn
with --pid some-file-path.pid
to make it write its pid.
This approach seems very feasible, but relies on the pid file setup. Note that other servers like uWSGI also support reloading via the HUP signal, so it's probably a reasonable to assume support for this. It's however not supported with the dev flask server.
Other possibilities are to read the config on reach request, which would be really slow. We could also cache it for e.g. 1 minute in each worker or even read it from some hot cache like redis (probably overkill).
You can use the gunicon reload function to perform this as described here: https://docs.pygeoapi.io/en/latest/admin-api.html#pygeoapi-hot-reloading-in-gunicorn. The docker example for pygeoapi admin implements this strategy in the entrypoint, as does the entrypoint in wis2box-api.
There is a security concern for users who do not want to update their configuration which is why it is not in the default Docker image of pygeoapi. If it is their wish to not update the configuration, i.e. the Admin API is not enabled, I do not think pygeoapi should hot-reload changes made to the configuration.
I am partial to some solution that exists below the (Flask, Starlette, Django) framework to avoid all these potential deployment variations.
Maybe it could be an option to consider a database-driven configuration (at least for the resources)? I described something similar a while ago in a feature request (https://github.com/geopython/pygeoapi/issues/1351). Restarting/hot-reloading seems like a workaround to me. I do not see an issue in performance when introducing a lightweight database. This would solve the thread issues as well.
Another note, anytime the configuration gets updated, the Open API document must also be recreated and read into memory. Considerations need to be taken for both PYGEOAPI_CONFIG
and PYGEOAPI_OPENAPI
.
This Issue has been inactive for 90 days. As per RFC4, in order to manage maintenance burden, it will be automatically closed in 7 days.
@tomkralidis please confirm this is still ongoing
There is a related PR. I did not test it yet but from looking at the changes it seems to not update the OpenAPI (I might be wrong, @Alex-NRCan).
Thanks for hooking me into this issue @matthesrieke .
I've read the comments in this thread.
What I could add on to the discussion is that my PR:
Extends the API class to easily tweak its behavior and how it loads/reloads its collections. You simply override 2 methods and you're good to go:
on_load_resources_check
in which you implement a check to see if the resources should be hot-reloaded and you return True
when they must be. You can implement the check however you want it in your override based on your environment/context. This check is automatically called on every collections call;on_load_resources
in which you do the actual fetching of the resources (e.g. from an SQL database) and essentially build the resources dictionary which you return in your implementation of the method. The mother class (the API) automatically will take care of updating the actual self.config['resources']
object.Supports multiple instances of pygeoapi workers (supports gunicorn (or others) load-balancing patterns) as each worker performs its own checks-and-reloads with its in-memory implementation. We could use help testing this, but seems to be working fine for us right now.
Is non-breaking at all with the native behavior of pygeoapi. When the methods aren't overridden, pygeoapi simply acts like regular.
Seems to provide an implementation that aligns with issue #1351, indeed.
As for the OpenAPI spec, that needed to be refreshed on every collection that was added/removed on the fly, yes, that was a tricky one. In our case, we ended up creating a 'generic' openapi spec yaml. So, instead of having 1 "paragraph" per collection, we have 1 generic paragraph with the collectionId
in the query path. It looks like this:
*Note: It does have the caveat that we had to consider a user error message handling when one tries to hit a /coverage endpoint on a feature type collection for example. We thought about this and, for our case, we still prefer to have a generic OpenAPI spec, for these reasons:
pygeoapi openapi generate ${PYGEOAPI_CONFIG} --output-file ${PYGEOAPI_OPENAPI}
command when deploying anymore (the line in the docker\entrypoint.sh file is commented out for us).@Alex-NRCan really interesting, and reasonable decisions on the OpenAPI part. I generally support the approach of a generic document (as you said, loading times, complexity, ...), but am wondering if there are use cases where an explicit OpenAPI with all collections is required. @tomkralidis @webb-ben thoughts?
Description Ensure that changes to the configuration are visible without requiring a service restart.
Steps to Reproduce
Expected behavior pygeoapi always provides service based on the latest configuration changes
Screenshots/Tracebacks
Environment
Additional context cc @totycro