Make background updates controllable via a plugin

[erikjohnston]

Fixes #11261

[babolivier]

Just passing by and sharing some thoughts:

Since the rewrite of the module system, we've generally tried to steer away from forcing modules to register whole classes, and instead registering individual callbacks, as

• it avoids forcing method names on the module or one of its subclasses (eg what if we're expanding an existing module which happens to already have a run_update method?),
• it allows modules that need to implement callbacks from various categories to be more versatile without having to add too much overhead, and
• it means if we want to modify the list of callbacks in the future we don't have to add an extra layer of wrapping to cover the case where the module doesn't implement these changes

The implementation in this PR feels a bit like a step backwards to me (on top of creating an inconsistency with the rest of the system), since it forces modules to register a whole class with a set list of callbacks, and it doesn't look to me like we absolutely can't have modules register these callbacks independently. FWIW if the concern is that we need callbacks to only be registered once and/or we want to make sure a module that registers a callback also registers another one, we can do that even when asking the module to register callbacks individually (actually the presence router code already does the latter).

On top of that, it looks like only one controller can be configured (not sure whether this is a conscious decision) but there isn't anything in the code that ensures it (the rule of thumb is generally that in this kind of situations where only one module can register, the first one (in the order they appear in the config file) that tries to do so wins).

[erikjohnston]

Just passing by and sharing some thoughts:

Since the rewrite of the module system, we've generally tried to steer away from forcing modules to register whole classes, and instead registering individual callbacks, as

• it avoids forcing method names on the module or one of its subclasses (eg what if we're expanding an existing module which happens to already have a run_update method?),
• it allows modules that need to implement callbacks from various categories to be more versatile without having to add too much overhead, and
• it means if we want to modify the list of callbacks in the future we don't have to add an extra layer of wrapping to cover the case where the module doesn't implement these changes

Interesting. I agree that makes a lot of sense for things like spam checkers, where really all you're doing is hooking in independent callbacks into various places. I'm less convinced for situations where it doesn't make sense for the callbacks to be independent or mixed up with other callbacks. Personally, I think if you have a single class that is doing spam checking and background update control then its going to be a right old confusing mess, and should absolutely be split into separate classes.

I'll have to have a think a bit about how I feel about doing everything as callbacks. It's also worth noting in this case that implementations are going to have to implement a AsyncContextManager class anyway.

The implementation in this PR feels a bit like a step backwards to me (on top of creating an inconsistency with the rest of the system), since it forces modules to register a whole class with a set list of callbacks, and it doesn't look to me like we absolutely can't have modules register these callbacks independently. FWIW if the concern is that we need callbacks to only be registered once and/or we want to make sure a module that registers a callback also registers another one, we can do that even when asking the module to register callbacks individually (actually the presence router code already does the latter).

I don't really understand why that function takes optionals when it a) does nothing if they're both None and b) you can't only set one.

[babolivier]

I'm less convinced for situations where it doesn't make sense for the callbacks to be independent or mixed up with other callbacks

I think it still makes sense so that module developers have more freedom around naming the callbacks, and so that we don't have to implement layers of wrapping to cover for the lack of a callback added in a later version (and if eg we end up adding an optional callback it feels hackier to me to getattr to see if the module has named a method in a specific way, at the risk of conflicting with an existing method, than checking if the module has proactively registered its capacity to handle this callback).

Personally, I think if you have a single class that is doing spam checking and background update control then its going to be a right old confusing mess, and should absolutely be split into separate classes.

Probably, but a) I don't feel comfortable imposing this onto module maintainers, especially if it's not the case for other callback types (eg presence routers), and b) imposing the specific structure the class should have feels a bit too much, especially since the current system aims at giving module developers as much freedom in the code they write as possible.

It's also worth noting in this case that implementations are going to have to implement a AsyncContextManager class anyway.

About that, why can't we just have the callback return an int?
Also on this point, I think the name of the run_update callback is confusing because it sounds to me like the module is expected to run the update itself.

I don't really understand why that function takes optionals when it a) does nothing if they're both None and b) you can't only set one.

Hmm yeah that's a good point, I guess I missed that during review.

[erikjohnston]

I'm less convinced for situations where it doesn't make sense for the callbacks to be independent or mixed up with other callbacks

I think it still makes sense so that module developers have more freedom around naming the callbacks, and so that we don't have to implement layers of wrapping to cover for the lack of a callback added in a later version (and if eg we end up adding an optional callback it feels hackier to me to getattr to see if the module has named a method in a specific way, at the risk of conflicting with an existing method, than checking if the module has proactively registered its capacity to handle this callback).

That's true that adding new functions to the class becomes hard in a backwards compatible way. Which is probably a showstopper here, bleurgh.

Personally, I think if you have a single class that is doing spam checking and background update control then its going to be a right old confusing mess, and should absolutely be split into separate classes.

Probably, but a) I don't feel comfortable imposing this onto module maintainers, especially if it's not the case for other callback types (eg presence routers), and b) imposing the specific structure the class should have feels a bit too much, especially since the current system aims at giving module developers as much freedom in the code they write as possible.

I'd rather we aimed for an ergonomic API that made it easy to write decent plugins clearly and effectively. In my experience allowing tonnes of flexibility in how to structure the code can make it much harder to write plugins as a) there's no clear natural structure that the code should have, which means authors have to spend time figuring it out each time, and b) tends to encourage a lack of structure, as you just have a single class with tonnes of functions as that is the path of least resistance.

But this is more a philosophical discussion, rather than a practical one.

It's also worth noting in this case that implementations are going to have to implement a AsyncContextManager class anyway.

About that, why can't we just have the callback return an int? Also on this point, I think the name of the run_update callback is confusing because it sounds to me like the module is expected to run the update itself.

Because we also need to know when the background update finishes, e.g. I want to be able to limit the number of inflight background updates across a fleet of hosts. We could have two callbacks for that, but an async context manager really is built for this.

[babolivier]

Heads up I've dropped the initial BackgroundUpdateController classes present in a previous version of this PR, as they made things more complex and potentially flaky than they needed to for managing callbacks. I've also updated tests to not use AsyncMocks since these are only supported on Py3.8+.

[babolivier]

Note that in a previous incarnation of this PR, module callbacks would be ignored if the updates are run with sleep=False. The initial idea was that modules would only use this callback for rate-limiting, but that might not necessarily be the case for future modules, so I've removed that constraint.

[babolivier]

CI is currently failing on one test (test_status_bg_update), I'm having a look but in the meantime it's probably good for review.

[babolivier]

While not directly related with the background update work in this PR it's going to be used by https://gitlab.matrix.org/new-vector/ems-synapse-background-update-controller and it's a small change so I thought I'd include it in here. I can appreciate if people think it's too much scope-creeping, and would prefer me to open a separate PR for it.

[DMRobertson]

I think this needs merge conflicts resolving @babolivier

[babolivier]

I think this needs merge conflicts resolving @babolivier

Should be fine now @DMRobertson

[DMRobertson]

Code changes look sensible. A few suggestions re the documentation:

[DMRobertson]

Does the plugin author get to know if their callback was ignored?

[babolivier]

Not currently.

[DMRobertson]

LGTM!