Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Fix a bug introduced in Synapse v1.84.0 where workers do not start up when no instance_map was provided. #15672

Merged
merged 6 commits into from
May 26, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .github/workflows/tests.yml
Original file line number Diff line number Diff line change
Expand Up @@ -314,6 +314,7 @@ jobs:
# There aren't wheels for some of the older deps, so we need to install
# their build dependencies
- run: |
sudo apt-get -qq update
sudo apt-get -qq install build-essential libffi-dev python-dev \
libxml2-dev libxslt-dev xmlsec1 zlib1g-dev libjpeg-dev libwebp-dev

Expand Down
1 change: 1 addition & 0 deletions changelog.d/15672.bugfix
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fix a bug introduced in Synapse v1.84.0 where workers do not start up when no `instance_map` was provided.
2 changes: 1 addition & 1 deletion synapse/config/workers.py
Original file line number Diff line number Diff line change
Expand Up @@ -222,7 +222,7 @@ def read_config(self, config: JsonDict, **kwargs: Any) -> None:
# 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 instance_map and self.instance_name is not MAIN_PROCESS_INSTANCE_NAME:
if self.instance_name is not MAIN_PROCESS_INSTANCE_NAME:
# The host used to connect to the main synapse
main_host = config.get("worker_replication_host", None)

Expand Down
2 changes: 2 additions & 0 deletions tests/app/test_homeserver_start.py
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@ 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"])

# Ensure that starting master process with worker config raises an exception
with self.assertRaises(ConfigError):
Expand Down
1 change: 1 addition & 0 deletions tests/app/test_openid_listener.py
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ def default_config(self) -> JsonDict:
# have to tell the FederationHandler not to try to access stuff that is only
# in the primary store.
conf["worker_app"] = "yes"
conf["instance_map"] = {"main": {"host": "127.0.0.1", "port": 0}}

return conf

Expand Down
43 changes: 39 additions & 4 deletions tests/config/test_workers.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
from immutabledict import immutabledict

from synapse.config import ConfigError
from synapse.config.workers import WorkerConfig
from synapse.config.workers import InstanceLocationConfig, WorkerConfig

from tests.unittest import TestCase

Expand Down Expand Up @@ -94,6 +94,7 @@ def test_old_configs_appservice_worker(self) -> None:
# so that it doesn't raise an exception here.
# (This is not read by `_should_this_worker_perform_duty`.)
"notify_appservices": False,
"instance_map": {"main": {"host": "127.0.0.1", "port": 0}},
DMRobertson marked this conversation as resolved.
Show resolved Hide resolved
},
)

Expand Down Expand Up @@ -138,7 +139,9 @@ def test_transitional_configs_master(self) -> None:
"""

main_process_config = self._make_worker_config(
worker_app="synapse.app.homeserver", worker_name=None
worker_app="synapse.app.homeserver",
worker_name=None,
extras={"instance_map": {"main": {"host": "127.0.0.1", "port": 0}}},
DMRobertson marked this conversation as resolved.
Show resolved Hide resolved
)

self.assertTrue(
Expand Down Expand Up @@ -203,6 +206,7 @@ def test_transitional_configs_appservice_worker(self) -> None:
# so that it doesn't raise an exception here.
# (This is not read by `_should_this_worker_perform_duty`.)
"notify_appservices": False,
"instance_map": {"main": {"host": "127.0.0.1", "port": 0}},
},
)

Expand Down Expand Up @@ -236,7 +240,9 @@ def test_new_configs_master(self) -> None:
Tests new config options. This is for the master's config.
"""
main_process_config = self._make_worker_config(
worker_app="synapse.app.homeserver", worker_name=None
worker_app="synapse.app.homeserver",
worker_name=None,
extras={"instance_map": {"main": {"host": "127.0.0.1", "port": 0}}},
)

self.assertTrue(
Expand All @@ -262,7 +268,9 @@ def test_new_configs_appservice_worker(self) -> None:
Tests new config options. This is for the worker's config.
"""
appservice_worker_config = self._make_worker_config(
worker_app="synapse.app.generic_worker", worker_name="worker1"
worker_app="synapse.app.generic_worker",
worker_name="worker1",
extras={"instance_map": {"main": {"host": "127.0.0.1", "port": 0}}},
)

self.assertTrue(
Expand Down Expand Up @@ -298,6 +306,7 @@ def test_worker_duty_configs(self) -> None:
extras={
"notify_appservices_from_worker": "worker2",
"update_user_directory_from_worker": "worker1",
"instance_map": {"main": {"host": "127.0.0.1", "port": 0}},
},
)
self.assertFalse(worker1_config.should_notify_appservices)
Expand All @@ -309,7 +318,33 @@ def test_worker_duty_configs(self) -> None:
extras={
"notify_appservices_from_worker": "worker2",
"update_user_directory_from_worker": "worker1",
"instance_map": {"main": {"host": "127.0.0.1", "port": 0}},
},
)
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
),
},
)
1 change: 1 addition & 0 deletions tests/replication/test_federation_ack.py
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ def default_config(self) -> dict:
config["worker_app"] = "synapse.app.generic_worker"
config["worker_name"] = "federation_sender1"
config["federation_sender_instances"] = ["federation_sender1"]
config["instance_map"] = {"main": {"host": "127.0.0.1", "port": 0}}
return config

def make_homeserver(self, reactor: MemoryReactor, clock: Clock) -> HomeServer:
Expand Down
1 change: 1 addition & 0 deletions tests/storage/test_rollback_worker.py
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ def default_config(self) -> JsonDict:

# Mark this as a worker app.
conf["worker_app"] = "yes"
conf["instance_map"] = {"main": {"host": "127.0.0.1", "port": 0}}

return conf

Expand Down