Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Retention policy purge jobs fail if run_background_tasks_on is set to something other than master #9927

Closed
babolivier opened this issue May 5, 2021 · 7 comments · Fixed by #13632
Labels
P3 (OBSOLETE: use S- labels.) Approved backlog: not yet scheduled, will accept patches S-Minor Blocks non-critical functionality, workarounds exist. T-Defect Bugs, crashes, hangs, security vulnerabilities, or other reported issues.

Comments

@babolivier
Copy link
Contributor

If run_background_tasks_on is set to anything other than master, Synapse will try to run background jobs on a worker, including the message retention policies support's purge jobs. However, these jobs end up calling the purge_history function of the storage layer (in the PurgeEventsStorage class) which doesn't exist on a worker, therefore the jobs fail with AttributeError: 'GenericWorkerSlavedStore' object has no attribute 'purge_history'.

This bug was introduced in #8513

cc @iansinclair

@babolivier babolivier added P3 (OBSOLETE: use S- labels.) Approved backlog: not yet scheduled, will accept patches S-Minor Blocks non-critical functionality, workarounds exist. T-Defect Bugs, crashes, hangs, security vulnerabilities, or other reported issues. labels May 5, 2021
@clokep
Copy link
Member

clokep commented May 5, 2021

I guess we either need to move the method to the worker store or backout the changes that run that on a background worker. I think they're potentially heavy though, so if we could run it on a worker that would be best!

@babolivier
Copy link
Contributor Author

Yes I tend to agree - though having something that fiddles with the database as much as purge_history does run on a worker sounds a bit scary to me.

@callahad callahad added P2 and removed P3 (OBSOLETE: use S- labels.) Approved backlog: not yet scheduled, will accept patches labels Nov 11, 2021
@richvdh richvdh self-assigned this Nov 11, 2021
@richvdh richvdh changed the title Retention policy purge jobs can't run if run_background_tasks_on is set to something other than master Retention policy purge jobs fail if run_background_tasks_on is set to something other than master Nov 11, 2021
@callahad
Copy link
Contributor

Noting that @richvdh self-assigned: I believe you're a bit overloaded at the moment; would it make sense to hand this off to someone else (@DMRobertson?) with you providing review once impl'd?

@richvdh
Copy link
Member

richvdh commented Nov 29, 2021

Noting that @richvdh self-assigned:

sorry, at one point it sounded like this was a drop-the-world level of urgency, but that turned out to be incorrect. Yes, it's unlikely I'll have time to look at this in the near future!

@richvdh richvdh removed their assignment Nov 29, 2021
@callahad
Copy link
Contributor

Let's tentatively try to address this after FOSDEM next year... noting that retention policies are still an experimental feature themselves.

@callahad callahad added P3 (OBSOLETE: use S- labels.) Approved backlog: not yet scheduled, will accept patches and removed P2 labels Nov 29, 2021
@squahtx
Copy link
Contributor

squahtx commented Dec 13, 2021

related: #11165

@Fred-Treebal
Copy link

Very interested in a fix on this issue, we run matrix under Kubernetes, running purges from the master creates high load cpu spikes for several minutes on each scheduled purge, I'd rather have those spikes on a non critical worker like backgroundtasks. thx.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
P3 (OBSOLETE: use S- labels.) Approved backlog: not yet scheduled, will accept patches S-Minor Blocks non-critical functionality, workarounds exist. T-Defect Bugs, crashes, hangs, security vulnerabilities, or other reported issues.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants