Use inspect to get tasks from celery workers

[ryanhiebert]

Fixes #8

In-progress celery tasks haven't been considered. This adds getting those active jobs.

Unfortunately, the methods to run to get those jobs are quite expensive. Fortunately, they can be run once per request and get all the needed data. Add a request-wide cache dictionary for use in optimizations like this. In this case, having a cache allows us to keep the Celery complexity in the celery-specific proc, and keep the request-wide code clean, agnostic, and reasonably simple.

[ryanhiebert]

There were a few more issues I found, and fixed, in my testing. I've now deployed this to my production environment, and I'll be watching for issues.

---

There are a couple questions worth asking. I've tried to err on the side of magic-free and completely non-breaking, but that may not necessarily be the right choice.

1. Should these be turned on by default? They can take a long time to run, so I've left them off in my initial implementation, but there's a reasonable argument to be made that the current implementation, while fast, is just plain broken. That was definitely true for me.
2. Should it be a simple boolean switch, rather than allowing the customizations of which types of tasks to use inspect to look for? If you're sure that there will never be any scheduled tasks, for instance, then you may be able to optimize it slightly by not using that. But is that possible benefit worth the configuration complexity?
3. Should there be special handling for scheduled tasks? Since they aren't actually running, there's no real reason to have extra dynos running, apart from the one that is holding it. One possibility is that we could just say that there is 1 task if there are any in the queue, and 0 only if there are none scheduled in the queue.

/cc @touilleMan, @joshowen, @syphar

[syphar]

@ryanhiebert

Should these be turned on by default?

not sure... but I tend to turn it on by default.

let's assume

• we have a minimum dyno count of 1 and
• we have tasks surviving the downscale-event (so celery requeues them on SIGTERM) then the sane default (and the best one for reducing costs) would be not to inspect. I think we can ignore prefetched tasks here (but they also could have an effect on the worker count you want to scale to)

If you have a kind of unclean celery setup, tasks not surviving the worker-downscaling (because they run longer than some seconds) then you definitely need the inspect calls (combined with the non-decrementable setting). And also if you have to include scheduled / prefetched tasks this can be the best setting ...

This definitely needs some documentation :)

Should it be a simple boolean switch, rather than allowing the customizations of which types of tasks to use inspect to look for?

both? Most users would IMHO inspect everything. Some would try to optimize and reduce the calls. Another tought: how many actual network calls does celery do here? One per type? Or only one for the global inspect() and then nothing else?

We don't need to configure the default if they don't have an effect on performance for example.

Should there be special handling for scheduled tasks?

cool idea :) I'm +1 on this. When scaling down, this could lead to re-distribution of the scheduled tasks on the workers, but I think this is fine. Is there a limit on how many scheduled tasks celery can hold in-memory on the workers? So is 1 the right number?

[mjtamlyn]

| definitely have long running tasks which don't currently survive the downscaling process, and get frequent sentry errors. I've designed most long running tasks to have eventual consistency as I couldn't easily see how to fix this. It seems this patch might do so. I'd love to see some clear documentation (with this patch?) which explains how to make sure that doesn't happen - what parameters @task should have, config settings etc.

Not sure if it's relevant but I also have three worker types, which can share tasks down a priority scale (so the high priority queue stays quiet but the lower priority workers can pick up high priority tasks if they have nothing of low priority to do).

I don't currently use any scheduled tasks cos I didn't trust them, mainly for this reason.

[ryanhiebert]

• we have a minimum dyno count of 1 and
• we have tasks surviving the downscale-event (so celery requeues them on SIGTERM) then the sane default (and the best one for reducing costs) would be not to inspect. I think we can ignore prefetched tasks here (but they also could have an effect on the worker count you want to scale to)

Unfortunately, with the current stable version of Celery, tasks do not survive a Heroku shutdown. Even if they are acks_late, because of how Celery internally uses SIGTERM, the same signal that Heroku uses to warn processes that it's shutting down. There's a fix in master, but I don't know when it'll get to a stable version.

If you have a kind of unclean celery setup, tasks not surviving the worker-downscaling (because they run longer than some seconds) then you definitely need the inspect calls (combined with the non-decrementable setting). And also if you have to include scheduled / prefetched tasks this can be the best setting ...

Ignoring the prefetched tasks is a recipe for flapping. I found that, with just a few worker dynos (4), my long-running tasks would cause this flapping. This would happen because I would have a full set of reserved (prefetched) tasks, and the queue would be empty, so HireFire would shut the workers down. They would get put back in the task queue during shut down, so HireFire would then spin up the workers, less a few that were running and died at shutdown. The workers would pick up the reserved tasks again, which would empty the queue again, starting the cycle over again. This continued until all the remaining tasks failed.

how many actual network calls does celery do here? One per type? Or only one for the global inspect() and then nothing else?

I'm not entirely sure. But I can make an educated guess based on what I know about using the inspect() methods.

• Calling inspect() doesn't make any network calls at all, it's just creating an instance of the Inspect class.
• Each method run on inspect() takes additional time, so there is network traffic happening for each method call.
• All the methods we use are thin proxies to Inspect._request, which sends a broadcast message to all the workers. I suspect that means that the network requests increase as the number of workers increases, though I also suspect that the requests are done in parallel, as blocking on them one at a time would be quickly overwhelming. I haven't tested it with a very large number of dynos yet, though I have tested with around 30.

Re: special handling for scheduled:

When scaling down, this could lead to re-distribution of the scheduled tasks on the workers, but I think this is fine. Is there a limit on how many scheduled tasks celery can hold in-memory on the workers? So is 1 the right number?

I don't know of any limit to the number of scheduled tasks Celery can hold in memory, apart from the restriction of memory limits, so 1 very well may be the right number. It won't speed things up, but if you're using dynos that can be dropped down dynamically (not possible on Heroku with current Celery AFAICT), then it should be fine.

[ryanhiebert]

| definitely have long running tasks which don't currently survive the downscaling process, and get frequent sentry errors. I've designed most long running tasks to have eventual consistency as I couldn't easily see how to fix this. It seems this patch might do so. I'd love to see some clear documentation (with this patch?) which explains how to make sure that doesn't happen - what parameters @task should have, config settings etc.

When celery/celery#2839 is in a release, then using acks_late will be the way to make sure tasks survive, along with using that newly respect environment variable, REMAP_SIGTERM=SIGQUIT. Until then, there's nothing that you can do to make it survive a downscale, so I have HireFire set to only downscale when there are no tasks running.

I don't currently use any scheduled tasks cos I didn't trust them, mainly for this reason.u

Just to clarify, the scheduled tasks we are talking about here are ones that are apply_async'd with an eta, not django-celery's scheduled tasks, which are simply triggers to do something cron-like. I don't trust those. I don't have any experience with the kind of scheduled tasks we're talking about here.

@mjtamlyn , do you think that the behavior is broken enough that we should change the default to checking for active tasks, even though it makes the request slower? Is it worth that trade-off? I'm leaning that way.

[mjtamlyn]

Great to know about that celery patch, I already use acks_late everywhere so next update should hopefully fix that issue for me.

Ah, now I guess I probably do use eta (blindly) as I use retries for some tasks (like sending email). so I'd say that supporting those would be useful.

My instinct suggests that it does make sense to check active tasks by default.

[ryanhiebert]

Great to know about that celery patch, I already use acks_late everywhere so next update should hopefully fix that issue for me.

Well, it's in master, which is the for the dev branch. I don't know if it'll get backported to 3.1 or not, and I don't know the timeline for 3.2.

[ryanhiebert]

I've gone ahead and turned this handling on by default. Since I don't see a case where you'd want to disregard all of them blindly, and without knowing what they are, I've opted to not switch to a boolean, but rather just have any removals be done by overriding the setting, potentially with an empty list. I've also documented the possible values, what they mean, when you might be able to get away with removing them, and any pitfalls that seem likely.

I've also gone ahead and added the special handling for scheduled tasks, so that no matter how many are on a queue, it will always report as 1. This seems logical, as you don't want all the dynos for a queue with scheduled tasks to shut down, or it'll just cause flapping. It still makes sense to separate out scheduled tasks if possible so that your dynos can be empty of running jobs.

[ryanhiebert]

@jezdez @mjtamlyn : What's the current thought on merging this? Does it need additional work, verification, or review? I've been using it in production for a couple months now. It is certainly better than what currently exists, which was completely unworkable for me.

[mjtamlyn]

I would definitely be in favour, it's less critical for me than you but causes the same issue.

[ryanhiebert]

@jezdez: Pinging you in hopes that you'll be able to find the time to merge this. I've been using it in production for about 4 months now, and would love to have it on PyPI. I'd also be willing to help maintain this package if you were amiable, it's an important dependency for me.

[jezdez]

@ryanhiebert This looks great, @ryanhiebert. Do you have time to open another PR with doc updates so we can do a release?

[ask]

Nice work! Happy to see this resolved.

Celery 3.2 is now Celery 4.0 and is on target in the next month(s). Mostly ready, just finishing doc touches left

[jezdez]

@ask \o/

[ryanhiebert]

I should be able to get that done shortly.

@ask: That's awesome! I'm looking forward to Celery 4. I might see about giving it a go early, since it has the fix I need to scale down dynos safely.

[ryanhiebert]

@jezdez: What documentation are you looking for? I updated documentation in all the related docstrings, which are included in the docs.

[ask]

@ryanhiebert Please do test that feature, because I haven't tested it on Heroku (if you mean the remap of SIGTERM to SIGQUIT. You can reach me on twitter/mail to discuss, so we don't occupy this thread :)

[ryanhiebert]

@jezdez: I'd like to get the documentation that you've asked for, but I'm not sure what it is that you need that I've not done, sorry. Can you point me in the right direction?

[jezdez]

@ryanhiebert D'oh, you're right, I forgot all of this is autodoc'd. Nevermind then 👍

[ryanhiebert]

Thanks. Does that mean a release is imminent?

[ryanhiebert]

@jezdez: release?

[jezdez]

@ryanhiebert coming up..

[jezdez]

0.4 is out: https://pypi.python.org/pypi/HireFire/0.4