Clean up `ShardedWorkerHandlingConfig` #9466

erikjohnston · 2021-02-22T18:10:22Z

See commit messages, but basically the idea is to make ShardedWorkerHandlingConfig behave in a saner way, rather than having special cases for situations where there are only one or no instances defined. This is done by consolidating all the logic to handle the different ways of configuring pusher and federation senders into one place.

The special casing masked problems such as those described in #9465

This is so that we have a type level understanding of when it is safe to call `get_instance(..)` (as opposed to `should_handle(..)`).

`ShardedWorkerHandlingConfig` tried to handle the various different ways it was possible to configure federation senders and pushers. This led to special cases that weren't hit during testing. To fix this the handling of the different cases is moved from there and `generic_worker` into the worker config class. This allows us to have the logic in one place and allows the rest of the code to ignore the different cases.

synapse/config/_base.py

synapse/config/_base.pyi

…r in RoutableShardedWorkerHandlingConfig

clokep · 2021-02-23T16:28:32Z

Will this fix #8015?

erikjohnston · 2021-02-23T16:29:33Z

Will this fix #8015?

Oooh, yes , I think so! Maybe

synapse/config/_base.py

synapse/config/workers.py

clokep · 2021-02-23T16:39:51Z

synapse/config/workers.py

+            # Default to an empty list, which means "another, unknown, worker is
+            # responsible for it".
+            federation_sender_instances = []


This happens if you're using send_federation, but this config is being parsed on a non-federation sender worker. Is that correct?

Yes, exactly.

clokep · 2021-02-23T16:41:19Z

synapse/config/workers.py

+        if len(self.writers.events) == 0:
+            raise ConfigError("Must specify at least one instance to handle `events`.")


I wonder if it would help us remember to add these if we used validators on the WriterLocations attributes?

Yeah, though I'm not sure how easy it is to make ConfigError out of validation failures?

There's a few places that we raise a ConfigError from another exception, but not sure if it is worth refactoring that here. 🤷

clokep · 2021-02-23T16:44:32Z

synapse/config/workers.py

-        # applies to some federation traffic, and so shouldn't be used to
-        # "disable" federation
-        self.send_federation = config.get("send_federation", True)
+        # Handle federation sender configuration.


It looks like the logic for sharding federation sender and push is identical, can we abstract this somehow?

I think something like self.send_federation = self._handle_shared_config("send_federation", "federation_sender_instances", "synapse.app.federation_sender") might do it?

I'm slightly in two minds about this. On the one had, yes, on the other we should never have more than these two, and so I think it might actually be clearer just writing it out rather than factoring it out and passing a bunch of stuff in etc 🤷

If there isn't additional workers that we'll need to apply this to than that sounds OK.

Yeah, this only for working around the old style config options, new stuff shouldn't have that problem

clokep · 2021-02-23T16:45:45Z

tests/replication/test_pusher_shard.py

