notifications-admin has now been changed to always pass the service_id
to the 'service/unique' endpoint. This means we don't need to cover the
case of there being no service_id and the tests can also be updated.
Switched to using a isinstance check on the string.
Added an order by clause to dao_get_template_usage_stats_by_service, it was causing an itermitten failure in the tests.
Sometimes you just wanna run some tests directly using the `pytest`
command. But you’re in a new shell, and have forgotten to do
`source environment_test.sh`. The screen fills with red, and your day
just got a little bit worse.
This commit will stop this from ever happening again, by making the
setting environment variables part of running Pytest. It does this with
a plugin called pytest-env[1].
pytest.ini is the standard way of configuring pytest. Creating this file
where it didn’t exist before changes the behaviour of pytest, in that
it will now look for tests in the same directory as the file, rather
than defaulting to the `tests/` directory. So we also have to explicitly
configure pytest[2] to tell it that it should only look in this
directory. Otherwise it gets lost in the weeds of `node_modules`.
This also changes the way that `SQLALCHEMY_DATABASE_URI` is overriden to
the convention used by this plugin.
1. https://github.com/MobileDynasty/pytest-env
2. https://docs.pytest.org/en/latest/customize.html#confval-testpaths
fixup! Remove environment_test.sh
If a PR is going to fail because tests aren’t passing then you:
- should know about it as quick as possible
- shouldn’t waste precious Jenkins CPU running subsequent tests
This commit adds the `-x` flag to pytest, which stops the test run as
soon as one failing test is discovered.
Changed the '/service/unique' endpoint to optionally accept the
service_id parameter. It now doesn't matter if a user tries to change
the capitalization or add punctuation to their own service name. But
there should still be an error if a user tries to change the punctuation
or capitalization of another service.
service_id needs to be allowed to be None until notifications-admin is
updated to always pass in the service_id.
Service callbacks are I/O bound and can take a long time if the
callback URL takes a long time to respond. This is a perfect use
case for an eventlet worker pool since it allows spawning multiple
green threads (1000 in the proposed configuration) to execute HTTP
requests concurrently without a significant increase in CPU load
or memory usage needed for adding additional worker processes.
Flask-SQLAlchemy sets up a connection pool with 5 connections and will
create up to 10 additional connections if all the pool ones are in use.
If all connections in the pool and all overflow connections are in
use, SQLAlchemy will block new DB sessions until a connection becomes
available. If a session can't acquire a connections for a specified
time (we set it to 30s) then a TimeoutError is raised.
By default db.session is deleted with the related context object
(so when the request is finished or app context is discarded).
This effectively limits the number of concurrent requests/tasks with
multithreaded gunicorn/celery workers to the maximum DB connection pool
size. Most of the time these limits are fine since the API requests are
relatively quick and are mainly interacting with the database anyway.
Service callbacks however have to make an HTTP request to a third party.
If these requests start taking a long time and the number of threads is
larger than the number of DB connections then remaining threads will
start blocking and potentially failing if it takes more than 30s to
acquire a connection. For example if a 100 threads start running tasks
that take 20s each with a max DB connection pool size of 10 then first 10
threads will acquire a connection right away, next 10 tasks will block for
20 seconds before the initial connections are released and all other tasks
will raise a TimeoutError after 30 seconds.
To avoid this, we perform all database operations at the beginning of
the task and then explicitly close the DB session before sending the
HTTP request to the service callback URL. Closing the session ends
the transaction and frees up the connection, making it available for
other tasks. Making calls to the DB after calling `close` will acquire
a new connection. This means that tasks are still limited to running
at most 15 queries at the same time, but can have a lot more concurrent
HTTP requests in progress.
Celery tasks require an active app context in order to use app logger,
database or app configuration values. Since there's no builtin support
we create this context by using a custom celery task class NotifyTask.
However, NotifyTask was using `current_app`, which itself is only
available within an app context so the code was pushing an initial
context in run_celery.py.
This works with the default prefork pool implementation, but raises
a "working outside of application context" error with an eventlet
pool since that initial application context is local to a thread
and eventlet celery worker pool will create multiple green threads.
To avoid this, we bind NotifyTask to the app variable with a closure
and use that variable instead of `current_app` to create the context
for executing the task. This avoids any issues caused by shared
initial app context being lost when spawning additional worker threads.
We still need to keep the context push in run_celery.py for prefork
workers since it's required for logging events outside of tasks
(eg logging during `worker_process_shutdown` signal processing).
as it failed on prodction and I managed to get it to fail locally
{'file_location': '2018-02-12/NOTIFY.FOO.D.2.C.N.20180212164427.PDF'}
!= {'file_location': '2018-02-12/NOTIFY.FOO.D.2.C.N.20180212164428.PDF'}
A freeze time till ensure that the finame which is based on the time
will not fail.
