On monday, we had a build of emails in the email queue that weren't
getting picked up by the sender worker and causing delays.
After further investigation with Andy from the PaaS, we believe the
following happened.
We received a bunch of traffic at 8:30ish which consisted of some
very large emails in terms of their length and complexity. The amount
of memory used by the app instances got very high and a few apps
crashed due to OOM (recorded by 5 cf app event crashes). When new
app instances tried to spin up, they weren't able to as they
potentially also ran out of memory immediately.
This left us in the position of having fewer app instances than we
needed, on top of which they were all using a very large amount of
CPU and may have been limited how quickly an individual app
instance would process tasks. This meant that we were overall
processing fewer tasks then we needed to and our queue of emails
started to build up.
So it appears our sender workers did not have the memory available that
they needed. By looking at a graph for the past 30 days of memory usage
on the sender workers, we see that it on several days breached 90%
memory usage for long periods of time. This in combination of the
hypothesis above of what happened leads us to decide that we want to
give the app instances a bigger memory quota so it has been upped from
3GB to 4GB.
Whilst doing, I also looked at long term memory usage graphs for our
other workers and saw that the letters worker was similarly close to
around 90% of memory used so have taken the opportunity to bump that
too.
we might need to deal with a potential large volume of SMS delivery
receipts. These receipts are POSTed to oru public api to two URLs. The
actual endpoint just parses the response body and puts a task on an SQS
queue - no database connections are required or anything like that.
Split up this traffic from other traffic, so that any increase in volume
of callbacks won't affect the scaling/load/etc of the main api apps.
We've hard coded instance counts to 10 on prod for now until we get an
idea of load.
We have seen the reporting app run out of memory multiple times when
dealing with overnight tasks. The app runs 11 worker threads and we
reduce this to 2 worker threads to put less pressure on a single
instance.
The number 2 was chosen as most of the tasks processed by the reporting
app only take a few minutes and only one or two usually take more than
an hour. This would mean with 2 processes across our current 2
instances, a long running task should hopefully only wait behind a few
short running tasks before being picked up and therefore we shouldn't
see large increase in overall time taken to run all our overnight
reporting tasks.
On top of reducing the concurrency for the reporting app, we also set
CELERYD_PREFETCH_MULTIPLIER=1. We do this as suggested by the celery
docs because this app deals with long running tasks.
https://docs.celeryproject.org/en/3.1/userguide/optimizing.html#optimizing-prefetch-limit
The chance in prefetch multiplier should again optimise the overall time
it takes to process our tasks by ensuring that tasks are given to
instances that have (or will soon have) spare workers to deal with them,
rather than committing to putting all the tasks on certain workers in
advance.
Note, another suggestion for improving suggested by the docs for
optimising is to start setting `ACKS_LATE` on the long running tasks.
This setting would effectively change us from prefetching 1 task per
worker to prefetching 0 tasks per worker and further optimise how we
distribute our tasks across instances. However, we decided not to try
this setting as we weren't sure whether it would conflict with our
visibility_timeout. We decided not to spend the time investigating but
it may be worth revisiting in the future, as long as tasks are
idempotent.
Overall, this commit takes us from potentially having all 18 of our
reporting tasks get fetched onto a single instance to now having a
process that will ensure tasks are distributed more fairly across
instances based on when they have available workers to process the
tasks.
The reporting worker tasks fetch large amounts of data from the db, do
some processing then store back in the database. As the reporting worker
only processes the create nightly billing/stats table tasks, which
aren't high performance or high volume, we're fine with the performance
hit from restarting the worker between every task (which based on
limited local testing takes about a second or so).
This causes some real funky shit with the app_context (used for
accessing current_app.logger). To access flask's global state we use the
standard way of importing `from flask import current_app`. However,
processes after the first one don't have the current_app available on
shut down (they're fine during the actual task running), and are unable
to call `with current_app.app_context()` to create it. They _are_ able
to call `with app.app_context()` to create it, where `app` is the
initial app that we pass in to `NotifyCelery.init_app`.
NotifyCelery.init_app is only called once, in the master process - I
think the application state is then stored and passed to the celery
workers. But then it looks like the teardown might clear it, but it
never gets set up again for the new workers? Unsure.
To fix this, store a copy of the initial flask app on the NotifyCelery
object and then use that from within the shutdown signal logging
function.
Nothing's ever easy ¯\_(ツ)_/¯
Instead of saving the email notification to the db add it to a queue to save later.
This is an attempt to alleviate pressure on the db from the api requests.
This initial PR is to trial it see if we see improvement in the api performance an a reduction in queue pool errors. If we are happy with this we could remove the hard coding of the service id.
In a nutshell:
- If POST /v2/notification/email is from our high volume service (hard coded for now) then create a notification to send to a queue to persist the notification to the db.
- create a save_api_email task to persist the notification
- return the notification
- New worker app to process the save_api_email tasks.
This should add an extra 400 connections maximum, which will not tip us over the allowable 5000 db connections. And it may help with the queue pool connection errors.
We moved from sending statsd metrics to hosted graphite to sending to
one that is running on the paas. Therefore we no longer need to send
statsd metrics to a particular prefix at the statsd app as it is only
receiving statsd metrics from our apps (not other users like would have
been the case with HostedGraphite).
This should change no behaviour as the only place the environment
variable was being used was in the gunicorn config and it was an empty
string which is the default behaviour anyway as per:
https://docs.gunicorn.org/en/stable/settings.html#statsd-prefix
This was after we saw an instance of the API failing it's healthcheck
even though it was still healthy enough to serve requests to users.
This follows the change we've also made to template-preview and admin of
upping the health check timeout. Unlike those where we set it to be 10
seconds, we have been less allowing here and only chosen 2.5 seconds.
This was at suggestion of Toby from PaaS as the api should generally
have quicker response times and more annoyance might be created for
users if we let an instance stick around for 10 seconds where it was
unable to serve requests successfully.
these URLs never change, and it lead to surprising issues where an
updated default MMG_URL wasn't actually respected on PaaS. These urls
aren't private and don't need to be stored in credentials.
By not defining them in the manifest, we expect them to use the default
unless `cf set-env` has been specifically used to modify them in an app.
we don't use it since we wrote our own provider stubs for performance
tests.
this removes it from the api - it's still in the DB and will be
retrieved by queries, but is set to disabled on prod
deploys take up to five minutes, during which notify-paas-autoscaler
can't scale the app. We saw 502s due to a large volume of traffic
coming in during that time, and we couldn't react cos we were
deploying.
scale up to 25 instances, the autoscaler won't be able to downscale
until after the deploy has finished.
- We are running the statsd-exporter on the PaaS now so we can use the
internal UDP route to talk to it
- Only update in preview and staging still so that we can get the
dashboards fully up to date before switching prod
- We are running a statsd exporter on tools to collect all our statsd
metrics for scraping by Prometheus
- Update preview to point there instead of at the local one which has
issues with redeployment and DNS changing
- We've been seeing an issue when traffic spikes of the http health
checks taking over 1s and PaaS killing the app
- Port health checks won't care about being stuck in a queue so should
continue to work even at high loads
- We have functional tests to catch if a deployment brings up the app
(and so passes port health check) but then doesn't work
- We are running statsd exporter as an app with a public route for
Prometheus to scrape
- This updates preview to send statsd metrics over the CF internal
networking to the statsd exporter
- Removes the sidecar statsd exporters too
all apps get a route assigned when using v3-zdt-push.
> By default, the web process has a route and one instance. Other processes have zero instances by default.
([source](https://docs.cloudfoundry.org/devguide/multiple-processes.html))
When we push apps to multiple environments they need different routes
or the second push will fail, so this means that we need to define
routes ourselves for every app.
We're also manually flagging the health-check as either "http" or
"process" - http for the api, process for all others.
If not specified, healthcheck is set to `port` by cloudfoundry - we've
seen some issues with upgrading the deployment from v2 to v3 when using
port - it adds apps to load balancer when they're not ready, which can
result in 404s. by setting healthcheck to http it'll wait for the
/status endpoint to return 200, which will wait for flask to get
everything up and running properly
all apps get a route assigned when using v3-zdt-push.
> By default, the web process has a route and one instance. Other processes have zero instances by default.
([source](https://docs.cloudfoundry.org/devguide/multiple-processes.html))
When we push apps to multiple environments they need different routes
or the second push will fail, so this means that we need to define
routes ourselves for every app.
We're also manually flagging the health-check as either "http" or
"process" - http for the api, process for all others.
This is so that retry-tasks queue, which can have quite a lot of
load, has its own worker, and other queues are paired with queues
that flow similarly:
- letter-tasks with create-letters-pdf-tasks
- job-tasks with database-tasks
Running `statsd_exporter` alongside the app process allows us to get
StatsD metrics pushed by workers to Prometheus.
This requires adding a route to the worker instances and binding the
RE prometheus discovery service. So this approach won't work for API
and admin since they already have `gunicorn` bound to the `$PORT`.
Since we're not ready to switch all apps to Prometheus metrics at once
and we don't currently have a way to push statsd metrics to multiple
destination we're using a configuration setting in the manifest template
to switch individual workers in specific environments.
`local_statsd` contains a list of environments where the app should
use local `statsd_exporter` for pushing statsd metrics instead of
HostedGraphite.
NOTIFY_APP_NAME follows precedent and just tries to strip 'notify-'
from the beginning of the string.
instances is not specified at all if not defined - it'll scale up to
the same amount of instances as currently present, and then the
autoscaler will take over anyway
newer versions of cf api don't allow you to have multiple apps per
manifest file. So, instead of our current inheritance based model, move
to the newer doc-dl/antivirus/template-preview approved jinja based
model.
the new single manifest.yml.j2 file sets a bunch of variables based on
the CF_APP variable - things like NOTIFY_APP_NAME, default instances,
etc. Then the manifest is built up to define all of the app options
based on these defaults. Things default to sensible values, which can
vary based on environment.
When adding new environment variables, you'll need to add them to the
manifest file. If they're json encoded lists, you'll need to pass them
back to the `tojson` filter, or jinja2 will print them as python lists,
with single quotes around strings.