@@ -95,7 +95,7 @@ def test_send_push_single_worker(self):

        self.make_worker_hs(
            "synapse.app.pusher",
-            {"start_pushers": True},
+            {"start_pushers": False},


I'm a bit confused why we need to provide this? Does the test config default to starting push?

start_pushers should be false in the config file, then previously it got set to true during generic_worker.start (which doesn't get run from here) and now gets set to true in the worker config class (which does get run)

@dklimpel

Synapse 1.29.0 (2021-03-08) =========================== Note that synapse now expects an `X-Forwarded-Proto` header when used with a reverse proxy. Please see [UPGRADE.rst](UPGRADE.rst#upgrading-to-v1290) for more details on this change. No significant changes. Synapse 1.29.0rc1 (2021-03-04) ============================== Features -------- - Add rate limiters to cross-user key sharing requests. ([\#8957](matrix-org/synapse#8957)) - Add `order_by` to the admin API `GET /_synapse/admin/v1/users/<user_id>/media`. Contributed by @dklimpel. ([\#8978](matrix-org/synapse#8978)) - Add some configuration settings to make users' profile data more private. ([\#9203](matrix-org/synapse#9203)) - The `no_proxy` and `NO_PROXY` environment variables are now respected in proxied HTTP clients with the lowercase form taking precedence if both are present. Additionally, the lowercase `https_proxy` environment variable is now respected in proxied HTTP clients on top of existing support for the uppercase `HTTPS_PROXY` form and takes precedence if both are present. Contributed by Timothy Leung. ([\#9372](matrix-org/synapse#9372)) - Add a configuration option, `user_directory.prefer_local_users`, which when enabled will make it more likely for users on the same server as you to appear above other users. ([\#9383](matrix-org/synapse#9383), [\#9385](matrix-org/synapse#9385)) - Add support for regenerating thumbnails if they have been deleted but the original image is still stored. ([\#9438](matrix-org/synapse#9438)) - Add support for `X-Forwarded-Proto` header when using a reverse proxy. ([\#9472](matrix-org/synapse#9472), [\#9501](matrix-org/synapse#9501), [\#9512](matrix-org/synapse#9512), [\#9539](matrix-org/synapse#9539)) Bugfixes -------- - Fix a bug where users' pushers were not all deleted when they deactivated their account. ([\#9285](matrix-org/synapse#9285), [\#9516](matrix-org/synapse#9516)) - Fix a bug where a lot of unnecessary presence updates were sent when joining a room. ([\#9402](matrix-org/synapse#9402)) - Fix a bug that caused multiple calls to the experimental `shared_rooms` endpoint to return stale results. ([\#9416](matrix-org/synapse#9416)) - Fix a bug in single sign-on which could cause a "No session cookie found" error. ([\#9436](matrix-org/synapse#9436)) - Fix bug introduced in v1.27.0 where allowing a user to choose their own username when logging in via single sign-on did not work unless an `idp_icon` was defined. ([\#9440](matrix-org/synapse#9440)) - Fix a bug introduced in v1.26.0 where some sequences were not properly configured when running `synapse_port_db`. ([\#9449](matrix-org/synapse#9449)) - Fix deleting pushers when using sharded pushers. ([\#9465](matrix-org/synapse#9465), [\#9466](matrix-org/synapse#9466), [\#9479](matrix-org/synapse#9479), [\#9536](matrix-org/synapse#9536)) - Fix missing startup checks for the consistency of certain PostgreSQL sequences. ([\#9470](matrix-org/synapse#9470)) - Fix a long-standing bug where the media repository could leak file descriptors while previewing media. ([\#9497](matrix-org/synapse#9497)) - Properly purge the event chain cover index when purging history. ([\#9498](matrix-org/synapse#9498)) - Fix missing chain cover index due to a schema delta not being applied correctly. Only affected servers that ran development versions. ([\#9503](matrix-org/synapse#9503)) - Fix a bug introduced in v1.25.0 where `/_synapse/admin/join/` would fail when given a room alias. ([\#9506](matrix-org/synapse#9506)) - Prevent presence background jobs from running when presence is disabled. ([\#9530](matrix-org/synapse#9530)) - Fix rare edge case that caused a background update to fail if the server had rejected an event that had duplicate auth events. ([\#9537](matrix-org/synapse#9537)) Improved Documentation ---------------------- - Update the example systemd config to propagate reloads to individual units. ([\#9463](matrix-org/synapse#9463)) Internal Changes ---------------- - Add documentation and type hints to `parse_duration`. ([\#9432](matrix-org/synapse#9432)) - Remove vestiges of `uploads_path` configuration setting. ([\#9462](matrix-org/synapse#9462)) - Add a comment about systemd-python. ([\#9464](matrix-org/synapse#9464)) - Test that we require validated email for email pushers. ([\#9496](matrix-org/synapse#9496)) - Allow python to generate bytecode for synapse. ([\#9502](matrix-org/synapse#9502)) - Fix incorrect type hints. ([\#9515](matrix-org/synapse#9515), [\#9518](matrix-org/synapse#9518)) - Add type hints to device and event report admin API. ([\#9519](matrix-org/synapse#9519)) - Add type hints to user admin API. ([\#9521](matrix-org/synapse#9521)) - Bump the versions of mypy and mypy-zope used for static type checking. ([\#9529](matrix-org/synapse#9529))

erikjohnston force-pushed the erikj/pusher_shard_fix branch from 4431274 to d9061ca Compare February 22, 2021 19:07

erikjohnston added 3 commits February 22, 2021 21:14

Split ShardedWorkerHandlingConfig

931b9a9

This is so that we have a type level understanding of when it is safe to call `get_instance(..)` (as opposed to `should_handle(..)`).

Newsfile

35b8416

erikjohnston force-pushed the erikj/pusher_shard_fix branch from d9061ca to 35b8416 Compare February 22, 2021 21:15

erikjohnston mentioned this pull request Feb 22, 2021

Fix deleting pushers when using sharded pushers. #9465

Merged

erikjohnston requested a review from a team February 22, 2021 21:17

clokep reviewed Feb 23, 2021

View reviewed changes

synapse/config/_base.py Outdated Show resolved Hide resolved

synapse/config/_base.py Show resolved Hide resolved

synapse/config/_base.pyi Show resolved Hide resolved

Check 'self.writers.events' is non-empty, and raise more helpful erro…

13af8a2

…r in RoutableShardedWorkerHandlingConfig

erikjohnston requested a review from clokep February 23, 2021 16:13

clokep reviewed Feb 23, 2021

View reviewed changes

Fix up comments/errors

173c79e

erikjohnston requested a review from clokep February 24, 2021 10:40

clokep approved these changes Feb 24, 2021

View reviewed changes

erikjohnston merged commit 2927921 into develop Feb 24, 2021

erikjohnston deleted the erikj/pusher_shard_fix branch February 24, 2021 13:23

clokep mentioned this pull request Feb 24, 2021

federation sender doesn't work when invoked as synapse.app.generic_worker #8015

Closed

roeltm mentioned this pull request Aug 26, 2021

Pusher instances not starting from v1.29.0 #10700

Closed

richvdh mentioned this pull request Aug 10, 2022

Remove the concept of worker "types" #10441

Open

6 tasks

realtyem mentioned this pull request Oct 26, 2022

Modernize configure_workers_and_start.py bootstrapping script for Dockerfile-workers. #14294

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clean up `ShardedWorkerHandlingConfig` #9466

Clean up `ShardedWorkerHandlingConfig` #9466

erikjohnston commented Feb 22, 2021

clokep commented Feb 23, 2021

erikjohnston commented Feb 23, 2021 •

edited

clokep Feb 23, 2021

erikjohnston Feb 23, 2021

clokep Feb 23, 2021

erikjohnston Feb 23, 2021

clokep Feb 23, 2021

clokep Feb 23, 2021

erikjohnston Feb 24, 2021

clokep Feb 24, 2021

erikjohnston Feb 24, 2021

clokep Feb 23, 2021

erikjohnston Feb 23, 2021

		if len(self.writers.events) == 0:
		raise ConfigError("Must specify at least one instance to handle `events`.")

Clean up ShardedWorkerHandlingConfig #9466

Clean up ShardedWorkerHandlingConfig #9466

Conversation

erikjohnston commented Feb 22, 2021

clokep commented Feb 23, 2021

erikjohnston commented Feb 23, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Clean up `ShardedWorkerHandlingConfig` #9466

Clean up `ShardedWorkerHandlingConfig` #9466

erikjohnston commented Feb 23, 2021 •

edited