Allow Admin API delete room v2 actions to be run on worker #17904
Conversation
| self.is_room_blocked, | ||
| (room_id,), | ||
| ) | ||
|
|
There was a problem hiding this comment.
It's not super clear from the diff but this was moved from the RoomStore to the RoomWorkerStore
| The following admin APIs are now available to be handled by workers, with more forthcoming: | ||
|
|
||
| # Admin APIs | ||
| "^/_synapse/admin/v2/rooms/.*$" |
There was a problem hiding this comment.
This regex currently matches anything after /_synapse/admin/v2/rooms/
But I think we want the following explicit list based on what synapse/rest/admin/__init__.py -> register_servlets looks like:
/rooms/<room_id>/rooms/delete_status/<delete_id>/rooms/<room_id>/delete_status
For example, we don't support the BlockRoomRestServlet at /rooms/<room_id>/block yet.
I guess this also applies to the endpoint_patterns in docker/configure_workers_and_start.py but perhaps we can re-use the PATTERNS of each servlet?
| @@ -168,6 +168,32 @@ def process_replication_position( | |||
| self._un_partial_stated_rooms_stream_id_gen.advance(instance_name, token) | |||
| return super().process_replication_position(stream_name, instance_name, token) | |||
|
|
|||
| async def block_room(self, room_id: str, user_id: str) -> None: | |||
There was a problem hiding this comment.
It looks like this was moved but BlockRoomRestServlet isn't used on a worker yet. Just some leftover work from trying it out before? (fine to keep, just clarifying)
| @@ -135,6 +135,13 @@ | |||
| }, | |||
| "worker_extra_conf": "enable_media_repo: true", | |||
| }, | |||
| "admin": { | |||
There was a problem hiding this comment.
Perhaps we need a comment above for WORKERS_CONFIG alongside the others
Watching /_synapse/admin needs a "admin" listener
| @@ -135,6 +135,13 @@ | |||
| }, | |||
| "worker_extra_conf": "enable_media_repo: true", | |||
| }, | |||
| "admin": { | |||
| "app": "synapse.app.generic_worker", | |||
| "listener_resources": ["replication", "admin"], | |||
There was a problem hiding this comment.
one question I had was whether a worker handling these admin requests needs a replication listener - since the code requires invalidating the cache and streaming, I guessed that the answer is yes, but would like to know if that is correct.
I'm not confident but I think it may not need "replication" 🤔
The admin API's probably use the various handlers which have the logic built-in to make the write or send it off over replication to the appropriate worker.
For example, the /_synapse/admin/v1/join/<room_id_or_alias> endpoint uses the normal event creation flow that hands it off to the event_persisters which has "replication".
When it invalidates caches, it will _send_invalidation_to_replication(...) and the stores mixin CacheInvalidationWorkerStore to bust their cache locally when they receive those messages.
I'm not sure if we're completely studious about this as looking at the device creation code, it seems a bit sketchy (check_device_registered(...)) but must be fine because /_matrix/client/v3/register is normally handled on a cleint_reader worker without "replication".
In any case, I think the bottom line is any code that isn't doing the "right" thing and asking the appropriate worker to make the "write" should be updated to do so. I haven't deep-dived into whether we have any of those problems now.
| @@ -1076,6 +1084,7 @@ def main(args: List[str], environ: MutableMapping[str, str]) -> None: | |||
| # Split type names by comma, ignoring whitespace. | |||
| worker_types = split_and_strip_string(worker_types_env, ",") | |||
| requested_worker_types = parse_worker_types(worker_types) | |||
| log(f"requested_worker_types {requested_worker_types}") | |||
| @@ -0,0 +1 @@ | |||
| Allow Admin API delete room v2 actions to be run on workers. | |||
There was a problem hiding this comment.
Complement test for this is at matrix-org/complement@main...H-Shay:complement:shay/admin_tests. I didn't open a PR with this as it is unclear to me if this test belongs in the Complement repo - opinions welcome. FWIW since I have to push to a fork the test here isn't run against this PR anyway, although I can say it runs locally.
If we were to stick with Complement, probably belongs in the Synapse repo but would require some boilerplate to get it working.
But since it's a Synapse specific thing, we probably prefer to just write a Twisted test instead (I'm no dictator on this so feel free to ask in the Synapse rooms what people think). If we want it at the end-to-end level, probably in tests/rest/admin with some worker override config
As the title states - support shifting the admin api delete room v2 actions to a generic worker. Also adds support for testing this in complement. The hope is to move other admin actions to be available on a worker in follow-up PRs.
I'm new to working with workers, so there may be things that I have missed/done ineptly. Eagle eyes appreciated. In particular, one question I had was whether a worker handling these admin requests needs a replication listener - since the code requires invalidating the cache and streaming, I guessed that the answer is yes, but would like to know if that is correct.
Complement test for this is at matrix-org/complement@main...H-Shay:complement:shay/admin_tests. I didn't open a PR with this as it is unclear to me if this test belongs in the Complement repo - opinions welcome. FWIW since I have to push to a fork the test here isn't run against this PR anyway, although I can say it runs locally.
Should be reviewable commit-by-commit.