From 6ca6200cfd1ffaf742c22555d8aa7e7e1fb046f4 Mon Sep 17 00:00:00 2001 From: Jason Little Date: Sat, 1 Jul 2023 20:28:49 -0500 Subject: [PATCH 1/9] Remove the deprecated settings for worker_replication_*, but leave ConfigError statements to gently push admins in the right direction --- synapse/config/workers.py | 50 +++++++++++++++++++++++---------------- 1 file changed, 29 insertions(+), 21 deletions(-) diff --git a/synapse/config/workers.py b/synapse/config/workers.py index 38e13dd7b5e0..ccfe75eaf3a6 100644 --- a/synapse/config/workers.py +++ b/synapse/config/workers.py @@ -41,11 +41,17 @@ _MISSING_MAIN_PROCESS_INSTANCE_MAP_DATA = """ Missing data for a worker to connect to main process. Please include '%s' in the -`instance_map` declared in your shared yaml configuration, or optionally(as a deprecated -solution) in every worker's yaml as various `worker_replication_*` settings as defined -in workers documentation here: +`instance_map` declared in your shared yaml configuration as defined in configuration +documentation here: +`https://matrix-org.github.io/synapse/latest/usage/configuration/config_documentation.html#instance_map` +""" + +WORKER_REPLICATION_SETTING_DEPRECATED_MESSAGE = """ +'%s' is no longer a supported worker setting, please place '%s' onto your shared +configuration under `main` inside the `instance_map`. See workers documentation here: `https://matrix-org.github.io/synapse/latest/workers.html#worker-configuration` """ + # This allows for a handy knob when it's time to change from 'master' to # something with less 'history' MAIN_PROCESS_INSTANCE_NAME = "master" @@ -216,22 +222,37 @@ def read_config(self, config: JsonDict, **kwargs: Any) -> None: ) # A map from instance name to host/port of their HTTP replication endpoint. - # Check if the main process is declared. Inject it into the map if it's not, - # based first on if a 'main' block is declared then on 'worker_replication_*' - # data. If both are available, default to instance_map. The main process - # itself doesn't need this data as it would never have to talk to itself. + # Check if the main process is declared. The main process itself doesn't need + # this data as it would never have to talk to itself. instance_map: Dict[str, Any] = config.get("instance_map", {}) if self.instance_name is not MAIN_PROCESS_INSTANCE_NAME: + # TODO: The next 3 condition blocks can be deleted after some time has + # passed and we're ready to stop checking for these settings. # The host used to connect to the main synapse main_host = config.get("worker_replication_host", None) + if main_host: + raise ConfigError( + WORKER_REPLICATION_SETTING_DEPRECATED_MESSAGE + % ("worker_replication_host", main_host) + ) # The port on the main synapse for HTTP replication endpoint main_port = config.get("worker_replication_http_port") + if main_port: + raise ConfigError( + WORKER_REPLICATION_SETTING_DEPRECATED_MESSAGE + % ("worker_replication_http_port", main_port) + ) # The tls mode on the main synapse for HTTP replication endpoint. # For backward compatibility this defaults to False. main_tls = config.get("worker_replication_http_tls", False) + if main_tls: + raise ConfigError( + WORKER_REPLICATION_SETTING_DEPRECATED_MESSAGE + % ("worker_replication_http_tls", main_tls) + ) # For now, accept 'main' in the instance_map, but the replication system # expects 'master', force that into being until it's changed later. @@ -241,22 +262,9 @@ def read_config(self, config: JsonDict, **kwargs: Any) -> None: ] del instance_map[MAIN_PROCESS_INSTANCE_MAP_NAME] - # This is the backwards compatibility bit that handles the - # worker_replication_* bits using setdefault() to not overwrite anything. - elif main_host is not None and main_port is not None: - instance_map.setdefault( - MAIN_PROCESS_INSTANCE_NAME, - { - "host": main_host, - "port": main_port, - "tls": main_tls, - }, - ) - else: # If we've gotten here, it means that the main process is not on the - # instance_map and that not enough worker_replication_* variables - # were declared in the worker's yaml. + # instance_map. raise ConfigError( _MISSING_MAIN_PROCESS_INSTANCE_MAP_DATA % MAIN_PROCESS_INSTANCE_MAP_NAME From 9f8439df0f7dbe7c6c82e1238d70bcee345fd39d Mon Sep 17 00:00:00 2001 From: Jason Little Date: Sat, 1 Jul 2023 20:29:37 -0500 Subject: [PATCH 2/9] Fix tests to not rely on worker_replication_* settings anymore(delete the one that checked for it) --- tests/app/test_homeserver_start.py | 6 +++--- tests/config/test_workers.py | 27 +-------------------------- 2 files changed, 4 insertions(+), 29 deletions(-) diff --git a/tests/app/test_homeserver_start.py b/tests/app/test_homeserver_start.py index cd117b739473..0201933b04da 100644 --- a/tests/app/test_homeserver_start.py +++ b/tests/app/test_homeserver_start.py @@ -25,9 +25,9 @@ def test_wrong_start_caught(self) -> None: # Add a blank line as otherwise the next addition ends up on a line with a comment self.add_lines_to_config([" "]) self.add_lines_to_config(["worker_app: test_worker_app"]) - self.add_lines_to_config(["worker_replication_host: 127.0.0.1"]) - self.add_lines_to_config(["worker_replication_http_port: 0"]) - + self.add_lines_to_config(["worker_log_config: /data/logconfig.config"]) + self.add_lines_to_config(["instance_map:"]) + self.add_lines_to_config([" main:", " host: 127.0.0.1", " port: 1234"]) # Ensure that starting master process with worker config raises an exception with self.assertRaises(ConfigError): synapse.app.homeserver.setup(["-c", self.config_file]) diff --git a/tests/config/test_workers.py b/tests/config/test_workers.py index 086359fd71a2..2a643ae4f3f3 100644 --- a/tests/config/test_workers.py +++ b/tests/config/test_workers.py @@ -17,7 +17,7 @@ from immutabledict import immutabledict from synapse.config import ConfigError -from synapse.config.workers import InstanceLocationConfig, WorkerConfig +from synapse.config.workers import WorkerConfig from tests.unittest import TestCase @@ -323,28 +323,3 @@ def test_worker_duty_configs(self) -> None: ) self.assertTrue(worker2_config.should_notify_appservices) self.assertFalse(worker2_config.should_update_user_directory) - - def test_worker_instance_map_compat(self) -> None: - """ - Test that `worker_replication_*` settings are compatibly handled by - adding them to the instance map as a `main` entry. - """ - - worker1_config = self._make_worker_config( - worker_app="synapse.app.generic_worker", - worker_name="worker1", - extras={ - "notify_appservices_from_worker": "worker2", - "update_user_directory_from_worker": "worker1", - "worker_replication_host": "127.0.0.42", - "worker_replication_http_port": 1979, - }, - ) - self.assertEqual( - worker1_config.instance_map, - { - "master": InstanceLocationConfig( - host="127.0.0.42", port=1979, tls=False - ), - }, - ) From a5bb3fdabbf7efc32fed3183d5118209692b9ddd Mon Sep 17 00:00:00 2001 From: Jason Little Date: Sat, 1 Jul 2023 20:30:14 -0500 Subject: [PATCH 3/9] Update docs(This tags Synapse 1.88.0, update if it's a later merge) --- docs/workers.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/docs/workers.md b/docs/workers.md index 735128762a47..22d7809f1219 100644 --- a/docs/workers.md +++ b/docs/workers.md @@ -147,7 +147,9 @@ In the config file for each worker, you must specify: the main process (`worker_main_http_uri`). This config option is no longer required and is ignored when running Synapse 1.73 and newer. * **Synapse 1.83 and older:** The HTTP replication endpoint that the worker should talk to on the main synapse process ([`worker_replication_host`](usage/configuration/config_documentation.md#worker_replication_host) and - [`worker_replication_http_port`](usage/configuration/config_documentation.md#worker_replication_http_port)). If using Synapse 1.84 and newer, these are not needed if `main` is defined on the [shared configuration](#shared-configuration) `instance_map` + [`worker_replication_http_port`](usage/configuration/config_documentation.md#worker_replication_http_port)). If + using Synapse 1.84 and newer, these are not needed if `main` is defined on the + [shared configuration](#shared-configuration) `instance_map`. **Backwards compatibility fully removed as of Synapse 1.88.0.** For example: From 548acbfdf0663a627846a397db863639fe6a4669 Mon Sep 17 00:00:00 2001 From: Jason Little Date: Sat, 1 Jul 2023 20:47:43 -0500 Subject: [PATCH 4/9] Upgrade Notes --- docs/upgrade.md | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/docs/upgrade.md b/docs/upgrade.md index 4cd38b13932a..12d3cac5ad23 100644 --- a/docs/upgrade.md +++ b/docs/upgrade.md @@ -87,6 +87,14 @@ process, for example: wget https://packages.matrix.org/debian/pool/main/m/matrix-synapse-py3/matrix-synapse-py3_1.3.0+stretch1_amd64.deb dpkg -i matrix-synapse-py3_1.3.0+stretch1_amd64.deb ``` +# Upgrading to v1.88.0 + +## Removal of `worker_replication_*` settings +As mentioned in v1.84.0, these settings are deprecated and are being removed. Ensure you +have migrated to using `main` on your shared configuration `instance_map`(or create one +if necessary) if you have ***any*** workers at all. + + # Upgrading to v1.86.0 ## Minimum supported Rust version From 8c760f4562bf52818d159d48403fcb5c4c8782f4 Mon Sep 17 00:00:00 2001 From: Jason Little Date: Sat, 1 Jul 2023 20:47:50 -0500 Subject: [PATCH 5/9] changelog --- changelog.d/15860.removal | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/15860.removal diff --git a/changelog.d/15860.removal b/changelog.d/15860.removal new file mode 100644 index 000000000000..0bd2a6158c11 --- /dev/null +++ b/changelog.d/15860.removal @@ -0,0 +1 @@ +Remove deprecated `worker_replication_host`, `worker_replication_http_port` and `worker_replication_http_tls`. From 8e3874168499dc51c9173dd7b04fc7ae88e898f5 Mon Sep 17 00:00:00 2001 From: Jason Little Date: Mon, 3 Jul 2023 06:28:10 -0500 Subject: [PATCH 6/9] Update changelog.d/15860.removal Co-authored-by: reivilibre --- changelog.d/15860.removal | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelog.d/15860.removal b/changelog.d/15860.removal index 0bd2a6158c11..1993bf02997b 100644 --- a/changelog.d/15860.removal +++ b/changelog.d/15860.removal @@ -1 +1 @@ -Remove deprecated `worker_replication_host`, `worker_replication_http_port` and `worker_replication_http_tls`. +Remove deprecated `worker_replication_host`, `worker_replication_http_port` and `worker_replication_http_tls` configuration options. From be0bb9ecbeb16408d2535f17c3187248cd289ffb Mon Sep 17 00:00:00 2001 From: Jason Little Date: Mon, 3 Jul 2023 16:16:31 -0500 Subject: [PATCH 7/9] Update to Upgrade Notes to attach links and flesh out more specifics --- docs/upgrade.md | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/docs/upgrade.md b/docs/upgrade.md index 12d3cac5ad23..0c0cba475f6b 100644 --- a/docs/upgrade.md +++ b/docs/upgrade.md @@ -90,10 +90,15 @@ process, for example: # Upgrading to v1.88.0 ## Removal of `worker_replication_*` settings -As mentioned in v1.84.0, these settings are deprecated and are being removed. Ensure you +As mentioned below in [Upgrading to v1.84.0](#upgrading-to-v1840), these settings are deprecated and are being removed. Ensure you have migrated to using `main` on your shared configuration `instance_map`(or create one if necessary) if you have ***any*** workers at all. +Specifically, to be removed(links for relevant docs attached): +* [`worker_replication_host`](usage/configuration/config_documentation.md#worker_replication_host) +* [`worker_replication_http_port`](usage/configuration/config_documentation.md#worker_replication_http_port) +* [`worker_replication_http_tls`](usage/configuration/config_documentation.md#worker_replication_http_tls) + # Upgrading to v1.86.0 From 1748f87869c2895454dd7f8760d6aa99c465d7b6 Mon Sep 17 00:00:00 2001 From: reivilibre Date: Tue, 4 Jul 2023 16:44:37 +0100 Subject: [PATCH 8/9] Update upgrade.md --- docs/upgrade.md | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-) diff --git a/docs/upgrade.md b/docs/upgrade.md index 0c0cba475f6b..cf7aeb6a3b52 100644 --- a/docs/upgrade.md +++ b/docs/upgrade.md @@ -90,14 +90,19 @@ process, for example: # Upgrading to v1.88.0 ## Removal of `worker_replication_*` settings -As mentioned below in [Upgrading to v1.84.0](#upgrading-to-v1840), these settings are deprecated and are being removed. Ensure you -have migrated to using `main` on your shared configuration `instance_map`(or create one -if necessary) if you have ***any*** workers at all. - -Specifically, to be removed(links for relevant docs attached): -* [`worker_replication_host`](usage/configuration/config_documentation.md#worker_replication_host) -* [`worker_replication_http_port`](usage/configuration/config_documentation.md#worker_replication_http_port) -* [`worker_replication_http_tls`](usage/configuration/config_documentation.md#worker_replication_http_tls) + +As mentioned previously in [Upgrading to v1.84.0](#upgrading-to-v1840), the following deprecated settings +are being removed in this release of Synapse: + +* [`worker_replication_host`](https://matrix-org.github.io/synapse/v1.86/usage/configuration/config_documentation.html#worker_replication_host) +* [`worker_replication_http_port`](https://matrix-org.github.io/synapse/v1.86/usage/configuration/config_documentation.html#worker_replication_http_port) +* [`worker_replication_http_tls`](https://matrix-org.github.io/synapse/v1.86/usage/configuration/config_documentation.html#worker_replication_http_tls) + +Please ensure that you have migrated to using `main` on your shared configuration's `instance_map` +(or create one if necessary). This is required if you have ***any*** workers at all; +administrators of single-process (monolith) installations don't need to do anything. + +For an illustrative example, please see [Upgrading to v1.84.0](#upgrading-to-v1840) below. # Upgrading to v1.86.0 From a99cdac85137aa34440f2bf7569df7a1e217e1da Mon Sep 17 00:00:00 2001 From: Jason Little Date: Thu, 6 Jul 2023 19:03:35 -0500 Subject: [PATCH 9/9] Strip out old documentation as it is preserved in older versions of the manual --- .../configuration/config_documentation.md | 45 ------------------- docs/workers.md | 5 --- 2 files changed, 50 deletions(-) diff --git a/docs/usage/configuration/config_documentation.md b/docs/usage/configuration/config_documentation.md index 89a92c4682be..04e8390ffedf 100644 --- a/docs/usage/configuration/config_documentation.md +++ b/docs/usage/configuration/config_documentation.md @@ -4107,51 +4107,6 @@ Example configuration: worker_name: generic_worker1 ``` --- -### `worker_replication_host` -*Deprecated as of version 1.84.0. Place `host` under `main` entry on the [`instance_map`](#instance_map) in your shared yaml configuration instead.* - -The HTTP replication endpoint that it should talk to on the main Synapse process. -The main Synapse process defines this with a `replication` resource in -[`listeners` option](#listeners). - -Example configuration: -```yaml -worker_replication_host: 127.0.0.1 -``` ---- -### `worker_replication_http_port` -*Deprecated as of version 1.84.0. Place `port` under `main` entry on the [`instance_map`](#instance_map) in your shared yaml configuration instead.* - -The HTTP replication port that it should talk to on the main Synapse process. -The main Synapse process defines this with a `replication` resource in -[`listeners` option](#listeners). - -Example configuration: -```yaml -worker_replication_http_port: 9093 -``` ---- -### `worker_replication_http_tls` -*Deprecated as of version 1.84.0. Place `tls` under `main` entry on the [`instance_map`](#instance_map) in your shared yaml configuration instead.* - -Whether TLS should be used for talking to the HTTP replication port on the main -Synapse process. -The main Synapse process defines this with the `tls` option on its [listener](#listeners) that -has the `replication` resource enabled. - -**Please note:** by default, it is not safe to expose replication ports to the -public Internet, even with TLS enabled. -See [`worker_replication_secret`](#worker_replication_secret). - -Defaults to `false`. - -*Added in Synapse 1.72.0.* - -Example configuration: -```yaml -worker_replication_http_tls: true -``` ---- ### `worker_listeners` A worker can handle HTTP requests. To do so, a `worker_listeners` option diff --git a/docs/workers.md b/docs/workers.md index 267d436d937b..03415c6eb3e4 100644 --- a/docs/workers.md +++ b/docs/workers.md @@ -145,11 +145,6 @@ In the config file for each worker, you must specify: with an `http` listener. * **Synapse 1.72 and older:** if handling the `^/_matrix/client/v3/keys/upload` endpoint, the HTTP URI for the main process (`worker_main_http_uri`). This config option is no longer required and is ignored when running Synapse 1.73 and newer. - * **Synapse 1.83 and older:** The HTTP replication endpoint that the worker should talk to on the main synapse process - ([`worker_replication_host`](usage/configuration/config_documentation.md#worker_replication_host) and - [`worker_replication_http_port`](usage/configuration/config_documentation.md#worker_replication_http_port)). If - using Synapse 1.84 and newer, these are not needed if `main` is defined on the - [shared configuration](#shared-configuration) `instance_map`. **Backwards compatibility fully removed as of Synapse 1.88.0.** For example: