Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

'Memory Exhausted' when removing monitored tag #625

Closed
gdebrauwer opened this issue Jul 1, 2019 · 11 comments · Fixed by #690
Closed

'Memory Exhausted' when removing monitored tag #625

gdebrauwer opened this issue Jul 1, 2019 · 11 comments · Fixed by #690
Labels

Comments

@gdebrauwer
Copy link
Contributor

  • Horizon Version: 3.2.3
  • Laravel Version: 5.8.24
  • PHP Version: 7.3.4
  • Redis Driver & Version: predis 1.1.1
  • Database Driver & Version: mysql 5.7

Description:

I was monitoring a tag on my production server. That tag was linked to a couple of 100 000 jobs.
I suddenly noticed that my redis-server was using a lot of memory (almost 10GB) and I discovered that it was caused by the monitoring.
When I removed the monitoring, the issue persisted and the RAM usage did not chage.
After further investigation, I saw that the StopMonitoringTag job class failed because the memory was exhausted. That's because all the jobs (couple of 100 000 jobs) are all fetched/loaded in php and then deleted.

Should this not be done paginated?
I suspect this may be an issue in other places as well, if Horizon fetches a huge amount of jobs (e.g. trimming recent/failed/monitored jobs)

(The only way I could fix this issue, was redis-cli flushdb, which is normally not ideal on a production server 😅)

Steps To Reproduce:

  • Add monitoring for a certain tag
  • Generate huge amounts of jobs that use the monitored tag
  • Check the RAM usage increase by your redis-server + check the amount of redis keys on the redis-server
  • Remove the monitored tag
  • See that your RAM usage did not change + see that the amount of redis keys did not change
  • Try to run dispatch the StopMonitoringTag job class manually via tinker. This will throw a 'memory exhausted' error.
@mfn
Copy link
Contributor

mfn commented Jul 1, 2019

I reported the same some time ago: #271

Apparently some fix was applied related to expiration, but to me it was clear the overhead is too risky for production when you suddenly get an influx o jobs. Which is kind of a paradox if you consider the purpose of a queue system: it shouldn't matter whether you have 100 or 100.000.000 jobs, regardless of the monitoring tag or not.

@driesvints
Copy link
Member

I really have no clue on how to even begin approaching this tbh. @mfn to me it seems this is just out of Horizon's scope to handle?

@mfn
Copy link
Contributor

mfn commented Jul 2, 2019

I really have no clue on how to even begin approaching this tbh

Maybe it's a documentation/warning issue: if you use tags, meta data gets written to redis which increases the size drastically.

I think it's easy to reproduce, one just has to compare redis DB size locally / RAM :

  • create 5.000.000 jobs

Versus:

  • enable tags for a job
  • create 5.000.000 jobs

@gdebrauwer
Copy link
Contributor Author

gdebrauwer commented Jul 2, 2019

the StopMonitoringTag job class uses the jobs($tag) method of the RedisTagRepository class to fetch the jobs.
The RedisTagRepository class also has a paginate($tag) method, so maybe that method could be used instead? This will limit the amount of jobs that will be loaded in php and prevent exhausting your memory.

@driesvints
Copy link
Member

@guntherdebrauwer maybe yeah. I'm not sure what the impact of that would be? Ping @themsaid

@mfn
Copy link
Contributor

mfn commented Jul 2, 2019

the StopMonitoringTag job class uses the jobs($tag) method of the RedisTagRepository class to fetch the jobs.
The RedisTagRepository class also has a paginate($tag) method, so maybe that method could be used instead? This will limit the amount of jobs that will be loaded in php and prevent exhausting your memory.

Please note that this and my issue is about Redis memory exhaustion, not PHP; it's a problem with the amount of data stored within redis with tagging and large amount of jobs.

@gdebrauwer
Copy link
Contributor Author

@mfn why is this a Redis memory exhaustian? It's true that monitoring causes a lot of data storage within redis, but that didn't cause the exhaustion error (my server memory usage was only 65-70%). The problem was removing my monitored tag did not deleted all jobs of that tag. It tried to load all jobs into php memory, to then delete them one by one. And loading couple of 100 000 jobs from redis-server caused the memory exhaustion. That's what I think happens. Correct me if I'm wrong

class StopMonitoringTag
{
    public function handle(JobRepository $jobs, TagRepository $tags)
    {
        // Remove tag from monitoring
        $tags->stopMonitoring($this->tag);

        // Load all tags
        // This causes memory exhaustion, as it tries to load couple of 100 000 jobs into php memory
        $tagJobs = $tags->jobs($this->tag);

        // Delete all loaded tag jobs one by one
        $jobs->deleteMonitored($tagJobs);

        $tags->forget($this->tag);
    }
}

@mfn
Copy link
Contributor

mfn commented Jul 2, 2019

@guntherdebrauwer apologies, I think I must have misread some parts and thought it was exactly the issue I once experienced => in this case, please disregard my comments 🙇

@gdebrauwer
Copy link
Contributor Author

@mfn No problem 🙂 I do agree that the documentation maybe should include a warning regarding the increased memory usage / saved data in redis when monitoring a tag.

@driesvints
Copy link
Member

Checking in on this again. I agree that we should maybe batch the deletions somehow. Not sure atm how that would be done.

@gdebrauwer
Copy link
Contributor Author

the TagRepository appears to have a paginate($tag, $startingAt = 0, $limit = 25), so maybe we can write a while loop inside the StopMonitoringTag job 🤔

$tagJobs = $tags->paginate($this->tag);

while (count($tagJogs) !== 0) {
    $jobs->deleteMonitored($tagJobs);

    $tagJobs = $tags->paginate($this->tag);
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment