Modernize configure_workers_and_start.py bootstrapping script for Dockerfile-workers.

[realtyem]

• Needs: Add all Stream Writer worker types to configure_workers_and_start.py #14197
  • (For the removal of the gating that processes single instances)

Trying to help solve

• Remove the concept of worker "types" #10441

It appears that since Feb 25th, 2020, all workers are generic workers, full stop. Some stragglers might have caused problems at that time, but everything seems to work as it's supposed to now. Let's modernize configure_workers_and_start.py to reflect this. Documentation should be a separate pull request. This file is used frequently by curious people to setup their own servers. Although there are further issues to explore, this PR will only focus on configure_workers_and_start.py.

Checklist of existing known 'non'-generic_workers to switch over here:

• synapse.app.federation_sender
• synapse.app.media_repository (See Internal Issue 3 below)
• synapse.app.pusher
• synapse.app.frontend_proxy

Details:

1. Lines 432 - 444 in generic_worker.py list all the historical apps by name. This is reflected in the actual directory synapse/app. All of those worker python files have the same content loading generic_worker.py instead. So, all workers are generic workers and can be labeled such in worker configuration yaml files. This has been true for synapse.app.federation_sender and synapse.app.pusher since 1.29.0 on Clean up ShardedWorkerHandlingConfig #9466

2. There is a comment that references using '*' to declare all workers. That doesn't seem to have worked since Apr 14, 2021 when the logic was removed. I like the idea of this functionality, but I think it should be a string instead of an asterisk. Perhaps "full" or "all". Until this is decided, delete the comment as it can be confusing when it doesn't work.

3. For some reason, the setting of media_instance_running_background_jobs was orphaned at the bottom of a subroutine. There is logic for handling this and including it in the main WORKER_CONFIG template near the top of the file. Made this consistent with other settings like this, for instance background_worker's run_background_tasks_on.

Internal Issues:

1. For Details 1: Using synapse.app.generic_worker instead of synapse.app.federation_sender means the send_federation configuration option is ignored, as written in workers.py. The way forward appears to be to use the special "Map" of federation_sender_instances which is placed into shared or homeserver config(if using the main configuration as the shared config). This appears to be necessary if there are any federation_sender's. See comment where I discover this and go into it with psuedo-logic to try and rationalize. Since then, I've read into it more, leading here. I'm not sure that was the intent, but it seems to be the result. It works and it appears cleaner to my eyes. See comment in source where it's said this is the way that is preferred.

2. For Details 1: Same as Issue 1 for synapse.app.pusher, start_pushers and pusher_instances, respectively. See line 271 of workers.py. The logic appears to be largely the same as for federation senders.

3. For Details 1: synapse.app.media_repository is a synapse.app.generic_worker, however because of this line, enable_media_repo won't work with synapse.app.generic_worker. As such, this won't be changed now. I feel like this logic should be in the workers.py file with similar type options(like run_background_tasks_on, notify_appservices_from_worker and update_user_directory_from_worker), and possibly adapted to use _should_this_worker_perform_duty(). Ideally, this should all be on a map instance that hashes by worker_name in the shared config, similar to pusher_instances and federation_sender_instances.

External Issues(and Questions) to explore further:

1. There are a lot of duplicate values in worker config files that can be accessed by the server config directly, like the worker_main_http_uri, worker_replication_host and worker_replication_http_port. Can these be referenced directly, instead of being in an actual file?
2. Tests which contain the names of deprecated synapse.app.*

• synapse.app.appservice
  • https://github.com/matrix-org/synapse/blob/develop/tests/config/test_workers.py
• synapse.app.client_reader
  • https://github.com/matrix-org/synapse/blob/develop/tests/replication/test_client_reader_shard.py
  • https://github.com/matrix-org/synapse/blob/develop/tests/replication/test_auth.py
• synapse.app.federation_sender
  • https://github.com/matrix-org/synapse/blob/develop/tests/replication/test_federation_sender_shard.py
  • https://github.com/matrix-org/synapse/blob/develop/tests/replication/test_federation_ack.py
  • https://github.com/matrix-org/synapse/blob/develop/tests/replication/tcp/streams/test_federation.py
  • https://github.com/matrix-org/synapse/blob/develop/tests/replication/_base.py (only in a comment)
• synapse.app.pusher
  • https://github.com/matrix-org/synapse/blob/develop/tests/replication/test_pusher_shard.py

3. At this point(or rather since 2020), it should be possible to remove needing to declare the app type at all in any config since they are all the same(exception being synapse.app.media_repository, see above Internal Issue 3), thereby removing the need for worker 'types'. Need someone who knows more to double check this is accurate. Perhaps a conditional that if app isn't declared it will automatically default to synapse.app.generic_worker would be sufficient? I also feel like there are more things like this that can be done.

Notes:

1. Documentation details to be updated:

• account_data endpoints listed twice, once in stream writers and once for generic_workers. Which is correct since it is a stream writer? Needs clarification so this isn't accidently duplicated in reverse proxy configuration.
• receipts, same as above.
• presence, same as above.

Pull Request Checklist

• Pull request is based on the develop branch
• Pull request includes a changelog file.
• Pull request includes a sign off
• Code style is correct
  (run the linters)

Signed-off-by: Jason Little realtyem@gmail.com

[dklimpel]

1. There are a lot of duplicate values in worker config files that can be accessed by the server config directly, like the worker_main_http_uri, worker_replication_host and worker_replication_http_port. Can these be referenced directly, instead of being in an actual file?

I've had that thought too. Why do not move this to the shared homeserver.yaml?

[realtyem]

1. There are a lot of duplicate values in worker config files that can be accessed by the server config directly, like the worker_main_http_uri, worker_replication_host and worker_replication_http_port. Can these be referenced directly, instead of being in an actual file?

I've had that thought too. Why do not move this to the shared homeserver.yaml?

I've got something better than that coming through the thought pipeline. How about getting rid of it altogether? Obviously, in a different PR than this.

[reivilibre]

1. There are a lot of duplicate values in worker config files that can be accessed by the server config directly, like the worker_main_http_uri, worker_replication_host and worker_replication_http_port. Can these be referenced directly, instead of being in an actual file?

I've had that thought too. Why do not move this to the shared homeserver.yaml?

This would be completely reasonable and surprised me too. Possibly it has come about because matrix.org is across multiple machines and so the address you use for contacting the master can depend based on which machine the workers are on (?). Who knows. It should be simplified for the docs though.

[reivilibre]

Seems reasonable; ping me again to check when the underlying PR has merged first

[reivilibre]

This now has conflicts :(

[realtyem]

Should be fixed now.

[realtyem]

Incorrectly requesting a worker type will now error and exit. Change requested by @reivilibre out-of-band.

[reivilibre]

Thanks!! :)